[Question] MFD driver that handles clocks/resets and populates child nodes

[Question] MFD driver that handles clocks/resets and populates child nodes

[Masahiro Yamada]

Hi.


Some MFD drivers populate child nodes
after some basic hardware setups.


My question is, is it acceptable to upstream
a MFD driver that handles only clocks/resets,
then calls of_platform_populate.


The probe function will look like follows:


static int uniphier_usb3_glue_probe(struct platform_device *pdev)
{
        [get some clocks];

        [get some resets];

        [enable some clocks];

        [deassert some resets];

        return devm_of_platform_populate(dev);
}


With this driver, the child devices do not need
to handle clocks/resets.
But, I am not sure if it is the right thing to do.



Background of this question
---------------------------

Socionext is trying to upstream
the glue layer for DWC3 USB IP.

The original patch is this.
https://patchwork.kernel.org/patch/10180167/

This driver enables clocks, deassert resets,
and sets up some more registers,
then populates the DWC3 core node.

The maintainer of DWC3, Felipe Balbi, requested to
split the glue layer driver into small parts such as
reset, regulator, phy, etc.

So, the node will look like follows:

   usb-glue {
           reset {
                   ...
           };

           regulator {
                   ...
           };

           hs-phy {
                   ...
           };

           ss-phy {
                   ...
           };
   }

Here, one question popped up.
Where should the hardware resources be described?
The register, clocks, resets are shared among the sub-nodes.
This is the obvious result since it is a single hardware block
in the first place, and we are splitting it into small chunks.


If the MFD driver approach is acceptable,
clocks, resets will be handles in the parent node.
(Such a driver looks stupid, though)


   usb-glue {
           compatible = "socionext,uniphier-ld20-usb3-glue",
                        "syscon";
           reg = <0x65b00000 0x1000>;
           clocks = <sys_clk 14>;
           resets = <sys_clk 14>;

           reset {
                   ...
           };

           regulator {
                   ...
           };

           hs-phy {
                   ...
           };

           ss-phy {
                   ...
           };
   }


Of course, it is possible to use "simple-mfd"
and each driver can repeat the same clock/reset like follows.
(this also looks a bit clumsy...)

   usb-glue {
           compatible = "socionext,uniphier-ld20-usb3-glue",
                        "simple-mfd", "syscon";
           reg = <0x65b00000 0x1000>;

           reset {
                   clocks = <sys_clk 14>;
                   resets = <sys_clk 14>;
                   ...
           };

           regulator {
                   clocks = <sys_clk 14>;
                   resets = <sys_clk 14>;
                   ...
           };

           hs-phy {
                   clocks = <sys_clk 14>;
                   resets = <sys_clk 14>;
                   ...
           };

           ss-phy {
                   clocks = <sys_clk 14>;
                   resets = <sys_clk 14>;
                   ...
           };
   }


Which is better?
Or any other good idea?

Thanks.


-- 
Best Regards
Masahiro Yamada

Re: [Question] MFD driver that handles clocks/resets and populates child nodes

[Andrew Lunn]

> The maintainer of DWC3, Felipe Balbi, requested to
> split the glue layer driver into small parts such as
> reset, regulator, phy, etc.

What exactly did Felipe ask for? Did he ask that the patch be split
up, one patch per reset, regulator, phy etc?

Are all these resources used just by the DWC3? Or is it a true MFD,
multiple functions?

	 Andrew
 

Re: [Question] MFD driver that handles clocks/resets and populates child nodes

[Masahiro Yamada]

2018-04-02 21:04 GMT+09:00 Andrew Lunn <andrew@lunn.ch>:
>> The maintainer of DWC3, Felipe Balbi, requested to
>> split the glue layer driver into small parts such as
>> reset, regulator, phy, etc.
>
> What exactly did Felipe ask for? Did he ask that the patch be split
> up, one patch per reset, regulator, phy etc?


Yeah.  That is what we understood from his comments.


These are the feed-backs from him.

https://lkml.org/lkml/2018/1/23/298
https://lkml.org/lkml/2018/1/24/352




> Are all these resources used just by the DWC3? Or is it a true MFD,
> multiple functions?

I do not think this is a real MFD.

This is a DWC3 glue layer, i.e.
a collection of misc registers that control
the DWC3 IP.


