| Message ID | 1715679210-9588-4-git-send-email-shengjiu.wang@nxp.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | clk: imx: clk-audiomix: Improvement for audiomix | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Tue, May 14, 2024 at 05:33:27PM +0800, Shengjiu Wang wrote: > The Audio Block Control contains clock distribution and gating > controls, as well as reset handling to several of the AUDIOMIX > peripherals. Especially the reset controls for Enhanced Audio > Return Channel (EARC) PHY and Controller. > > So Audio Block Control is a Multi-Function Devices. > > Add reset-controller sub node which is a reset provider for EARC. > Add compatible string "syscon", "simple-mfd" which make Audio > Block Control device support reset-controller sub-node. > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> > --- > .../bindings/clock/imx8mp-audiomix.yaml | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > index 0a6dc1a6e122..a403ace4d11f 100644 > --- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > +++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > @@ -15,7 +15,10 @@ description: | > > properties: > compatible: > - const: fsl,imx8mp-audio-blk-ctrl > + items: > + - const: fsl,imx8mp-audio-blk-ctrl > + - const: syscon > + - const: simple-mfd > > reg: > maxItems: 1 > @@ -44,6 +47,11 @@ properties: > ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8mp-clock.h > for the full list of i.MX8MP IMX8MP_CLK_AUDIOMIX_ clock IDs. > > + reset-controller: > + type: object > + $ref: /schemas/reset/fsl,imx8mp-audiomix-reset.yaml# > + description: The child reset devices of AudioMIX Block Control. Why not just set #reset-cells = <1> in the existing node? IIRC it was already suggested to you to do that and use auxdev to set up the reset driver. Cheers, Conor. > + > required: > - compatible > - reg > @@ -60,7 +68,7 @@ examples: > #include <dt-bindings/clock/imx8mp-clock.h> > > clock-controller@30e20000 { > - compatible = "fsl,imx8mp-audio-blk-ctrl"; > + compatible = "fsl,imx8mp-audio-blk-ctrl", "syscon", "simple-mfd"; > reg = <0x30e20000 0x10000>; > #clock-cells = <1>; > clocks = <&clk IMX8MP_CLK_AUDIO_ROOT>, > @@ -74,6 +82,11 @@ examples: > "sai1", "sai2", "sai3", > "sai5", "sai6", "sai7"; > power-domains = <&pgc_audio>; > + > + reset-controller { > + compatible = "fsl,imx8mp-audiomix-reset"; > + #reset-cells = <1>; > + }; > }; > > ... > -- > 2.34.1 >
Quoting Conor Dooley (2024-05-14 11:06:14) > On Tue, May 14, 2024 at 05:33:27PM +0800, Shengjiu Wang wrote: > > diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > index 0a6dc1a6e122..a403ace4d11f 100644 > > --- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > +++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > @@ -15,7 +15,10 @@ description: | > > > > properties: > > compatible: > > - const: fsl,imx8mp-audio-blk-ctrl > > + items: > > + - const: fsl,imx8mp-audio-blk-ctrl > > + - const: syscon > > + - const: simple-mfd > > > > reg: > > maxItems: 1 > > @@ -44,6 +47,11 @@ properties: > > ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8mp-clock.h > > for the full list of i.MX8MP IMX8MP_CLK_AUDIOMIX_ clock IDs. > > > > + reset-controller: > > + type: object > > + $ref: /schemas/reset/fsl,imx8mp-audiomix-reset.yaml# > > + description: The child reset devices of AudioMIX Block Control. > > Why not just set #reset-cells = <1> in the existing node? IIRC it was > already suggested to you to do that and use auxdev to set up the reset > driver. Yes, do that.
On Wed, May 15, 2024 at 5:09 AM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Conor Dooley (2024-05-14 11:06:14) > > On Tue, May 14, 2024 at 05:33:27PM +0800, Shengjiu Wang wrote: > > > diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > index 0a6dc1a6e122..a403ace4d11f 100644 > > > --- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > +++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > @@ -15,7 +15,10 @@ description: | > > > > > > properties: > > > compatible: > > > - const: fsl,imx8mp-audio-blk-ctrl > > > + items: > > > + - const: fsl,imx8mp-audio-blk-ctrl > > > + - const: syscon > > > + - const: simple-mfd > > > > > > reg: > > > maxItems: 1 > > > @@ -44,6 +47,11 @@ properties: > > > ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8mp-clock.h > > > for the full list of i.MX8MP IMX8MP_CLK_AUDIOMIX_ clock IDs. > > > > > > + reset-controller: > > > + type: object > > > + $ref: /schemas/reset/fsl,imx8mp-audiomix-reset.yaml# > > > + description: The child reset devices of AudioMIX Block Control. > > > > Why not just set #reset-cells = <1> in the existing node? IIRC it was > > already suggested to you to do that and use auxdev to set up the reset > > driver. > > Yes, do that. Can I know why sub nodes can't be used? the relationship of parent and child devices looks better with sub nodes. A further question is can I use the reset-ti-syscon? which is a generic reset device for SoCs. with it I don't even need to write a new reset device driver. it is more simple. Best regards Shengjiu Wang
On Wed, May 15, 2024 at 10:47 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote: > > On Wed, May 15, 2024 at 5:09 AM Stephen Boyd <sboyd@kernel.org> wrote: > > > > Quoting Conor Dooley (2024-05-14 11:06:14) > > > On Tue, May 14, 2024 at 05:33:27PM +0800, Shengjiu Wang wrote: > > > > diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > > index 0a6dc1a6e122..a403ace4d11f 100644 > > > > --- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > > +++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > > @@ -15,7 +15,10 @@ description: | > > > > > > > > properties: > > > > compatible: > > > > - const: fsl,imx8mp-audio-blk-ctrl > > > > + items: > > > > + - const: fsl,imx8mp-audio-blk-ctrl > > > > + - const: syscon > > > > + - const: simple-mfd > > > > > > > > reg: > > > > maxItems: 1 > > > > @@ -44,6 +47,11 @@ properties: > > > > ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8mp-clock.h > > > > for the full list of i.MX8MP IMX8MP_CLK_AUDIOMIX_ clock IDs. > > > > > > > > + reset-controller: > > > > + type: object > > > > + $ref: /schemas/reset/fsl,imx8mp-audiomix-reset.yaml# > > > > + description: The child reset devices of AudioMIX Block Control. > > > > > > Why not just set #reset-cells = <1> in the existing node? IIRC it was > > > already suggested to you to do that and use auxdev to set up the reset > > > driver. > > > > Yes, do that. > > Can I know why sub nodes can't be used? the relationship of parent and > child devices looks better with sub nodes. > > A further question is can I use the reset-ti-syscon? which is a generic reset > device for SoCs. with it I don't even need to write a new reset device driver. > it is more simple. > The document link is: https://www.kernel.org/doc/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt Then example is: examples: # Clock Control Module node: - | #include <dt-bindings/clock/imx8mp-clock.h> #include <dt-bindings/reset/ti-syscon.h> clock-controller@30e20000 { compatible = "fsl,imx8mp-audio-blk-ctrl", "syscon", "simple-mfd"; reg = <0x30e20000 0x10000>; #clock-cells = <1>; clocks = <&clk IMX8MP_CLK_AUDIO_ROOT>, <&clk IMX8MP_CLK_SAI1>, <&clk IMX8MP_CLK_SAI2>, <&clk IMX8MP_CLK_SAI3>, <&clk IMX8MP_CLK_SAI5>, <&clk IMX8MP_CLK_SAI6>, <&clk IMX8MP_CLK_SAI7>; clock-names = "ahb", "sai1", "sai2", "sai3", "sai5", "sai6", "sai7"; power-domains = <&pgc_audio>; reset-controller { compatible = "ti,syscon-reset"; #reset-cells = <1>; ti,reset-bits = < 0x200 0 0x200 0 0 0 (ASSERT_CLEAR | DEASSERT_SET | STATUS_NONE) 0x200 1 0x200 1 0 0 (ASSERT_CLEAR | DEASSERT_SET | STATUS_NONE) >; }; }; Best regards Shengjiu Wang
On Wed, May 15, 2024 at 10:47:57AM +0800, Shengjiu Wang wrote: > On Wed, May 15, 2024 at 5:09 AM Stephen Boyd <sboyd@kernel.org> wrote: > > > > Quoting Conor Dooley (2024-05-14 11:06:14) > > > On Tue, May 14, 2024 at 05:33:27PM +0800, Shengjiu Wang wrote: > > > > diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > > index 0a6dc1a6e122..a403ace4d11f 100644 > > > > --- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > > +++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > > @@ -15,7 +15,10 @@ description: | > > > > > > > > properties: > > > > compatible: > > > > - const: fsl,imx8mp-audio-blk-ctrl > > > > + items: > > > > + - const: fsl,imx8mp-audio-blk-ctrl > > > > + - const: syscon > > > > + - const: simple-mfd > > > > > > > > reg: > > > > maxItems: 1 > > > > @@ -44,6 +47,11 @@ properties: > > > > ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8mp-clock.h > > > > for the full list of i.MX8MP IMX8MP_CLK_AUDIOMIX_ clock IDs. > > > > > > > > + reset-controller: > > > > + type: object > > > > + $ref: /schemas/reset/fsl,imx8mp-audiomix-reset.yaml# > > > > + description: The child reset devices of AudioMIX Block Control. > > > > > > Why not just set #reset-cells = <1> in the existing node? IIRC it was > > > already suggested to you to do that and use auxdev to set up the reset > > > driver. > > > > Yes, do that. > > Can I know why sub nodes can't be used? the relationship of parent and > child devices looks better with sub nodes. That's pretty subjective. I don't think it looks better to have a clock node that is also a syscon with a reset child node as it is rather inconsistent. > > A further question is can I use the reset-ti-syscon? which is a generic reset > device for SoCs. with it I don't even need to write a new reset device driver. > it is more simple. That is for a TI SoC. You're working on an imx. I don't think that you should be using that...
On Wed, May 15, 2024 at 05:04:48PM +0100, Conor Dooley wrote: > On Wed, May 15, 2024 at 10:47:57AM +0800, Shengjiu Wang wrote: > > On Wed, May 15, 2024 at 5:09 AM Stephen Boyd <sboyd@kernel.org> wrote: > > > > > > Quoting Conor Dooley (2024-05-14 11:06:14) > > > > On Tue, May 14, 2024 at 05:33:27PM +0800, Shengjiu Wang wrote: > > > > > diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > > > index 0a6dc1a6e122..a403ace4d11f 100644 > > > > > --- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > > > +++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > > > @@ -15,7 +15,10 @@ description: | > > > > > > > > > > properties: > > > > > compatible: > > > > > - const: fsl,imx8mp-audio-blk-ctrl > > > > > + items: > > > > > + - const: fsl,imx8mp-audio-blk-ctrl > > > > > + - const: syscon > > > > > + - const: simple-mfd > > > > > > > > > > reg: > > > > > maxItems: 1 > > > > > @@ -44,6 +47,11 @@ properties: > > > > > ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8mp-clock.h > > > > > for the full list of i.MX8MP IMX8MP_CLK_AUDIOMIX_ clock IDs. > > > > > > > > > > + reset-controller: > > > > > + type: object > > > > > + $ref: /schemas/reset/fsl,imx8mp-audiomix-reset.yaml# > > > > > + description: The child reset devices of AudioMIX Block Control. > > > > > > > > Why not just set #reset-cells = <1> in the existing node? IIRC it was > > > > already suggested to you to do that and use auxdev to set up the reset > > > > driver. > > > > > > Yes, do that. > > > > Can I know why sub nodes can't be used? the relationship of parent and > > child devices looks better with sub nodes. > > That's pretty subjective. I don't think it looks better to have a clock > node that is also a syscon with a reset child node as it is rather > inconsistent. I think it is multi function device syscon node. it should be like mfd { clock { ... } reset { ... } } clock and reset are difference device node with totally difference's compatible string. > > > > A further question is can I use the reset-ti-syscon? which is a generic reset > > device for SoCs. with it I don't even need to write a new reset device driver. > > it is more simple. > > That is for a TI SoC. You're working on an imx. I don't think that you > should be using that... I think this statement violate the linux basic reuse prinicple. If the code logic are the same why need duplicate it just because it is difference company. Of coures, if it is generic enough, it'd better to add a more generic compatible string. Frank
On Wed, May 15, 2024 at 02:28:51PM -0400, Frank Li wrote: > On Wed, May 15, 2024 at 05:04:48PM +0100, Conor Dooley wrote: > > On Wed, May 15, 2024 at 10:47:57AM +0800, Shengjiu Wang wrote: > > > On Wed, May 15, 2024 at 5:09 AM Stephen Boyd <sboyd@kernel.org> wrote: > > > > > > > > Quoting Conor Dooley (2024-05-14 11:06:14) > > > > > On Tue, May 14, 2024 at 05:33:27PM +0800, Shengjiu Wang wrote: > > > > > > diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > > > > index 0a6dc1a6e122..a403ace4d11f 100644 > > > > > > --- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > > > > +++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > > > > @@ -15,7 +15,10 @@ description: | > > > > > > > > > > > > properties: > > > > > > compatible: > > > > > > - const: fsl,imx8mp-audio-blk-ctrl > > > > > > + items: > > > > > > + - const: fsl,imx8mp-audio-blk-ctrl > > > > > > + - const: syscon > > > > > > + - const: simple-mfd > > > > > > > > > > > > reg: > > > > > > maxItems: 1 > > > > > > @@ -44,6 +47,11 @@ properties: > > > > > > ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8mp-clock.h > > > > > > for the full list of i.MX8MP IMX8MP_CLK_AUDIOMIX_ clock IDs. > > > > > > > > > > > > + reset-controller: > > > > > > + type: object > > > > > > + $ref: /schemas/reset/fsl,imx8mp-audiomix-reset.yaml# > > > > > > + description: The child reset devices of AudioMIX Block Control. > > > > > > > > > > Why not just set #reset-cells = <1> in the existing node? IIRC it was > > > > > already suggested to you to do that and use auxdev to set up the reset > > > > > driver. > > > > > > > > Yes, do that. > > > > > > Can I know why sub nodes can't be used? the relationship of parent and > > > child devices looks better with sub nodes. > > > > That's pretty subjective. I don't think it looks better to have a clock > > node that is also a syscon with a reset child node as it is rather > > inconsistent. > > I think it is multi function device syscon node. it should be like > > mfd > { > clock > { > ... > } > > reset > { > ... > } > } > > clock and reset are difference device node with totally difference's > compatible string. Which is I suspect is gonna require a change to your clock driver, because the range in the existing clock nodes: audio_blk_ctrl: clock-controller@30e20000 { compatible = "fsl,imx8mp-audio-blk-ctrl"; reg = <0x30e20000 0x10000>; }; would then have to move to the mfd parent node, and your clock child would have a reg property that overlaps the reset region. You'd need to then define a new binding that splits the range in two - obviously doable, but significantly more work and more disruptive than using an auxdev. > > > A further question is can I use the reset-ti-syscon? which is a generic reset > > > device for SoCs. with it I don't even need to write a new reset device driver. > > > it is more simple. > > > > That is for a TI SoC. You're working on an imx. I don't think that you > > should be using that... > > I think this statement violate the linux basic reuse prinicple. If the > code logic are the same why need duplicate it just because it is difference > company. Of coures, if it is generic enough, it'd better to add a more > generic compatible string. That's true, but I suspect it only works because only through (ab)use of the ti,reset-bits property not because you're actually compatible with TI's reset hardware. Cheers, Conor.
Quoting Shengjiu Wang (2024-05-14 19:47:57) > On Wed, May 15, 2024 at 5:09 AM Stephen Boyd <sboyd@kernel.org> wrote: > > > > Quoting Conor Dooley (2024-05-14 11:06:14) > > > On Tue, May 14, 2024 at 05:33:27PM +0800, Shengjiu Wang wrote: > > > > diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > > index 0a6dc1a6e122..a403ace4d11f 100644 > > > > --- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > > +++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > > @@ -15,7 +15,10 @@ description: | > > > > > > > > properties: > > > > compatible: > > > > - const: fsl,imx8mp-audio-blk-ctrl > > > > + items: > > > > + - const: fsl,imx8mp-audio-blk-ctrl > > > > + - const: syscon > > > > + - const: simple-mfd > > > > > > > > reg: > > > > maxItems: 1 > > > > @@ -44,6 +47,11 @@ properties: > > > > ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8mp-clock.h > > > > for the full list of i.MX8MP IMX8MP_CLK_AUDIOMIX_ clock IDs. > > > > > > > > + reset-controller: > > > > + type: object > > > > + $ref: /schemas/reset/fsl,imx8mp-audiomix-reset.yaml# > > > > + description: The child reset devices of AudioMIX Block Control. > > > > > > Why not just set #reset-cells = <1> in the existing node? IIRC it was > > > already suggested to you to do that and use auxdev to set up the reset > > > driver. > > > > Yes, do that. > > Can I know why sub nodes can't be used? the relationship of parent and > child devices looks better with sub nodes. Don't make nodes for drivers. Only make nodes for devices. This device is multi-function, but is still only a single device.
On Thu, May 16, 2024 at 06:15:18PM +0100, Conor Dooley wrote: > On Wed, May 15, 2024 at 02:28:51PM -0400, Frank Li wrote: > > On Wed, May 15, 2024 at 05:04:48PM +0100, Conor Dooley wrote: > > > On Wed, May 15, 2024 at 10:47:57AM +0800, Shengjiu Wang wrote: > > > > On Wed, May 15, 2024 at 5:09 AM Stephen Boyd <sboyd@kernel.org> wrote: > > > > > > > > > > Quoting Conor Dooley (2024-05-14 11:06:14) > > > > > > On Tue, May 14, 2024 at 05:33:27PM +0800, Shengjiu Wang wrote: > > > > > > > diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > > > > > index 0a6dc1a6e122..a403ace4d11f 100644 > > > > > > > --- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > > > > > +++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml > > > > > > > @@ -15,7 +15,10 @@ description: | > > > > > > > > > > > > > > properties: > > > > > > > compatible: > > > > > > > - const: fsl,imx8mp-audio-blk-ctrl > > > > > > > + items: > > > > > > > + - const: fsl,imx8mp-audio-blk-ctrl > > > > > > > + - const: syscon > > > > > > > + - const: simple-mfd > > > > > > > > > > > > > > reg: > > > > > > > maxItems: 1 > > > > > > > @@ -44,6 +47,11 @@ properties: > > > > > > > ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8mp-clock.h > > > > > > > for the full list of i.MX8MP IMX8MP_CLK_AUDIOMIX_ clock IDs. > > > > > > > > > > > > > > + reset-controller: > > > > > > > + type: object > > > > > > > + $ref: /schemas/reset/fsl,imx8mp-audiomix-reset.yaml# > > > > > > > + description: The child reset devices of AudioMIX Block Control. > > > > > > > > > > > > Why not just set #reset-cells = <1> in the existing node? IIRC it was > > > > > > already suggested to you to do that and use auxdev to set up the reset > > > > > > driver. > > > > > > > > > > Yes, do that. > > > > > > > > Can I know why sub nodes can't be used? the relationship of parent and > > > > child devices looks better with sub nodes. > > > > > > That's pretty subjective. I don't think it looks better to have a clock > > > node that is also a syscon with a reset child node as it is rather > > > inconsistent. > > > > I think it is multi function device syscon node. it should be like > > > > mfd > > { > > clock > > { > > ... > > } > > > > reset > > { > > ... > > } > > } > > > > clock and reset are difference device node with totally difference's > > compatible string. > > Which is I suspect is gonna require a change to your clock driver, > because the range in the existing clock nodes: > audio_blk_ctrl: clock-controller@30e20000 { > compatible = "fsl,imx8mp-audio-blk-ctrl"; > reg = <0x30e20000 0x10000>; > }; > would then have to move to the mfd parent node, and your clock child > would have a reg property that overlaps the reset region. You'd need to > then define a new binding that splits the range in two - obviously > doable, but significantly more work and more disruptive than using an > auxdev. I am new for auxdev. according to doc: https://docs.kernel.org/driver-api/auxiliary_bus.html "key requirement for utilizing the auxiliary bus is that there is no dependency on a physical bus, device, register accesses or regmap support. These individual devices split from the core cannot live on the platform bus as they are not physical devices that are controlled by DT/ACPI." ^^^^ ^^^ Look like it is easy to register auxdev "reset" devices. But I have a problem. How to use it by DT phandle? "reset" devices is service provider. Some client will use it. Generally, reset node will used by other devices nodes. like ABC: reset { compatible="simple-reset"; ... } other node will use "reset = <&ABC 0>". If use auxdev, how to get &ABC in dts file. > > > > > A further question is can I use the reset-ti-syscon? which is a generic reset > > > > device for SoCs. with it I don't even need to write a new reset device driver. > > > > it is more simple. > > > > > > That is for a TI SoC. You're working on an imx. I don't think that you > > > should be using that... > > > > I think this statement violate the linux basic reuse prinicple. If the > > code logic are the same why need duplicate it just because it is difference > > company. Of coures, if it is generic enough, it'd better to add a more > > generic compatible string. > > That's true, but I suspect it only works because only through (ab)use > of the ti,reset-bits property not because you're actually compatible > with TI's reset hardware. Reset's implement is very simple. Most design is similar in difference SOC. Just toggle a register bit. If regiser layout is the same, it should be compatible. this ti driver is suitable for most case. I think call it as simple-reset-syscon are more reasonable. > > Cheers, > Conor.
On Thu, May 16, 2024 at 11:56:27PM -0400, Frank Li wrote: > Look like it is easy to register auxdev "reset" devices. But I have a > problem. How to use it by DT phandle? "reset" devices is service provider. > Some client will use it. > > Generally, reset node will used by other devices nodes. like > > ABC: reset { > compatible="simple-reset"; > ... > } > > other node will use "reset = <&ABC 0>". If use auxdev, how to get &ABC > in dts file. Whether or not you use auxdev or any other method etc, does not matter in a DT system, the consumer will always have a phandle to the provider node: ABC: whatever { compatible = "whatever"; #clock-cells = <...>; #reset-cells = <...>; } something-else { clocks = <&ABC ...>; resets = <&ABC ...>; }
On Fri, May 17, 2024 at 05:21:32PM +0100, Conor Dooley wrote: > On Thu, May 16, 2024 at 11:56:27PM -0400, Frank Li wrote: > > > Look like it is easy to register auxdev "reset" devices. But I have a > > problem. How to use it by DT phandle? "reset" devices is service provider. > > Some client will use it. > > > > Generally, reset node will used by other devices nodes. like > > > > ABC: reset { > > compatible="simple-reset"; > > ... > > } > > > > other node will use "reset = <&ABC 0>". If use auxdev, how to get &ABC > > in dts file. > > Whether or not you use auxdev or any other method etc, does not matter > in a DT system, the consumer will always have a phandle to the provider > node: > > ABC: whatever { > compatible = "whatever"; > #clock-cells = <...>; > #reset-cells = <...>; > } > > something-else { > clocks = <&ABC ...>; > resets = <&ABC ...>; > } It goes back to old problem, "reset-cells" will be in "clock-controller". clock-controller@30e20000 { compatible = "fsl,imx8mp-audio-blk-ctrl", "syscon", "simple-mfd"; reg = <0x30e20000 0x10000>; ... #reset-cells = <...>; ^^^ }; If create new "whatever" auxdev bus driver which included two aux devices, (clock and reset). it will be similar with mfd. Still need change clock-controller@30e20000 drivers. "Which is I suspect is gonna require a change to your clock driver, because the range in the existing clock nodes: audio_blk_ctrl: clock-controller@30e20000 { compatible = "fsl,imx8mp-audio-blk-ctrl"; reg = <0x30e20000 0x10000>; }; would then have to move to the mfd parent node, and your clock child would have a reg property that overlaps the reset region. You'd need to then define a new binding that splits the range in two - obviously doable, but significantly more work and more disruptive than using an auxdev." So I don't know why auxdev will be better than mfd. A possible benefit may be that Auxdev needn't binding doc for clock and reset node devices. Frank Frank
On Fri, May 17, 2024 at 01:10:05PM -0400, Frank Li wrote: > On Fri, May 17, 2024 at 05:21:32PM +0100, Conor Dooley wrote: > > On Thu, May 16, 2024 at 11:56:27PM -0400, Frank Li wrote: > > > > > Look like it is easy to register auxdev "reset" devices. But I have a > > > problem. How to use it by DT phandle? "reset" devices is service provider. > > > Some client will use it. > > > > > > Generally, reset node will used by other devices nodes. like > > > > > > ABC: reset { > > > compatible="simple-reset"; > > > ... > > > } > > > > > > other node will use "reset = <&ABC 0>". If use auxdev, how to get &ABC > > > in dts file. > > > > Whether or not you use auxdev or any other method etc, does not matter > > in a DT system, the consumer will always have a phandle to the provider > > node: > > > > ABC: whatever { > > compatible = "whatever"; > > #clock-cells = <...>; > > #reset-cells = <...>; > > } > > > > something-else { > > clocks = <&ABC ...>; > > resets = <&ABC ...>; > > } > > > It goes back to old problem, "reset-cells" will be in "clock-controller". > > clock-controller@30e20000 { > compatible = "fsl,imx8mp-audio-blk-ctrl", "syscon", "simple-mfd"; > reg = <0x30e20000 0x10000>; > ... > > #reset-cells = <...>; > ^^^ > }; > > If create new "whatever" auxdev bus driver which included two aux devices, > (clock and reset). > > it will be similar with mfd. Still need change > clock-controller@30e20000 drivers. > > "Which is I suspect is gonna require a change to your clock driver, > because the range in the existing clock nodes: > audio_blk_ctrl: clock-controller@30e20000 { > compatible = "fsl,imx8mp-audio-blk-ctrl"; > reg = <0x30e20000 0x10000>; > }; > would then have to move to the mfd parent node, and your clock child > would have a reg property that overlaps the reset region. You'd need to > then define a new binding that splits the range in two - obviously > doable, but significantly more work and more disruptive than using an > auxdev." > > So I don't know why auxdev will be better than mfd. I think Stephen and I have spent enough time trying to explain why using auxdev is beneficial here. I, at least, won't be wasting any more of my (metaphorical) breath. > A possible benefit may be that Auxdev needn't binding doc for clock and > reset node devices.
On Fri, May 17, 2024 at 06:13:21PM +0100, Conor Dooley wrote: > On Fri, May 17, 2024 at 01:10:05PM -0400, Frank Li wrote: > > On Fri, May 17, 2024 at 05:21:32PM +0100, Conor Dooley wrote: > > > On Thu, May 16, 2024 at 11:56:27PM -0400, Frank Li wrote: > > > > > > > Look like it is easy to register auxdev "reset" devices. But I have a > > > > problem. How to use it by DT phandle? "reset" devices is service provider. > > > > Some client will use it. > > > > > > > > Generally, reset node will used by other devices nodes. like > > > > > > > > ABC: reset { > > > > compatible="simple-reset"; > > > > ... > > > > } > > > > > > > > other node will use "reset = <&ABC 0>". If use auxdev, how to get &ABC > > > > in dts file. > > > > > > Whether or not you use auxdev or any other method etc, does not matter > > > in a DT system, the consumer will always have a phandle to the provider > > > node: > > > > > > ABC: whatever { > > > compatible = "whatever"; > > > #clock-cells = <...>; > > > #reset-cells = <...>; > > > } > > > > > > something-else { > > > clocks = <&ABC ...>; > > > resets = <&ABC ...>; > > > } > > > > > > It goes back to old problem, "reset-cells" will be in "clock-controller". > > > > clock-controller@30e20000 { > > compatible = "fsl,imx8mp-audio-blk-ctrl", "syscon", "simple-mfd"; > > reg = <0x30e20000 0x10000>; > > ... > > > > #reset-cells = <...>; > > ^^^ > > }; > > > > If create new "whatever" auxdev bus driver which included two aux devices, > > (clock and reset). > > > > it will be similar with mfd. Still need change > > clock-controller@30e20000 drivers. > > > > "Which is I suspect is gonna require a change to your clock driver, > > because the range in the existing clock nodes: > > audio_blk_ctrl: clock-controller@30e20000 { > > compatible = "fsl,imx8mp-audio-blk-ctrl"; > > reg = <0x30e20000 0x10000>; > > }; > > would then have to move to the mfd parent node, and your clock child > > would have a reg property that overlaps the reset region. You'd need to > > then define a new binding that splits the range in two - obviously > > doable, but significantly more work and more disruptive than using an > > auxdev." > > > > So I don't know why auxdev will be better than mfd. > > I think Stephen and I have spent enough time trying to explain why using > auxdev is beneficial here. I, at least, won't be wasting any more of my > (metaphorical) breath. Thanks for your time. After I did more rearch, especially read your previous patch: https://lore.kernel.org/linux-clk/20240424-strangle-sharpener-34755c5e6e3e@spud/ I have better view about auxdev. Frank > > > A possible benefit may be that Auxdev needn't binding doc for clock and > > reset node devices. >
diff --git a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml index 0a6dc1a6e122..a403ace4d11f 100644 --- a/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml +++ b/Documentation/devicetree/bindings/clock/imx8mp-audiomix.yaml @@ -15,7 +15,10 @@ description: | properties: compatible: - const: fsl,imx8mp-audio-blk-ctrl + items: + - const: fsl,imx8mp-audio-blk-ctrl + - const: syscon + - const: simple-mfd reg: maxItems: 1 @@ -44,6 +47,11 @@ properties: ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx8mp-clock.h for the full list of i.MX8MP IMX8MP_CLK_AUDIOMIX_ clock IDs. + reset-controller: + type: object + $ref: /schemas/reset/fsl,imx8mp-audiomix-reset.yaml# + description: The child reset devices of AudioMIX Block Control. + required: - compatible - reg @@ -60,7 +68,7 @@ examples: #include <dt-bindings/clock/imx8mp-clock.h> clock-controller@30e20000 { - compatible = "fsl,imx8mp-audio-blk-ctrl"; + compatible = "fsl,imx8mp-audio-blk-ctrl", "syscon", "simple-mfd"; reg = <0x30e20000 0x10000>; #clock-cells = <1>; clocks = <&clk IMX8MP_CLK_AUDIO_ROOT>, @@ -74,6 +82,11 @@ examples: "sai1", "sai2", "sai3", "sai5", "sai6", "sai7"; power-domains = <&pgc_audio>; + + reset-controller { + compatible = "fsl,imx8mp-audiomix-reset"; + #reset-cells = <1>; + }; }; ...
The Audio Block Control contains clock distribution and gating controls, as well as reset handling to several of the AUDIOMIX peripherals. Especially the reset controls for Enhanced Audio Return Channel (EARC) PHY and Controller. So Audio Block Control is a Multi-Function Devices. Add reset-controller sub node which is a reset provider for EARC. Add compatible string "syscon", "simple-mfd" which make Audio Block Control device support reset-controller sub-node. Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> --- .../bindings/clock/imx8mp-audiomix.yaml | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)