| Message ID | 4389453.TP96FZcCbx@phil (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Heiko and Joachim, > -----Original Message----- > From: Heiko Stübner [mailto:heiko@sntech.de] > Sent: Thursday, September 25, 2014 5:52 PM > To: Pankaj Dubey > Cc: Joachim Eastwood; linux-arm-kernel@lists.infradead.org; linux-samsung- > soc@vger.kernel.org; linux-kernel@vger.kernel.org; kgene.kim@samsung.com; > Russell King - ARM Linux; Arnd Bergmann; naushad@samsung.com; > b29396@freescale.com; tomasz.figa@gmail.com; joshi@samsung.com; > thomas.ab@samsung.com; Li.Xiubo@freescale.com; vikas.sajjan@samsung.com; > chow.kim@samsung.com; lee.jones@linaro.org; dianders@chromium.org > Subject: Re: [PATCH v5] mfd: syscon: Decouple syscon interface from platform > devices > > Am Mittwoch, 24. September 2014, 20:35:10 schrieb Heiko Stübner: > > Hi Pankaj, Joachim, > > > > Am Dienstag, 23. September 2014, 20:12:50 schrieb Joachim Eastwood: > > > On 22 September 2014 06:40, Pankaj Dubey <pankaj.dubey@samsung.com> > wrote: > > > > Currently a syscon entity can be only registered directly through > > > > a platform device that binds to a dedicated syscon driver. However > > > > in certain use cases it is desirable to make a device used with > > > > another driver a syscon interface provider. > > > > > > > > For example, certain SoCs (e.g. Exynos) contain system controller > > > > blocks which perform various functions such as power domain > > > > control, CPU power management, low power mode control, but in > > > > addition contain certain IP integration glue, such as various > > > > signal masks, coprocessor power control, etc. In such case, there > > > > is a need to have a dedicated driver for such system controller > > > > but also share registers with other drivers. The latter is where the syscon > interface is helpful. > > > > > > > > In case of DT based platforms, this patch decouples syscon object > > > > from syscon platform driver, and allows to create syscon objects > > > > first time when it is required by calling of > > > > syscon_regmap_lookup_by APIs and keep a list of such syscon > > > > objects along with syscon provider device_nodes and regmap handles. > > > > > > > > For non-DT based platforms, this patch keeps syscon platform > > > > driver structure where is can be probed and such non-DT based > > > > drivers can use syscon_regmap_lookup_by_pdev API and get access to > regmap handles. > > > > Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT > > > > based, we can completly remove platform driver of syscon, and keep > > > > only helper functions to get regmap handles. > > > > > > > > Suggested-by: Arnd Bergmann <arnd@arndb.de> > > > > Suggested-by: Tomasz Figa <tomasz.figa@gmail.com> > > > > Tested-by: Vivek Gautam <gautam.vivek@samsung.com> > > > > Tested-by: Javier Martinez Canillas > > > > <javier.martinez@collabora.co.uk> > > > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > > > > > > I wrote a clk driver using syscon and your patch. clk driver uses > > > CLK_OF_DECLARE, btw. > > > > > > It works but I get a '(null): Failed to create debugfs directory' > > > message in the boot log. > > > > > > Tested-by: Joachim Eastwood <manabian@gmail.com> > > > > on Rockchip platforms this syscon support also helps quite a bit, as > > the pll lock-status is sitting in an external syscon register, so > > setting target pll-rates through assigned-clocks is not easily doable without it. > > Therefore I'm very much looking forward to this. > > > > > > Similar to Joachim I get an error about debugfs from regmap, which > > seems to be caused by > > name = dev_name(map->dev); > > returning NULL in regmap_debugfs_init in regmap-debugfs.c for such an > > "early" syscon. > > It looks like of_device_make_bus_id would be able to do the necessary steps to > populate the dev_name seemingly correctly. > > With the diff below I now get a syscon that can init clocks and also a sane regmap > debugfs init: > > /debug/regmap # ls -la > total 0 > drwxr-xr-x 5 0 0 0 Jan 1 1970 . > drwx------ 19 0 0 0 Jan 1 1970 .. > drwxr-xr-x 2 0 0 0 Jan 1 1970 0-001b > drwxr-xr-x 2 0 0 0 Jan 1 1970 ff730000.power-management > drwxr-xr-x 2 0 0 0 Jan 1 1970 ff770000.syscon > > > But of course I don't know enough about device-internals to determine if this is an > insane solution or not :-) > Thanks Heiko for figuring out issue and proposed solution. As you and Joachim pointed out that current patch failed to create regmap debugfs entry, I also investigated and found that it fails to create regmap debugfs entry either you call it early (from init_irq or clk_init function) or you call it in later stage before actual device is populated (from init_machine before of_platform_populate_device). One point is regmap debugfs code should have handled it gracefully instead of kernel panic, so looks like it needs some fix in that part of code. I tried Heiko's suggested solution of calling "of_device_make_bus_id" after platform_device_alloc and it worked well and I tested it from init_irq as well as clk_init, which happens at very early stage. Maybe Joachim can also try if it's working for him. Only concerns for this approach: Is it proper way of doing this? In my opinion it could be, if we are not getting any other approach of handling early syscon. I also tried to get any other solution for handling debugfs entry, and found one more solution. But it will work only for late users of syscon: pdev = platform_device_alloc("dummy-syscon", -1); ret = platform_device_add(pdev); It can solve issue of regmap debugfs for late users, i.e. if we try to use syscon_lookup_by APIs not before init_machine. But this will not work for early users of syscon, as I can see that "platform_device_add" fails (kernel panic) if we call it from init_irq or clk_init. Meanwhile I have posted a fix [1] for this so that if someone calls platform_device_add at very early stage at least it should not panic and kernel should handle it gracefully. [1]: https://lkml.org/lkml/2014/9/24/348 Also as I mentioned earlier, providing feature of early syscon availability was not the main objective of this patch, so just to make sure that this should not be a blocking factor for current patch I would like to split patch in two steps to address to different issues: 1: First patch will cover existing users and allow any such users to create actual platform device and register their own platform driver, instead of syscon getting probed first. I will make sure that it should not break existing users or any existing functionality (including debugfs). 2: Second on top of this patch we can have discussion how to go for early syscon users. Thanks, Pankaj Dubey > > Heiko > > > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c index 8ebc1c6..3734434 > 100644 > --- a/drivers/mfd/syscon.c > +++ b/drivers/mfd/syscon.c > @@ -73,6 +73,7 @@ static struct syscon *of_syscon_register(struct device_node > *np) > goto err_pdev; > } > pdev->dev.of_node = of_node_get(np); > + of_device_make_bus_id(&pdev->dev); > } > > regmap = devm_regmap_init_mmio(&pdev->dev, base, > &syscon_regmap_config); -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26 September 2014 06:56, Pankaj Dubey <pankaj.dubey@samsung.com> wrote: > Hi Heiko and Joachim, > >> -----Original Message----- >> From: Heiko Stübner [mailto:heiko@sntech.de] >> Sent: Thursday, September 25, 2014 5:52 PM >> To: Pankaj Dubey >> Cc: Joachim Eastwood; linux-arm-kernel@lists.infradead.org; linux-samsung- >> soc@vger.kernel.org; linux-kernel@vger.kernel.org; kgene.kim@samsung.com; >> Russell King - ARM Linux; Arnd Bergmann; naushad@samsung.com; >> b29396@freescale.com; tomasz.figa@gmail.com; joshi@samsung.com; >> thomas.ab@samsung.com; Li.Xiubo@freescale.com; vikas.sajjan@samsung.com; >> chow.kim@samsung.com; lee.jones@linaro.org; dianders@chromium.org >> Subject: Re: [PATCH v5] mfd: syscon: Decouple syscon interface from > platform >> devices >> >> Am Mittwoch, 24. September 2014, 20:35:10 schrieb Heiko Stübner: >> > Hi Pankaj, Joachim, >> > >> > Am Dienstag, 23. September 2014, 20:12:50 schrieb Joachim Eastwood: >> > > On 22 September 2014 06:40, Pankaj Dubey <pankaj.dubey@samsung.com> >> wrote: >> > > > Currently a syscon entity can be only registered directly through >> > > > a platform device that binds to a dedicated syscon driver. However >> > > > in certain use cases it is desirable to make a device used with >> > > > another driver a syscon interface provider. >> > > > >> > > > For example, certain SoCs (e.g. Exynos) contain system controller >> > > > blocks which perform various functions such as power domain >> > > > control, CPU power management, low power mode control, but in >> > > > addition contain certain IP integration glue, such as various >> > > > signal masks, coprocessor power control, etc. In such case, there >> > > > is a need to have a dedicated driver for such system controller >> > > > but also share registers with other drivers. The latter is where the > syscon >> interface is helpful. >> > > > >> > > > In case of DT based platforms, this patch decouples syscon object >> > > > from syscon platform driver, and allows to create syscon objects >> > > > first time when it is required by calling of >> > > > syscon_regmap_lookup_by APIs and keep a list of such syscon >> > > > objects along with syscon provider device_nodes and regmap handles. >> > > > >> > > > For non-DT based platforms, this patch keeps syscon platform >> > > > driver structure where is can be probed and such non-DT based >> > > > drivers can use syscon_regmap_lookup_by_pdev API and get access to >> regmap handles. >> > > > Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT >> > > > based, we can completly remove platform driver of syscon, and keep >> > > > only helper functions to get regmap handles. >> > > > >> > > > Suggested-by: Arnd Bergmann <arnd@arndb.de> >> > > > Suggested-by: Tomasz Figa <tomasz.figa@gmail.com> >> > > > Tested-by: Vivek Gautam <gautam.vivek@samsung.com> >> > > > Tested-by: Javier Martinez Canillas >> > > > <javier.martinez@collabora.co.uk> >> > > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> >> > > >> > > I wrote a clk driver using syscon and your patch. clk driver uses >> > > CLK_OF_DECLARE, btw. >> > > >> > > It works but I get a '(null): Failed to create debugfs directory' >> > > message in the boot log. >> > > >> > > Tested-by: Joachim Eastwood <manabian@gmail.com> >> > >> > on Rockchip platforms this syscon support also helps quite a bit, as >> > the pll lock-status is sitting in an external syscon register, so >> > setting target pll-rates through assigned-clocks is not easily doable > without it. >> > Therefore I'm very much looking forward to this. >> > >> > >> > Similar to Joachim I get an error about debugfs from regmap, which >> > seems to be caused by >> > name = dev_name(map->dev); >> > returning NULL in regmap_debugfs_init in regmap-debugfs.c for such an >> > "early" syscon. >> >> It looks like of_device_make_bus_id would be able to do the necessary > steps to >> populate the dev_name seemingly correctly. >> >> With the diff below I now get a syscon that can init clocks and also a > sane regmap >> debugfs init: >> >> /debug/regmap # ls -la >> total 0 >> drwxr-xr-x 5 0 0 0 Jan 1 1970 . >> drwx------ 19 0 0 0 Jan 1 1970 .. >> drwxr-xr-x 2 0 0 0 Jan 1 1970 0-001b >> drwxr-xr-x 2 0 0 0 Jan 1 1970 > ff730000.power-management >> drwxr-xr-x 2 0 0 0 Jan 1 1970 ff770000.syscon >> >> >> But of course I don't know enough about device-internals to determine if > this is an >> insane solution or not :-) >> > > Thanks Heiko for figuring out issue and proposed solution. > > As you and Joachim pointed out that current patch failed to create regmap > debugfs entry, > I also investigated and found that it fails to create regmap debugfs entry > either you call it > early (from init_irq or clk_init function) or you call it in later stage > before actual device is > populated (from init_machine before of_platform_populate_device). > > One point is regmap debugfs code should have handled it gracefully instead > of kernel panic, > so looks like it needs some fix in that part of code. Just for the records. My kernel didn't panic. Don't know why it behaves different from Heiko's kernel but I was able to boot into user space with your patch. I wouldn't have given my 'Tested-by' if didn't boot properly. I am working on Cortex-M4 no-MMU platform that isn't upstream yet, btw. regards Joachim Eastwood -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 26 September 2014 07:34:12 Joachim Eastwood wrote: > I am working on Cortex-M4 no-MMU platform that isn't upstream yet, btw. > Sorry for drifting off-topic, but this is very interesting to me. Can you say which one you are working on and what your timeline is for submitting it upstream? I generally like to know about new platforms early, and sometimes we get multiple people working on the same one. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26 September 2014 09:16, Arnd Bergmann <arnd@arndb.de> wrote: > On Friday 26 September 2014 07:34:12 Joachim Eastwood wrote: >> I am working on Cortex-M4 no-MMU platform that isn't upstream yet, btw. >> > > Sorry for drifting off-topic, but this is very interesting to me. Can you > say which one you are working on and what your timeline is for submitting > it upstream? It's NXP LPC18xx/43xx which is Cortex-M3/M4. 3.19 or 3.20 might be target. Right now everything is in a github repository here: https://github.com/manabian/linux-lpc Most stuff are working now and I am in the process of clean it up and adding documentation. > I generally like to know about new platforms early, and sometimes we get > multiple people working on the same one. I see. regards, Joachim Eastwood -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 26 September 2014 09:48:24 Joachim Eastwood wrote: > On 26 September 2014 09:16, Arnd Bergmann <arnd@arndb.de> wrote: > > On Friday 26 September 2014 07:34:12 Joachim Eastwood wrote: > >> I am working on Cortex-M4 no-MMU platform that isn't upstream yet, btw. > >> > > > > Sorry for drifting off-topic, but this is very interesting to me. Can you > > say which one you are working on and what your timeline is for submitting > > it upstream? > > It's NXP LPC18xx/43xx which is Cortex-M3/M4. > > 3.19 or 3.20 might be target. Ah, very nice! > Right now everything is in a github repository here: > https://github.com/manabian/linux-lpc > > Most stuff are working now and I am in the process of clean it up and > adding documentation. Ok, looks like you are making good progress. I noticed three high-level issues that you may want to address: - the watchdog driver should use the generic watchdog framework rather than registering a misc device. - the ehci glue can probably go away if you make very small changes to the generic ehci platform driver - I don't like the way that the stmmac glue drivers are added, I thought we had fixed this before but I think I need to dig up old emails. The driver should really be a loadable module that hooks calls into the common code rather than being linked into one module. I also have a plan for doing multiplatform builds of nommu kernels, for build testing mostly, I wouldn't expect you to run that configuration. No need for you to address that yourself though, we'll get there. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c index 8ebc1c6..3734434 100644 --- a/drivers/mfd/syscon.c +++ b/drivers/mfd/syscon.c @@ -73,6 +73,7 @@ static struct syscon *of_syscon_register(struct device_node *np) goto err_pdev; } pdev->dev.of_node = of_node_get(np); + of_device_make_bus_id(&pdev->dev); } regmap = devm_regmap_init_mmio(&pdev->dev, base, &syscon_regmap_config);