| Message ID | 54787A8A.6040209@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Fri, Nov 28, 2014 at 02:37:14PM +0100, Hans de Goede wrote: > Hi, > > On 11/27/2014 08:05 PM, Maxime Ripard wrote: > >Hi, > > > >On Thu, Nov 27, 2014 at 11:10:51AM +0100, Hans de Goede wrote: > >>Hi, > >> > >>On 11/27/2014 10:28 AM, Chen-Yu Tsai wrote: > >>>Hi, > >>> > >>>On Thu, Nov 27, 2014 at 4:41 PM, Hans de Goede <hdegoede@redhat.com> wrote: > >> > >><snip> > >> > >>>>I notice that you've not responded to my proposal to simple make the clock > >>>>node a child node of the clocks node in the dt, that should work nicely, and > >>>>avoid the need for any kernel level changes to support it, I'm beginning to > >>>>think that that is probably the best solution. > >>> > >>>Would that not cause an overlap of the io regions, and cause one of them > >>>to fail? AFAIK the OF subsystem doesn't like overlapping resources. > >> > >>No the overlap check is done by the platform dev resource code, and of_clk_declare > >>does not use that. So the overlap would be there, but not an issue (in theory > >>I did not test this). > > > >An overlap is always an issue. > > > >>Thinking more about this, I believe that using the MFD framework for the prcm seems > >>like a mistake to me. It gains us nothing, since we have no irq to de-multiplex or > >>some such. We're not using MFD for the CMU, why use it for CMU2 (which the prcm > >>effectively is) ? > > > >Because the PRCM is much more than that. It also handles the power > >domains for example. > > Ok, so thinking more about this, I'm still convinced that the MFD > framework is only getting in the way here. You still haven't said of what exactly it's getting in the way of. > But I can see having things represented in devicetree properly, with > the clocks, etc. as child nodes of the prcm being something which we > want. Clocks and reset are the only thing set so far, because we need reference to them from the DT itself, nothing more. We could very much have more devices instatiated from the MFD itself. > So since all we are using the MFD for is to instantiate platform > devices under the prcm nodes, and assign an io resource for the regs > to them, why not simply make the prcm node itself a simple-bus. No, this is really not a bus. It shouldn't be described at all as such. It is a device, that has multiple functionnalities in the system => MFD. It really is that simple. > This does everything the MFD prcm driver currently does, without > actually needing a specific kernel driver, and as added bonus this > will move the definition of the mfd function reg offsets out of the > kernel and into the devicetree where they belong in the first place. Which was nacked in the first place because such offsets are not supposed to be in the DT. Really, we have something that work here, there's no need to refactor it. > Please see the attached patches, I've tested this on sun6i, if we go > this route we should make the same change on sun8i (I can make the > change, but not test it). > > Regards, > > Hans > From 6756574293a1f291a8dcc29427b27f32f83acb2d Mon Sep 17 00:00:00 2001 > From: Hans de Goede <hdegoede@redhat.com> > Date: Fri, 28 Nov 2014 13:48:58 +0100 > Subject: [PATCH v2 1/2] ARM: dts: sun6i: Change prcm node into a simple-bus > > The prcm node's purpose is to group the various prcm sub-devices together, > it does not need any special handling beyond that, there is no need to > handle shared resources like a shared (multiplexed) interrupt or a shared > i2c bus. > > As such there really is no need to have a separate compatible for it, using > simple-bus for it works fine. This also allows us to specify the register > offsets of the various child-devices directly into the dts, rather then having > to specify them in the OS implementation, putting the register offsets where > the belong. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > arch/arm/boot/dts/sun6i-a31.dtsi | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi > index 29e6438..4b8541f 100644 > --- a/arch/arm/boot/dts/sun6i-a31.dtsi > +++ b/arch/arm/boot/dts/sun6i-a31.dtsi > @@ -846,11 +846,15 @@ > }; > > prcm@01f01400 { > - compatible = "allwinner,sun6i-a31-prcm"; > + compatible = "simple-bus"; And this breaks the SMP bringup code. Maxime
Hi, On 12/02/2014 04:45 PM, Maxime Ripard wrote: >> Ok, so thinking more about this, I'm still convinced that the MFD >> framework is only getting in the way here. > > You still haven't said of what exactly it's getting in the way of. Of using of_clk_define to bind to the mod0 clk in the prcm, because the ir_clk node does not have its own reg property when the mfd framework is used and of_clk_define requires the node to have its own reg property. >> But I can see having things represented in devicetree properly, with >> the clocks, etc. as child nodes of the prcm being something which we >> want. > > Clocks and reset are the only thing set so far, because we need > reference to them from the DT itself, nothing more. > > We could very much have more devices instatiated from the MFD itself. > >> So since all we are using the MFD for is to instantiate platform >> devices under the prcm nodes, and assign an io resource for the regs >> to them, why not simply make the prcm node itself a simple-bus. > > No, this is really not a bus. It shouldn't be described at all as > such. It is a device, that has multiple functionnalities in the system > => MFD. It really is that simple. Ok, I can live with that, but likewise the clocks node is not a bus either! So it should not have a simple-bus compatible either, and as such we cannot simply change the mod0 driver from of_clk_define to a platform driver because then we need to instantiate platform devs for the mod0 clock nodes, which means making the clock node a simple-bus. I can see your logic in wanting the ir_clk prcm sub-node to use the mod0 compatible string, so how about we make the mod0 driver both register through of_declare and as a platform driver. Note this means that it will try to bind twice to the ir_clk node, since of_clk_declare will cause it to try and bind there too AFAIK. The of_clk_declare bind will fail though because there is no regs property, so this double bind is not an issue as long as we do not log errors on the first bind failure. Note that the ir_clk node will still need an "ir-clk" compatible as well for the MFD to find it and assign the proper resources to it. But this way we will have the clk driver binding to the mod0 clk compatible, which is what you want, while having the MFD assign resources on the fact that it is the ir-clk node, so that things will still work if there are multiple mod0 clks in the prcm. >> This does everything the MFD prcm driver currently does, without >> actually needing a specific kernel driver, and as added bonus this >> will move the definition of the mfd function reg offsets out of the >> kernel and into the devicetree where they belong in the first place. > > Which was nacked in the first place because such offsets are not > supposed to be in the DT. > > Really, we have something that work here, there's no need to refactor > it. Ok, but that does bring us back to the original problem wrt the ir-clk, see above for how I think we should solve this then. If you agree I can implement the proposed fix. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 03, 2014 at 10:49:20AM +0100, Hans de Goede wrote: > Hi, > > On 12/02/2014 04:45 PM, Maxime Ripard wrote: > > >> Ok, so thinking more about this, I'm still convinced that the MFD > >>framework is only getting in the way here. > > > >You still haven't said of what exactly it's getting in the way of. > > Of using of_clk_define to bind to the mod0 clk in the prcm, because the > ir_clk node does not have its own reg property when the mfd framework is > used and of_clk_define requires the node to have its own reg property. Yes. And the obvious solution to that is to just not use OF_CLK_DECLARE but a platform_driver, which is totally fine. > > >>But I can see having things represented in devicetree properly, with > >>the clocks, etc. as child nodes of the prcm being something which we > >>want. > > > >Clocks and reset are the only thing set so far, because we need > >reference to them from the DT itself, nothing more. > > > >We could very much have more devices instatiated from the MFD itself. > > > >>So since all we are using the MFD for is to instantiate platform > >>devices under the prcm nodes, and assign an io resource for the regs > >>to them, why not simply make the prcm node itself a simple-bus. > > > >No, this is really not a bus. It shouldn't be described at all as > >such. It is a device, that has multiple functionnalities in the system > >=> MFD. It really is that simple. > > Ok, I can live with that, but likewise the clocks node is not a bus either! True, and just like we just saw with Chen-Yu patches, it can even leak implementation details. Which was one of the arguments from Mark against it iirc. > So it should not have a simple-bus compatible either, and as such we cannot > simply change the mod0 driver from of_clk_define to a platform driver because > then we need to instantiate platform devs for the mod0 clock nodes, which > means making the clock node a simple-bus. I guess we can do that as a temporary measure until we get things right on that front. I'm totally open to doing that work, so I'm not asking you to do it. > I can see your logic in wanting the ir_clk prcm sub-node to use the > mod0 compatible string, so how about we make the mod0 driver both > register through of_declare and as a platform driver. Note this means > that it will try to bind twice to the ir_clk node, since of_clk_declare > will cause it to try and bind there too AFAIK. Hmmm, I could live with that for a while too. That shouldn't even require too much work, since the first thing we check in the mod0 code is that we actually have something in reg, which will not be the case in the OF_CLK_DECLARE case. > The of_clk_declare bind will fail though because there is no regs > property, so this double bind is not an issue as long as we do not > log errors on the first bind failure. Yep, exactly. > Note that the ir_clk node will still need an "ir-clk" compatible as > well for the MFD to find it and assign the proper resources to it. No, it really doesn't. At least for now, we have a single mod0 clock under the PRCM MFD. If (and only if) one day, we find ourselves in a position where we have two mod0 clocks under the PRCM, then we'll fix the MFD code to deal with that, because it really should deal with it. > But this way we will have the clk driver binding to the mod0 clk > compatible, which is what you want, while having the MFD assign > resources on the fact that it is the ir-clk node, so that things > will still work if there are multiple mod0 clks in the prcm. I really think having a different compatible is both wrong on a theoritical point of view and doesn't bring anything to the table now (and might never do) Maxime
Hi, On 07-12-14 19:08, Maxime Ripard wrote: > On Wed, Dec 03, 2014 at 10:49:20AM +0100, Hans de Goede wrote: <snip> >> So it should not have a simple-bus compatible either, and as such we cannot >> simply change the mod0 driver from of_clk_define to a platform driver because >> then we need to instantiate platform devs for the mod0 clock nodes, which >> means making the clock node a simple-bus. > > I guess we can do that as a temporary measure until we get things > right on that front. I'm totally open to doing that work, so I'm not > asking you to do it. > >> I can see your logic in wanting the ir_clk prcm sub-node to use the >> mod0 compatible string, so how about we make the mod0 driver both >> register through of_declare and as a platform driver. Note this means >> that it will try to bind twice to the ir_clk node, since of_clk_declare >> will cause it to try and bind there too AFAIK. > > Hmmm, I could live with that for a while too. That shouldn't even > require too much work, since the first thing we check in the mod0 code > is that we actually have something in reg, which will not be the case > in the OF_CLK_DECLARE case. > >> The of_clk_declare bind will fail though because there is no regs >> property, so this double bind is not an issue as long as we do not >> log errors on the first bind failure. > > Yep, exactly. > >> Note that the ir_clk node will still need an "ir-clk" compatible as >> well for the MFD to find it and assign the proper resources to it. > > No, it really doesn't. At least for now, we have a single mod0 clock > under the PRCM MFD. If (and only if) one day, we find ourselves in a > position where we have two mod0 clocks under the PRCM, then we'll fix > the MFD code to deal with that, because it really should deal with it. Ok, using only the mod0 compat string works for me. I'll respin my patch-set (minus the one patch you've already merged) to make the modo clk driver use both of_clk_declare and make it a platfrom driver, and use the mod0 compat string for the ir-clk node. Not sure when I'll get this done exactly though, but we still have a while before 3.20 :) Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 08, 2014 at 09:19:02AM +0100, Hans de Goede wrote: > Hi, > > On 07-12-14 19:08, Maxime Ripard wrote: > >On Wed, Dec 03, 2014 at 10:49:20AM +0100, Hans de Goede wrote: > > <snip> > > >>So it should not have a simple-bus compatible either, and as such we cannot > >>simply change the mod0 driver from of_clk_define to a platform driver because > >>then we need to instantiate platform devs for the mod0 clock nodes, which > >>means making the clock node a simple-bus. > > > >I guess we can do that as a temporary measure until we get things > >right on that front. I'm totally open to doing that work, so I'm not > >asking you to do it. > > > >>I can see your logic in wanting the ir_clk prcm sub-node to use the > >>mod0 compatible string, so how about we make the mod0 driver both > >>register through of_declare and as a platform driver. Note this means > >>that it will try to bind twice to the ir_clk node, since of_clk_declare > >>will cause it to try and bind there too AFAIK. > > > >Hmmm, I could live with that for a while too. That shouldn't even > >require too much work, since the first thing we check in the mod0 code > >is that we actually have something in reg, which will not be the case > >in the OF_CLK_DECLARE case. > > > >>The of_clk_declare bind will fail though because there is no regs > >>property, so this double bind is not an issue as long as we do not > >>log errors on the first bind failure. > > > >Yep, exactly. > > > >>Note that the ir_clk node will still need an "ir-clk" compatible as > >>well for the MFD to find it and assign the proper resources to it. > > > >No, it really doesn't. At least for now, we have a single mod0 clock > >under the PRCM MFD. If (and only if) one day, we find ourselves in a > >position where we have two mod0 clocks under the PRCM, then we'll fix > >the MFD code to deal with that, because it really should deal with it. > > Ok, using only the mod0 compat string works for me. I'll respin my > patch-set (minus the one patch you've already merged) to make the modo > clk driver use both of_clk_declare and make it a platfrom driver, and > use the mod0 compat string for the ir-clk node. > > Not sure when I'll get this done exactly though, but we still have > a while before 3.20 :) Indeed :) Thanks! Maxime
From a152b0405d446c748fef146915736e4a8fc548b1 Mon Sep 17 00:00:00 2001 From: Hans de Goede <hdegoede@redhat.com> Date: Fri, 28 Nov 2014 13:54:14 +0100 Subject: [PATCH v2 2/2] ARM: dts: sun6i: Add ir_clk node Add an ir_clk sub-node to the prcm node. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- arch/arm/boot/dts/sun6i-a31.dtsi | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi index 4b8541f..92a1c95 100644 --- a/arch/arm/boot/dts/sun6i-a31.dtsi +++ b/arch/arm/boot/dts/sun6i-a31.dtsi @@ -888,6 +888,14 @@ "apb0_i2c"; }; + ir_clk: ir_clk { + reg = <0x01f01454 0x4>; + #clock-cells = <0>; + compatible = "allwinner,sun4i-a10-mod0-clk"; + clocks = <&osc32k>, <&osc24M>; + clock-output-names = "ir"; + }; + apb0_rst: apb0_rst { compatible = "allwinner,sun6i-a31-clock-reset"; reg = <0x01f014b0 0x4>; -- 2.1.0