devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] dt-bindings: soc: imx8mp-media-blk-ctrl: Add LDB into DT example
@ 2022-06-30 17:39 Marek Vasut
  2022-06-30 19:11 ` Krzysztof Kozlowski
  2022-06-30 23:28 ` Rob Herring
  0 siblings, 2 replies; 11+ messages in thread
From: Marek Vasut @ 2022-06-30 17:39 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Marek Vasut, Krzysztof Kozlowski, Laurent Pinchart, Lucas Stach,
	NXP Linux Team, Paul Elder, Peng Fan, Rob Herring, devicetree

Document the LDB bridge subnode and add the subnode into the example.
For the subnode to work, the block control must be compatible with
simple-mfd in addition to the existing compatibles.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: NXP Linux Team <linux-imx@nxp.com>
Cc: Paul Elder <paul.elder@ideasonboard.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
---
 .../soc/imx/fsl,imx8mp-media-blk-ctrl.yaml    | 54 ++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml b/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
index b246d8386ba4a..05a19d3229830 100644
--- a/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
+++ b/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
@@ -18,11 +18,18 @@ properties:
   compatible:
     items:
       - const: fsl,imx8mp-media-blk-ctrl
+      - const: simple-mfd
       - const: syscon
 
   reg:
     maxItems: 1
 
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 1
+
   '#power-domain-cells':
     const: 1
 
@@ -64,9 +71,16 @@ properties:
       - const: isp
       - const: phy
 
+  bridge@5c:
+    type: object
+    $ref: "/schemas/display/bridge/fsl,ldb.yaml#"
+    unevaluatedProperties: false
+
 required:
   - compatible
   - reg
+  - '#address-cells'
+  - '#size-cells'
   - '#power-domain-cells'
   - power-domains
   - power-domain-names
@@ -81,7 +95,7 @@ examples:
     #include <dt-bindings/power/imx8mp-power.h>
 
     media_blk_ctl: blk-ctl@32ec0000 {
-        compatible = "fsl,imx8mp-media-blk-ctrl", "syscon";
+        compatible = "fsl,imx8mp-media-blk-ctrl", "simple-mfd", "syscon";
         reg = <0x32ec0000 0x138>;
         power-domains = <&mediamix_pd>, <&mipi_phy1_pd>, <&mipi_phy1_pd>,
                         <&mediamix_pd>, <&mediamix_pd>, <&mipi_phy2_pd>,
@@ -100,5 +114,43 @@ examples:
         clock-names = "apb", "axi", "cam1", "cam2", "disp1", "disp2",
                       "isp", "phy";
         #power-domain-cells = <1>;
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        bridge@5c {
+            compatible = "fsl,imx8mp-ldb";
+            clocks = <&clk IMX8MP_CLK_MEDIA_LDB>;
+            clock-names = "ldb";
+            reg = <0x5c 0x4>, <0x128 0x4>;
+            reg-names = "ldb", "lvds";
+
+            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_ch0: endpoint {
+                        remote-endpoint = <&ldb_to_lvdsx4panel>;
+                    };
+                };
+
+                port@2 {
+                    reg = <2>;
+
+                    ldb_lvds_ch1: endpoint {
+                    };
+                };
+            };
+        };
     };
 ...
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] dt-bindings: soc: imx8mp-media-blk-ctrl: Add LDB into DT example
  2022-06-30 17:39 [PATCH] dt-bindings: soc: imx8mp-media-blk-ctrl: Add LDB into DT example Marek Vasut
