[RFC] early init and DT platform devices allocation/registration

[RFC] early init and DT platform devices allocation/registration

[Lorenzo Pieralisi]

Hi all,

I am dealing with a lingering problem related to init and probing of platform
devices early (before initcalls) in the kernel boot process. The problem,
which is nothing new, is related to how platform devices are created in the
kernel from DT and when they become available. Platform devices are created
through (assuming any legacy static initialization is removed from the kernel)

of_platform_populate()

at arch_initcall time on ARM. This is a problem for drivers that need to be
probed and initialized before arch_initcall (ie early_initcall) because
the corresponding platform_device has not been created yet.

To work around this awkward situation, a driver, instead of relying on
platform driver probing mechanism, can get initialized using early_initcall
mechanism and rely on DT helpers (ie of_iomap() and company) to initialize
resources. The problem comes when the driver functions must rely on an API
(eg regmap) that requires a struct device to function properly; since the
platform_device has not been created yet at early_initcall time, the
driver cannot rely on it. The only solution consists in allocating a
platform_device on the fly at driver init through

of_platform_device_create()

and use that device to initialize the (eg regmap) API. There is an issue
with this solution, basically a platform device is allocated twice for
a given device node - compatible property (because of_platform_populate()
allocates a platform device for a node containing a compatible string even if
a platform device has already been allocated for that device node).

The second solution relies on early platform devices. Early platform devices
are added by mach code and driver probing is carried out using the early
platform device probing mechanism

early_platform_driver_probe()

The drawback with this solution is, AFAIK, it does not play well with DT,
since the platform device duplication issue is still there (ie
of_platform_populate() will allocate a platform device which is a duplicate
of the one allocated at early boot to make the early platform device
initialization possible).

Please correct me if I am wrong, just trying to untangle a problem that does
not look easy to solve.

How this problem is supposed to be solved in the kernel ?

1- drivers that are to be up and running at early_initcall time must not
   rely on the device/driver model (but then they cannot use any API that
   requires a struct device to function (eg regmap))
2- the driver should allocate a platform device at early initcall from
   a DT compatible node. Do not know how to deal with platform device
   duplication though, since of_platform_populate() will create another
   platform device when the node is parsed
3- driver should rely on early platform device/driver, but this does not
   solve (?) the platform device duplication problem either, it will happen
   when of_platform_populate() parses the DT and creates devices on the fly

In theory there are other solutions such as:

(a) declaring the platform device statically in arm/mach-* code and do not
    define the device node in dts, basically going back in time to ARM legacy
    kernel mechanism for this kind of devices
(b) add a way to of_platform_populate() to exclude some compatible strings
    from being matched

Honestly there is not a solution I can say I like and maybe I am trying to
solve a problem that has already been solved, apologies if so, that's why
I am asking on the list to people with more knowledge than me on the subject.

Comments are really really welcome, thank you !
Lorenzo

Re: [RFC] early init and DT platform devices allocation/registration

[Grant Likely]

On Mon, Jun 24, 2013 at 5:33 PM, Lorenzo Pieralisi
<lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org> wrote:
> Hi all,
>
> I am dealing with a lingering problem related to init and probing of platform
> devices early (before initcalls) in the kernel boot process. The problem,
> which is nothing new, is related to how platform devices are created in the
> kernel from DT and when they become available. Platform devices are created
> through (assuming any legacy static initialization is removed from the kernel)
>
> of_platform_populate()
>
> at arch_initcall time on ARM. This is a problem for drivers that need to be
> probed and initialized before arch_initcall (ie early_initcall) because
> the corresponding platform_device has not been created yet.
>
> To work around this awkward situation, a driver, instead of relying on
> platform driver probing mechanism, can get initialized using early_initcall
> mechanism and rely on DT helpers (ie of_iomap() and company) to initialize
> resources. The problem comes when the driver functions must rely on an API
> (eg regmap) that requires a struct device to function properly; since the
> platform_device has not been created yet at early_initcall time, the
> driver cannot rely on it. The only solution consists in allocating a
> platform_device on the fly at driver init through
>
> of_platform_device_create()
>
> and use that device to initialize the (eg regmap) API. There is an issue
> with this solution, basically a platform device is allocated twice for
> a given device node - compatible property (because of_platform_populate()
> allocates a platform device for a node containing a compatible string even if
> a platform device has already been allocated for that device node).
>
> The second solution relies on early platform devices. Early platform devices
> are added by mach code and driver probing is carried out using the early
> platform device probing mechanism
>
> early_platform_driver_probe()
>
> The drawback with this solution is, AFAIK, it does not play well with DT,
> since the platform device duplication issue is still there (ie
> of_platform_populate() will allocate a platform device which is a duplicate
> of the one allocated at early boot to make the early platform device
> initialization possible).
>
> Please correct me if I am wrong, just trying to untangle a problem that does
> not look easy to solve.
>
> How this problem is supposed to be solved in the kernel ?
>
> 1- drivers that are to be up and running at early_initcall time must not
>    rely on the device/driver model (but then they cannot use any API that
>    requires a struct device to function (eg regmap))
> 2- the driver should allocate a platform device at early initcall from
>    a DT compatible node. Do not know how to deal with platform device
>    duplication though, since of_platform_populate() will create another
>    platform device when the node is parsed

While I've resisted it in the past, I would be okay with adding struct
device pointer in the device_node structure. I've resisted because I
don't want drivers following the device_node pointer and making an
assumption about what /kind/ of device is pointed to by it. However,
this is an important use case and it makes it feasible to use an early
platform device with of_platform_populate.

Alternately, if others chime in and think it is too risky to have the
device pointer in the device node, we could simply add a flag to the
device_node which indicates the node has been parsed by
of_platform_populate.

There would need to be some careful coding to make sure that any call
to early platform device creation sets the pointer in the device node
correctly.

I would also make it a requirement that any early platform device
*must* be converted into a 'real' platform device during initcall
time. That includes being possible to be freed correctly. Static early
platform device definitions should not be allowed, otherwise there
needs to be a special case for the platform device release hook. I
really don't want that.

Something else that needs to be investigated is how the device
hierarchy will be affected by using early_platform_device. We still
want the platform_device to appear in the same place in the hierarchy
regardless of whether or not it was created 'early'. the question is
how to make sure that actually happens.

> 3- driver should rely on early platform device/driver, but this does not
>    solve (?) the platform device duplication problem either, it will happen
>    when of_platform_populate() parses the DT and creates devices on the fly
>
> In theory there are other solutions such as:
>
> (a) declaring the platform device statically in arm/mach-* code and do not
>     define the device node in dts, basically going back in time to ARM legacy
>     kernel mechanism for this kind of devices

As stated above, no. Static device definitions are not a good idea,
and it doesn't scale in a multiplatform kernel world.

> (b) add a way to of_platform_populate() to exclude some compatible strings
>     from being matched

This method will probably be too error prone. It would be better to
add the check in of_platform_populate to skip nodes that have already
been populated. I can't think of any use-cases where we would want
of_platform_populate to process a single node more than once.

>
> Honestly there is not a solution I can say I like and maybe I am trying to
> solve a problem that has already been solved, apologies if so, that's why
> I am asking on the list to people with more knowledge than me on the subject.

No, it hasn't been solved yet, but it is worth solving. Either that or
figure out how to do of_platform_populate much earlier.

g.

Re: [RFC] early init and DT platform devices allocation/registration

[Stephen Warren]

On 06/25/2013 05:45 AM, Grant Likely wrote:
> On Mon, Jun 24, 2013 at 5:33 PM, Lorenzo Pieralisi
> <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org> wrote:
>> Hi all,
>>
>> I am dealing with a lingering problem related to init and probing of platform
>> devices early (before initcalls) in the kernel boot process. The problem,
>> which is nothing new, is related to how platform devices are created in the
>> kernel from DT and when they become available. Platform devices are created
>> through (assuming any legacy static initialization is removed from the kernel)
>>
>> of_platform_populate()
>>
>> at arch_initcall time on ARM. This is a problem for drivers that need to be
>> probed and initialized before arch_initcall (ie early_initcall) because
>> the corresponding platform_device has not been created yet.

What's the use-case here; why does the driver /have/ to be
allocated/probed so early? Can't drivers all be allocated from DT in the
usual fashion, and any dependencies resolved using deferred probe? In
almost all cases, that should work.

...
>> How this problem is supposed to be solved in the kernel ?
>>
>> 1- drivers that are to be up and running at early_initcall time must not
>>    rely on the device/driver model (but then they cannot use any API that
>>    requires a struct device to function (eg regmap))
>> 2- the driver should allocate a platform device at early initcall from
>>    a DT compatible node. Do not know how to deal with platform device
>>    duplication though, since of_platform_populate() will create another
>>    platform device when the node is parsed
> 
> While I've resisted it in the past, I would be okay with adding struct
> device pointer in the device_node structure. I've resisted because I
> don't want drivers following the device_node pointer and making an
> assumption about what /kind/ of device is pointed to by it. However,
> this is an important use case and it makes it feasible to use an early
> platform device with of_platform_populate.