Just splitting it into small pieces
to use PHY, reset, regulator framework in Linux.

Of course, the price of this approach
is so cluttered Device Tree,
honestly I do not like it much.



>
>          Andrew
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Regards
Masahiro Yamada

Re: [Question] MFD driver that handles clocks/resets and populates child nodes

[Andrew Lunn]

On Mon, Apr 02, 2018 at 10:21:01PM +0900, Masahiro Yamada wrote:
> 2018-04-02 21:04 GMT+09:00 Andrew Lunn <andrew@lunn.ch>:
> >> The maintainer of DWC3, Felipe Balbi, requested to
> >> split the glue layer driver into small parts such as
> >> reset, regulator, phy, etc.
> >
> > What exactly did Felipe ask for? Did he ask that the patch be split
> > up, one patch per reset, regulator, phy etc?
> 
> 
> Yeah.  That is what we understood from his comments.
> 
> 
> These are the feed-backs from him.
> 
> https://lkml.org/lkml/2018/1/23/298
> https://lkml.org/lkml/2018/1/24/352

> > Are all these resources used just by the DWC3? Or is it a true MFD,
> > multiple functions?
> 
> I do not think this is a real MFD.
> 
> This is a DWC3 glue layer, i.e.
> a collection of misc registers that control
> the DWC3 IP.
> 
> 
> Just splitting it into small pieces
> to use PHY, reset, regulator framework in Linux.
> 
> Of course, the price of this approach
> is so cluttered Device Tree,
> honestly I do not like it much.

This however the correct way to do this. You should have a phy driver,
and a regulator driver, and a reset driver. The DWC3 then uses
phandles to these drivers.

How is the IO map area 65b00000 split up. Can you cleanly separate it
into sub areas, which do not overlap, so you have a sub-area for the
PHY driver, a sub-area for the regulator driver and a sub-area for the
reset area? If you can cleanly split it up, you don't need an MFD. If
however the registers are in overlapping areas, you do need an
MFD. The MFD core provides access to the registers, while its children
implement PHY, reset, regulator etc.

	  Andrew

Re: [Question] MFD driver that handles clocks/resets and populates child nodes

[Lee Jones]

On Mon, 02 Apr 2018, Andrew Lunn wrote:

> On Mon, Apr 02, 2018 at 10:21:01PM +0900, Masahiro Yamada wrote:
> > 2018-04-02 21:04 GMT+09:00 Andrew Lunn <andrew@lunn.ch>:
> > >> The maintainer of DWC3, Felipe Balbi, requested to
> > >> split the glue layer driver into small parts such as
> > >> reset, regulator, phy, etc.
> > >
> > > What exactly did Felipe ask for? Did he ask that the patch be split
> > > up, one patch per reset, regulator, phy etc?
> > 
> > 
> > Yeah.  That is what we understood from his comments.
> > 
> > 
> > These are the feed-backs from him.
> > 
> > https://lkml.org/lkml/2018/1/23/298
> > https://lkml.org/lkml/2018/1/24/352
> 
> > > Are all these resources used just by the DWC3? Or is it a true MFD,
> > > multiple functions?
> > 
> > I do not think this is a real MFD.
> > 
> > This is a DWC3 glue layer, i.e.
> > a collection of misc registers that control
> > the DWC3 IP.
> > 
> > 
> > Just splitting it into small pieces
> > to use PHY, reset, regulator framework in Linux.
> > 
> > Of course, the price of this approach
> > is so cluttered Device Tree,
> > honestly I do not like it much.
> 
> This however the correct way to do this. You should have a phy driver,
> and a regulator driver, and a reset driver. The DWC3 then uses
> phandles to these drivers.
> 
> How is the IO map area 65b00000 split up. Can you cleanly separate it
> into sub areas, which do not overlap, so you have a sub-area for the
> PHY driver, a sub-area for the regulator driver and a sub-area for the
> reset area? If you can cleanly split it up, you don't need an MFD. If
> however the registers are in overlapping areas, you do need an
> MFD. The MFD core provides access to the registers, while its children
> implement PHY, reset, regulator etc.

This device certainly sounds like an MFD to me.

Can you share the DT you've written please?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [Question] MFD driver that handles clocks/resets and populates child nodes