@ 2022-06-30 19:11 ` Krzysztof Kozlowski
  2022-06-30 19:16   ` Marek Vasut
  2022-06-30 23:28 ` Rob Herring
  1 sibling, 1 reply; 11+ messages in thread
From: Krzysztof Kozlowski @ 2022-06-30 19:11 UTC (permalink / raw)
  To: Marek Vasut, linux-arm-kernel
  Cc: Laurent Pinchart, Lucas Stach, NXP Linux Team, Paul Elder,
	Peng Fan, Rob Herring, devicetree

On 30/06/2022 19:39, Marek Vasut wrote:
> Document the LDB bridge subnode and add the subnode into the example.
> For the subnode to work, the block control must be compatible with
> simple-mfd in addition to the existing compatibles.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: Paul Elder <paul.elder@ideasonboard.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
>  .../soc/imx/fsl,imx8mp-media-blk-ctrl.yaml    | 54 ++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml b/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
> index b246d8386ba4a..05a19d3229830 100644
> --- a/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
> +++ b/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
> @@ -18,11 +18,18 @@ properties:
>    compatible:
>      items:
>        - const: fsl,imx8mp-media-blk-ctrl
> +      - const: simple-mfd

Not really... simple-mfd means devices is really simple and you just use
it to instantiate children. However this is not simple - it's a power
domain controller with several clocks and power domains as input.

It's not a simple MFD, but a regular device.

>        - const: syscon
>  
>    reg:
>      maxItems: 1
>  
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 1
> +
>    '#power-domain-cells':
>      const: 1
>  
> @@ -64,9 +71,16 @@ properties:
>        - const: isp
>        - const: phy
>  
> +  bridge@5c:
> +    type: object
> +    $ref: "/schemas/display/bridge/fsl,ldb.yaml#"

Skip the quotes, especially that it's inconsistent with others (').

> +    unevaluatedProperties: false
> +



Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dt-bindings: soc: imx8mp-media-blk-ctrl: Add LDB into DT example
  2022-06-30 19:11 ` Krzysztof Kozlowski
@ 2022-06-30 19:16   ` Marek Vasut
  2022-06-30 19:31     ` Krzysztof Kozlowski
  0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2022-06-30 19:16 UTC (permalink / raw)
  To: Krzysztof Kozlowski, linux-arm-kernel
  Cc: Laurent Pinchart, Lucas Stach, NXP Linux Team, Paul Elder,
	Peng Fan, Rob Herring, devicetree

On 6/30/22 21:11, Krzysztof Kozlowski wrote:
> On 30/06/2022 19:39, Marek Vasut wrote:
>> Document the LDB bridge subnode and add the subnode into the example.
>> For the subnode to work, the block control must be compatible with
>> simple-mfd in addition to the existing compatibles.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Cc: NXP Linux Team <linux-imx@nxp.com>
>> Cc: Paul Elder <paul.elder@ideasonboard.com>
>> Cc: Peng Fan <peng.fan@nxp.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: devicetree@vger.kernel.org
>> ---
>>   .../soc/imx/fsl,imx8mp-media-blk-ctrl.yaml    | 54 ++++++++++++++++++-
>>   1 file changed, 53 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml b/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
>> index b246d8386ba4a..05a19d3229830 100644
>> --- a/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
>> +++ b/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
>> @@ -18,11 +18,18 @@ properties:
>>     compatible:
>>       items:
>>         - const: fsl,imx8mp-media-blk-ctrl
>> +      - const: simple-mfd
> 
> Not really... simple-mfd means devices is really simple and you just use
> it to instantiate children. However this is not simple - it's a power
> domain controller with several clocks and power domains as input.
> 
> It's not a simple MFD, but a regular device.