Hiroshi (who I have CC'd here) has also been asking about this same
issue downstream. The issue for us is that we need to initialize an SMMU
driver before any devices that are translated by the SMMU. One option is
to force the register/probe of the SMMU driver explicitly, early in the
machine's .init_machine() callback, before of_platform_populate(), to
ensure the ordering. Then, we run into the same duplicate device issue,
and the change Grant mentions above would help solve this.

Re: [RFC] early init and DT platform devices allocation/registration

[Hiroshi Doyu]

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Tue, 25 Jun 2013 16:56:22 +0200:

> >> How this problem is supposed to be solved in the kernel ?
> >>
> >> 1- drivers that are to be up and running at early_initcall time must not
> >>    rely on the device/driver model (but then they cannot use any API that
> >>    requires a struct device to function (eg regmap))
> >> 2- the driver should allocate a platform device at early initcall from
> >>    a DT compatible node. Do not know how to deal with platform device
> >>    duplication though, since of_platform_populate() will create another
> >>    platform device when the node is parsed
> > 
> > While I've resisted it in the past, I would be okay with adding struct
> > device pointer in the device_node structure. I've resisted because I
> > don't want drivers following the device_node pointer and making an
> > assumption about what /kind/ of device is pointed to by it. However,
> > this is an important use case and it makes it feasible to use an early
> > platform device with of_platform_populate.
> 
> Hiroshi (who I have CC'd here) has also been asking about this same
> issue downstream. The issue for us is that we need to initialize an SMMU
> driver before any devices that are translated by the SMMU. One option is
> to force the register/probe of the SMMU driver explicitly, early in the
> machine's .init_machine() callback, before of_platform_populate(), to
> ensure the ordering. Then, we run into the same duplicate device issue,
> and the change Grant mentions above would help solve this.

Here's my workaround. I need to call of_detach_node() with OF_DYNAMIC
to avoid duplicated device registration.

>From f4d88b8521c278b41b72028d326c03cfd2e90af8 Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Date: Fri, 14 Jun 2013 15:22:02 +0300
Subject: [PATCH 1/1] ARM: tegra: Populate AHB/IOMMU earlier than others

Populate AHB/IOMMU earlier than others. IOMMU depends on AHB. IOMMU
needs to be instanciated earlier than others so that IOMMU can
register other platform devices as IOMMU'able. Once IOMMU is
populated, IOMMU/AHB nodes are detached to prevent another
registeration.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/mach-tegra/Kconfig |  1 +
 arch/arm/mach-tegra/tegra.c | 24 ++++++++++++++++++++++++
 drivers/iommu/tegra-smmu.c  |  3 +++
 3 files changed, 28 insertions(+)

diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
index ef3a8da..79905fe 100644
--- a/arch/arm/mach-tegra/Kconfig
+++ b/arch/arm/mach-tegra/Kconfig
@@ -15,6 +15,7 @@ config ARCH_TEGRA
 	select SOC_BUS
 	select SPARSE_IRQ
 	select USE_OF
+	select OF_DYNAMIC
 	help
 	  This enables support for NVIDIA Tegra based systems.
 
diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
index 0d1e412..0c494b5 100644
--- a/arch/arm/mach-tegra/tegra.c
+++ b/arch/arm/mach-tegra/tegra.c
@@ -80,6 +80,28 @@ static struct of_dev_auxdata tegra20_auxdata_lookup[] __initdata = {
 	{}
 };
 
+static void tegra_of_platform_populate_iommu(void)
+{
+	int i;
+	struct platform_device *pdev;
+	const char * const dname[] = {"ahb", "iommu", };
+
+	for (i = 0; i < ARRAY_SIZE(dname); i++) {
+		struct device_node *np;
+		char path[NAME_MAX];
+
+		snprintf(path, sizeof(path), "/%s", dname[i]);
+		np  = of_find_node_by_path(path);
+		if (!np)
+			break;
+
+		pdev = of_platform_device_create(np, NULL, NULL);
+		of_node_put(np);
+		if (!pdev)
+			break;
+	}
+}
+
 static void __init tegra_dt_init(void)
 {
 	struct soc_device_attribute *soc_dev_attr;
@@ -107,6 +129,8 @@ static void __init tegra_dt_init(void)
 
 	parent = soc_device_to_device(soc_dev);
 
+	tegra_of_platform_populate_iommu();
+
 	/*
 	 * Finished with the static registrations now; fill in the missing
 	 * devices
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index f6f120e..6c9de3f 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -1238,6 +1238,9 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 	smmu_debugfs_create(smmu);
 	smmu_handle = smmu;
 	bus_set_iommu(&platform_bus_type, &smmu_iommu_ops);
+
+	of_detach_node(dev->of_node);
+	of_detach_node(smmu->ahb);
 	return 0;
 }
 
-- 
1.8.1.5

Re: [RFC] early init and DT platform devices allocation/registration

[Grant Likely]

On Tue, Jun 25, 2013 at 5:36 PM, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Tue, 25 Jun 2013 16:56:22 +0200:
>
>> >> How this problem is supposed to be solved in the kernel ?
>> >>
>> >> 1- drivers that are to be up and running at early_initcall time must not
>> >>    rely on the device/driver model (but then they cannot use any API that
>> >>    requires a struct device to function (eg regmap))
>> >> 2- the driver should allocate a platform device at early initcall from
>> >>    a DT compatible node. Do not know how to deal with platform device
>> >>    duplication though, since of_platform_populate() will create another
>> >>    platform device when the node is parsed
>> >
>> > While I've resisted it in the past, I would be okay with adding struct
>> > device pointer in the device_node structure. I've resisted because I
>> > don't want drivers following the device_node pointer and making an
>> > assumption about what /kind/ of device is pointed to by it. However,
>> > this is an important use case and it makes it feasible to use an early
>> > platform device with of_platform_populate.
>>
>> Hiroshi (who I have CC'd here) has also been asking about this same
>> issue downstream. The issue for us is that we need to initialize an SMMU
>> driver before any devices that are translated by the SMMU. One option is
>> to force the register/probe of the SMMU driver explicitly, early in the
>> machine's .init_machine() callback, before of_platform_populate(), to
>> ensure the ordering. Then, we run into the same duplicate device issue,
>> and the change Grant mentions above would help solve this.
>
> Here's my workaround. I need to call of_detach_node() with OF_DYNAMIC
> to avoid duplicated device registration.

Gah! my eyes!

Don't do that. It is incredibly problematic. Look at inhibiting
duplicate device creation instead.

g.

>
> From f4d88b8521c278b41b72028d326c03cfd2e90af8 Mon Sep 17 00:00:00 2001
> From: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Date: Fri, 14 Jun 2013 15:22:02 +0300
> Subject: [PATCH 1/1] ARM: tegra: Populate AHB/IOMMU earlier than others
>
> Populate AHB/IOMMU earlier than others. IOMMU depends on AHB. IOMMU
> needs to be instanciated earlier than others so that IOMMU can
> register other platform devices as IOMMU'able. Once IOMMU is
> populated, IOMMU/AHB nodes are detached to prevent another
> registeration.
>
> Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm/mach-tegra/Kconfig |  1 +
>  arch/arm/mach-tegra/tegra.c | 24 ++++++++++++++++++++++++
>  drivers/iommu/tegra-smmu.c  |  3 +++
>  3 files changed, 28 insertions(+)
>
> diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
> index ef3a8da..79905fe 100644
> --- a/arch/arm/mach-tegra/Kconfig
> +++ b/arch/arm/mach-tegra/Kconfig
> @@ -15,6 +15,7 @@ config ARCH_TEGRA
>         select SOC_BUS
>         select SPARSE_IRQ
>         select USE_OF
> +       select OF_DYNAMIC
>         help
>           This enables support for NVIDIA Tegra based systems.
>
> diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
> index 0d1e412..0c494b5 100644
> --- a/arch/arm/mach-tegra/tegra.c
> +++ b/arch/arm/mach-tegra/tegra.c
> @@ -80,6 +80,28 @@ static struct of_dev_auxdata tegra20_auxdata_lookup[] __initdata = {
>         {}
>  };
>
> +static void tegra_of_platform_populate_iommu(void)
> +{
> +       int i;
> +       struct platform_device *pdev;
> +       const char * const dname[] = {"ahb", "iommu", };
> +
> +       for (i = 0; i < ARRAY_SIZE(dname); i++) {
> +               struct device_node *np;
> +               char path[NAME_MAX];
> +
> +               snprintf(path, sizeof(path), "/%s", dname[i]);
> +               np  = of_find_node_by_path(path);
> +               if (!np)
> +                       break;
> +
> +               pdev = of_platform_device_create(np, NULL, NULL);
> +               of_node_put(np);
> +               if (!pdev)
> +                       break;
> +       }
> +}
> +
>  static void __init tegra_dt_init(void)
>  {
>         struct soc_device_attribute *soc_dev_attr;
> @@ -107,6 +129,8 @@ static void __init tegra_dt_init(void)
>
>         parent = soc_device_to_device(soc_dev);
>
> +       tegra_of_platform_populate_iommu();
> +
>         /*
>          * Finished with the static registrations now; fill in the missing
>          * devices
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index f6f120e..6c9de3f 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -1238,6 +1238,9 @@ static int tegra_smmu_probe(struct platform_device *pdev)
>         smmu_debugfs_create(smmu);
>         smmu_handle = smmu;
>         bus_set_iommu(&platform_bus_type, &smmu_iommu_ops);
> +
> +       of_detach_node(dev->of_node);
> +       of_detach_node(smmu->ahb);
>         return 0;
>  }
>
> --
> 1.8.1.5
>

Re: [RFC] early init and DT platform devices allocation/registration

[Hiroshi Doyu]

Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote @ Tue, 25 Jun 2013 19:52:33 +0200:

> > Here's my workaround. I need to call of_detach_node() with OF_DYNAMIC
> > to avoid duplicated device registration.
> 
> Gah! my eyes!
> 
> Don't do that. It is incredibly problematic. Look at inhibiting
> duplicate device creation instead.

I may not follow this thread correctly, but could anyone point out the
above "inhibiting duplicate device creation" if there's already such
solution?

Re: [RFC] early init and DT platform devices allocation/registration

[Grant Likely]

On Wed, Jun 26, 2013 at 7:00 AM, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote @ Tue, 25 Jun 2013 19:52:33 +0200:
>
>> > Here's my workaround. I need to call of_detach_node() with OF_DYNAMIC
>> > to avoid duplicated device registration.
>>
>> Gah! my eyes!
>>
>> Don't do that. It is incredibly problematic. Look at inhibiting
>> duplicate device creation instead.
>
> I may not follow this thread correctly, but could anyone point out the
> above "inhibiting duplicate device creation" if there's already such
> solution?

No, the solution doesn't exist yet, but it wouldn't be hard to
implement. What you need to do is to add a struct device pointer to
struct device_node, and set the pointer to the struct device when
of_platform_device_create creates a device. (it would also need to be
set for early_platform_device creation, but that's not something that
should affect you). You would also add a check to
of_platform_device_create to check if the device pointer is already
set. If it is, then skip creation of the device.

g.

Re: [RFC] early init and DT platform devices allocation/registration

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1697 bytes --]

On Wed, Jun 26, 2013 at 11:03:20AM +0100, Grant Likely wrote:
> On Wed, Jun 26, 2013 at 7:00 AM, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote @ Tue, 25 Jun 2013 19:52:33 +0200:
> >
> >> > Here's my workaround. I need to call of_detach_node() with OF_DYNAMIC
> >> > to avoid duplicated device registration.
> >>
> >> Gah! my eyes!
> >>
> >> Don't do that. It is incredibly problematic. Look at inhibiting
> >> duplicate device creation instead.
> >
> > I may not follow this thread correctly, but could anyone point out the
> > above "inhibiting duplicate device creation" if there's already such
> > solution?
> 
> No, the solution doesn't exist yet, but it wouldn't be hard to
> implement. What you need to do is to add a struct device pointer to
> struct device_node, and set the pointer to the struct device when
> of_platform_device_create creates a device. (it would also need to be
> set for early_platform_device creation, but that's not something that
> should affect you). You would also add a check to
> of_platform_device_create to check if the device pointer is already
> set. If it is, then skip creation of the device.

One problem with this method is that every driver that needs or wants
the device early has to do it explicitly, but I guess we can't have it
all.

I find this solution rather appealing because it allows for instance the
Marvell Armada 370/XP IRQ chip driver to do this as well so we no longer
have to keep around the extra struct device_node * in msi_chip but we
can actually reuse the one from struct device instead.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC] early init and DT platform devices allocation/registration

[Grant Likely]

On Wed, Jun 26, 2013 at 11:31 AM, Thierry Reding
<thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Jun 26, 2013 at 11:03:20AM +0100, Grant Likely wrote:
>> On Wed, Jun 26, 2013 at 7:00 AM, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> > Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote @ Tue, 25 Jun 2013 19:52:33 +0200:
>> >
>> >> > Here's my workaround. I need to call of_detach_node() with OF_DYNAMIC
>> >> > to avoid duplicated device registration.
>> >>
>> >> Gah! my eyes!
>> >>
>> >> Don't do that. It is incredibly problematic. Look at inhibiting
>> >> duplicate device creation instead.
>> >
>> > I may not follow this thread correctly, but could anyone point out the
>> > above "inhibiting duplicate device creation" if there's already such
>> > solution?
>>
>> No, the solution doesn't exist yet, but it wouldn't be hard to
>> implement. What you need to do is to add a struct device pointer to
>> struct device_node, and set the pointer to the struct device when
>> of_platform_device_create creates a device. (it would also need to be
>> set for early_platform_device creation, but that's not something that
>> should affect you). You would also add a check to
>> of_platform_device_create to check if the device pointer is already
>> set. If it is, then skip creation of the device.
>
> One problem with this method is that every driver that needs or wants
> the device early has to do it explicitly, but I guess we can't have it
> all.

The other option is to modify of_device_add() to allow it to be called
before the platform bus is set up, and then to 'fixup' the
registrations at initcall time. That would mean of_platform_populate
can be called really early and it would solve the problem of
preserving the device hierarchy.

g.

Re: [RFC] early init and DT platform devices allocation/registration

[Sebastian Hesselbarth]

On 06/26/13 12:03, Grant Likely wrote:
> On Wed, Jun 26, 2013 at 7:00 AM, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote @ Tue, 25 Jun 2013 19:52:33 +0200:
>>
>>>> Here's my workaround. I need to call of_detach_node() with OF_DYNAMIC
>>>> to avoid duplicated device registration.
>>>
>>> Gah! my eyes!
>>>
>>> Don't do that. It is incredibly problematic. Look at inhibiting
>>> duplicate device creation instead.
>>
>> I may not follow this thread correctly, but could anyone point out the
>> above "inhibiting duplicate device creation" if there's already such
>> solution?
>
> No, the solution doesn't exist yet, but it wouldn't be hard to
> implement. What you need to do is to add a struct device pointer to
> struct device_node, and set the pointer to the struct device when
> of_platform_device_create creates a device. (it would also need to be
> set for early_platform_device creation, but that's not something that
> should affect you). You would also add a check to
> of_platform_device_create to check if the device pointer is already
> set. If it is, then skip creation of the device.

Grant,

What about the other way round, i.e. check if there is a device with
.of_node pointed to the struct device_node currently at
of_platform_device_create?

That will avoid adding struct device to struct device_node which you
fought against for good reasons.

Also, I guess of_platform_device_create could be exported and used
by anyone who wants to create platform_devices early.

Sebastian

Re: [RFC] early init and DT platform devices allocation/registration

[Grant Likely]

On Wed, Jun 26, 2013 at 1:44 PM, Sebastian Hesselbarth
<sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 06/26/13 12:03, Grant Likely wrote:
>>
>> On Wed, Jun 26, 2013 at 7:00 AM, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>
>>> Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote @ Tue, 25 Jun 2013
>>> 19:52:33 +0200:
>>>
>>>>> Here's my workaround. I need to call of_detach_node() with OF_DYNAMIC
>>>>> to avoid duplicated device registration.
>>>>
>>>>
>>>> Gah! my eyes!
>>>>
>>>> Don't do that. It is incredibly problematic. Look at inhibiting
>>>> duplicate device creation instead.
>>>
>>>
>>> I may not follow this thread correctly, but could anyone point out the
>>> above "inhibiting duplicate device creation" if there's already such
>>> solution?
>>
>>
>> No, the solution doesn't exist yet, but it wouldn't be hard to
>> implement. What you need to do is to add a struct device pointer to
>> struct device_node, and set the pointer to the struct device when
>> of_platform_device_create creates a device. (it would also need to be
>> set for early_platform_device creation, but that's not something that
>> should affect you). You would also add a check to
>> of_platform_device_create to check if the device pointer is already
>> set. If it is, then skip creation of the device.
>
>
> Grant,
>
> What about the other way round, i.e. check if there is a device with
> .of_node pointed to the struct device_node currently at
> of_platform_device_create?
>
> That will avoid adding struct device to struct device_node which you
> fought against for good reasons.

The main thing is that it means searching through the entire list of
platform devices every time a new platform device is created. That
seems unnecessarily expensive to me.

>
> Also, I guess of_platform_device_create could be exported and used
> by anyone who wants to create platform_devices early.

Yes, but in that case it is probably better for them to call
of_platform_populate early if of_platform_device_create is fixed to
support early calling. Then you'd just set up all the devices earlier
in init, allow drivers that support early probing to do so, and then
everything else uses the normal initcall path.

g.

Re: [RFC] early init and DT platform devices allocation/registration

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3288 bytes --]

On Wed, Jun 26, 2013 at 02:12:06PM +0100, Grant Likely wrote:
> On Wed, Jun 26, 2013 at 1:44 PM, Sebastian Hesselbarth
> <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On 06/26/13 12:03, Grant Likely wrote:
> >>
> >> On Wed, Jun 26, 2013 at 7:00 AM, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> >>>
> >>> Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote @ Tue, 25 Jun 2013
> >>> 19:52:33 +0200:
> >>>
> >>>>> Here's my workaround. I need to call of_detach_node() with OF_DYNAMIC
> >>>>> to avoid duplicated device registration.
> >>>>
> >>>>
> >>>> Gah! my eyes!
> >>>>
> >>>> Don't do that. It is incredibly problematic. Look at inhibiting
> >>>> duplicate device creation instead.
> >>>
> >>>
> >>> I may not follow this thread correctly, but could anyone point out the
> >>> above "inhibiting duplicate device creation" if there's already such
> >>> solution?
> >>
> >>
> >> No, the solution doesn't exist yet, but it wouldn't be hard to
> >> implement. What you need to do is to add a struct device pointer to
> >> struct device_node, and set the pointer to the struct device when
> >> of_platform_device_create creates a device. (it would also need to be
> >> set for early_platform_device creation, but that's not something that
> >> should affect you). You would also add a check to
> >> of_platform_device_create to check if the device pointer is already
> >> set. If it is, then skip creation of the device.
> >
> >
> > Grant,
> >
> > What about the other way round, i.e. check if there is a device with
> > .of_node pointed to the struct device_node currently at
> > of_platform_device_create?
> >
> > That will avoid adding struct device to struct device_node which you
> > fought against for good reasons.
> 
> The main thing is that it means searching through the entire list of
> platform devices every time a new platform device is created. That
> seems unnecessarily expensive to me.

There's not really much reason to not add a pointer to the struct device
of a device_node because you can already obtain the platform_device by
calling of_find_device_by_node(). That doesn't work early, but if that
gets fixed, then of_find_device_by_node() could also be used. And since
adding a struct device * to device_node is pretty much required to fix
of_platform_device_create() for early, of_find_device_by_node() can be
implemented much more efficiently.

> > Also, I guess of_platform_device_create could be exported and used
> > by anyone who wants to create platform_devices early.
> 
> Yes, but in that case it is probably better for them to call
> of_platform_populate early if of_platform_device_create is fixed to
> support early calling. Then you'd just set up all the devices earlier
> in init, allow drivers that support early probing to do so, and then
> everything else uses the normal initcall path.

I've been bugged by the recent additions in drivers/irqchip lately
because they all rely on only the device_node and therefore none of the
functions that require a struct device can be used. If things can indeed
be fixed to call of_platform_populate() early that would restore a whole
lot of consistency.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC] early init and DT platform devices allocation/registration

[Hiroshi Doyu]

Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote @ Wed, 26 Jun 2013 12:03:20 +0200:

> On Wed, Jun 26, 2013 at 7:00 AM, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote @ Tue, 25 Jun 2013 19:52:33 +0200:
> >
> >> > Here's my workaround. I need to call of_detach_node() with OF_DYNAMIC
> >> > to avoid duplicated device registration.
> >>
> >> Gah! my eyes!
> >>
> >> Don't do that. It is incredibly problematic. Look at inhibiting
> >> duplicate device creation instead.
> >
> > I may not follow this thread correctly, but could anyone point out the
> > above "inhibiting duplicate device creation" if there's already such
> > solution?
> 
> No, the solution doesn't exist yet, but it wouldn't be hard to
> implement. What you need to do is to add a struct device pointer to
> struct device_node, and set the pointer to the struct device when
> of_platform_device_create creates a device. (it would also need to be
> set for early_platform_device creation, but that's not something that
> should affect you). You would also add a check to
> of_platform_device_create to check if the device pointer is already
> set. If it is, then skip creation of the device.

Implemented as Grant suggested. At least this works for our case,
where IOMMU needs to be instanciated earlier than other device[1].
early_platform_device case still need to be covered.

[1] https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/036685.html

>From da563c72f7fc37fedd4b7e3a957f41a484a19788 Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Date: Fri, 28 Jun 2013 11:43:20 +0300
Subject: [PATCH RFC 1/1] of: dev_node has struct device pointer

To prevent of_platform_populate() from trying to populate duplicate
devices if a device has been already populated.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/of/base.c     |   23 +++++++++++++++++++++++
 drivers/of/platform.c |    8 ++++++++
 include/linux/of.h    |   16 ++++++++++++++++
 3 files changed, 47 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 5c54279..99062dd 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -230,6 +230,29 @@ const void *of_get_property(const struct device_node *np, const char *name,
 }
 EXPORT_SYMBOL(of_get_property);
 
+struct device *of_get_device(const struct device_node *node)
+{
+	struct device *dev;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&devtree_lock, flags);
+	dev = node->dev;
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+
+	return dev;
+}
+EXPORT_SYMBOL(of_get_device);
+
+void of_set_device(struct device_node *node, struct device *dev)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&devtree_lock, flags);
+	node->dev = dev;
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+}
+EXPORT_SYMBOL(of_set_device);
+
 /** Checks if the given "compat" string matches one of the strings in
  * the device's "compatible" property
  */
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index e0a6514..a8f6b09 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -203,10 +203,17 @@ struct platform_device *of_platform_device_create_pdata(
 					struct device *parent)
 {
 	struct platform_device *dev;
+	struct device *tmp;
 
 	if (!of_device_is_available(np))
 		return NULL;
 
+	tmp = of_get_device(np);
+	if (tmp) {
+		dev_info(tmp, "Already populated\n");
+		return to_platform_device(tmp);
+	}
+
 	dev = of_device_alloc(np, bus_id, parent);
 	if (!dev)
 		return NULL;
@@ -228,6 +235,7 @@ struct platform_device *of_platform_device_create_pdata(
 		return NULL;
 	}
 
+	of_set_device(np, &dev->dev);
 	return dev;
 }
 
diff --git a/include/linux/of.h b/include/linux/of.h
index 1fd08ca..b548522 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -60,6 +60,9 @@ struct device_node {
 	struct	kref kref;
 	unsigned long _flags;
 	void	*data;
+
+	struct device *dev;		/* Set only after populated */
+
 #if defined(CONFIG_SPARC)
 	const char *path_component_name;
 	unsigned int unique_id;
@@ -268,6 +271,8 @@ extern const void *of_get_property(const struct device_node *node,
 				int *lenp);
 #define for_each_property_of_node(dn, pp) \
 	for (pp = dn->properties; pp != NULL; pp = pp->next)
+extern struct device *of_get_device(const struct device_node *node);
+extern void of_set_device(struct device_node *node, struct device *dev);
 
 extern int of_n_addr_cells(struct device_node *np);
 extern int of_n_size_cells(struct device_node *np);
@@ -459,6 +464,17 @@ static inline const void *of_get_property(const struct device_node *node,
 	return NULL;
 }
 
+static inline struct device *of_get_device(const struct device_node *node)
+{
+	return NULL;
+}
+
+static inline void of_set_device(const struct device_node *node,
+				 struct device *dev);
+{
+	return -ENOSYS;
+}
+
 static inline int of_property_read_u64(const struct device_node *np,
 				       const char *propname, u64 *out_value)
 {
-- 
1.7.9.5

Re: [RFC] early init and DT platform devices allocation/registration

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2387 bytes --]

On Fri, Jun 28, 2013 at 10:49:15AM +0200, Hiroshi Doyu wrote:
> Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote @ Wed, 26 Jun 2013 12:03:20 +0200:
> 
> > On Wed, Jun 26, 2013 at 7:00 AM, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > > Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote @ Tue, 25 Jun 2013 19:52:33 +0200:
> > >
> > >> > Here's my workaround. I need to call of_detach_node() with OF_DYNAMIC
> > >> > to avoid duplicated device registration.
> > >>
> > >> Gah! my eyes!
> > >>
> > >> Don't do that. It is incredibly problematic. Look at inhibiting
> > >> duplicate device creation instead.
> > >
> > > I may not follow this thread correctly, but could anyone point out the
> > > above "inhibiting duplicate device creation" if there's already such
> > > solution?
> > 
> > No, the solution doesn't exist yet, but it wouldn't be hard to
> > implement. What you need to do is to add a struct device pointer to
> > struct device_node, and set the pointer to the struct device when
> > of_platform_device_create creates a device. (it would also need to be
> > set for early_platform_device creation, but that's not something that
> > should affect you). You would also add a check to
> > of_platform_device_create to check if the device pointer is already
> > set. If it is, then skip creation of the device.
> 
> Implemented as Grant suggested. At least this works for our case,
> where IOMMU needs to be instanciated earlier than other device[1].
> early_platform_device case still need to be covered.

I think we arrived at a different conclusion in another branch of this
thread. With the patch below every driver needs to explicitly allocate
the platform device and set the struct device_node's .dev field, which
has other side-effects such as the device hierarchy getting messed up.

A better alternative would be to have of_platform_populate() run early
such that the .dev field in the struct device_node can be set by core
code, which would not require every driver to be changed.

I'm not sure exactly what needs to be done to make this work, however.
Grant, can you provide some guidance here as to how this may be fixed?
Where would we have to call of_platform_populate() from and what makes
this break with the current implementation?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC] early init and DT platform devices allocation/registration

[Grant Likely]

On Fri, Jun 28, 2013 at 11:38 AM, Thierry Reding
<thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Jun 28, 2013 at 10:49:15AM +0200, Hiroshi Doyu wrote:
>> Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote @ Wed, 26 Jun 2013 12:03:20 +0200:
>>
>> > On Wed, Jun 26, 2013 at 7:00 AM, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> > > Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote @ Tue, 25 Jun 2013 19:52:33 +0200:
>> > >
>> > >> > Here's my workaround. I need to call of_detach_node() with OF_DYNAMIC
>> > >> > to avoid duplicated device registration.
>> > >>
>> > >> Gah! my eyes!
>> > >>
>> > >> Don't do that. It is incredibly problematic. Look at inhibiting
>> > >> duplicate device creation instead.
>> > >
>> > > I may not follow this thread correctly, but could anyone point out the
>> > > above "inhibiting duplicate device creation" if there's already such
>> > > solution?
>> >
>> > No, the solution doesn't exist yet, but it wouldn't be hard to
>> > implement. What you need to do is to add a struct device pointer to
>> > struct device_node, and set the pointer to the struct device when
>> > of_platform_device_create creates a device. (it would also need to be
>> > set for early_platform_device creation, but that's not something that
>> > should affect you). You would also add a check to
>> > of_platform_device_create to check if the device pointer is already
>> > set. If it is, then skip creation of the device.
>>
>> Implemented as Grant suggested. At least this works for our case,
>> where IOMMU needs to be instanciated earlier than other device[1].
>> early_platform_device case still need to be covered.
>
> I think we arrived at a different conclusion in another branch of this
> thread. With the patch below every driver needs to explicitly allocate
> the platform device and set the struct device_node's .dev field, which
> has other side-effects such as the device hierarchy getting messed up.
>
> A better alternative would be to have of_platform_populate() run early
> such that the .dev field in the struct device_node can be set by core
> code, which would not require every driver to be changed.
>
> I'm not sure exactly what needs to be done to make this work, however.
> Grant, can you provide some guidance here as to how this may be fixed?
> Where would we have to call of_platform_populate() from and what makes
> this break with the current implementation?

Try it and see!  :-) I cannot give a definite answer, but I suspect
that it will fall down when registering the devices on to the
platform_bus because it isn't initialized yet. If called early, the
code would need to hold the platform_device in some kind of deferred
list until the platform bus was initialize, and then have a cleanup
function at initcall time to finish the registration of all early
devices.

g.

Re: [RFC] early init and DT platform devices allocation/registration

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4079 bytes --]

On Fri, Jun 28, 2013 at 01:27:15PM +0100, Grant Likely wrote:
> On Fri, Jun 28, 2013 at 11:38 AM, Thierry Reding
> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Fri, Jun 28, 2013 at 10:49:15AM +0200, Hiroshi Doyu wrote:
> >> Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote @ Wed, 26 Jun 2013 12:03:20 +0200:
> >>
> >> > On Wed, Jun 26, 2013 at 7:00 AM, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> >> > > Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote @ Tue, 25 Jun 2013 19:52:33 +0200:
> >> > >
> >> > >> > Here's my workaround. I need to call of_detach_node() with OF_DYNAMIC
> >> > >> > to avoid duplicated device registration.
> >> > >>
> >> > >> Gah! my eyes!
> >> > >>
> >> > >> Don't do that. It is incredibly problematic. Look at inhibiting
> >> > >> duplicate device creation instead.
> >> > >
> >> > > I may not follow this thread correctly, but could anyone point out the
> >> > > above "inhibiting duplicate device creation" if there's already such
> >> > > solution?
> >> >
> >> > No, the solution doesn't exist yet, but it wouldn't be hard to
> >> > implement. What you need to do is to add a struct device pointer to
> >> > struct device_node, and set the pointer to the struct device when
> >> > of_platform_device_create creates a device. (it would also need to be
> >> > set for early_platform_device creation, but that's not something that
> >> > should affect you). You would also add a check to
> >> > of_platform_device_create to check if the device pointer is already
> >> > set. If it is, then skip creation of the device.
> >>
> >> Implemented as Grant suggested. At least this works for our case,
> >> where IOMMU needs to be instanciated earlier than other device[1].
> >> early_platform_device case still need to be covered.
> >
> > I think we arrived at a different conclusion in another branch of this
> > thread. With the patch below every driver needs to explicitly allocate
> > the platform device and set the struct device_node's .dev field, which
> > has other side-effects such as the device hierarchy getting messed up.
> >
> > A better alternative would be to have of_platform_populate() run early
> > such that the .dev field in the struct device_node can be set by core
> > code, which would not require every driver to be changed.
> >
> > I'm not sure exactly what needs to be done to make this work, however.
> > Grant, can you provide some guidance here as to how this may be fixed?
> > Where would we have to call of_platform_populate() from and what makes
> > this break with the current implementation?
> 
> Try it and see!  :-) I cannot give a definite answer, but I suspect
> that it will fall down when registering the devices on to the
> platform_bus because it isn't initialized yet. If called early, the
> code would need to hold the platform_device in some kind of deferred
> list until the platform bus was initialize, and then have a cleanup
> function at initcall time to finish the registration of all early
> devices.

