* Re: [Freedreno] [v1] dt-bindings: msm:disp: update dsi and dpu bindings
2020-02-04 14:15 [v1] dt-bindings: msm:disp: update dsi and dpu bindings Harigovindan P
@ 2020-02-04 14:44 ` Jeffrey Hugo
2020-02-04 18:22 ` Doug Anderson
2020-02-06 19:13 ` Rob Herring
2 siblings, 0 replies; 5+ messages in thread
From: Jeffrey Hugo @ 2020-02-04 14:44 UTC (permalink / raw)
To: Harigovindan P
Cc: open list:DRM PANEL DRIVERS, MSM, freedreno, DTML, lkml,
Rob Clark, nganji, Sean Paul, kalyan_t, Kristian H. Kristensen
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [v1] dt-bindings: msm:disp: update dsi and dpu bindings
2020-02-04 14:15 [v1] dt-bindings: msm:disp: update dsi and dpu bindings Harigovindan P
2020-02-04 14:44 ` [Freedreno] " Jeffrey Hugo
@ 2020-02-04 18:22 ` Doug Anderson
2020-03-06 11:41 ` harigovi
2020-02-06 19:13 ` Rob Herring
2 siblings, 1 reply; 5+ messages in thread
From: Doug Anderson @ 2020-02-04 18:22 UTC (permalink / raw)
To: Harigovindan P
Cc: dri-devel, linux-arm-msm, freedreno,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
Rob Clark, Sean Paul, Kristian H. Kristensen, Kalyan Thota,
nganji
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [v1] dt-bindings: msm:disp: update dsi and dpu bindings
2020-02-04 18:22 ` Doug Anderson
@ 2020-03-06 11:41 ` harigovi
0 siblings, 0 replies; 5+ messages in thread
From: harigovi @ 2020-03-06 11:41 UTC (permalink / raw)
To: Doug Anderson
Cc: dri-devel, linux-arm-msm, freedreno,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
Rob Clark, Sean Paul, Kristian H. Kristensen, Kalyan Thota,
nganji
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/
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [v1] dt-bindings: msm:disp: update dsi and dpu bindings
2020-02-04 14:15 [v1] dt-bindings: msm:disp: update dsi and dpu bindings Harigovindan P
2020-02-04 14:44 ` [Freedreno] " Jeffrey Hugo
2020-02-04 18:22 ` Doug Anderson
@ 2020-02-06 19:13 ` Rob Herring
2 siblings, 0 replies; 5+ messages in thread
From: Rob Herring @ 2020-02-06 19:13 UTC (permalink / raw)
To: Harigovindan P
Cc: dri-devel, linux-arm-msm, freedreno, devicetree, linux-kernel,
robdclark, seanpaul, hoegsberg, kalyan_t, nganji
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread