| Message ID | 20241002-private-unequal-33cfa6101338@spud (mailing list archive) |
|---|---|
| Headers | show |
| Series | Redo PolarFire SoC's mailbox/clock devicestrees and related code | expand |
On 02/10/2024 12:48, Conor Dooley wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > I like this one better than qualcomms and wish to use it for the > PolarFire SoC clock drivers. > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > drivers/clk/Kconfig | 4 ++ > drivers/clk/Makefile | 1 + > drivers/clk/{meson => }/clk-regmap.c | 2 +- > drivers/clk/meson/Kconfig | 46 +++++++++---------- > drivers/clk/meson/Makefile | 1 - > drivers/clk/meson/a1-peripherals.c | 2 +- > drivers/clk/meson/a1-pll.c | 2 +- > drivers/clk/meson/axg-aoclk.c | 2 +- > drivers/clk/meson/axg-audio.c | 2 +- > drivers/clk/meson/axg.c | 2 +- > drivers/clk/meson/c3-peripherals.c | 2 +- > drivers/clk/meson/c3-pll.c | 2 +- > drivers/clk/meson/clk-cpu-dyndiv.c | 2 +- > drivers/clk/meson/clk-dualdiv.c | 2 +- > drivers/clk/meson/clk-mpll.c | 2 +- > drivers/clk/meson/clk-phase.c | 2 +- > drivers/clk/meson/clk-pll.c | 2 +- > drivers/clk/meson/g12a-aoclk.c | 2 +- > drivers/clk/meson/g12a.c | 2 +- > drivers/clk/meson/gxbb-aoclk.c | 2 +- > drivers/clk/meson/gxbb.c | 2 +- > drivers/clk/meson/meson-aoclk.h | 2 +- > drivers/clk/meson/meson-eeclk.c | 2 +- > drivers/clk/meson/meson-eeclk.h | 2 +- > drivers/clk/meson/meson8-ddr.c | 2 +- > drivers/clk/meson/meson8b.c | 2 +- > drivers/clk/meson/s4-peripherals.c | 2 +- > drivers/clk/meson/s4-pll.c | 2 +- > drivers/clk/meson/sclk-div.c | 2 +- > drivers/clk/meson/vclk.h | 2 +- > drivers/clk/meson/vid-pll-div.c | 2 +- > .../meson => include/linux/clk}/clk-regmap.h | 0 > 32 files changed, 53 insertions(+), 53 deletions(-) > rename drivers/clk/{meson => }/clk-regmap.c (99%) > rename {drivers/clk/meson => include/linux/clk}/clk-regmap.h (100%) > <snip> I don't have objections, but I think Stephen didn't like the idea a few years ago, but perhaps it has changed... Anyway, take my: Acked-by: Neil Armstrong <neil.armstrong@linaro.org> Thanks, Neil
On Wed 02 Oct 2024 at 13:20, Neil Armstrong <neil.armstrong@linaro.org> wrote: > On 02/10/2024 12:48, Conor Dooley wrote: >> From: Conor Dooley <conor.dooley@microchip.com> >> I like this one better than qualcomms and wish to use it for the >> PolarFire SoC clock drivers. >> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> >> --- >> drivers/clk/Kconfig | 4 ++ >> drivers/clk/Makefile | 1 + >> drivers/clk/{meson => }/clk-regmap.c | 2 +- >> drivers/clk/meson/Kconfig | 46 +++++++++---------- >> drivers/clk/meson/Makefile | 1 - >> drivers/clk/meson/a1-peripherals.c | 2 +- >> drivers/clk/meson/a1-pll.c | 2 +- >> drivers/clk/meson/axg-aoclk.c | 2 +- >> drivers/clk/meson/axg-audio.c | 2 +- >> drivers/clk/meson/axg.c | 2 +- >> drivers/clk/meson/c3-peripherals.c | 2 +- >> drivers/clk/meson/c3-pll.c | 2 +- >> drivers/clk/meson/clk-cpu-dyndiv.c | 2 +- >> drivers/clk/meson/clk-dualdiv.c | 2 +- >> drivers/clk/meson/clk-mpll.c | 2 +- >> drivers/clk/meson/clk-phase.c | 2 +- >> drivers/clk/meson/clk-pll.c | 2 +- >> drivers/clk/meson/g12a-aoclk.c | 2 +- >> drivers/clk/meson/g12a.c | 2 +- >> drivers/clk/meson/gxbb-aoclk.c | 2 +- >> drivers/clk/meson/gxbb.c | 2 +- >> drivers/clk/meson/meson-aoclk.h | 2 +- >> drivers/clk/meson/meson-eeclk.c | 2 +- >> drivers/clk/meson/meson-eeclk.h | 2 +- >> drivers/clk/meson/meson8-ddr.c | 2 +- >> drivers/clk/meson/meson8b.c | 2 +- >> drivers/clk/meson/s4-peripherals.c | 2 +- >> drivers/clk/meson/s4-pll.c | 2 +- >> drivers/clk/meson/sclk-div.c | 2 +- >> drivers/clk/meson/vclk.h | 2 +- >> drivers/clk/meson/vid-pll-div.c | 2 +- >> .../meson => include/linux/clk}/clk-regmap.h | 0 >> 32 files changed, 53 insertions(+), 53 deletions(-) >> rename drivers/clk/{meson => }/clk-regmap.c (99%) >> rename {drivers/clk/meson => include/linux/clk}/clk-regmap.h (100%) >> > <snip> > > I don't have objections, but I think Stephen didn't like the idea > a few years ago, but perhaps it has changed... > > Anyway, take my: > Acked-by: Neil Armstrong <neil.armstrong@linaro.org> We had a similar discussion 3y ago indeed: https://lore.kernel.org/linux-clk/162734682512.2368309.12015873010777083014@swboyd.mtv.corp.google.com/ There are needs for a common regmap backed clocks indeed, but allowing meson flavored regmap clocks to spread in the kernel was not really the prefered way to do it. IIRC, Stephen's idea was more the bring regmap support in clk-gate.c, clk-mux, etc ... I'm not quite sure how make iomem and regmap co-exist in a manageable/maintainable way within those drivers (without adding yet another level of abstraction I mean) ? Silently creating a regmap maybe ? but that's probably a bit heavy. I did not really had time to dig more on this, I guess no one did. I don't really have a preference one way or the other but if it is going to be exposed in 'include/linux', we need to be sure that's how we want to do it. With clocks poping in many driver subsystems, it will difficult to change afterward. > > Thanks, > Neil
On Wed, Oct 02, 2024 at 03:21:16PM +0200, Jerome Brunet wrote: > On Wed 02 Oct 2024 at 13:20, Neil Armstrong <neil.armstrong@linaro.org> wrote: > > > On 02/10/2024 12:48, Conor Dooley wrote: > >> From: Conor Dooley <conor.dooley@microchip.com> > >> I like this one better than qualcomms and wish to use it for the > >> PolarFire SoC clock drivers. > >> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > >> --- > >> drivers/clk/Kconfig | 4 ++ > >> drivers/clk/Makefile | 1 + > >> drivers/clk/{meson => }/clk-regmap.c | 2 +- > >> drivers/clk/meson/Kconfig | 46 +++++++++---------- > >> drivers/clk/meson/Makefile | 1 - > >> drivers/clk/meson/a1-peripherals.c | 2 +- > >> drivers/clk/meson/a1-pll.c | 2 +- > >> drivers/clk/meson/axg-aoclk.c | 2 +- > >> drivers/clk/meson/axg-audio.c | 2 +- > >> drivers/clk/meson/axg.c | 2 +- > >> drivers/clk/meson/c3-peripherals.c | 2 +- > >> drivers/clk/meson/c3-pll.c | 2 +- > >> drivers/clk/meson/clk-cpu-dyndiv.c | 2 +- > >> drivers/clk/meson/clk-dualdiv.c | 2 +- > >> drivers/clk/meson/clk-mpll.c | 2 +- > >> drivers/clk/meson/clk-phase.c | 2 +- > >> drivers/clk/meson/clk-pll.c | 2 +- > >> drivers/clk/meson/g12a-aoclk.c | 2 +- > >> drivers/clk/meson/g12a.c | 2 +- > >> drivers/clk/meson/gxbb-aoclk.c | 2 +- > >> drivers/clk/meson/gxbb.c | 2 +- > >> drivers/clk/meson/meson-aoclk.h | 2 +- > >> drivers/clk/meson/meson-eeclk.c | 2 +- > >> drivers/clk/meson/meson-eeclk.h | 2 +- > >> drivers/clk/meson/meson8-ddr.c | 2 +- > >> drivers/clk/meson/meson8b.c | 2 +- > >> drivers/clk/meson/s4-peripherals.c | 2 +- > >> drivers/clk/meson/s4-pll.c | 2 +- > >> drivers/clk/meson/sclk-div.c | 2 +- > >> drivers/clk/meson/vclk.h | 2 +- > >> drivers/clk/meson/vid-pll-div.c | 2 +- > >> .../meson => include/linux/clk}/clk-regmap.h | 0 > >> 32 files changed, 53 insertions(+), 53 deletions(-) > >> rename drivers/clk/{meson => }/clk-regmap.c (99%) > >> rename {drivers/clk/meson => include/linux/clk}/clk-regmap.h (100%) > >> > > <snip> > > > > I don't have objections, but I think Stephen didn't like the idea > > a few years ago, but perhaps it has changed... > > > > Anyway, take my: > > Acked-by: Neil Armstrong <neil.armstrong@linaro.org> > > We had a similar discussion 3y ago indeed: > https://lore.kernel.org/linux-clk/162734682512.2368309.12015873010777083014@swboyd.mtv.corp.google.com/ > > There are needs for a common regmap backed clocks indeed, but allowing > meson flavored regmap clocks to spread in the kernel was not really the > prefered way to do it. Cool, thanks for that link. > IIRC, Stephen's idea was more the bring regmap support in clk-gate.c, > clk-mux, etc ... I'm not quite sure how make iomem and regmap co-exist > in a manageable/maintainable way within those drivers (without adding yet > another level of abstraction I mean) ? Silently creating a regmap maybe > ? but that's probably a bit heavy. I did not really had time to dig more > on this, I guess no one did. I guess I have some motivation to looking into it at the moment. I had my reservations about the Meson approach too, liking it more than Qualcomm's didn't mean I completely liked it. It was already my intention to implement point b of your mail, had the general idea here been acceptable, cos that's a divergence from how the generic clock types (that the driver in question currently uses) work. And on that note, I just noticed I left the mild-annoyance variable name "sigh" in the submitted driver changes, which I had used for the clk_regmap struct that your point b in the link relates to. > I don't really have a preference one way or the other but if it is going > to be exposed in 'include/linux', we need to be sure that's how we want > to do it. With clocks poping in many driver subsystems, it will > difficult to change afterward. Yeah, I agree. I didn't expect this to go in right away, and I also didn't want to surge ahead on some rework of the clock types, were people to hate even the reuse.
Stephen, On Thu, Oct 03, 2024 at 12:33:40PM +0100, Conor Dooley wrote: > On Wed, Oct 02, 2024 at 03:21:16PM +0200, Jerome Brunet wrote: > > On Wed 02 Oct 2024 at 13:20, Neil Armstrong <neil.armstrong@linaro.org> wrote: > > > > > On 02/10/2024 12:48, Conor Dooley wrote: > > >> From: Conor Dooley <conor.dooley@microchip.com> > > >> I like this one better than qualcomms and wish to use it for the > > >> PolarFire SoC clock drivers. > > >> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > >> --- > > >> drivers/clk/Kconfig | 4 ++ > > >> drivers/clk/Makefile | 1 + > > >> drivers/clk/{meson => }/clk-regmap.c | 2 +- > > >> drivers/clk/meson/Kconfig | 46 +++++++++---------- > > >> drivers/clk/meson/Makefile | 1 - > > >> drivers/clk/meson/a1-peripherals.c | 2 +- > > >> drivers/clk/meson/a1-pll.c | 2 +- > > >> drivers/clk/meson/axg-aoclk.c | 2 +- > > >> drivers/clk/meson/axg-audio.c | 2 +- > > >> drivers/clk/meson/axg.c | 2 +- > > >> drivers/clk/meson/c3-peripherals.c | 2 +- > > >> drivers/clk/meson/c3-pll.c | 2 +- > > >> drivers/clk/meson/clk-cpu-dyndiv.c | 2 +- > > >> drivers/clk/meson/clk-dualdiv.c | 2 +- > > >> drivers/clk/meson/clk-mpll.c | 2 +- > > >> drivers/clk/meson/clk-phase.c | 2 +- > > >> drivers/clk/meson/clk-pll.c | 2 +- > > >> drivers/clk/meson/g12a-aoclk.c | 2 +- > > >> drivers/clk/meson/g12a.c | 2 +- > > >> drivers/clk/meson/gxbb-aoclk.c | 2 +- > > >> drivers/clk/meson/gxbb.c | 2 +- > > >> drivers/clk/meson/meson-aoclk.h | 2 +- > > >> drivers/clk/meson/meson-eeclk.c | 2 +- > > >> drivers/clk/meson/meson-eeclk.h | 2 +- > > >> drivers/clk/meson/meson8-ddr.c | 2 +- > > >> drivers/clk/meson/meson8b.c | 2 +- > > >> drivers/clk/meson/s4-peripherals.c | 2 +- > > >> drivers/clk/meson/s4-pll.c | 2 +- > > >> drivers/clk/meson/sclk-div.c | 2 +- > > >> drivers/clk/meson/vclk.h | 2 +- > > >> drivers/clk/meson/vid-pll-div.c | 2 +- > > >> .../meson => include/linux/clk}/clk-regmap.h | 0 > > >> 32 files changed, 53 insertions(+), 53 deletions(-) > > >> rename drivers/clk/{meson => }/clk-regmap.c (99%) > > >> rename {drivers/clk/meson => include/linux/clk}/clk-regmap.h (100%) > > >> > > > <snip> > > > > > > I don't have objections, but I think Stephen didn't like the idea > > > a few years ago, but perhaps it has changed... > > > > > > Anyway, take my: > > > Acked-by: Neil Armstrong <neil.armstrong@linaro.org> > > > > We had a similar discussion 3y ago indeed: > > https://lore.kernel.org/linux-clk/162734682512.2368309.12015873010777083014@swboyd.mtv.corp.google.com/ > > > > There are needs for a common regmap backed clocks indeed, but allowing > > meson flavored regmap clocks to spread in the kernel was not really the > > prefered way to do it. > > Cool, thanks for that link. > > > IIRC, Stephen's idea was more the bring regmap support in clk-gate.c, > > clk-mux, etc ... I'm not quite sure how make iomem and regmap co-exist > > in a manageable/maintainable way within those drivers (without adding yet > > another level of abstraction I mean) ? Silently creating a regmap maybe > > ? but that's probably a bit heavy. I did not really had time to dig more > > on this, I guess no one did. > > I guess I have some motivation to looking into it at the moment. I had > my reservations about the Meson approach too, liking it more than > Qualcomm's didn't mean I completely liked it. > It was already my intention to implement point b of your mail, had the > general idea here been acceptable, cos that's a divergence from how the > generic clock types (that the driver in question currently uses) work. > And on that note, I just noticed I left the mild-annoyance variable name > "sigh" in the submitted driver changes, which I had used for the > clk_regmap struct that your point b in the link relates to. > > > I don't really have a preference one way or the other but if it is going > > to be exposed in 'include/linux', we need to be sure that's how we want > > to do it. With clocks poping in many driver subsystems, it will > > difficult to change afterward. > > Yeah, I agree. I didn't expect this to go in right away, and I also > didn't want to surge ahead on some rework of the clock types, were > people to hate even the reuse. Hmm, so how (in-)complete of a regmap implementation can I get away with? I only need clk-gate and clk-divider for this patchset... Shoving the regmap into the clk structs makes things pretty trivial as I don't need to do anything special in any function other than clk_*_readl()/clk_*_writel() and the registration code. A flag isn't even needed to determine if a clock is a regmap one I don't think, since you can just check if the regmap pointer is non-NULL. My use case doesn't actually need the registration code changes either as, currently, only reg gets set at runtime, but leaving that out is a level of incomplete I'd not let myself away with. Obviously shoving the extra members into the clk structs has the downside of taking up a pointer and a offset worth of memory for each clock of that type registered, but it is substantially easier to support devices with multiple regmaps that way. Probably moot though since the approach you suggested in the thread linked above that implements a clk_hw_get_regmap() has to store a pointer to the regmap's identifier which would take up an identical amount of memory. I don't really care which way you want it done, both are pretty easy to implement if I can get away with just doing so for the two standard clock types that I am using - is it okay to just do those two? Cheers, Conor.
Quoting Conor Dooley (2024-11-06 04:56:25) > On Thu, Oct 03, 2024 at 12:33:40PM +0100, Conor Dooley wrote: > > > IIRC, Stephen's idea was more the bring regmap support in clk-gate.c, > > > clk-mux, etc ... I'm not quite sure how make iomem and regmap co-exist > > > in a manageable/maintainable way within those drivers (without adding yet > > > another level of abstraction I mean) ? Silently creating a regmap maybe > > > ? but that's probably a bit heavy. I did not really had time to dig more > > > on this, I guess no one did. > > > > I guess I have some motivation to looking into it at the moment. I had > > my reservations about the Meson approach too, liking it more than > > Qualcomm's didn't mean I completely liked it. > > It was already my intention to implement point b of your mail, had the > > general idea here been acceptable, cos that's a divergence from how the > > generic clock types (that the driver in question currently uses) work. > > And on that note, I just noticed I left the mild-annoyance variable name > > "sigh" in the submitted driver changes, which I had used for the > > clk_regmap struct that your point b in the link relates to. > > > > > I don't really have a preference one way or the other but if it is going > > > to be exposed in 'include/linux', we need to be sure that's how we want > > > to do it. With clocks poping in many driver subsystems, it will > > > difficult to change afterward. > > > > Yeah, I agree. I didn't expect this to go in right away, and I also > > didn't want to surge ahead on some rework of the clock types, were > > people to hate even the reuse. > > Hmm, so how (in-)complete of a regmap implementation can I get away > with? I only need clk-gate and clk-divider for this patchset... > > Shoving the regmap into the clk structs makes things pretty trivial as I > don't need to do anything special in any function other than > clk_*_readl()/clk_*_writel() and the registration code. A flag isn't > even needed to determine if a clock is a regmap one I don't think, since > you can just check if the regmap pointer is non-NULL. For the basic clk types I think it would be good to leave the old stuff alone. We have already split the logic out into helpers, so I wonder if we can do this better by making kernel modules for the different basic regmap clk types and exposing registration APIs. If we force drivers that use the basic regmap types to 'select' the module then we'll make it so that we don't include code that isn't used anywhere. That's one of the problems with the basic clk types, it's always built. It also lets us avoid making regmap a dependency for the clk framework at large. Doing that would also let us avoid the flag because it will be explicit in any registration API, clk_register_divider() vs. clk_register_regmap_divider(). Yes we duplicate some boiler plate logic around read-only and registration paths, but this is alright as long as we can share most of the code and gain the advantage of removing the code entirely when it isn't used. I wonder if we can even make a regmap on the fly for the iomem pointers so that clk_divider_readl() can always use the regmap API to access the hardware. Sounds wasteful but maybe it would work. > My use case doesn't > actually need the registration code changes either as, currently, only reg > gets set at runtime, but leaving that out is a level of incomplete I'd not > let myself away with. > Obviously shoving the extra members into the clk structs has the downside > of taking up a pointer and a offset worth of memory for each clock of > that type registered, but it is substantially easier to support devices > with multiple regmaps that way. Probably moot though since the approach you > suggested in the thread linked above that implements a clk_hw_get_regmap() > has to store a pointer to the regmap's identifier which would take up an > identical amount of memory. We don't need to store the regmap identifier in the struct clk. We can store it in the 'struct clk_init_data' with some new field, and only do that when/if we actually need to. We would need to pass the init data to the clk_ops::init() callback though. We currently knock that out during registration so that clk_hw->init is NULL. Probably we can just set that to NULL after the init routine runs in __clk_core_init(). Long story short, don't add something to 'struct clk_core', 'struct clk', or 'struct clk_hw' for these details. We can have a 'struct clk_regmap_hw' that everyone else can build upon: struct clk_regmap_hw { struct regmap *regmap; struct clk_hw hw; }; and then set the regmap pointer during registration in clk_hw_init_regmap(). int clk_hw_init_regmap(struct clk_hw *hw) { struct device *dev; struct regmap *regmap; struct clk_regmap_hw *rhw; rhw = clk_hw_to_clk_regmap_hw(hw); dev = clk_hw_get_dev(hw); if (!dev) return -EINVAL; regmap = dev_get_regmap(dev, hw->init->regmap_name); if (!regmap) return -EINVAL; // Print helpful message rhw->regmap = regmap; return 0; } or we can even make it so that there's clk_hw_init_regmap() and clk_hw_init_regmap_name() so that drivers can have multiple functions if the clks need different regmaps. int my_init_regmap(struct clk_hw *hw) { int ret; ret = clk_hw_init_regmap_name(hw, "my_name"); ... } If you don't need the multiple regmap support then it's fine to punt here until later. > > I don't really care which way you want it done, both are pretty easy to > implement if I can get away with just doing so for the two standard > clock types that I am using - is it okay to just do those two? > Of course, doing only what is minimally required is better than changing everything if you're not sure the approach is going to land.
On Thu, Nov 14, 2024 at 05:29:54PM -0800, Stephen Boyd wrote: > Quoting Conor Dooley (2024-11-06 04:56:25) > > My use case doesn't > > actually need the registration code changes either as, currently, only reg > > gets set at runtime, but leaving that out is a level of incomplete I'd not > > let myself away with. > > Obviously shoving the extra members into the clk structs has the downside > > of taking up a pointer and a offset worth of memory for each clock of > > that type registered, but it is substantially easier to support devices > > with multiple regmaps that way. Probably moot though since the approach you > > suggested in the thread linked above that implements a clk_hw_get_regmap() > > has to store a pointer to the regmap's identifier which would take up an > > identical amount of memory. > > We don't need to store the regmap identifier in the struct clk. We can > store it in the 'struct clk_init_data' with some new field, and only do > that when/if we actually need to. We would need to pass the init data to > the clk_ops::init() callback though. We currently knock that out during > registration so that clk_hw->init is NULL. Probably we can just set that > to NULL after the init routine runs in __clk_core_init(). > > Long story short, don't add something to 'struct clk_core', 'struct > clk', or 'struct clk_hw' for these details. We can have a 'struct > clk_regmap_hw' that everyone else can build upon: > > struct clk_regmap_hw { > struct regmap *regmap; > struct clk_hw hw; > }; What's the point of this? I don't understand why you want to do this over what clk_divider et al already do, where clk_hw and the iomem pointer are in the struct itself. > > and then set the regmap pointer during registration in > clk_hw_init_regmap(). > > int clk_hw_init_regmap(struct clk_hw *hw) > { > struct device *dev; > struct regmap *regmap; > struct clk_regmap_hw *rhw; > > rhw = clk_hw_to_clk_regmap_hw(hw); > > dev = clk_hw_get_dev(hw); > if (!dev) > return -EINVAL; > > regmap = dev_get_regmap(dev, hw->init->regmap_name); > if (!regmap) > return -EINVAL; // Print helpful message > rhw->regmap = regmap; > > return 0; > }
Quoting Conor Dooley (2024-11-28 02:36:16) > On Thu, Nov 14, 2024 at 05:29:54PM -0800, Stephen Boyd wrote: > > Quoting Conor Dooley (2024-11-06 04:56:25) > > > My use case doesn't > > > actually need the registration code changes either as, currently, only reg > > > gets set at runtime, but leaving that out is a level of incomplete I'd not > > > let myself away with. > > > Obviously shoving the extra members into the clk structs has the downside > > > of taking up a pointer and a offset worth of memory for each clock of > > > that type registered, but it is substantially easier to support devices > > > with multiple regmaps that way. Probably moot though since the approach you > > > suggested in the thread linked above that implements a clk_hw_get_regmap() > > > has to store a pointer to the regmap's identifier which would take up an > > > identical amount of memory. > > > > We don't need to store the regmap identifier in the struct clk. We can > > store it in the 'struct clk_init_data' with some new field, and only do > > that when/if we actually need to. We would need to pass the init data to > > the clk_ops::init() callback though. We currently knock that out during > > registration so that clk_hw->init is NULL. Probably we can just set that > > to NULL after the init routine runs in __clk_core_init(). > > > > Long story short, don't add something to 'struct clk_core', 'struct > > clk', or 'struct clk_hw' for these details. We can have a 'struct > > clk_regmap_hw' that everyone else can build upon: > > > > struct clk_regmap_hw { > > struct regmap *regmap; > > struct clk_hw hw; > > }; > > What's the point of this? I don't understand why you want to do this over > what clk_divider et al already do, where clk_hw and the iomem pointer > are in the struct itself. Can you give an example? I don't understand what you're suggesting. I prefer a struct clk_regmap_hw like above so that the existing struct clk_hw in the kernel aren't increased by a pointer. SoC drivers can use the same struct as a replacement for their struct clk_hw member today.
On Tue, Dec 03, 2024 at 02:50:31PM -0800, Stephen Boyd wrote: > Quoting Conor Dooley (2024-11-28 02:36:16) > > On Thu, Nov 14, 2024 at 05:29:54PM -0800, Stephen Boyd wrote: > > > Quoting Conor Dooley (2024-11-06 04:56:25) > > > > My use case doesn't > > > > actually need the registration code changes either as, currently, only reg > > > > gets set at runtime, but leaving that out is a level of incomplete I'd not > > > > let myself away with. > > > > Obviously shoving the extra members into the clk structs has the downside > > > > of taking up a pointer and a offset worth of memory for each clock of > > > > that type registered, but it is substantially easier to support devices > > > > with multiple regmaps that way. Probably moot though since the approach you > > > > suggested in the thread linked above that implements a clk_hw_get_regmap() > > > > has to store a pointer to the regmap's identifier which would take up an > > > > identical amount of memory. > > > > > > We don't need to store the regmap identifier in the struct clk. We can > > > store it in the 'struct clk_init_data' with some new field, and only do > > > that when/if we actually need to. We would need to pass the init data to > > > the clk_ops::init() callback though. We currently knock that out during > > > registration so that clk_hw->init is NULL. Probably we can just set that > > > to NULL after the init routine runs in __clk_core_init(). > > > > > > Long story short, don't add something to 'struct clk_core', 'struct > > > clk', or 'struct clk_hw' for these details. We can have a 'struct > > > clk_regmap_hw' that everyone else can build upon: > > > > > > struct clk_regmap_hw { > > > struct regmap *regmap; > > > struct clk_hw hw; > > > }; > > > > What's the point of this? I don't understand why you want to do this over > > what clk_divider et al already do, where clk_hw and the iomem pointer > > are in the struct itself. > > Can you give an example? I don't understand what you're suggesting. I > prefer a struct clk_regmap_hw like above so that the existing struct > clk_hw in the kernel aren't increased by a pointer. SoC drivers can use > the same struct as a replacement for their struct clk_hw member today. Best example I guess is to link what I did? This one is the core changes: https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=syscon-rework-2&id=35904222355e971c24b3eb9b9fad3dd0c38d1393 clk-gate has my original hack that I did while trying to figure out what you wanted, clk-divider-regmap is a 99% copy of clk-divider with the types, function names and readl()/writel() implementations modified. Before your last set of comments I was doing something identical to the clk-gate change for clk-divider also. Here's the changes required to my driver to make it work with the updated: https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=syscon-rework-2&id=ea40211fe20f8bc6ef0320b93e1baa5b3f244601 It's pretty much a drop in replacement, other than the additional complexity in probe. Hopefully that either gets my point across or lets you spot why I don't understand the benefit of a wrapper around clk_hw. Cheers, Conor.
From: Conor Dooley <conor.dooley@microchip.com> Yo, Here's something that I've been mulling over for a while, since I started to understand how devicetree stuff was "meant" to be done. There'd been little reason to actually press forward with it, because it is fairly disruptive. I've finally opted to do it, because a user has come along with a hwmon driver that needs to access the same register region as the mailbox and the author is not keen on using the aux bus, and because I do not want the new pic64gx SoC that's based on PolarFire SoC to use bindings etc that I know to be incorrect. Given backwards compatibility needs to be maintained, this patch series isn't the prettiest thing I have ever written. The reset driver needs to retain support for the auxiliary bus, which looks a bit mess, but not much can be done there. The mailbox and clock drivers both have to have an "old probe" function to handle the old layout. Thankfully in the clock driver, the Meson clk-regmap support can be used to identically handle both old and new devicetree formats - but using a regmap in the mailbox driver was only really possible for the new format, so the code there is unfortunately a bit of an if/else mess that I'm both not proud of, nor really sure is worth "improving". The series should be pretty splitable per subsystem, only the dts change has some sort of dependency, but I'll not be applying that till everything else is in Linus' tree, so that's not a big deal. I've got all the Meson clock folks on CC, given I am moving their clk-regmap implementation to common code. There were one or two things I didn't really like about the implementation, but it is better than the Qualcomm one IMO and creating a third copy of clk-regmap is certainly not an improvement on having two copies! I don't really want this stuff in stable, hence a lack of cc: stable anywhere here, since what's currently in the tree works fine for the currently supported hardware. AFAIK, the only other project affected here is U-Boot - I've got some WIP work that I need to rebase and send for it before the dts patches here will be applied: https://github.com/ConchuOD/u-boot/commits/syscon-clocks/ I previously submitted this as an RFC, only to Lee and the dt list, in order to get some feedback on the syscon/mfd bindings: https://lore.kernel.org/all/20240815-shindig-bunny-fd42792d638a@spud/ I'm not really going to bother with a proper changelog, since that was submitted with lots of WIP code to get answers to some questions. The main change was "removing" some of the child nodes of the syscons. Cheers, Conor. CC: Conor Dooley <conor.dooley@microchip.com> CC: Daire McNamara <daire.mcnamara@microchip.com> CC: pierre-henry.moussay@microchip.com CC: valentina.fernandezalanis@microchip.com CC: Michael Turquette <mturquette@baylibre.com> CC: Stephen Boyd <sboyd@kernel.org> CC: Rob Herring <robh@kernel.org> CC: Krzysztof Kozlowski <krzk+dt@kernel.org> CC: Jassi Brar <jassisinghbrar@gmail.com> CC: Lee Jones <lee@kernel.org> CC: Paul Walmsley <paul.walmsley@sifive.com> CC: Palmer Dabbelt <palmer@dabbelt.com> CC: Albert Ou <aou@eecs.berkeley.edu> CC: Neil Armstrong <neil.armstrong@linaro.org> CC: Jerome Brunet <jbrunet@baylibre.com> CC: Kevin Hilman <khilman@baylibre.com> CC: Martin Blumenstingl <martin.blumenstingl@googlemail.com> CC: Philipp Zabel <p.zabel@pengutronix.de> CC: linux-riscv@lists.infradead.org CC: linux-clk@vger.kernel.org CC: devicetree@vger.kernel.org CC: linux-kernel@vger.kernel.org CC: linux-amlogic@lists.infradead.org CC: linux-arm-kernel@lists.infradead.org Conor Dooley (11): dt-bindings: mailbox: mpfs: fix reg properties mailbox: mpfs: support new, syscon based, devicetree configuration dt-bindings: mfd: syscon document the non simple-mfd syscon on PolarFire SoC dt-bindings: soc: microchip: document the two simple-mfd syscons on PolarFire SoC soc: microchip: add mfd drivers for two syscon regions on PolarFire SoC reset: mpfs: add non-auxiliary bus probing dt-bindings: clk: microchip: mpfs: remove first reg region clk: move meson clk-regmap implementation to common code clk: microchip: mpfs: use regmap clock types riscv: dts: microchip: fix mailbox description riscv: dts: microchip: convert clock and reset to use syscon .../bindings/clock/microchip,mpfs-clkcfg.yaml | 36 +++--- .../mailbox/microchip,mpfs-mailbox.yaml | 13 +- .../devicetree/bindings/mfd/syscon.yaml | 2 + .../microchip/microchip,mpfs-control-scb.yaml | 44 +++++++ .../microchip,mpfs-mss-top-sysreg.yaml | 54 +++++++++ arch/riscv/boot/dts/microchip/mpfs.dtsi | 34 ++++-- drivers/clk/Kconfig | 4 + drivers/clk/Makefile | 1 + drivers/clk/{meson => }/clk-regmap.c | 2 +- drivers/clk/meson/Kconfig | 46 ++++--- drivers/clk/meson/Makefile | 1 - drivers/clk/meson/a1-peripherals.c | 2 +- drivers/clk/meson/a1-pll.c | 2 +- drivers/clk/meson/axg-aoclk.c | 2 +- drivers/clk/meson/axg-audio.c | 2 +- drivers/clk/meson/axg.c | 2 +- drivers/clk/meson/c3-peripherals.c | 2 +- drivers/clk/meson/c3-pll.c | 2 +- drivers/clk/meson/clk-cpu-dyndiv.c | 2 +- drivers/clk/meson/clk-dualdiv.c | 2 +- drivers/clk/meson/clk-mpll.c | 2 +- drivers/clk/meson/clk-phase.c | 2 +- drivers/clk/meson/clk-pll.c | 2 +- drivers/clk/meson/g12a-aoclk.c | 2 +- drivers/clk/meson/g12a.c | 2 +- drivers/clk/meson/gxbb-aoclk.c | 2 +- drivers/clk/meson/gxbb.c | 2 +- drivers/clk/meson/meson-aoclk.h | 2 +- drivers/clk/meson/meson-eeclk.c | 2 +- drivers/clk/meson/meson-eeclk.h | 2 +- drivers/clk/meson/meson8-ddr.c | 2 +- drivers/clk/meson/meson8b.c | 2 +- drivers/clk/meson/s4-peripherals.c | 2 +- drivers/clk/meson/s4-pll.c | 2 +- drivers/clk/meson/sclk-div.c | 2 +- drivers/clk/meson/vclk.h | 2 +- drivers/clk/meson/vid-pll-div.c | 2 +- drivers/clk/microchip/Kconfig | 3 + drivers/clk/microchip/clk-mpfs.c | 114 +++++++++++++----- drivers/mailbox/Kconfig | 1 + drivers/mailbox/mailbox-mpfs.c | 87 ++++++++++--- drivers/reset/reset-mpfs.c | 83 +++++++++++-- drivers/soc/microchip/Kconfig | 13 ++ drivers/soc/microchip/Makefile | 1 + drivers/soc/microchip/mpfs-control-scb.c | 45 +++++++ drivers/soc/microchip/mpfs-mss-top-sysreg.c | 48 ++++++++ .../meson => include/linux/clk}/clk-regmap.h | 0 47 files changed, 541 insertions(+), 143 deletions(-) create mode 100644 Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-control-scb.yaml create mode 100644 Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-mss-top-sysreg.yaml rename drivers/clk/{meson => }/clk-regmap.c (99%) create mode 100644 drivers/soc/microchip/mpfs-control-scb.c create mode 100644 drivers/soc/microchip/mpfs-mss-top-sysreg.c rename {drivers/clk/meson => include/linux/clk}/clk-regmap.h (100%)