So what I did is move the call to of_platform_populate() on Tegra from
.init_machine() to .init_early(). The problem with .init_early() is that
the Slab allocator isn't available yet. One can use bootmem to work
around that, but it involves sprinkling a lot of slab_is_available() and
alloc_bootmem() where otherwise just kmalloc() (or one of its variants)
would be called.

What I ended up doing was implement early_kmalloc() and early_kfree() to
replace some occurrences in code that was called at some point from
of_platform_populate(). That got me quite far, though I haven't fully
completed the conversion. I'm not sure if it's really worth it either
because that code is unlikely to ever get merged.

Instead I'll try to move the call to of_platform_populate() shortly
after the slab has been initialized, which happens to be around the time
that .init_irq() is called and see if that makes things any easier.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC] early init and DT platform devices allocation/registration

[Lorenzo Pieralisi]

On Tue, Jun 25, 2013 at 03:56:22PM +0100, Stephen Warren wrote:
> On 06/25/2013 05:45 AM, Grant Likely wrote:
> > On Mon, Jun 24, 2013 at 5:33 PM, Lorenzo Pieralisi
> > <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org> wrote:
> >> Hi all,
> >>
> >> I am dealing with a lingering problem related to init and probing of platform
> >> devices early (before initcalls) in the kernel boot process. The problem,
> >> which is nothing new, is related to how platform devices are created in the
> >> kernel from DT and when they become available. Platform devices are created
> >> through (assuming any legacy static initialization is removed from the kernel)
> >>
> >> of_platform_populate()
> >>
> >> at arch_initcall time on ARM. This is a problem for drivers that need to be
> >> probed and initialized before arch_initcall (ie early_initcall) because
> >> the corresponding platform_device has not been created yet.
> 
> What's the use-case here; why does the driver /have/ to be
> allocated/probed so early? Can't drivers all be allocated from DT in the
> usual fashion, and any dependencies resolved using deferred probe? In
> almost all cases, that should work.