[Masahiro Yamada]

2018-04-03 17:03 GMT+09:00 Lee Jones <lee.jones@linaro.org>:
> On Mon, 02 Apr 2018, Andrew Lunn wrote:
>
>> On Mon, Apr 02, 2018 at 10:21:01PM +0900, Masahiro Yamada wrote:
>> > 2018-04-02 21:04 GMT+09:00 Andrew Lunn <andrew@lunn.ch>:
>> > >> The maintainer of DWC3, Felipe Balbi, requested to
>> > >> split the glue layer driver into small parts such as
>> > >> reset, regulator, phy, etc.
>> > >
>> > > What exactly did Felipe ask for? Did he ask that the patch be split
>> > > up, one patch per reset, regulator, phy etc?
>> >
>> >
>> > Yeah.  That is what we understood from his comments.
>> >
>> >
>> > These are the feed-backs from him.
>> >
>> > https://lkml.org/lkml/2018/1/23/298
>> > https://lkml.org/lkml/2018/1/24/352
>>
>> > > Are all these resources used just by the DWC3? Or is it a true MFD,
>> > > multiple functions?
>> >
>> > I do not think this is a real MFD.
>> >
>> > This is a DWC3 glue layer, i.e.
>> > a collection of misc registers that control
>> > the DWC3 IP.
>> >
>> >
>> > Just splitting it into small pieces
>> > to use PHY, reset, regulator framework in Linux.
>> >
>> > Of course, the price of this approach
>> > is so cluttered Device Tree,
>> > honestly I do not like it much.
>>
>> This however the correct way to do this. You should have a phy driver,
>> and a regulator driver, and a reset driver. The DWC3 then uses
>> phandles to these drivers.
>>
>> How is the IO map area 65b00000 split up. Can you cleanly separate it
>> into sub areas, which do not overlap, so you have a sub-area for the
>> PHY driver, a sub-area for the regulator driver and a sub-area for the
>> reset area? If you can cleanly split it up, you don't need an MFD. If
>> however the registers are in overlapping areas, you do need an
>> MFD. The MFD core provides access to the registers, while its children
>> implement PHY, reset, regulator etc.
>
> This device certainly sounds like an MFD to me.
>
> Can you share the DT you've written please?


This is still under the internal review in Socionext,
but I attached it below FWIW.

(I am not the author of this DT.
 Written by Kunihiko Hayashi,
 and clocks/resets parts were slightly modified by me.)


Just skimming the driver, I guess it will be possible to flatten
the node structure by separating the register space into sub-areas.
If this is success, we do not the MFD driver.


usb@65b00000 {
        compatible = "socionext,uniphier-ld20-usb3-glue",
                     "syscon";
        reg = <0x65b00000 0x1000>;
        clock-names = "usb";
        clocks = <&sys_clk 14>;
        reset-names = "usb";
        resets = <&sys_rst 14>;

        usb_rst: reset {
                compatible = "socionext,uniphier-ld20-usb3-reset";
                #reset-cells = <1>;
        };

        regulators {
                compatible = "socionext,uniphier-ld20-usb3-regulator";

                usb_vbus0: vbus-0 { };
                usb_vbus1: vbus-1 { };
                usb_vbus2: vbus-2 { };
                usb_vbus3: vbus-3 { };
        };

        usb_hsphy: hs-phy {
                compatible = "socionext,uniphier-ld20-usb3-hsphy";
                #address-cells = <1>;
                #size-cells = <0>;
                #phy-cells = <0>;
                clock-names = "phy-clk0", "phy-clk1";
                clocks = <&sys_clk 16>, <&sys_clk 17>;
                reset-names = "phy-rst0", "phy-rst1";
                resets = <&sys_rst 16>, <&sys_rst 17>;
                port0-supply = <&usb_vbus0>;
                port1-supply = <&usb_vbus1>;
                port2-supply = <&usb_vbus2>;
                port3-supply = <&usb_vbus3>;

                port@0 {
                        reg = <0>;
                        nvmem-cell-names = "rterm", "sel_t",
                                           "hs_i";
                        nvmem-cells = <&usb_rterm0>,
                                      <&usb_sel_t0>,
                                      <&usb_hs_i0>;
                };
                port@1 {
                        reg = <1>;
                        nvmem-cell-names = "rterm", "sel_t",
                                           "hs_i";
                        nvmem-cells = <&usb_rterm1>,
                                      <&usb_sel_t1>,
                                      <&usb_hs_i0>;
                };
                port@2 {
                        reg = <2>;
                        nvmem-cell-names = "rterm", "sel_t",
                                           "hs_i";
                        nvmem-cells = <&usb_rterm2>,
                                      <&usb_sel_t2>,
                                      <&usb_hs_i2>;
                };
                port@3 {
                        reg = <3>;
                        nvmem-cell-names = "rterm", "sel_t",
                                           "hs_i";
                        nvmem-cells = <&usb_rterm3>,
                                      <&usb_sel_t3>,
                                      <&usb_hs_i2>;
                };
        };

        usb_ssphy: ss-phy {
                compatible = "socionext,uniphier-ld20-usb3-ssphy";
                #address-cells = <1>;
                #size-cells = <0>;
                #phy-cells = <0>;
                reset-names = "phy-rst0", "phy-rst1";
                resets = <&sys_rst 18>, <&sys_rst 19>;
                port0-supply = <&usb_vbus0>;
                port1-supply = <&usb_vbus1>;

                port@0 {
                        reg = <0>;
                };
                port@1 {
                        reg = <1>;
                };
        };
};






