Message-ID: <c7129d8e-87de-444c-be52-b2eedf03585f@kernel.org>
Date: Wed, 5 Mar 2025 12:35:15 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Alexander Stein <alexander.stein@...tq-group.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
 Sascha Hauer <s.hauer@...gutronix.de>,
 Pengutronix Kernel Team <kernel@...gutronix.de>,
 Fabio Estevam <festevam@...il.com>, Abel Vesa <abelvesa@...nel.org>,
 Peng Fan <peng.fan@....com>, Michael Turquette <mturquette@...libre.com>,
 Stephen Boyd <sboyd@...nel.org>, Ulf Hansson <ulf.hansson@...aro.org>,
 devicetree@...r.kernel.org, imx@...ts.linux.dev,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 linux@...tq-group.com, linux-clk@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v3 2/6] dt-bindings: soc: imx93-media-blk-ctrl: Add LDB
 subnode into schema and example

On 05/03/2025 10:02, Alexander Stein wrote:
> Hi,
> 
> Am Mittwoch, 5. März 2025, 08:13:04 CET schrieb Krzysztof Kozlowski:
>> On Tue, Mar 04, 2025 at 04:49:21PM +0100, Alexander Stein wrote:
>>> Document the LDB bridge subnode and add the subnode into the example.
>>> For the subnode to work, the block control must scan its subnodes and
>>
>> Don't describe drivers, but describe the hardware.
> 
> Thanks, I'll rephrase to describe the hardware better regarding LVDS.
> 
>>
>>> bind drivers to them, do not misuse either simple-bus or simple-mfd
>>> here.
>>
>> I don't understand that simple-bus or simple-mfd statement. There are no
>> such compatibles here.
> 
> Same as above, the wording stems from 1cb0c87d27dcc ("dt-bindings: soc:
> imx8mp-media-blk-ctrl: Add LDB subnode into schema and example").
> I'll drop it to avoid confusion.
> 
>>
>>>
>>> Signed-off-by: Alexander Stein <alexander.stein@...tq-group.com>
>>> ---
>>>  .../soc/imx/fsl,imx93-media-blk-ctrl.yaml     | 51 +++++++++++++++++++
>>>  1 file changed, 51 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/soc/imx/fsl,imx93-media-blk-ctrl.yaml b/Documentation/devicetree/bindings/soc/imx/fsl,imx93-media-blk-ctrl.yaml
>>> index b3554e7f9e76d..cd785111928bf 100644
>>> --- a/Documentation/devicetree/bindings/soc/imx/fsl,imx93-media-blk-ctrl.yaml
>>> +++ b/Documentation/devicetree/bindings/soc/imx/fsl,imx93-media-blk-ctrl.yaml
>>> @@ -24,6 +24,14 @@ properties:
>>>    reg:
>>>      maxItems: 1
>>>  
>>> +  ranges: true
>>> +
>>> +  '#address-cells':
>>> +    const: 1
>>> +
>>> +  '#size-cells':
>>> +    const: 1
>>> +
>>>    '#power-domain-cells':
>>>      const: 1
>>>  
>>> @@ -46,9 +54,20 @@ properties:
>>>        - const: csi
>>>        - const: dsi
>>>  
>>> +  bridge@20:
>>
>> @20 looks wrong. Use 'ranges;' and try again your DTS...
>>
>> Binding is supposed to be complete. We have several examples when people
>> added children one-by-one, everytime with different reasoning about
>> child addressing.
>>
>> So please confirm: this is complete and no other children will ever be
>> added here... or you are 100% sure that all future children will be
>> unit-addressable (will have unit address and appropriate properties).
> 
> This block control is a collection of registers for different purposes:
> * MIPI-DSI
> * MIPI-CSI
> * Parallel camera
> * LVDS
> * CAMERA_MUX
> 
> At lease for parallel camera, another subnode is expected ([1]).


That one at least have MMIO as well. No I wonder whether the others will
come without MMIO one day.

Why this cannot be sent all at once? Entire device binding?

> 
> [1] https://lore.kernel.org/all/20240819024001.850065-1-victor.liu@nxp.com/
> 
>> BTW, I don't quite get why this is both syscon and has translation for
>> child addresses. Does it mean your child does not use the same MMIO as
>> parent, thus leading to unsynchronized reg access?
> 
> I'm not sure what the best practices are. This LDB has two registers
> inside this block. So it seems reasonable to me to indicate this using

reg is fine, but you added ranges which means there is translation
between child addressing and parent MMIO. Usually, although not always,
when child is expected to use parent's syscon, you do not have ranges,
because the syscon deals with that translation.

What's more, your @20 means you depend on one specific value of ranges.

I would just skip the ranges and claim there is no direct mapping of
addresses. Reg defines offsets within this device addressing and this
device will handle it.

> a reg property. On the other hand, access is solely done by accessing
> via syscon, so unsynchronized reg access is not an issue.
> 
> What I am getting from your comments this node should not have 'reg'
> property, as it uses syscon anyway.


> 
>>> +    type: object
>>> +    additionalProperties: true
>>> +    properties:
>>> +      compatible:
>>> +        contains:
>>> +          const: fsl,imx93-ldb
>>> +
>>>  required:
>>>    - compatible
>>>    - reg
>>> +  - ranges
>>> +  - '#address-cells'
>>> +  - '#size-cells'
>>>    - power-domains
>>>    - clocks
>>>    - clock-names
>>> @@ -77,4 +96,36 @@ examples:
>>>                 clock-names = "apb", "axi", "nic", "disp", "cam",
>>>                               "pxp", "lcdif", "isi", "csi", "dsi";
>>>        #power-domain-cells = <1>;
>>> +      #address-cells = <1>;
>>> +      #size-cells = <1>;
>>> +      ranges = <0x0 0x4ac10000 0x10000>;
>>> +
>>> +      bridge@20 {
>>> +          compatible = "fsl,imx93-ldb";
>>> +          reg = <0x20 0x4>, <0x24 0x4>;
>>> +          reg-names = "ldb", "lvds";
>>> +          clocks = <&clk IMX93_CLK_LVDS_GATE>;
>>> +          clock-names = "ldb";
>>> +
>>> +          ports {
>>> +              #address-cells = <1>;
>>> +              #size-cells = <0>;
>>> +
>>> +              port@0 {
>>> +                  reg = <0>;
>>> +
>>> +                  ldb_from_lcdif2: endpoint {
>>> +                      remote-endpoint = <&lcdif2_to_ldb>;
>>> +                  };
>>> +              };
>>> +
>>> +              port@1 {
>>> +                  reg = <1>;
>>> +
>>> +                  ldb_lvds: endpoint {
>>> +                      remote-endpoint = <&ldb_to_panel>;
>>> +                  };
>>> +              };
>>> +          };
>>
>> Messed indentation.
> 
> This is already from the original binding. I'll fix in a separate commit.

How? I did not see any '-', only adding here.


Best regards,
Krzysztof

Powered by blists - more mailing lists