I don't understand this comment. The LDB bridge is literally two 
registers with a few bits in this media block controller register area. 
Can you expand on why the simple-mfd is unsuitable and what should it be 
instead ?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dt-bindings: soc: imx8mp-media-blk-ctrl: Add LDB into DT example
  2022-06-30 19:16   ` Marek Vasut
@ 2022-06-30 19:31     ` Krzysztof Kozlowski
  2022-06-30 19:50       ` Marek Vasut
  0 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Kozlowski @ 2022-06-30 19:31 UTC (permalink / raw)
  To: Marek Vasut, linux-arm-kernel
  Cc: Laurent Pinchart, Lucas Stach, NXP Linux Team, Paul Elder,
	Peng Fan, Rob Herring, devicetree

On 30/06/2022 21:16, Marek Vasut wrote:
> On 6/30/22 21:11, Krzysztof Kozlowski wrote:
>> On 30/06/2022 19:39, Marek Vasut wrote:
>>> Document the LDB bridge subnode and add the subnode into the example.
>>> For the subnode to work, the block control must be compatible with
>>> simple-mfd in addition to the existing compatibles.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Cc: Lucas Stach <l.stach@pengutronix.de>
>>> Cc: NXP Linux Team <linux-imx@nxp.com>
>>> Cc: Paul Elder <paul.elder@ideasonboard.com>
>>> Cc: Peng Fan <peng.fan@nxp.com>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: devicetree@vger.kernel.org
>>> ---
>>>   .../soc/imx/fsl,imx8mp-media-blk-ctrl.yaml    | 54 ++++++++++++++++++-
>>>   1 file changed, 53 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml b/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
>>> index b246d8386ba4a..05a19d3229830 100644
>>> --- a/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
>>> +++ b/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
>>> @@ -18,11 +18,18 @@ properties:
>>>     compatible:
>>>       items:
>>>         - const: fsl,imx8mp-media-blk-ctrl
>>> +      - const: simple-mfd
>>
>> Not really... simple-mfd means devices is really simple and you just use
>> it to instantiate children. However this is not simple - it's a power
>> domain controller with several clocks and power domains as input.
>>
>> It's not a simple MFD, but a regular device.
> 
> I don't understand this comment. The LDB bridge is literally two 
> registers with a few bits in this media block controller register area. 
> Can you expand on why the simple-mfd is unsuitable and what should it be 
> instead ?

Looking at the bindings you have there 10 power domains, 10 input clocks
and a domain provider. The driver is also not that simple which is
another argument that this is not simple-mfd. Simply, it is not simple.

What I meant, is that probably you should populate children from the
driver instead of adding simple-mfd compatible. Once you add simple-mfd,
you cannot remove it and children cannot use anything from the parent.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dt-bindings: soc: imx8mp-media-blk-ctrl: Add LDB into DT example
  2022-06-30 19:31     ` Krzysztof Kozlowski
@ 2022-06-30 19:50       ` Marek Vasut
  2022-07-01  0:09         ` Rob Herring
  0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2022-06-30 19:50 UTC (permalink / raw)
  To: Krzysztof Kozlowski, linux-arm-kernel
  Cc: Laurent Pinchart, Lucas Stach, NXP Linux Team, Paul Elder,
	Peng Fan, Rob Herring, devicetree

On 6/30/22 21:31, Krzysztof Kozlowski wrote:

Hi,

[...]

>>>> diff --git a/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml b/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
>>>> index b246d8386ba4a..05a19d3229830 100644
>>>> --- a/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
>>>> +++ b/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
>>>> @@ -18,11 +18,18 @@ properties:
>>>>      compatible:
>>>>        items:
>>>>          - const: fsl,imx8mp-media-blk-ctrl
>>>> +      - const: simple-mfd
>>>
>>> Not really... simple-mfd means devices is really simple and you just use
>>> it to instantiate children. However this is not simple - it's a power
>>> domain controller with several clocks and power domains as input.
>>>
>>> It's not a simple MFD, but a regular device.
>>
>> I don't understand this comment. The LDB bridge is literally two
>> registers with a few bits in this media block controller register area.
>> Can you expand on why the simple-mfd is unsuitable and what should it be
>> instead ?
> 
> Looking at the bindings you have there 10 power domains, 10 input clocks
> and a domain provider. The driver is also not that simple which is
> another argument that this is not simple-mfd. Simply, it is not simple.
> 
> What I meant, is that probably you should populate children from the
> driver instead of adding simple-mfd compatible. Once you add simple-mfd,
> you cannot remove it and children cannot use anything from the parent.

No, I don't think so.

The block controller provides those 10 power domains, those are separate 
things controlled by separate registers within the block control 
register space.

This LDB bridge are two more completely unrelated registers which have 
nothing to do with those power domains . They are just in the same 
register block because they had to put those registers somewhere. And 
they are mixed literally in the middle of the register block, because 
there was space it seems. Hence the simple-mfd is I think the right 
thing here.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dt-bindings: soc: imx8mp-media-blk-ctrl: Add LDB into DT example
  2022-06-30 17:39 [PATCH] dt-bindings: soc: imx8mp-media-blk-ctrl: Add LDB into DT example Marek Vasut
  2022-06-30 19:11 ` Krzysztof Kozlowski
@ 2022-06-30 23:28 ` Rob Herring
  1 sibling, 0 replies; 11+ messages in thread
From: Rob Herring @ 2022-06-30 23:28 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Paul Elder, Rob Herring, NXP Linux Team, Laurent Pinchart,
	Peng Fan, devicetree, linux-arm-kernel, Lucas Stach,
	Krzysztof Kozlowski