The driver must be initialized in order to boot secondary CPUs, so
very very early, its initialization cannot be deferred.
Otherwise we would end up with MCPM back-end having to add ad-hoc
kludges to boot secondary CPUs, and then replace the early function calls
to the power controller drivers when the power controller has been
properly initialized. Feasible, but really horrible.

On top of that, the HW component is an IP that provides functionality
beyond power management, so we really want it to be initialized and
running after boot, we cannot just hide a couple of function calls in
the MCPM back-end to boot the CPUs and forget about that IP afterwards.

> ...
> >> How this problem is supposed to be solved in the kernel ?
> >>
> >> 1- drivers that are to be up and running at early_initcall time must not
> >>    rely on the device/driver model (but then they cannot use any API that
> >>    requires a struct device to function (eg regmap))
> >> 2- the driver should allocate a platform device at early initcall from
> >>    a DT compatible node. Do not know how to deal with platform device
> >>    duplication though, since of_platform_populate() will create another
> >>    platform device when the node is parsed
> > 
> > While I've resisted it in the past, I would be okay with adding struct
> > device pointer in the device_node structure. I've resisted because I
> > don't want drivers following the device_node pointer and making an
> > assumption about what /kind/ of device is pointed to by it. However,
> > this is an important use case and it makes it feasible to use an early
> > platform device with of_platform_populate.
> 
> Hiroshi (who I have CC'd here) has also been asking about this same
> issue downstream. The issue for us is that we need to initialize an SMMU
> driver before any devices that are translated by the SMMU. One option is
> to force the register/probe of the SMMU driver explicitly, early in the
> machine's .init_machine() callback, before of_platform_populate(), to
> ensure the ordering. Then, we run into the same duplicate device issue,
> and the change Grant mentions above would help solve this.

Good to hear we are not alone. I still do not know if we are dealing
with specific use cases or this is a problem that should be solved
generically at kernel level. I think it is best to solve it once for
all, otherwise we will all come up with workarounds to make things work,
possibly violating the driver/device model usage guidelines.

Lorenzo

Re: [RFC] early init and DT platform devices allocation/registration

[Magnus Damm]

Hi Lorenzo,

On Wed, Jun 26, 2013 at 2:07 AM, Lorenzo Pieralisi
<lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org> wrote:
> On Tue, Jun 25, 2013 at 03:56:22PM +0100, Stephen Warren wrote:
>> On 06/25/2013 05:45 AM, Grant Likely wrote:
>> > On Mon, Jun 24, 2013 at 5:33 PM, Lorenzo Pieralisi
>> > <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org> wrote:
>> >> Hi all,
>> >>
>> >> I am dealing with a lingering problem related to init and probing of platform
>> >> devices early (before initcalls) in the kernel boot process. The problem,
>> >> which is nothing new, is related to how platform devices are created in the
>> >> kernel from DT and when they become available. Platform devices are created
>> >> through (assuming any legacy static initialization is removed from the kernel)
>> >>
>> >> of_platform_populate()
>> >>
>> >> at arch_initcall time on ARM. This is a problem for drivers that need to be
>> >> probed and initialized before arch_initcall (ie early_initcall) because
>> >> the corresponding platform_device has not been created yet.
>>
>> What's the use-case here; why does the driver /have/ to be
>> allocated/probed so early? Can't drivers all be allocated from DT in the
>> usual fashion, and any dependencies resolved using deferred probe? In
>> almost all cases, that should work.
>
> The driver must be initialized in order to boot secondary CPUs, so
> very very early, its initialization cannot be deferred.
> Otherwise we would end up with MCPM back-end having to add ad-hoc
> kludges to boot secondary CPUs, and then replace the early function calls
> to the power controller drivers when the power controller has been
> properly initialized. Feasible, but really horrible.

How about moving part of the SMP initialization to later during boot?

In particular I mean the bring up of the secondary cores. Just booting
with a single CPU for a bit longer is certainly doable because it is
today possible to bring secondary CPU cores up from user space via the
hotplug interface. Below is some old prototype code that I hacked up a
while ago - note that I copied the original comment about that this
should be done in user space. =)

I'd like to have secondary CPU cores initialized later so I can use
the regular device model (DT or platform device) to describe one or
more power domain hardware devices. These power domains control the
CPU cores, so there is a dependency on the SMP bring up code...

Cheers,

/ magnus

--- 0001/include/linux/smp.h
+++ work/include/linux/smp.h    2013-05-21 20:08:42.000000000 +0900
@@ -124,6 +124,7 @@ void smp_prepare_boot_cpu(void);
 extern unsigned int setup_max_cpus;
 extern void __init setup_nr_cpu_ids(void);
 extern void __init smp_init(void);
+extern void __init smp_late_init(void);

 #else /* !SMP */

--- 0001/init/main.c
+++ work/init/main.c    2013-05-21 20:09:34.000000000 +0900
@@ -334,6 +334,7 @@ static void __init smp_init(void)

 static inline void setup_nr_cpu_ids(void) { }
 static inline void smp_prepare_cpus(unsigned int maxcpus) { }
+static inline void smp_late_init(void) { }
 #endif

 /*
@@ -879,6 +880,8 @@ static noinline void __init kernel_init_

        do_basic_setup();

+       smp_late_init();
+
        /* Open the /dev/console on the rootfs, this should never fail */
        if (sys_open((const char __user *) "/dev/console", O_RDWR, 0) < 0)
                pr_err("Warning: unable to open an initial console.\n");
