Re: [PATCH v5] mfd: syscon: Decouple syscon interface from platform devices

[Joachim Eastwood]

On 26 September 2014 06:56, Pankaj Dubey <pankaj.dubey@xxxxxxxxxxx> wrote:
> 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.

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/