On Thu, 30 Jun 2022 19:39:22 +0200, Marek Vasut wrote:
> Document the LDB bridge subnode and add the subnode into the example.
> For the subnode to work, the block control must be compatible with
> simple-mfd in addition to the existing compatibles.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: Paul Elder <paul.elder@ideasonboard.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
>  .../soc/imx/fsl,imx8mp-media-blk-ctrl.yaml    | 54 ++++++++++++++++++-
>  1 file changed, 53 insertions(+), 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/soc/imx/fsl,imx8mp-media-blk-ctrl.example.dtb: blk-ctl@32ec0000: bridge@5c: 'reg', 'reg-names' do not match any of the regexes: 'pinctrl-[0-9]+'
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.example.dtb: blk-ctl@32ec0000: bridge@5c: Unevaluated properties are not allowed ('reg', 'reg-names' were unexpected)
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.example.dtb: bridge@5c: 'reg', 'reg-names' do not match any of the regexes: 'pinctrl-[0-9]+'
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/bridge/fsl,ldb.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.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dt-bindings: soc: imx8mp-media-blk-ctrl: Add LDB into DT example
  2022-06-30 19:50       ` Marek Vasut
@ 2022-07-01  0:09         ` Rob Herring
  2022-07-01  8:06           ` Marek Vasut
  0 siblings, 1 reply; 11+ messages in thread
From: Rob Herring @ 2022-07-01  0:09 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Krzysztof Kozlowski, linux-arm-kernel, Laurent Pinchart,
	Lucas Stach, NXP Linux Team, Paul Elder, Peng Fan, devicetree

On Thu, Jun 30, 2022 at 09:50:31PM +0200, Marek Vasut wrote:
> On 6/30/22 21:31, Krzysztof Kozlowski wrote:
> 
> Hi,
> 
> [...]
> 
> > > > > diff --git a/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml b/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
> > > > > index b246d8386ba4a..05a19d3229830 100644
> > > > > --- a/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
> > > > > +++ b/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
> > > > > @@ -18,11 +18,18 @@ properties:
> > > > >      compatible:
> > > > >        items:
> > > > >          - const: fsl,imx8mp-media-blk-ctrl
> > > > > +      - const: simple-mfd
> > > > 
> > > > Not really... simple-mfd means devices is really simple and you just use
> > > > it to instantiate children. However this is not simple - it's a power
> > > > domain controller with several clocks and power domains as input.
> > > > 
> > > > It's not a simple MFD, but a regular device.
> > > 
> > > I don't understand this comment. The LDB bridge is literally two
> > > registers with a few bits in this media block controller register area.
> > > Can you expand on why the simple-mfd is unsuitable and what should it be
> > > instead ?
> > 
> > Looking at the bindings you have there 10 power domains, 10 input clocks
> > and a domain provider. The driver is also not that simple which is
> > another argument that this is not simple-mfd. Simply, it is not simple.
> > 
> > What I meant, is that probably you should populate children from the
> > driver instead of adding simple-mfd compatible. Once you add simple-mfd,
> > you cannot remove it and children cannot use anything from the parent.
> 
> No, I don't think so.
> 
> The block controller provides those 10 power domains, those are separate
> things controlled by separate registers within the block control register
> space.
> 
> This LDB bridge are two more completely unrelated registers which have
> nothing to do with those power domains . They are just in the same register
> block because they had to put those registers somewhere. And they are mixed
> literally in the middle of the register block, because there was space it
> seems. Hence the simple-mfd is I think the right thing here.

Was 'simple-mfd' missing or LDB bridge wasn't a child node? I though 
this was a no functional change patch. Seems more than just update the 
example.

'simple-mfd' is saying the child has 0 dependence on the parent. IMO, 
'syscon' contradicts that, but that's an all to common pattern. You are 
saying all the clocks (or any other resources) in the parent can be off 
and the LDB bridge is still functional.

Rob

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dt-bindings: soc: imx8mp-media-blk-ctrl: Add LDB into DT example
  2022-07-01  0:09         ` Rob Herring