--- 0001/kernel/smp.c
+++ work/kernel/smp.c   2013-05-21 20:07:06.000000000 +0900
@@ -549,6 +549,20 @@ void __init smp_init(void)
                if (!cpu_online(cpu))
                        cpu_up(cpu);
        }
+}
+
+/* Called by boot processor late during boot to activate the rest. */
+void __init smp_late_init(void)
+{
+       unsigned int cpu;
+
+       /* FIXME: This should be done in userspace --RR */
+       for_each_present_cpu(cpu) {
+               if (num_online_cpus() >= setup_max_cpus)
+                       break;
+               if (!cpu_online(cpu))
+                       cpu_up(cpu);
+       }

        /* Any cleanup work */
        printk(KERN_INFO "Brought up %ld CPUs\n", (long)num_online_cpus());


Cheers,

/ magnus

Re: [RFC] early init and DT platform devices allocation/registration

[Lorenzo Pieralisi]

Hi Magnus,

thank you.

On Wed, Jun 26, 2013 at 07:20:32AM +0100, Magnus Damm wrote:
> Hi Lorenzo,
> 
> On Wed, Jun 26, 2013 at 2:07 AM, Lorenzo Pieralisi
> <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org> wrote:
> > On Tue, Jun 25, 2013 at 03:56:22PM +0100, Stephen Warren wrote:
> >> On 06/25/2013 05:45 AM, Grant Likely wrote:
> >> > On Mon, Jun 24, 2013 at 5:33 PM, Lorenzo Pieralisi
> >> > <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org> wrote:
> >> >> Hi all,
> >> >>
> >> >> I am dealing with a lingering problem related to init and probing of platform
> >> >> devices early (before initcalls) in the kernel boot process. The problem,
> >> >> which is nothing new, is related to how platform devices are created in the
> >> >> kernel from DT and when they become available. Platform devices are created
> >> >> through (assuming any legacy static initialization is removed from the kernel)
> >> >>
> >> >> of_platform_populate()
> >> >>
> >> >> at arch_initcall time on ARM. This is a problem for drivers that need to be
> >> >> probed and initialized before arch_initcall (ie early_initcall) because
> >> >> the corresponding platform_device has not been created yet.
> >>
> >> What's the use-case here; why does the driver /have/ to be
> >> allocated/probed so early? Can't drivers all be allocated from DT in the
> >> usual fashion, and any dependencies resolved using deferred probe? In
> >> almost all cases, that should work.
> >
> > The driver must be initialized in order to boot secondary CPUs, so
> > very very early, its initialization cannot be deferred.
> > Otherwise we would end up with MCPM back-end having to add ad-hoc
> > kludges to boot secondary CPUs, and then replace the early function calls
> > to the power controller drivers when the power controller has been
> > properly initialized. Feasible, but really horrible.
> 
> How about moving part of the SMP initialization to later during boot?
> 
> In particular I mean the bring up of the secondary cores. Just booting
> with a single CPU for a bit longer is certainly doable because it is
> today possible to bring secondary CPU cores up from user space via the
> hotplug interface. Below is some old prototype code that I hacked up a
> while ago - note that I copied the original comment about that this
> should be done in user space. =)

Well, the question is probably why smp_init() is called before

do_basic_setup()

as the kernel does now. We need to check if there is something we are
missing or not, and I guess the only way to do it is to post your patch
to LKML.

> I'd like to have secondary CPU cores initialized later so I can use
> the regular device model (DT or platform device) to describe one or
> more power domain hardware devices. These power domains control the
> CPU cores, so there is a dependency on the SMP bring up code...

Looks like we are on the same boat then :-). This way we will solve
this particular issue(s), but still can not rely on DT to initialize early
platform devices.

This stuff requires some serious thought, that's for certain, I will try
to allocate some time to have a thorough look into this.

> --- 0001/include/linux/smp.h
> +++ work/include/linux/smp.h    2013-05-21 20:08:42.000000000 +0900
> @@ -124,6 +124,7 @@ void smp_prepare_boot_cpu(void);
>  extern unsigned int setup_max_cpus;
>  extern void __init setup_nr_cpu_ids(void);
>  extern void __init smp_init(void);
> +extern void __init smp_late_init(void);
> 
>  #else /* !SMP */
> 
> --- 0001/init/main.c
> +++ work/init/main.c    2013-05-21 20:09:34.000000000 +0900
> @@ -334,6 +334,7 @@ static void __init smp_init(void)
> 
>  static inline void setup_nr_cpu_ids(void) { }
>  static inline void smp_prepare_cpus(unsigned int maxcpus) { }
> +static inline void smp_late_init(void) { }
>  #endif
> 
>  /*
> @@ -879,6 +880,8 @@ static noinline void __init kernel_init_
> 
>         do_basic_setup();
> 
> +       smp_late_init();
> +
>         /* Open the /dev/console on the rootfs, this should never fail */
>         if (sys_open((const char __user *) "/dev/console", O_RDWR, 0) < 0)
>                 pr_err("Warning: unable to open an initial console.\n");
> --- 0001/kernel/smp.c
> +++ work/kernel/smp.c   2013-05-21 20:07:06.000000000 +0900
> @@ -549,6 +549,20 @@ void __init smp_init(void)
>                 if (!cpu_online(cpu))
>                         cpu_up(cpu);
>         }
> +}
> +
> +/* Called by boot processor late during boot to activate the rest. */
> +void __init smp_late_init(void)
> +{
> +       unsigned int cpu;
> +
> +       /* FIXME: This should be done in userspace --RR */
> +       for_each_present_cpu(cpu) {
> +               if (num_online_cpus() >= setup_max_cpus)
> +                       break;
> +               if (!cpu_online(cpu))
> +                       cpu_up(cpu);
> +       }
> 
>         /* Any cleanup work */
>         printk(KERN_INFO "Brought up %ld CPUs\n", (long)num_online_cpus());

You probably need to remove the same code from smp_init() then ? If you
feel like posting this patch as an RFC (even if it just were to draw
attention to the problem) please go ahead. I still think this is a work
around to make things work, and we are still not solving the core issue,
but it would definitely simplify our lives.

Anyway, thanks a lot for chiming in !
Lorenzo

Re: [RFC] early init and DT platform devices allocation/registration

[Lorenzo Pieralisi]

Hi Grant,

thanks for commenting.

On Tue, Jun 25, 2013 at 12:45:25PM +0100, Grant Likely wrote:
> On Mon, Jun 24, 2013 at 5:33 PM, Lorenzo Pieralisi
> <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org> wrote:
> > Hi all,
> >
> > I am dealing with a lingering problem related to init and probing of platform
> > devices early (before initcalls) in the kernel boot process. The problem,
> > which is nothing new, is related to how platform devices are created in the
> > kernel from DT and when they become available. Platform devices are created
> > through (assuming any legacy static initialization is removed from the kernel)
> >
> > of_platform_populate()
> >
> > at arch_initcall time on ARM. This is a problem for drivers that need to be
> > probed and initialized before arch_initcall (ie early_initcall) because
> > the corresponding platform_device has not been created yet.
> >
> > To work around this awkward situation, a driver, instead of relying on
> > platform driver probing mechanism, can get initialized using early_initcall
> > mechanism and rely on DT helpers (ie of_iomap() and company) to initialize
> > resources. The problem comes when the driver functions must rely on an API
> > (eg regmap) that requires a struct device to function properly; since the
> > platform_device has not been created yet at early_initcall time, the
> > driver cannot rely on it. The only solution consists in allocating a
> > platform_device on the fly at driver init through
> >
> > of_platform_device_create()
> >
> > and use that device to initialize the (eg regmap) API. There is an issue
> > with this solution, basically a platform device is allocated twice for
> > a given device node - compatible property (because of_platform_populate()
> > allocates a platform device for a node containing a compatible string even if
> > a platform device has already been allocated for that device node).
> >
> > The second solution relies on early platform devices. Early platform devices
> > are added by mach code and driver probing is carried out using the early
> > platform device probing mechanism
> >
> > early_platform_driver_probe()
> >
> > The drawback with this solution is, AFAIK, it does not play well with DT,
> > since the platform device duplication issue is still there (ie
> > of_platform_populate() will allocate a platform device which is a duplicate
> > of the one allocated at early boot to make the early platform device
> > initialization possible).
> >
> > Please correct me if I am wrong, just trying to untangle a problem that does
> > not look easy to solve.
> >
> > How this problem is supposed to be solved in the kernel ?
> >
> > 1- drivers that are to be up and running at early_initcall time must not
> >    rely on the device/driver model (but then they cannot use any API that
> >    requires a struct device to function (eg regmap))
> > 2- the driver should allocate a platform device at early initcall from
> >    a DT compatible node. Do not know how to deal with platform device
> >    duplication though, since of_platform_populate() will create another
> >    platform device when the node is parsed
> 
> While I've resisted it in the past, I would be okay with adding struct
> device pointer in the device_node structure. I've resisted because I
> don't want drivers following the device_node pointer and making an
> assumption about what /kind/ of device is pointed to by it. However,
> this is an important use case and it makes it feasible to use an early
> platform device with of_platform_populate.
> 
> Alternately, if others chime in and think it is too risky to have the
> device pointer in the device node, we could simply add a flag to the
> device_node which indicates the node has been parsed by
> of_platform_populate.
> 
> There would need to be some careful coding to make sure that any call
> to early platform device creation sets the pointer in the device node
> correctly.
> 
> I would also make it a requirement that any early platform device
> *must* be converted into a 'real' platform device during initcall
> time. That includes being possible to be freed correctly. Static early
> platform device definitions should not be allowed, otherwise there
> needs to be a special case for the platform device release hook. I
> really don't want that.
> 
> Something else that needs to be investigated is how the device
> hierarchy will be affected by using early_platform_device. We still
> want the platform_device to appear in the same place in the hierarchy
> regardless of whether or not it was created 'early'. the question is
> how to make sure that actually happens.

While acknowledging the complexity of adding this code, I tend to prefer
a solution that allows allocation of early platform devices from DT
instead of adding a flag to avoid duplicates, it is cleaner (but more
complex as you mentioned) that's the point I wanted to make, otherwise
we end up adding all kinds of kludges to the kernel to make early init
work.

The question is: should we live with workarounds that allow early allocation
of devices or should we fix the kernel to cope with these use cases ? I do not
know how many drivers/devices require these changes, and how many do we need
to justify this level of complexity.

> > 3- driver should rely on early platform device/driver, but this does not
> >    solve (?) the platform device duplication problem either, it will happen
> >    when of_platform_populate() parses the DT and creates devices on the fly
> >
> > In theory there are other solutions such as:
> >
> > (a) declaring the platform device statically in arm/mach-* code and do not
> >     define the device node in dts, basically going back in time to ARM legacy
> >     kernel mechanism for this kind of devices
> 
> As stated above, no. Static device definitions are not a good idea,
> and it doesn't scale in a multiplatform kernel world.

Agreed, I was just listing solutions, (a) is certainly not what we want.

> > (b) add a way to of_platform_populate() to exclude some compatible strings
> >     from being matched
> 
> This method will probably be too error prone. It would be better to
> add the check in of_platform_populate to skip nodes that have already
> been populated. I can't think of any use-cases where we would want
> of_platform_populate to process a single node more than once.

+1

> > Honestly there is not a solution I can say I like and maybe I am trying to
> > solve a problem that has already been solved, apologies if so, that's why
> > I am asking on the list to people with more knowledge than me on the subject.
> 
> No, it hasn't been solved yet, but it is worth solving. Either that or
> figure out how to do of_platform_populate much earlier.

Well, I am already relieved that you acknowledge it is something worth
solving, because that would simplify things a lot, as MCPM lower-layers
coding showed and I suspect that's not the only piece of PM code that
requires this early initialization.

Thank you !
Lorenzo