* [Question] MFD driver that handles clocks/resets and populates child nodes @ 2018-04-02 7:17 Masahiro Yamada 2018-04-02 12:04 ` Andrew Lunn 0 siblings, 1 reply; 7+ messages in thread From: Masahiro Yamada @ 2018-04-02 7:17 UTC (permalink / raw) To: Lee Jones, Rob Herring, devicetree Cc: linux-arm-kernel, Arnd Bergmann, Linux Kernel Mailing List, Felipe Balbi, linux-usb, Kunihiko Hayashi, Masami Hiramatsu, Jassi Brar 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Question] MFD driver that handles clocks/resets and populates child nodes 2018-04-02 7:17 [Question] MFD driver that handles clocks/resets and populates child nodes Masahiro Yamada @ 2018-04-02 12:04 ` Andrew Lunn 2018-04-02 13:21 ` Masahiro Yamada 0 siblings, 1 reply; 7+ messages in thread From: Andrew Lunn @ 2018-04-02 12:04 UTC (permalink / raw) To: Masahiro Yamada Cc: Lee Jones, Rob Herring, devicetree, Kunihiko Hayashi, Felipe Balbi, linux-usb, Linux Kernel Mailing List, Jassi Brar, Masami Hiramatsu, Arnd Bergmann, linux-arm-kernel > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Question] MFD driver that handles clocks/resets and populates child nodes 2018-04-02 12:04 ` Andrew Lunn @ 2018-04-02 13:21 ` Masahiro Yamada 2018-04-02 13:32 ` Andrew Lunn 0 siblings, 1 reply; 7+ messages in thread From: Masahiro Yamada @ 2018-04-02 13:21 UTC (permalink / raw) To: Andrew Lunn Cc: Lee Jones, Rob Herring, devicetree, Kunihiko Hayashi, Felipe Balbi, linux-usb, Linux Kernel Mailing List, Jassi Brar, Masami Hiramatsu, Arnd Bergmann, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Question] MFD driver that handles clocks/resets and populates child nodes 2018-04-02 13:21 ` Masahiro Yamada @ 2018-04-02 13:32 ` Andrew Lunn 2018-04-03 8:03 ` Lee Jones 0 siblings, 1 reply; 7+ messages in thread From: Andrew Lunn @ 2018-04-02 13:32 UTC (permalink / raw) To: Masahiro Yamada Cc: Lee Jones, Rob Herring, devicetree, Kunihiko Hayashi, Felipe Balbi, linux-usb, Linux Kernel Mailing List, Jassi Brar, Masami Hiramatsu, Arnd Bergmann, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Question] MFD driver that handles clocks/resets and populates child nodes 2018-04-02 13:32 ` Andrew Lunn @ 2018-04-03 8:03 ` Lee Jones 2018-04-03 12:14 ` Masahiro Yamada 0 siblings, 1 reply; 7+ messages in thread From: Lee Jones @ 2018-04-03 8:03 UTC (permalink / raw) To: Andrew Lunn Cc: Masahiro Yamada, Rob Herring, devicetree, Kunihiko Hayashi, Felipe Balbi, linux-usb, Linux Kernel Mailing List, Jassi Brar, Masami Hiramatsu, Arnd Bergmann, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Question] MFD driver that handles clocks/resets and populates child nodes 2018-04-03 8:03 ` Lee Jones @ 2018-04-03 12:14 ` Masahiro Yamada 2018-04-03 12:59 ` Lee Jones 0 siblings, 1 reply; 7+ messages in thread From: Masahiro Yamada @ 2018-04-03 12:14 UTC (permalink / raw) To: Lee Jones Cc: Andrew Lunn, Rob Herring, devicetree, Kunihiko Hayashi, Felipe Balbi, linux-usb, Linux Kernel Mailing List, Jassi Brar, Masami Hiramatsu, Arnd Bergmann, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Question] MFD driver that handles clocks/resets and populates child nodes 2018-04-03 12:14 ` Masahiro Yamada @ 2018-04-03 12:59 ` Lee Jones 0 siblings, 0 replies; 7+ messages in thread From: Lee Jones @ 2018-04-03 12:59 UTC (permalink / raw) To: Masahiro Yamada Cc: Andrew Lunn, Rob Herring, devicetree, Kunihiko Hayashi, Felipe Balbi, linux-usb, Linux Kernel Mailing List, Jassi Brar, Masami Hiramatsu, Arnd Bergmann, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-04-03 13:00 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-02 7:17 [Question] MFD driver that handles clocks/resets and populates child nodes Masahiro Yamada 2018-04-02 12:04 ` Andrew Lunn 2018-04-02 13:21 ` Masahiro Yamada 2018-04-02 13:32 ` Andrew Lunn 2018-04-03 8:03 ` Lee Jones 2018-04-03 12:14 ` Masahiro Yamada 2018-04-03 12:59 ` Lee Jones
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).