@ 2022-07-01  8:06           ` Marek Vasut
  2022-07-04  8:00             ` Krzysztof Kozlowski
  0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2022-07-01  8:06 UTC (permalink / raw)
  To: Rob Herring
  Cc: Krzysztof Kozlowski, linux-arm-kernel, Laurent Pinchart,
	Lucas Stach, NXP Linux Team, Paul Elder, Peng Fan, devicetree

On 7/1/22 02:09, Rob Herring wrote:
> On Thu, Jun 30, 2022 at 09:50:31PM +0200, Marek Vasut wrote:
>> On 6/30/22 21:31, Krzysztof Kozlowski wrote:
>>
>> Hi,
>>
>> [...]
>>
>>>>>> diff --git a/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml b/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
>>>>>> index b246d8386ba4a..05a19d3229830 100644
>>>>>> --- a/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-media-blk-ctrl.yaml
>>>>>> @@ -18,11 +18,18 @@ properties:
>>>>>>       compatible:
>>>>>>         items:
>>>>>>           - const: fsl,imx8mp-media-blk-ctrl
>>>>>> +      - const: simple-mfd
>>>>>
>>>>> Not really... simple-mfd means devices is really simple and you just use
>>>>> it to instantiate children. However this is not simple - it's a power
>>>>> domain controller with several clocks and power domains as input.
>>>>>
>>>>> It's not a simple MFD, but a regular device.
>>>>
>>>> I don't understand this comment. The LDB bridge is literally two
>>>> registers with a few bits in this media block controller register area.
>>>> Can you expand on why the simple-mfd is unsuitable and what should it be
>>>> instead ?
>>>
>>> Looking at the bindings you have there 10 power domains, 10 input clocks
>>> and a domain provider. The driver is also not that simple which is
>>> another argument that this is not simple-mfd. Simply, it is not simple.
>>>
>>> What I meant, is that probably you should populate children from the
>>> driver instead of adding simple-mfd compatible. Once you add simple-mfd,
>>> you cannot remove it and children cannot use anything from the parent.
>>
>> No, I don't think so.
>>
>> The block controller provides those 10 power domains, those are separate
>> things controlled by separate registers within the block control register
>> space.
>>
>> This LDB bridge are two more completely unrelated registers which have
>> nothing to do with those power domains . They are just in the same register
>> block because they had to put those registers somewhere. And they are mixed
>> literally in the middle of the register block, because there was space it
>> seems. Hence the simple-mfd is I think the right thing here.
> 
> Was 'simple-mfd' missing or LDB bridge wasn't a child node? I though
> this was a no functional change patch. Seems more than just update the
> example.

In the local patch I have to imx8mp.dtsi , the simple-mfd had to be 
added for the ldb bridge child node to bind.

> 'simple-mfd' is saying the child has 0 dependence on the parent. IMO,
> 'syscon' contradicts that, but that's an all to common pattern. You are
> saying all the clocks (or any other resources) in the parent can be off
> and the LDB bridge is still functional.

The bridge itself should work even if the block control is clocked off. 
The block controller apb (register) clock have to be enabled only when 
accessing these two LDB registers, which should be handled by runtime 
PM. Or what am I missing here ?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dt-bindings: soc: imx8mp-media-blk-ctrl: Add LDB into DT example
  2022-07-01  8:06           ` Marek Vasut
@ 2022-07-04  8:00             ` Krzysztof Kozlowski
  2022-07-08 15:32               ` Marek Vasut
  0 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Kozlowski @ 2022-07-04  8:00 UTC (permalink / raw)
  To: Marek Vasut, Rob Herring
  Cc: linux-arm-kernel, Laurent Pinchart, Lucas Stach, NXP Linux Team,
	Paul Elder, Peng Fan, devicetree

On 01/07/2022 10:06, Marek Vasut wrote:
> In the local patch I have to imx8mp.dtsi , the simple-mfd had to be 
> added for the ldb bridge child node to bind.
> 
>> 'simple-mfd' is saying the child has 0 dependence on the parent. IMO,
>> 'syscon' contradicts that, but that's an all to common pattern. You are
>> saying all the clocks (or any other resources) in the parent can be off
>> and the LDB bridge is still functional.
> 
> The bridge itself should work even if the block control is clocked off. 
> The block controller apb (register) clock have to be enabled only when 
> accessing these two LDB registers, which should be handled by runtime 
> PM. Or what am I missing here ?

If I understand correctly, you need the device's APB clocks to be
enabled to use the child device? You have a dependency between child and
parent, so it is not a simple-mfd.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dt-bindings: soc: imx8mp-media-blk-ctrl: Add LDB into DT example
  2022-07-04  8:00             ` Krzysztof Kozlowski
