Re: [PATCH RFC PoC 0/2] platform: different approach to early platform drivers

[Bartosz Golaszewski]

2018-04-27 16:48 GMT+02:00 Arnd Bergmann <arnd@xxxxxxxx>:
> On Fri, Apr 27, 2018 at 4:05 PM, Bartosz Golaszewski
> <bgolaszewski@xxxxxxxxxxxx> wrote:
>> 2018-04-27 14:40 GMT+02:00 Arnd Bergmann <arnd@xxxxxxxx>:
>
>>> For timer-ti-dm, it seems like a leftover from old times that can
>>> be removed. The other four are shared between arch/sh and
>>> arch/arm/mach-shmobile and already have some #ifdef
>>> to handle those two cases.
>>>
>>
>> I'm also seeing that we also call early_platform_cleanup() from
>> platform_bus_init(). Any ideas for this one?
>
> My first idea would be to call it immediately after registering all
> devices and drivers. It looks like it's only needed to make all
> devm_ allocations persistent by removing them from the list,
> so we have to call early_platform_cleanup() before getting
> to the real platform code, but it could be done much earlier
> if we want to, at least after both setup_arch() and sh_late_time_init()
> are complete.
>
>>> I'd rather keep those separate and would prefer not to have
>>> that many different ways of getting there instead. DT and
>>> board files can already share most of the code through the
>>> use of platform_device, especially when you start using
>>> device properties instead of platform_data, and the other
>>> two are rare corner cases and ideally left that way.
>>>
>>> The early boot code is always special and instead of making
>>> it easier to use, we should focus on using it as little as
>>> possible: every line of code that we call before even
>>> initializing timers and consoles means it gets harder to
>>> debug when something goes wrong.
>>>
>>
>> I'm afraid I don't quite understand your reasoning. I fully agree that
>> devices that need to be initialized that early are a rare corner case.
>> We should limit any such uses to the absolute minimum. But when we do
>> need to go this way, we should do it right. Having a unified mechanism
>> for early devices will allow maintainers to enforce good practices
>> (using resources for register mapping, devres, reusing driver code for
>> reading/writing to registers). Having initialization code in machine
>> code will make everybody use different APIs and duplicate solutions. I
>> normally assume that code consolidation is always good.
>>
>> If we add a way for DT-based platform devices to be probed early - it
>> would be based on platform device/driver structures anyway. Why would
>> we even want to not convert the board code into a simple call to
>> early_platform_device_register() if we'll already offer this API for
>> device tree?
>
> I think we first need to define what we really want to achieve here.
> It sounds like you still want to recreate a lot of what early_platform
> devices do, but it seems more important to me to add the missing
> functionality to the OF_DECLARE infrastructure. The most
> important pieces that seem to be missing are solved by finding
> a way to provide a platform_device pointer with the following
> properties:
>
> - allow being passed into dev_print()
> - allow using the pointer as a token for devres unwinding
> - access to device_private data that remains persistent
>   until real a platform_driver gets loaded
>
> That can probably be done as an extension to the current
> infrastructure.
>
> However, I'd be very cautious about the resource portion:
> filling the platform resources (registers, irqs, ...) the way
> we do for regular devices is much harder and can introduce
> additional (or circular) dependencies on other devices.
> OTOH, not using those resources means you have a hard
> time passing information from board files.
>
>           Arnd

So speaking in pseudo-C we basically have two ways for an imaginary
future timer driver:

int foo_probe(struct platform_device *pdev)
{
    struct clk *clk;

    if (probing_early(pdev)) {
        clk = devm_clk_get(dev, "earlyclock");

       /* Do early stuff. */
        return 0;
    }

    /* Do late stuff. */

    return 0;
}

    --- vs ---

int foo_probe(struct platform_device *pdev)
{
    /* Do late stuff. */

    return 0;
}

static int foo_init(struct device_node *np)
{
    struct clk *clk;
    struct device *dev = device_from_device_node(np);

    /* Do early stuff. */
    clk = devm_clk_get(dev, "earlyclock");

    return 0;
}

TIMER_OF_DECLARE(foo, "bar,foo", foo_init);

I still believe the first approach is easier to implement and has the
added benefit of supporting board files.

I'll give it a thought and will be back at it next week.

Best regards,
Bartosz Golaszewski