| Message ID | 20220707213204.2605816-2-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dt-bindings: msm/dp: cleanup Qualcomm DP and eDP bidndings | expand |
Quoting Dmitry Baryshkov (2022-07-07 14:31:56) > The p1 region was probably added by mistake, none of the DTS files > provides one (and the driver source code also doesn't use one). Drop it > now. Yes, looks like the driver doesn't use it. > > Fixes: 687825c402f1 ("dt-bindings: msm/dp: Change reg definition") > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > index 94bc6e1b6451..d6bbe58ef9e8 100644 > --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > @@ -29,7 +29,6 @@ properties: > - description: aux register block > - description: link register block > - description: p0 register block > - - description: p1 register block The p1 registers exist on sc7180. They start where the example starts, at 0xae91400.
On 08/07/2022 04:28, Stephen Boyd wrote: > Quoting Dmitry Baryshkov (2022-07-07 14:31:56) >> The p1 region was probably added by mistake, none of the DTS files >> provides one (and the driver source code also doesn't use one). Drop it >> now. > > Yes, looks like the driver doesn't use it. > >> >> Fixes: 687825c402f1 ("dt-bindings: msm/dp: Change reg definition") >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml >> index 94bc6e1b6451..d6bbe58ef9e8 100644 >> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml >> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml >> @@ -29,7 +29,6 @@ properties: >> - description: aux register block >> - description: link register block >> - description: p0 register block >> - - description: p1 register block > > The p1 registers exist on sc7180. They start where the example starts, > at 0xae91400. Do they exist on e.g. sc7280? In other words, should we add the region to the DTS? For now I'm going to mark it as optional.
Quoting Dmitry Baryshkov (2022-07-07 20:46:43) > On 08/07/2022 04:28, Stephen Boyd wrote: > > Quoting Dmitry Baryshkov (2022-07-07 14:31:56) > >> The p1 region was probably added by mistake, none of the DTS files > >> provides one (and the driver source code also doesn't use one). Drop it > >> now. > > > > Yes, looks like the driver doesn't use it. > > > >> > >> Fixes: 687825c402f1 ("dt-bindings: msm/dp: Change reg definition") > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >> --- > >> Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 1 - > >> 1 file changed, 1 deletion(-) > >> > >> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > >> index 94bc6e1b6451..d6bbe58ef9e8 100644 > >> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > >> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > >> @@ -29,7 +29,6 @@ properties: > >> - description: aux register block > >> - description: link register block > >> - description: p0 register block > >> - - description: p1 register block > > > > The p1 registers exist on sc7180. They start where the example starts, > > at 0xae91400. > > Do they exist on e.g. sc7280? In other words, should we add the region > to the DTS? For now I'm going to mark it as optional. > Yes I see the same address for P1 on sc7280. Maybe it's a typo? Abhinav, can you confirm?
+ kuogee On 7/8/2022 12:27 PM, Stephen Boyd wrote: > Quoting Dmitry Baryshkov (2022-07-07 20:46:43) >> On 08/07/2022 04:28, Stephen Boyd wrote: >>> Quoting Dmitry Baryshkov (2022-07-07 14:31:56) >>>> The p1 region was probably added by mistake, none of the DTS files >>>> provides one (and the driver source code also doesn't use one). Drop it >>>> now. >>> >>> Yes, looks like the driver doesn't use it. >>> >>>> >>>> Fixes: 687825c402f1 ("dt-bindings: msm/dp: Change reg definition") >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> --- >>>> Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 1 - >>>> 1 file changed, 1 deletion(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml >>>> index 94bc6e1b6451..d6bbe58ef9e8 100644 >>>> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml >>>> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml >>>> @@ -29,7 +29,6 @@ properties: >>>> - description: aux register block >>>> - description: link register block >>>> - description: p0 register block >>>> - - description: p1 register block >>> >>> The p1 registers exist on sc7180. They start where the example starts, >>> at 0xae91400. >> >> Do they exist on e.g. sc7280? In other words, should we add the region >> to the DTS? For now I'm going to mark it as optional. >> > > Yes I see the same address for P1 on sc7280. Maybe it's a typo? Abhinav, > can you confirm? P1 block does exist on sc7280 and yes its address is same as the address mentioned in sc7180. So its not a typo. Yes, we are not programming this today but I would prefer to keep this as optional. I did sync up with Kuogee on this change this morning, we will check a few things internally on the P1 block's usage as to which use-cases we need to program it for and update here. The idea behind having this register space listed in the yaml is thats how the software documents have the blocks listed so dropping P1 block just because its unused seemed wrong to me. Optional seems more appropriate. Thanks Abhinav
On 7/8/2022 12:38 PM, Abhinav Kumar wrote: > + kuogee > > On 7/8/2022 12:27 PM, Stephen Boyd wrote: >> Quoting Dmitry Baryshkov (2022-07-07 20:46:43) >>> On 08/07/2022 04:28, Stephen Boyd wrote: >>>> Quoting Dmitry Baryshkov (2022-07-07 14:31:56) >>>>> The p1 region was probably added by mistake, none of the DTS files >>>>> provides one (and the driver source code also doesn't use one). >>>>> Drop it >>>>> now. >>>> >>>> Yes, looks like the driver doesn't use it. >>>> >>>>> >>>>> Fixes: 687825c402f1 ("dt-bindings: msm/dp: Change reg definition") >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>> --- >>>>> Documentation/devicetree/bindings/display/msm/dp-controller.yaml | >>>>> 1 - >>>>> 1 file changed, 1 deletion(-) >>>>> >>>>> diff --git >>>>> a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml >>>>> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml >>>>> index 94bc6e1b6451..d6bbe58ef9e8 100644 >>>>> --- >>>>> a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml >>>>> +++ >>>>> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml >>>>> @@ -29,7 +29,6 @@ properties: >>>>> - description: aux register block >>>>> - description: link register block >>>>> - description: p0 register block >>>>> - - description: p1 register block >>>> >>>> The p1 registers exist on sc7180. They start where the example starts, >>>> at 0xae91400. >>> >>> Do they exist on e.g. sc7280? In other words, should we add the region >>> to the DTS? For now I'm going to mark it as optional. >>> >> >> Yes I see the same address for P1 on sc7280. Maybe it's a typo? Abhinav, >> can you confirm? > > P1 block does exist on sc7280 and yes its address is same as the > address mentioned in sc7180. So its not a typo. > > Yes, we are not programming this today but I would prefer to keep this > as optional. > > I did sync up with Kuogee on this change this morning, we will check a > few things internally on the P1 block's usage as to which use-cases we > need to program it for and update here. > P1 block is for dp MST application. This allow two dp streams can be mux into same DP phy. We should keep it since we may support MST later. > The idea behind having this register space listed in the yaml is thats > how the software documents have the blocks listed so dropping P1 block > just because its unused seemed wrong to me. Optional seems more > appropriate. > > Thanks > > Abhinav
Quoting Abhinav Kumar (2022-07-08 12:38:09) > + kuogee > > On 7/8/2022 12:27 PM, Stephen Boyd wrote: > > > > Yes I see the same address for P1 on sc7280. Maybe it's a typo? Abhinav, > > can you confirm? > > P1 block does exist on sc7280 and yes its address is same as the address > mentioned in sc7180. So its not a typo. Thanks! > > Yes, we are not programming this today but I would prefer to keep this > as optional. > > I did sync up with Kuogee on this change this morning, we will check a > few things internally on the P1 block's usage as to which use-cases we > need to program it for and update here. > > The idea behind having this register space listed in the yaml is thats > how the software documents have the blocks listed so dropping P1 block > just because its unused seemed wrong to me. Optional seems more appropriate. > It doesn't sound optional on sc7180 or sc7280. It exists in the hardware, so we should list the reg property. My understanding of optional properties is for the case where something could be different in the hardware design, like an optionally connected pin on a device.
On 7/8/2022 12:51 PM, Stephen Boyd wrote: > Quoting Abhinav Kumar (2022-07-08 12:38:09) >> + kuogee >> >> On 7/8/2022 12:27 PM, Stephen Boyd wrote: >>> >>> Yes I see the same address for P1 on sc7280. Maybe it's a typo? Abhinav, >>> can you confirm? >> >> P1 block does exist on sc7280 and yes its address is same as the address >> mentioned in sc7180. So its not a typo. > > Thanks! > >> >> Yes, we are not programming this today but I would prefer to keep this >> as optional. >> >> I did sync up with Kuogee on this change this morning, we will check a >> few things internally on the P1 block's usage as to which use-cases we >> need to program it for and update here. >> >> The idea behind having this register space listed in the yaml is thats >> how the software documents have the blocks listed so dropping P1 block >> just because its unused seemed wrong to me. Optional seems more appropriate. >> > > It doesn't sound optional on sc7180 or sc7280. It exists in the > hardware, so we should list the reg property. My understanding of > optional properties is for the case where something could be different > in the hardware design, like an optionally connected pin on a device. Ack, if thats the purpose of optional, then we should keep it and yes lets drop this change.
On Fri, 08 Jul 2022 00:31:56 +0300, Dmitry Baryshkov wrote: > The p1 region was probably added by mistake, none of the DTS files > provides one (and the driver source code also doesn't use one). Drop it > now. > > Fixes: 687825c402f1 ("dt-bindings: msm/dp: Change reg definition") > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 1 - > 1 file changed, 1 deletion(-) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/msm/dp-controller.example.dtb: displayport-controller@ae90000: reg: [[183042048, 512], [183042560, 512], [183043072, 3072], [183046144, 1024], [183047168, 1024]] is too long From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/msm/dp-controller.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
On 08/07/2022 22:47, Kuogee Hsieh wrote: > > On 7/8/2022 12:38 PM, Abhinav Kumar wrote: >> + kuogee >> >> On 7/8/2022 12:27 PM, Stephen Boyd wrote: >>> Quoting Dmitry Baryshkov (2022-07-07 20:46:43) >>>> On 08/07/2022 04:28, Stephen Boyd wrote: >>>>> Quoting Dmitry Baryshkov (2022-07-07 14:31:56) >>>>>> The p1 region was probably added by mistake, none of the DTS files >>>>>> provides one (and the driver source code also doesn't use one). >>>>>> Drop it >>>>>> now. >>>>> >>>>> Yes, looks like the driver doesn't use it. >>>>> >>>>>> >>>>>> Fixes: 687825c402f1 ("dt-bindings: msm/dp: Change reg definition") >>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>>> --- >>>>>> Documentation/devicetree/bindings/display/msm/dp-controller.yaml | >>>>>> 1 - >>>>>> 1 file changed, 1 deletion(-) >>>>>> >>>>>> diff --git >>>>>> a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml >>>>>> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml >>>>>> index 94bc6e1b6451..d6bbe58ef9e8 100644 >>>>>> --- >>>>>> a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml >>>>>> +++ >>>>>> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml >>>>>> @@ -29,7 +29,6 @@ properties: >>>>>> - description: aux register block >>>>>> - description: link register block >>>>>> - description: p0 register block >>>>>> - - description: p1 register block >>>>> >>>>> The p1 registers exist on sc7180. They start where the example starts, >>>>> at 0xae91400. >>>> >>>> Do they exist on e.g. sc7280? In other words, should we add the region >>>> to the DTS? For now I'm going to mark it as optional. >>>> >>> >>> Yes I see the same address for P1 on sc7280. Maybe it's a typo? Abhinav, >>> can you confirm? >> >> P1 block does exist on sc7280 and yes its address is same as the >> address mentioned in sc7180. So its not a typo. >> >> Yes, we are not programming this today but I would prefer to keep this >> as optional. >> >> I did sync up with Kuogee on this change this morning, we will check a >> few things internally on the P1 block's usage as to which use-cases we >> need to program it for and update here. >> > P1 block is for dp MST application. This allow two dp streams can be > mux into same DP phy. > > We should keep it since we may support MST later. Thanks for the confirmation. Next question, does it exist on eDP controllers? > >> The idea behind having this register space listed in the yaml is thats >> how the software documents have the blocks listed so dropping P1 block >> just because its unused seemed wrong to me. Optional seems more >> appropriate. >> >> Thanks >> >> Abhinav
diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml index 94bc6e1b6451..d6bbe58ef9e8 100644 --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml @@ -29,7 +29,6 @@ properties: - description: aux register block - description: link register block - description: p0 register block - - description: p1 register block interrupts: maxItems: 1
The p1 region was probably added by mistake, none of the DTS files provides one (and the driver source code also doesn't use one). Drop it now. Fixes: 687825c402f1 ("dt-bindings: msm/dp: Change reg definition") Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 1 - 1 file changed, 1 deletion(-)