-- 
Best Regards
Masahiro Yamada

Re: [Question] MFD driver that handles clocks/resets and populates child nodes

[Lee Jones]

On Tue, 03 Apr 2018, Masahiro Yamada wrote:

> 2018-04-03 17:03 GMT+09:00 Lee Jones <lee.jones@linaro.org>:
> > On Mon, 02 Apr 2018, Andrew Lunn wrote:
> >
> >> On Mon, Apr 02, 2018 at 10:21:01PM +0900, Masahiro Yamada wrote:
> >> > 2018-04-02 21:04 GMT+09:00 Andrew Lunn <andrew@lunn.ch>:
> >> > >> The maintainer of DWC3, Felipe Balbi, requested to
> >> > >> split the glue layer driver into small parts such as
> >> > >> reset, regulator, phy, etc.
> >> > >
> >> > > What exactly did Felipe ask for? Did he ask that the patch be split
> >> > > up, one patch per reset, regulator, phy etc?
> >> >
> >> >
> >> > Yeah.  That is what we understood from his comments.
> >> >
> >> >
> >> > These are the feed-backs from him.
> >> >
> >> > https://lkml.org/lkml/2018/1/23/298
> >> > https://lkml.org/lkml/2018/1/24/352
> >>
> >> > > Are all these resources used just by the DWC3? Or is it a true MFD,
> >> > > multiple functions?
> >> >
> >> > I do not think this is a real MFD.
> >> >
> >> > This is a DWC3 glue layer, i.e.
> >> > a collection of misc registers that control
> >> > the DWC3 IP.
> >> >
> >> >
> >> > Just splitting it into small pieces
> >> > to use PHY, reset, regulator framework in Linux.
> >> >
> >> > Of course, the price of this approach
> >> > is so cluttered Device Tree,
> >> > honestly I do not like it much.
> >>
> >> This however the correct way to do this. You should have a phy driver,
> >> and a regulator driver, and a reset driver. The DWC3 then uses
> >> phandles to these drivers.
> >>
> >> How is the IO map area 65b00000 split up. Can you cleanly separate it
> >> into sub areas, which do not overlap, so you have a sub-area for the
> >> PHY driver, a sub-area for the regulator driver and a sub-area for the
> >> reset area? If you can cleanly split it up, you don't need an MFD. If
> >> however the registers are in overlapping areas, you do need an
> >> MFD. The MFD core provides access to the registers, while its children
> >> implement PHY, reset, regulator etc.
> >
> > This device certainly sounds like an MFD to me.
> >
> > Can you share the DT you've written please?
> 
> 
> This is still under the internal review in Socionext,
> but I attached it below FWIW.
> 
> (I am not the author of this DT.
>  Written by Kunihiko Hayashi,
> 
> Just skimming the driver, I guess it will be possible to flatten
> the node structure by separating the register space into sub-areas.
> If this is success, we do not the MFD driver.

Sounds like a plan.  Pretty sure this isn't really an MFD.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog