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

login
register
mail settings
Submitter Hiroshi Doyu
Date June 25, 2013, 4:36 p.m.
Message ID <20130625.193628.2143557800560690024.hdoyu@nvidia.com>
Download mbox | patch
Permalink /patch/254240/
State Not Applicable, archived
Headers show

Comments

Hiroshi Doyu - June 25, 2013, 4:36 p.m.
Stephen Warren <swarren@wwwdotorg.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@nvidia.com>
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@nvidia.com>
---
 arch/arm/mach-tegra/Kconfig |  1 +
 arch/arm/mach-tegra/tegra.c | 24 ++++++++++++++++++++++++
 drivers/iommu/tegra-smmu.c  |  3 +++
 3 files changed, 28 insertions(+)
Grant Likely - June 25, 2013, 5:52 p.m.
On Tue, Jun 25, 2013 at 5:36 PM, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> Stephen Warren <swarren@wwwdotorg.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@nvidia.com>
> 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@nvidia.com>
> ---
>  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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hiroshi Doyu - June 26, 2013, 6 a.m.
Grant Likely <grant.likely@secretlab.ca> 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?
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grant Likely - June 26, 2013, 10:03 a.m.
On Wed, Jun 26, 2013 at 7:00 AM, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> Grant Likely <grant.likely@secretlab.ca> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding - June 26, 2013, 10:31 a.m.
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@nvidia.com> wrote:
> > Grant Likely <grant.likely@secretlab.ca> 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
Grant Likely - June 26, 2013, 11:04 a.m.
On Wed, Jun 26, 2013 at 11:31 AM, Thierry Reding
<thierry.reding@gmail.com> 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@nvidia.com> wrote:
>> > Grant Likely <grant.likely@secretlab.ca> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sebastian Hesselbarth - June 26, 2013, 12:44 p.m.
On 06/26/13 12:03, Grant Likely wrote:
> On Wed, Jun 26, 2013 at 7:00 AM, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
>> Grant Likely <grant.likely@secretlab.ca> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grant Likely - June 26, 2013, 1:12 p.m.
On Wed, Jun 26, 2013 at 1:44 PM, Sebastian Hesselbarth
<sebastian.hesselbarth@gmail.com> wrote:
> On 06/26/13 12:03, Grant Likely wrote:
>>
>> On Wed, Jun 26, 2013 at 7:00 AM, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
>>>
>>> Grant Likely <grant.likely@secretlab.ca> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding - June 27, 2013, 9:26 a.m.
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@gmail.com> wrote:
> > On 06/26/13 12:03, Grant Likely wrote:
> >>
> >> On Wed, Jun 26, 2013 at 7:00 AM, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> >>>
> >>> Grant Likely <grant.likely@secretlab.ca> 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

Patch

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;
 }