| Message ID | 1580825737-27189-1-git-send-email-harigovi@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] dt-bindings: msm:disp: update dsi and dpu bindings | expand |
On Tue, Feb 4, 2020 at 7:15 AM Harigovindan P <harigovi@codeaurora.org> wrote: > > Updating bindings of dsi and dpu by adding and removing certain > properties. > > Signed-off-by: Harigovindan P <harigovi@codeaurora.org> > --- > > Changes in v1: > - Adding "ahb" clock as a required property. > - Adding "bus", "rot", "lut" as optional properties for sc7180 device. > - Removing properties from dsi bindings that are unused. > - Removing power-domain property since DSI is the child node of MDSS > and it will inherit supply from its parent. > > Documentation/devicetree/bindings/display/msm/dpu.txt | 7 +++++++ > Documentation/devicetree/bindings/display/msm/dsi.txt | 5 ----- > 2 files changed, 7 insertions(+), 5 deletions(-) > diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt > index af95586..61d659a 100644 > --- a/Documentation/devicetree/bindings/display/msm/dsi.txt > +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt > @@ -8,13 +8,10 @@ Required properties: > - reg-names: The names of register regions. The following regions are required: > * "dsi_ctrl" > - interrupts: The interrupt signal from the DSI block. > -- power-domains: Should be <&mmcc MDSS_GDSC>. > - clocks: Phandles to device clocks. > - clock-names: the following clocks are required: > - * "mdp_core" > * "iface" > * "bus" > - * "core_mmss" Why do you think these are unused? I see them used in the driver, and as far as I can tell these get routed to the hardware, therefore they should be described in DT. > * "byte" > * "pixel" > * "core" > @@ -156,7 +153,6 @@ Example: > "core", > "core_mmss", > "iface", > - "mdp_core", > "pixel"; > clocks = > <&mmcc MDSS_AXI_CLK>, > @@ -164,7 +160,6 @@ Example: > <&mmcc MDSS_ESC0_CLK>, > <&mmcc MMSS_MISC_AHB_CLK>, > <&mmcc MDSS_AHB_CLK>, > - <&mmcc MDSS_MDP_CLK>, > <&mmcc MDSS_PCLK0_CLK>; > > assigned-clocks = > -- > 2.7.4 > > _______________________________________________ > Freedreno mailing list > Freedreno@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/freedreno
Hi, On Tue, Feb 4, 2020 at 6:15 AM Harigovindan P <harigovi@codeaurora.org> wrote: > > Updating bindings of dsi and dpu by adding and removing certain > properties. > > Signed-off-by: Harigovindan P <harigovi@codeaurora.org> > --- > > Changes in v1: > - Adding "ahb" clock as a required property. > - Adding "bus", "rot", "lut" as optional properties for sc7180 device. > - Removing properties from dsi bindings that are unused. > - Removing power-domain property since DSI is the child node of MDSS > and it will inherit supply from its parent. > > Documentation/devicetree/bindings/display/msm/dpu.txt | 7 +++++++ > Documentation/devicetree/bindings/display/msm/dsi.txt | 5 ----- > 2 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt b/Documentation/devicetree/bindings/display/msm/dpu.txt > index 551ae26..dd58472a 100644 > --- a/Documentation/devicetree/bindings/display/msm/dpu.txt > +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt > @@ -19,6 +19,7 @@ Required properties: > The following clocks are required: > * "iface" > * "bus" > + * "ahb" This is only required for sc7180? ...or old SoCs should have had it all along too? > * "core" > - interrupts: interrupt signal from MDSS. > - interrupt-controller: identifies the node as an interrupt controller. > @@ -50,6 +51,8 @@ Required properties: > - clock-names: device clock names, must be in same order as clocks property. > The following clocks are required. > * "bus" > + For the device "qcom,sc7180-dpu": > + * "bus" - is an optional property due to architecture change. This is a really odd way to write it for two reasons: * You're breaking up the flow of the list. * This shouldn't be listed as "optional" in sc7180 but unless there is some reason to ever provide it on sc7180. It should simply be disallowed. Maybe instead just: The following clocks are required. - * "bus" + * "bus" (anything other than qcom,sc7180-dpu) We really need to get this into yaml ASAP but that'd probably be OK to tide us over. NOTE: when converting to yaml, ideally we'll have a separate file per SoC to avoid crazy spaghetti, see commit 2a8aa18c1131 ("dt-bindings: clk: qcom: Fix self-validation, split, and clean cruft") in clk-next for an example of starting the transition to one yaml per SoC (at least for anything majorly different). > * "iface" > * "core" > * "vsync" > @@ -70,6 +73,10 @@ Optional properties: > - assigned-clocks: list of clock specifiers for clocks needing rate assignment > - assigned-clock-rates: list of clock frequencies sorted in the same order as > the assigned-clocks property. > +- For the device "qcom,sc7180-dpu": > + clock-names: optional device clocks, needed for accessing LUT blocks. > + * "rot" > + * "lut" > > Example: > > diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt > index af95586..61d659a 100644 > --- a/Documentation/devicetree/bindings/display/msm/dsi.txt > +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt > @@ -8,13 +8,10 @@ Required properties: > - reg-names: The names of register regions. The following regions are required: > * "dsi_ctrl" > - interrupts: The interrupt signal from the DSI block. > -- power-domains: Should be <&mmcc MDSS_GDSC>. Is this supposed to be removed from all SoCs using this bindings, or just yours? I'll also note that you left it in the "Example:" below. > - clocks: Phandles to device clocks. > - clock-names: the following clocks are required: > - * "mdp_core" > * "iface" > * "bus" > - * "core_mmss" As Jeffrey pointed out, you shouldn't be removing these from old SoCs. In "drivers/gpu/drm/msm/dsi/dsi_cfg.c" you can clearly see them used. Maybe it's time for you to do the yaml conversion and handle this correctly per-SoC. -Doug
On Tue, Feb 04, 2020 at 07:45:37PM +0530, Harigovindan P wrote: > Updating bindings of dsi and dpu by adding and removing certain > properties. Yes, the diff tells me that. The commit message should say why. This change breaks compatibility as well. > > Signed-off-by: Harigovindan P <harigovi@codeaurora.org> > --- > > Changes in v1: > - Adding "ahb" clock as a required property. > - Adding "bus", "rot", "lut" as optional properties for sc7180 device. > - Removing properties from dsi bindings that are unused. > - Removing power-domain property since DSI is the child node of MDSS > and it will inherit supply from its parent. > > Documentation/devicetree/bindings/display/msm/dpu.txt | 7 +++++++ > Documentation/devicetree/bindings/display/msm/dsi.txt | 5 ----- > 2 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt b/Documentation/devicetree/bindings/display/msm/dpu.txt > index 551ae26..dd58472a 100644 > --- a/Documentation/devicetree/bindings/display/msm/dpu.txt > +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt > @@ -19,6 +19,7 @@ Required properties: > The following clocks are required: > * "iface" > * "bus" > + * "ahb" You can't just add new clocks... > * "core" > - interrupts: interrupt signal from MDSS. > - interrupt-controller: identifies the node as an interrupt controller. > @@ -50,6 +51,8 @@ Required properties: > - clock-names: device clock names, must be in same order as clocks property. > The following clocks are required. > * "bus" > + For the device "qcom,sc7180-dpu": > + * "bus" - is an optional property due to architecture change. > * "iface" > * "core" > * "vsync" > @@ -70,6 +73,10 @@ Optional properties: > - assigned-clocks: list of clock specifiers for clocks needing rate assignment > - assigned-clock-rates: list of clock frequencies sorted in the same order as > the assigned-clocks property. > +- For the device "qcom,sc7180-dpu": > + clock-names: optional device clocks, needed for accessing LUT blocks. > + * "rot" > + * "lut" > > Example: > > diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt > index af95586..61d659a 100644 > --- a/Documentation/devicetree/bindings/display/msm/dsi.txt > +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt > @@ -8,13 +8,10 @@ Required properties: > - reg-names: The names of register regions. The following regions are required: > * "dsi_ctrl" > - interrupts: The interrupt signal from the DSI block. > -- power-domains: Should be <&mmcc MDSS_GDSC>. > - clocks: Phandles to device clocks. > - clock-names: the following clocks are required: > - * "mdp_core" > * "iface" > * "bus" > - * "core_mmss" > * "byte" > * "pixel" > * "core" > @@ -156,7 +153,6 @@ Example: > "core", > "core_mmss", > "iface", > - "mdp_core", > "pixel"; > clocks = > <&mmcc MDSS_AXI_CLK>, > @@ -164,7 +160,6 @@ Example: > <&mmcc MDSS_ESC0_CLK>, > <&mmcc MMSS_MISC_AHB_CLK>, > <&mmcc MDSS_AHB_CLK>, > - <&mmcc MDSS_MDP_CLK>, > <&mmcc MDSS_PCLK0_CLK>; > > assigned-clocks = > -- > 2.7.4 >
On 2020-02-04 23:52, Doug Anderson wrote: > Hi, > > On Tue, Feb 4, 2020 at 6:15 AM Harigovindan P <harigovi@codeaurora.org> > wrote: >> >> Updating bindings of dsi and dpu by adding and removing certain >> properties. >> >> Signed-off-by: Harigovindan P <harigovi@codeaurora.org> >> --- >> >> Changes in v1: >> - Adding "ahb" clock as a required property. >> - Adding "bus", "rot", "lut" as optional properties for sc7180 >> device. >> - Removing properties from dsi bindings that are unused. >> - Removing power-domain property since DSI is the child node >> of MDSS >> and it will inherit supply from its parent. >> >> Documentation/devicetree/bindings/display/msm/dpu.txt | 7 +++++++ >> Documentation/devicetree/bindings/display/msm/dsi.txt | 5 ----- >> 2 files changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt >> b/Documentation/devicetree/bindings/display/msm/dpu.txt >> index 551ae26..dd58472a 100644 >> --- a/Documentation/devicetree/bindings/display/msm/dpu.txt >> +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt >> @@ -19,6 +19,7 @@ Required properties: >> The following clocks are required: >> * "iface" >> * "bus" >> + * "ahb" > > This is only required for sc7180? ...or old SoCs should have had it > all along too? > > >> * "core" >> - interrupts: interrupt signal from MDSS. >> - interrupt-controller: identifies the node as an interrupt >> controller. >> @@ -50,6 +51,8 @@ Required properties: >> - clock-names: device clock names, must be in same order as clocks >> property. >> The following clocks are required. >> * "bus" >> + For the device "qcom,sc7180-dpu": >> + * "bus" - is an optional property due to architecture change. > > This is a really odd way to write it for two reasons: > * You're breaking up the flow of the list. > * This shouldn't be listed as "optional" in sc7180 but unless there is > some reason to ever provide it on sc7180. It should simply be > disallowed. > > Maybe instead just: > > The following clocks are required. > - * "bus" > + * "bus" (anything other than qcom,sc7180-dpu) > > We really need to get this into yaml ASAP but that'd probably be OK to > tide us over. > > NOTE: when converting to yaml, ideally we'll have a separate file per > SoC to avoid crazy spaghetti, see commit 2a8aa18c1131 ("dt-bindings: > clk: qcom: Fix self-validation, split, and clean cruft") in clk-next > for an example of starting the transition to one yaml per SoC (at > least for anything majorly different). > > >> * "iface" >> * "core" >> * "vsync" >> @@ -70,6 +73,10 @@ Optional properties: >> - assigned-clocks: list of clock specifiers for clocks needing rate >> assignment >> - assigned-clock-rates: list of clock frequencies sorted in the same >> order as >> the assigned-clocks property. >> +- For the device "qcom,sc7180-dpu": >> + clock-names: optional device clocks, needed for accessing LUT >> blocks. >> + * "rot" >> + * "lut" >> >> Example: >> >> diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt >> b/Documentation/devicetree/bindings/display/msm/dsi.txt >> index af95586..61d659a 100644 >> --- a/Documentation/devicetree/bindings/display/msm/dsi.txt >> +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt >> @@ -8,13 +8,10 @@ Required properties: >> - reg-names: The names of register regions. The following regions are >> required: >> * "dsi_ctrl" >> - interrupts: The interrupt signal from the DSI block. >> -- power-domains: Should be <&mmcc MDSS_GDSC>. > > Is this supposed to be removed from all SoCs using this bindings, or > just yours? > > I'll also note that you left it in the "Example:" below. > > >> - clocks: Phandles to device clocks. >> - clock-names: the following clocks are required: >> - * "mdp_core" >> * "iface" >> * "bus" >> - * "core_mmss" > > As Jeffrey pointed out, you shouldn't be removing these from old SoCs. > In "drivers/gpu/drm/msm/dsi/dsi_cfg.c" you can clearly see them used. > Maybe it's time for you to do the yaml conversion and handle this > correctly per-SoC. > > -Doug Hi, yaml files have been created and new patchset has been created for that. https://patchwork.kernel.org/patch/11423653/
diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt b/Documentation/devicetree/bindings/display/msm/dpu.txt index 551ae26..dd58472a 100644 --- a/Documentation/devicetree/bindings/display/msm/dpu.txt +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt @@ -19,6 +19,7 @@ Required properties: The following clocks are required: * "iface" * "bus" + * "ahb" * "core" - interrupts: interrupt signal from MDSS. - interrupt-controller: identifies the node as an interrupt controller. @@ -50,6 +51,8 @@ Required properties: - clock-names: device clock names, must be in same order as clocks property. The following clocks are required. * "bus" + For the device "qcom,sc7180-dpu": + * "bus" - is an optional property due to architecture change. * "iface" * "core" * "vsync" @@ -70,6 +73,10 @@ Optional properties: - assigned-clocks: list of clock specifiers for clocks needing rate assignment - assigned-clock-rates: list of clock frequencies sorted in the same order as the assigned-clocks property. +- For the device "qcom,sc7180-dpu": + clock-names: optional device clocks, needed for accessing LUT blocks. + * "rot" + * "lut" Example: diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt index af95586..61d659a 100644 --- a/Documentation/devicetree/bindings/display/msm/dsi.txt +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt @@ -8,13 +8,10 @@ Required properties: - reg-names: The names of register regions. The following regions are required: * "dsi_ctrl" - interrupts: The interrupt signal from the DSI block. -- power-domains: Should be <&mmcc MDSS_GDSC>. - clocks: Phandles to device clocks. - clock-names: the following clocks are required: - * "mdp_core" * "iface" * "bus" - * "core_mmss" * "byte" * "pixel" * "core" @@ -156,7 +153,6 @@ Example: "core", "core_mmss", "iface", - "mdp_core", "pixel"; clocks = <&mmcc MDSS_AXI_CLK>, @@ -164,7 +160,6 @@ Example: <&mmcc MDSS_ESC0_CLK>, <&mmcc MMSS_MISC_AHB_CLK>, <&mmcc MDSS_AHB_CLK>, - <&mmcc MDSS_MDP_CLK>, <&mmcc MDSS_PCLK0_CLK>; assigned-clocks =
Updating bindings of dsi and dpu by adding and removing certain properties. Signed-off-by: Harigovindan P <harigovi@codeaurora.org> --- Changes in v1: - Adding "ahb" clock as a required property. - Adding "bus", "rot", "lut" as optional properties for sc7180 device. - Removing properties from dsi bindings that are unused. - Removing power-domain property since DSI is the child node of MDSS and it will inherit supply from its parent. Documentation/devicetree/bindings/display/msm/dpu.txt | 7 +++++++ Documentation/devicetree/bindings/display/msm/dsi.txt | 5 ----- 2 files changed, 7 insertions(+), 5 deletions(-)