@ 2022-07-08 15:32               ` Marek Vasut
  2022-07-11  9:22                 ` Krzysztof Kozlowski
  0 siblings, 1 reply; 11+ messages in thread
From: Marek Vasut @ 2022-07-08 15:32 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Rob Herring
  Cc: linux-arm-kernel, Laurent Pinchart, Lucas Stach, NXP Linux Team,
	Paul Elder, Peng Fan, devicetree

On 7/4/22 10:00, Krzysztof Kozlowski wrote:
> On 01/07/2022 10:06, Marek Vasut wrote:
>> In the local patch I have to imx8mp.dtsi , the simple-mfd had to be
>> added for the ldb bridge child node to bind.
>>
>>> 'simple-mfd' is saying the child has 0 dependence on the parent. IMO,
>>> 'syscon' contradicts that, but that's an all to common pattern. You are
>>> saying all the clocks (or any other resources) in the parent can be off
>>> and the LDB bridge is still functional.
>>
>> The bridge itself should work even if the block control is clocked off.
>> The block controller apb (register) clock have to be enabled only when
>> accessing these two LDB registers, which should be handled by runtime
>> PM. Or what am I missing here ?
> 
> If I understand correctly, you need the device's APB clocks to be
> enabled to use the child device? You have a dependency between child and
> parent, so it is not a simple-mfd.

OK, then ... what is it ? Some sort of hint would be helpful.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] dt-bindings: soc: imx8mp-media-blk-ctrl: Add LDB into DT example
  2022-07-08 15:32               ` Marek Vasut
@ 2022-07-11  9:22                 ` Krzysztof Kozlowski
  0 siblings, 0 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2022-07-11  9:22 UTC (permalink / raw)
  To: Marek Vasut, Rob Herring
  Cc: linux-arm-kernel, Laurent Pinchart, Lucas Stach, NXP Linux Team,
	Paul Elder, Peng Fan, devicetree

On 08/07/2022 17:32, Marek Vasut wrote:
> On 7/4/22 10:00, Krzysztof Kozlowski wrote:
>> On 01/07/2022 10:06, Marek Vasut wrote:
>>> In the local patch I have to imx8mp.dtsi , the simple-mfd had to be
>>> added for the ldb bridge child node to bind.
>>>
>>>> 'simple-mfd' is saying the child has 0 dependence on the parent. IMO,
>>>> 'syscon' contradicts that, but that's an all to common pattern. You are
>>>> saying all the clocks (or any other resources) in the parent can be off
>>>> and the LDB bridge is still functional.
>>>
>>> The bridge itself should work even if the block control is clocked off.
>>> The block controller apb (register) clock have to be enabled only when
>>> accessing these two LDB registers, which should be handled by runtime
>>> PM. Or what am I missing here ?
>>
>> If I understand correctly, you need the device's APB clocks to be
>> enabled to use the child device? You have a dependency between child and
>> parent, so it is not a simple-mfd.
> 
> OK, then ... what is it ? Some sort of hint would be helpful.

I don't have the hardware manual to know what is it exactly. Some device
which has its own bindings and fsl,imx8mp-media-blk-ctrl compatible.

You already received hint what should you do with that device....
"What I meant, is that probably you should populate children from the
driver instead of adding simple-mfd compatible."


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-07-11  9:45 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-30 17:39 [PATCH] dt-bindings: soc: imx8mp-media-blk-ctrl: Add LDB into DT example Marek Vasut
2022-06-30 19:11 ` Krzysztof Kozlowski
2022-06-30 19:16   ` Marek Vasut
2022-06-30 19:31     ` Krzysztof Kozlowski
2022-06-30 19:50       ` Marek Vasut
2022-07-01  0:09         ` Rob Herring
2022-07-01  8:06           ` Marek Vasut
2022-07-04  8:00             ` Krzysztof Kozlowski
2022-07-08 15:32               ` Marek Vasut
2022-07-11  9:22                 ` Krzysztof Kozlowski
2022-06-30 23:28 ` Rob Herring

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).