Hi Heiko and Joachim, > -----Original Message----- > From: Heiko Stübner [mailto:heiko@xxxxxxxxx] > Sent: Thursday, September 25, 2014 5:52 PM > To: Pankaj Dubey > Cc: Joachim Eastwood; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-samsung- > soc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; kgene.kim@xxxxxxxxxxx; > Russell King - ARM Linux; Arnd Bergmann; naushad@xxxxxxxxxxx; > b29396@xxxxxxxxxxxxx; tomasz.figa@xxxxxxxxx; joshi@xxxxxxxxxxx; > thomas.ab@xxxxxxxxxxx; Li.Xiubo@xxxxxxxxxxxxx; vikas.sajjan@xxxxxxxxxxx; > chow.kim@xxxxxxxxxxx; lee.jones@xxxxxxxxxx; dianders@xxxxxxxxxxxx > 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@xxxxxxxxxxx> > 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@xxxxxxxx> > > > > Suggested-by: Tomasz Figa <tomasz.figa@xxxxxxxxx> > > > > Tested-by: Vivek Gautam <gautam.vivek@xxxxxxxxxxx> > > > > Tested-by: Javier Martinez Canillas > > > > <javier.martinez@xxxxxxxxxxxxxxx> > > > > Signed-off-by: Pankaj Dubey <pankaj.dubey@xxxxxxxxxxx> > > > > > > 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@xxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |