| Message ID | bfd173dc-3be5-421c-b7f7-af181bfb7032@DB8EHSMHS026.ehs.local |
|---|---|
| State | New |
| Headers | show |
On 08/23/13 19:19, Sören Brinkmann wrote: > On Fri, Aug 23, 2013 at 11:30:18AM +0200, Sebastian Hesselbarth wrote: >> On 08/23/13 02:59, Sören Brinkmann wrote: >>> On Thu, Aug 22, 2013 at 05:26:47PM -0700, Sören Brinkmann wrote: >>>> On Tue, Aug 20, 2013 at 04:04:31AM +0200, Sebastian Hesselbarth wrote: >>>>> With arch/arm calling of_clk_init(NULL) from time_init(), we can now >>>>> remove it from corresponding drivers/clk code. >>>> >>>> I think that would break Zynq. >>>> If I see this correctly you call of_clk_init() from common code, >>>> _before_ the SOC specific time init function is called. >>>> The problem is, that we have code setting up a global pointer which is >>>> required by zynq_clk_setup() which is triggered when of_clk_init() is >>>> called. > [ ... ] >> thanks for looking into this. I also had a look at the files in >> question. Based on Steffen's proposal, I prepared a diff that should do >> the trick. It moves zynq_slcr_init() to early_init, instead of reusing >> another hook that has magic cow powers (it calls irqchip_init that zynq >> also wants sooner or later). >> >> Also, it removes zynq_clock_init() and let zynq_clk_setup() map the >> register itself by finding the node and use of_iomap(). I realized that >> clock registers are quite separated within slcr, so you can consider >> to have your own node for the clk-provider. As Steffen is proposing >> this but mentioned incompatible DT changes, I chose that intermediate >> step above. >> >> It would be great, if you test the diff and prepare a patch out of >> it, that I pick-up in the patch set. That way, we also have your >> Signed-off on it. > > I looked into this. Looks like init_early() happens to early. I suspect > slab is missing to make zynq_slcr_init() work. So, I moved it into > init_irq(). Is there any init_call() type which is called at the correct > time? Sören, I mistakenly assumed init_early is after mm, so of course my proposal does not work as it should. I am fine with moving it to init_irq() until you find the best solution (or until we have the same "mess" with default init_irq hook). > I looked briefly into syscon and regmap, and that does actually look > promising and to really fix this mess, I guess we have to wait a little > until Steffen finishes his work on it. IIRC, both syscon and regmap will require you to have devices ready. I haven't followed all recent discussions about early device registration. Anyway, it will not help much in the current approach to get rid of custom .init_timer and maybe .init_irq later. > To facilitate Sebastian's series I came up with the patch below. > The problem I have is, I do not really want the clkc to map the > registers. They are in the SLCR and the SLCR driver is doing it, hence > we should work with what that driver provides - which ideally would be > based on regmap and syscon, but we're not there yet. Hence I somehow > need to pass the SLCR pointer to the clkc. To avoid accessing the global > pointer directly I kept the zynq_clock_init() routine which is called > from zynq_slcr_init(). For this patch set I'd be fine with the proposal below. For the short run, you could consider to hide register accesses to slcr by providing zynq_slcr_readl/writel instead of passing just the base address. But again, that will require either custom .init_time or .init_irq to set up slcr before clocks. > That is the best I could come up with quickly and w/o investing a lot of > time to figure out the regmap and syscon stuff, which seems to be handled > by Steffen already, anyway. > It is essentially a stripped down version of Sebastian's proposal. If there are no general objections, I take that one for the real patch set. Sebastian
On Fri, Aug 23, 2013 at 07:44:03PM +0200, Sebastian Hesselbarth wrote: > On 08/23/13 19:19, Sören Brinkmann wrote: > >On Fri, Aug 23, 2013 at 11:30:18AM +0200, Sebastian Hesselbarth wrote: > >>On 08/23/13 02:59, Sören Brinkmann wrote: > >>>On Thu, Aug 22, 2013 at 05:26:47PM -0700, Sören Brinkmann wrote: > >>>>On Tue, Aug 20, 2013 at 04:04:31AM +0200, Sebastian Hesselbarth wrote: > >>>>>With arch/arm calling of_clk_init(NULL) from time_init(), we can now > >>>>>remove it from corresponding drivers/clk code. > >>>> > >>>>I think that would break Zynq. > >>>>If I see this correctly you call of_clk_init() from common code, > >>>>_before_ the SOC specific time init function is called. > >>>>The problem is, that we have code setting up a global pointer which is > >>>>required by zynq_clk_setup() which is triggered when of_clk_init() is > >>>>called. > >[ ... ] > >>thanks for looking into this. I also had a look at the files in > >>question. Based on Steffen's proposal, I prepared a diff that should do > >>the trick. It moves zynq_slcr_init() to early_init, instead of reusing > >>another hook that has magic cow powers (it calls irqchip_init that zynq > >>also wants sooner or later). > >> > >>Also, it removes zynq_clock_init() and let zynq_clk_setup() map the > >>register itself by finding the node and use of_iomap(). I realized that > >>clock registers are quite separated within slcr, so you can consider > >>to have your own node for the clk-provider. As Steffen is proposing > >>this but mentioned incompatible DT changes, I chose that intermediate > >>step above. > >> > >>It would be great, if you test the diff and prepare a patch out of > >>it, that I pick-up in the patch set. That way, we also have your > >>Signed-off on it. > > > >I looked into this. Looks like init_early() happens to early. I suspect > >slab is missing to make zynq_slcr_init() work. So, I moved it into > >init_irq(). Is there any init_call() type which is called at the correct > >time? > > Sören, > > I mistakenly assumed init_early is after mm, so of course my proposal > does not work as it should. I am fine with moving it to init_irq() until > you find the best solution (or until we have the same "mess" with > default init_irq hook). > > >I looked briefly into syscon and regmap, and that does actually look > >promising and to really fix this mess, I guess we have to wait a little > >until Steffen finishes his work on it. > > IIRC, both syscon and regmap will require you to have devices ready. > I haven't followed all recent discussions about early device > registration. Anyway, it will not help much in the current approach > to get rid of custom .init_timer and maybe .init_irq later. > Yes. The syscon driver can not be used that early. It only makes sense for later stages. Therefore the slcr has to be mapped early to unlock and than remapped later. But with the current drivers, it is not essential to have the slcr as a seperate driver. > >To facilitate Sebastian's series I came up with the patch below. > >The problem I have is, I do not really want the clkc to map the > >registers. They are in the SLCR and the SLCR driver is doing it, hence > >we should work with what that driver provides - which ideally would be > >based on regmap and syscon, but we're not there yet. Hence I somehow > >need to pass the SLCR pointer to the clkc. To avoid accessing the global > >pointer directly I kept the zynq_clock_init() routine which is called > >from zynq_slcr_init(). > > For this patch set I'd be fine with the proposal below. For the short > run, you could consider to hide register accesses to slcr by providing > zynq_slcr_readl/writel instead of passing just the base address. > > But again, that will require either custom .init_time or .init_irq > to set up slcr before clocks. > > >That is the best I could come up with quickly and w/o investing a lot of > >time to figure out the regmap and syscon stuff, which seems to be handled > >by Steffen already, anyway. > >It is essentially a stripped down version of Sebastian's proposal. > > If there are no general objections, I take that one for the real patch > set. I haven't tested this, but I hope Sören did. For the time being, I am okay with it. Regards, Steffen
On Fri, Aug 23, 2013 at 07:44:03PM +0200, Sebastian Hesselbarth wrote: > On 08/23/13 19:19, Sören Brinkmann wrote: > >On Fri, Aug 23, 2013 at 11:30:18AM +0200, Sebastian Hesselbarth wrote: > >>On 08/23/13 02:59, Sören Brinkmann wrote: > >>>On Thu, Aug 22, 2013 at 05:26:47PM -0700, Sören Brinkmann wrote: > >>>>On Tue, Aug 20, 2013 at 04:04:31AM +0200, Sebastian Hesselbarth wrote: > >>>>>With arch/arm calling of_clk_init(NULL) from time_init(), we can now > >>>>>remove it from corresponding drivers/clk code. > >>>> > >>>>I think that would break Zynq. > >>>>If I see this correctly you call of_clk_init() from common code, > >>>>_before_ the SOC specific time init function is called. > >>>>The problem is, that we have code setting up a global pointer which is > >>>>required by zynq_clk_setup() which is triggered when of_clk_init() is > >>>>called. > >[ ... ] > >>thanks for looking into this. I also had a look at the files in > >>question. Based on Steffen's proposal, I prepared a diff that should do > >>the trick. It moves zynq_slcr_init() to early_init, instead of reusing > >>another hook that has magic cow powers (it calls irqchip_init that zynq > >>also wants sooner or later). > >> > >>Also, it removes zynq_clock_init() and let zynq_clk_setup() map the > >>register itself by finding the node and use of_iomap(). I realized that > >>clock registers are quite separated within slcr, so you can consider > >>to have your own node for the clk-provider. As Steffen is proposing > >>this but mentioned incompatible DT changes, I chose that intermediate > >>step above. > >> > >>It would be great, if you test the diff and prepare a patch out of > >>it, that I pick-up in the patch set. That way, we also have your > >>Signed-off on it. > > > >I looked into this. Looks like init_early() happens to early. I suspect > >slab is missing to make zynq_slcr_init() work. So, I moved it into > >init_irq(). Is there any init_call() type which is called at the correct > >time? > > Sören, > > I mistakenly assumed init_early is after mm, so of course my proposal > does not work as it should. I am fine with moving it to init_irq() until > you find the best solution (or until we have the same "mess" with > default init_irq hook). Looking at these two hooks. If the SOC provides init_irq(), the common code does nothing, but calling it. Ergo, the SOC is now responsible for otherwise commonly called code like of_irq_init(). It's probably an idea to design the common init_time() function the same way. That way SOCs that don't need any custom init at that stage get everything for free. And SOCs like Zynq have to still call of_clk_init() manually, but can ensure that dependencies like our SLCR init are satisfied before calling it. Just an idea, not sure if it makes sense since I didn't look beyond Zynq too much on this. Sören
diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c index 5f25256..f28046e 100644 --- a/arch/arm/mach-zynq/common.c +++ b/arch/arm/mach-zynq/common.c @@ -19,10 +19,9 @@ #include <linux/cpumask.h> #include <linux/platform_device.h> #include <linux/clk.h> -#include <linux/clk/zynq.h> -#include <linux/clocksource.h> #include <linux/of_address.h> #include <linux/of_irq.h> +#include <linux/irqchip.h> #include <linux/of_platform.h> #include <linux/of.h> @@ -58,10 +57,10 @@ static void __init zynq_init_machine(void) of_platform_bus_probe(NULL, zynq_of_bus_ids, NULL); } -static void __init zynq_timer_init(void) +static void __init zynq_init_irq(void) { + irqchip_init(); zynq_slcr_init(); - clocksource_of_init(); } static struct map_desc zynq_cortex_a9_scu_map __initdata = { @@ -104,8 +103,8 @@ static const char * const zynq_dt_match[] = { DT_MACHINE_START(XILINX_EP107, "Xilinx Zynq Platform") .smp = smp_ops(zynq_smp_ops), .map_io = zynq_map_io, + .init_irq = zynq_init_irq, .init_machine = zynq_init_machine, - .init_time = zynq_timer_init, .dt_compat = zynq_dt_match, .restart = zynq_system_reset, MACHINE_END diff --git a/drivers/clk/zynq/clkc.c b/drivers/clk/zynq/clkc.c index 089d3e3..53b851e 100644 --- a/drivers/clk/zynq/clkc.c +++ b/drivers/clk/zynq/clkc.c @@ -206,6 +206,9 @@ static void __init zynq_clk_setup(struct device_node *np) pr_info("Zynq clock init\n"); + if (WARN_ON(!zynq_slcr_base_priv)) + return; + /* get clock output names from DT */ for (i = 0; i < clk_max; i++) { if (of_property_read_string_index(np, "clock-output-names", @@ -532,5 +535,4 @@ CLK_OF_DECLARE(zynq_clkc, "xlnx,ps7-clkc", zynq_clk_setup); void __init zynq_clock_init(void __iomem *slcr_base) { zynq_slcr_base_priv = slcr_base; - of_clk_init(NULL); }