Grafting old platform drivers onto a new DT kernel

Grafting old platform drivers onto a new DT kernel

[Mason]

Hello,

My company provides a 3.4 kernel for an ARM-based SoC (there's also a 2.6.32
stashed somewhere for customers refusing to upgrade) along with drivers of
dubious quality written over the past decade.

I was tasked to release a new kernel, and took the opportunity to perform
a massive clean-up by discarding everything, and adding only enough code
on top of a recent kernel to boot to the shell.

I now have a small port with no drivers.
(I do have working serial and ethernet.)

Since I don't have time to rewrite the drivers at the moment, I'm wondering
if it's possible to "graft" old drivers (they're using the platform API, no
trace of DT support) onto my small base?

That way I can make management happy by providing a working new kernel,
and I can start working on converting drivers one-by-one.

Is that a realistic plan? What traps am I likely to fall into?

Regards.

Grafting old platform drivers onto a new DT kernel

[Andrew Lunn]

> Since I don't have time to rewrite the drivers at the moment, I'm wondering
> if it's possible to "graft" old drivers (they're using the platform API, no
> trace of DT support) onto my small base?

Platform drivers are still usable with DT systems. We used that fact
when converting platform based machines over to DT, one driver at a
time. Look in the git history for kirkwood devices. e.g. somewhere
around v3.7, arch/arm/mach-kirkwood. board-dt.c, and the various
board-*.c files, and the DT files in the usual place.

> Is that a realistic plan? What traps am I likely to fall into?

It is not just the move to DT where things are different. Kernel APIs
are not stable. So your old drivers might not even compile with a
current kernel.

	Andrew

Grafting old platform drivers onto a new DT kernel

[Javier Martinez Canillas]

Hello,

On Thu, Nov 5, 2015 at 12:15 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> Since I don't have time to rewrite the drivers at the moment, I'm wondering
>> if it's possible to "graft" old drivers (they're using the platform API, no
>> trace of DT support) onto my small base?
>
> Platform drivers are still usable with DT systems. We used that fact
> when converting platform based machines over to DT, one driver at a
> time. Look in the git history for kirkwood devices. e.g. somewhere
> around v3.7, arch/arm/mach-kirkwood. board-dt.c, and the various
> board-*.c files, and the DT files in the usual place.
>

OMAP did the same and still some boards use platform data and manually
register platform devices from board code. Take a look to
arch/arm/mach-omap2/pdata-quirks.c to see how that is being done.

Best regards,
Javier

Grafting old platform drivers onto a new DT kernel

[Mason]

On 05/11/2015 16:42, Javier Martinez Canillas wrote:
> Hello,
> 
> On Thu, Nov 5, 2015 at 12:15 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>>> Since I don't have time to rewrite the drivers at the moment, I'm wondering
>>> if it's possible to "graft" old drivers (they're using the platform API, no
>>> trace of DT support) onto my small base?
>>
>> Platform drivers are still usable with DT systems. We used that fact
>> when converting platform based machines over to DT, one driver at a
>> time. Look in the git history for kirkwood devices. e.g. somewhere
>> around v3.7, arch/arm/mach-kirkwood. board-dt.c, and the various
>> board-*.c files, and the DT files in the usual place.
>>
> 
> OMAP did the same and still some boards use platform data and manually
> register platform devices from board code. Take a look to
> arch/arm/mach-omap2/pdata-quirks.c to see how that is being done.

Hello,

I tried compiling an ancient SDHCI driver on a v4.2 system. It crashes
all over init because several host->ops functions are required, but the
old driver does not define them:
.reset
.set_clock
.set_bus_width
.set_uhs_signaling

So I downgraded to an older v3.14 kernel, and that problem vanished.
But I am having a problem with the IRQ setup.

# cat /proc/interrupts 
            CPU0       CPU1       
 18:         93          0      irq0   1 Level     serial
 55:       2832          0      irq0  38 Level     26000.ethernet
 60:          0          0      irq0  43 Edge      mmc0
211:        319       2603       GIC  29 Edge      twd

Ethernet is using IRQ 38, as specified in the DT.
mmc0 is supposed to use IRQ 60.

I see that the mmc0 has the index 60, so I must have messed up between
the real irq (hwirq?) and the index Linux uses internally (virq?)

static struct resource sdhci_resources[] = {
	{
		.start	= TANGOX_SDIO0_BASE_ADDR,
		.end	= TANGOX_SDIO0_BASE_ADDR + 0x1ff,
		.flags	= IORESOURCE_MEM,
	},
	{
		.start	= 60,	/* SDHCI0 IRQ */
		.flags	= IORESOURCE_IRQ,
	},
};

Both ethernet and sdhci driver call platform_get_irq(pdev, 0);
but the eth driver is DT, so calls of_irq_get() while sdhci is
legacy, so calls platform_get_resource() -- IIUC.

How do I specify a "hwirq" instead of a "Linux index"? and where?

Is this relevant?
https://www.kernel.org/doc/Documentation/IRQ-domain.txt

Should I use irq_linear_revmap() / irq_find_mapping() ?

Regards.

Grafting old platform drivers onto a new DT kernel

[Marc Zyngier]

On 09/11/15 15:15, Mason wrote:
> On 05/11/2015 16:42, Javier Martinez Canillas wrote:
>> Hello,
>>
>> On Thu, Nov 5, 2015 at 12:15 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>>>> Since I don't have time to rewrite the drivers at the moment, I'm wondering
>>>> if it's possible to "graft" old drivers (they're using the platform API, no
>>>> trace of DT support) onto my small base?
>>>
>>> Platform drivers are still usable with DT systems. We used that fact
>>> when converting platform based machines over to DT, one driver at a
>>> time. Look in the git history for kirkwood devices. e.g. somewhere
>>> around v3.7, arch/arm/mach-kirkwood. board-dt.c, and the various
>>> board-*.c files, and the DT files in the usual place.
>>>
>>
>> OMAP did the same and still some boards use platform data and manually
>> register platform devices from board code. Take a look to
>> arch/arm/mach-omap2/pdata-quirks.c to see how that is being done.
> 
> Hello,
> 
> I tried compiling an ancient SDHCI driver on a v4.2 system. It crashes
> all over init because several host->ops functions are required, but the
> old driver does not define them:
> .reset
> .set_clock
> .set_bus_width
> .set_uhs_signaling
> 
> So I downgraded to an older v3.14 kernel, and that problem vanished.
> But I am having a problem with the IRQ setup.
> 
> # cat /proc/interrupts 
>             CPU0       CPU1       
>  18:         93          0      irq0   1 Level     serial
>  55:       2832          0      irq0  38 Level     26000.ethernet
>  60:          0          0      irq0  43 Edge      mmc0
> 211:        319       2603       GIC  29 Edge      twd
> 
> Ethernet is using IRQ 38, as specified in the DT.
> mmc0 is supposed to use IRQ 60.
> 
> I see that the mmc0 has the index 60, so I must have messed up between
> the real irq (hwirq?) and the index Linux uses internally (virq?)
> 
> static struct resource sdhci_resources[] = {
> 	{
> 		.start	= TANGOX_SDIO0_BASE_ADDR,
> 		.end	= TANGOX_SDIO0_BASE_ADDR + 0x1ff,
> 		.flags	= IORESOURCE_MEM,
> 	},
> 	{
> 		.start	= 60,	/* SDHCI0 IRQ */
> 		.flags	= IORESOURCE_IRQ,
> 	},
> };
> 
> Both ethernet and sdhci driver call platform_get_irq(pdev, 0);
> but the eth driver is DT, so calls of_irq_get() while sdhci is
> legacy, so calls platform_get_resource() -- IIUC.
> 
> How do I specify a "hwirq" instead of a "Linux index"? and where?

You don't. Either you're using DT all over the place (and the problem is
moot), or you're not using DT at all (and you're left with hardcoding
everything). There is strictly no provision for "mix-and-match", because
that ends up being an intricate (and unmaintainable) mess.

If you want a temporary workaround, have a look at 0fb22a8 ("ARM: OMAP:
Work around hardcoded interrupts"). That will give you an idea of how to
convert one into the other at runtime. It will also give you an insight
as to why you really don't want this in a mainline kernel, and certainly
not on a new platform.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Grafting old platform drivers onto a new DT kernel

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> On 05/11/2015 16:42, Javier Martinez Canillas wrote:
>> Hello,
>> 
>> On Thu, Nov 5, 2015 at 12:15 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>>>> Since I don't have time to rewrite the drivers at the moment, I'm wondering
>>>> if it's possible to "graft" old drivers (they're using the platform API, no
>>>> trace of DT support) onto my small base?
>>>
>>> Platform drivers are still usable with DT systems. We used that fact
>>> when converting platform based machines over to DT, one driver at a
>>> time. Look in the git history for kirkwood devices. e.g. somewhere
>>> around v3.7, arch/arm/mach-kirkwood. board-dt.c, and the various
>>> board-*.c files, and the DT files in the usual place.
>>>
>> 
>> OMAP did the same and still some boards use platform data and manually
>> register platform devices from board code. Take a look to
>> arch/arm/mach-omap2/pdata-quirks.c to see how that is being done.
>
> Hello,
>
> I tried compiling an ancient SDHCI driver on a v4.2 system. It crashes
> all over init because several host->ops functions are required, but the
> old driver does not define them:
> .reset
> .set_clock
> .set_bus_width
> .set_uhs_signaling
>
> So I downgraded to an older v3.14 kernel, and that problem vanished.
> But I am having a problem with the IRQ setup.
>
> # cat /proc/interrupts 
>             CPU0       CPU1       
>  18:         93          0      irq0   1 Level     serial
>  55:       2832          0      irq0  38 Level     26000.ethernet
>  60:          0          0      irq0  43 Edge      mmc0
> 211:        319       2603       GIC  29 Edge      twd
>
> Ethernet is using IRQ 38, as specified in the DT.
> mmc0 is supposed to use IRQ 60.
>
> I see that the mmc0 has the index 60, so I must have messed up between
> the real irq (hwirq?) and the index Linux uses internally (virq?)
>
> static struct resource sdhci_resources[] = {
> 	{
> 		.start	= TANGOX_SDIO0_BASE_ADDR,
> 		.end	= TANGOX_SDIO0_BASE_ADDR + 0x1ff,
> 		.flags	= IORESOURCE_MEM,
> 	},
> 	{
> 		.start	= 60,	/* SDHCI0 IRQ */
> 		.flags	= IORESOURCE_IRQ,
> 	},
> };
>
> Both ethernet and sdhci driver call platform_get_irq(pdev, 0);
> but the eth driver is DT, so calls of_irq_get() while sdhci is
> legacy, so calls platform_get_resource() -- IIUC.
>
> How do I specify a "hwirq" instead of a "Linux index"? and where?

The simplest solution for you is probably to add a quick and dirty DT
binding to the old driver.  If it doesn't use any driver-specific
platform data struct, you only need to set .of_match_table in the
struct platform_driver.  If there is a platform data struct, you'll also
need to write some code to populate it from DT properties.  It shouldn't
take more than a few minutes per driver in most cases.

To get those drivers accepted upstream will obviously take a bit more
work.

-- 
M?ns Rullg?rd
mans at mansr.com

Grafting old platform drivers onto a new DT kernel

[Mason]

On 09/11/2015 16:40, M?ns Rullg?rd wrote:

> The simplest solution for you is probably to add a quick and dirty DT
> binding to the old driver.  If it doesn't use any driver-specific
> platform data struct, you only need to set .of_match_table in the
> struct platform_driver.  If there is a platform data struct, you'll also
> need to write some code to populate it from DT properties.  It shouldn't
> take more than a few minutes per driver in most cases.

I'll try that approach, although I fear that "a few minutes per driver"
is an optimistic assessment.

> To get those drivers accepted upstream will obviously take a bit more
> work.

AKA "rewrite from scratch" :-)

Regards.

Grafting old platform drivers onto a new DT kernel

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> On 09/11/2015 16:40, M?ns Rullg?rd wrote:
>
>> The simplest solution for you is probably to add a quick and dirty DT
>> binding to the old driver.  If it doesn't use any driver-specific
>> platform data struct, you only need to set .of_match_table in the
>> struct platform_driver.  If there is a platform data struct, you'll also
>> need to write some code to populate it from DT properties.  It shouldn't
>> take more than a few minutes per driver in most cases.
>
> I'll try that approach, although I fear that "a few minutes per driver"
> is an optimistic assessment.

If the driver only needs an MMIO region and an IRQ, it is literally five
lines of code.

-- 
M?ns Rullg?rd
mans at mansr.com

Grafting old platform drivers onto a new DT kernel

[Mason]

On 09/11/2015 17:12, M?ns Rullg?rd wrote:

> Mason writes:
> 
>> On 09/11/2015 16:40, M?ns Rullg?rd wrote:
>>
>>> The simplest solution for you is probably to add a quick and dirty DT
>>> binding to the old driver.  If it doesn't use any driver-specific
>>> platform data struct, you only need to set .of_match_table in the
>>> struct platform_driver.  If there is a platform data struct, you'll also
>>> need to write some code to populate it from DT properties.  It shouldn't
>>> take more than a few minutes per driver in most cases.
>>
>> I'll try that approach, although I fear that "a few minutes per driver"
>> is an optimistic assessment.
> 
> If the driver only needs an MMIO region and an IRQ, it is literally five
> lines of code.

It took me 7 days to figure out there were 2 lines missing in the
interrupt controller driver.

My problem is that I don't understand the platform API, nor the
interaction with the DT API.

Let me see...

In arch/arm/mach-tangox/platform_dev.c

static struct platform_device tangox_sdhci0_device = { ... };
static struct platform_device tangox_sdhci1_device = { ... };

static void tangox_init_sdhci(void)
{
	if (tangox_sdio_enabled(0))
		platform_device_register(&tangox_sdhci0_device);

	if (tangox_sdio_enabled(1))
		platform_device_register(&tangox_sdhci1_device);
}

called from tangox_init_devices() which is marked arch_initcall.



In the driver

static struct platform_driver tangox_platform_sdio0 = {
	.remove		= sdhci_tangox_remove,
	.suspend	= sdhci_tangox_suspend,
	.resume		= sdhci_tangox_resume,
	.driver		= {
		.name	= "tangox-sdhci",
		.owner	= THIS_MODULE,
	},
};


static struct platform_driver tangox_platform_sdio0 = {
	.remove		= sdhci_tangox_remove,
	.driver		= {
		.name	= "tangox-sdhci",
		.owner	= THIS_MODULE,
	},
};

static int __init tangox_sdhci_drv_init(void) {
	return platform_driver_probe(&tangox_platform_sdio0, sdhci_tangox_probe);
}

static void __exit tangox_sdhci_drv_exit(void) {
	platform_driver_unregister(&tangox_platform_sdio0);
}

module_init(tangox_sdhci_drv_init);
module_exit(tangox_sdhci_drv_exit);


The old way:

1) call platform_device_register() with a "struct platform_device"
2) call platform_driver_probe with a "struct platform_driver"

The new way(?)

The mess in 2) is hidden behind module_platform_driver?
The platform_device_register() is done by the DT core?
The struct platform_driver requires a probe function?

Regards.

Grafting old platform drivers onto a new DT kernel

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> On 09/11/2015 17:12, M?ns Rullg?rd wrote:
>
>> Mason writes:
>> 
>>> On 09/11/2015 16:40, M?ns Rullg?rd wrote:
>>>
>>>> The simplest solution for you is probably to add a quick and dirty DT
>>>> binding to the old driver.  If it doesn't use any driver-specific
>>>> platform data struct, you only need to set .of_match_table in the
>>>> struct platform_driver.  If there is a platform data struct, you'll also
>>>> need to write some code to populate it from DT properties.  It shouldn't
>>>> take more than a few minutes per driver in most cases.
>>>
>>> I'll try that approach, although I fear that "a few minutes per driver"
>>> is an optimistic assessment.
>> 
>> If the driver only needs an MMIO region and an IRQ, it is literally five
>> lines of code.
>
> It took me 7 days to figure out there were 2 lines missing in the
> interrupt controller driver.
>
> My problem is that I don't understand the platform API, nor the
> interaction with the DT API.
>
> Let me see...
>
> In arch/arm/mach-tangox/platform_dev.c
>
> static struct platform_device tangox_sdhci0_device = { ... };
> static struct platform_device tangox_sdhci1_device = { ... };
>
> static void tangox_init_sdhci(void)
> {
> 	if (tangox_sdio_enabled(0))
> 		platform_device_register(&tangox_sdhci0_device);
>
> 	if (tangox_sdio_enabled(1))
> 		platform_device_register(&tangox_sdhci1_device);
> }
>
> called from tangox_init_devices() which is marked arch_initcall.

Delete all of that.  The generic DT code will create the platform
devices based on the device tree.

> In the driver

Add something like this:

static const struct of_device_id tangox_sdio_dt_ids[] = {
	{ .compatible = "sigma,tangox-sdio" },
	{ }
};

> static struct platform_driver tangox_platform_sdio0 = {

	.probe		= sdhci_tangox_probe,

> 	.remove		= sdhci_tangox_remove,
> 	.suspend	= sdhci_tangox_suspend,
> 	.resume		= sdhci_tangox_resume,
> 	.driver		= {
> 		.name	= "tangox-sdhci",
> 		.owner	= THIS_MODULE,

		.of_match_table = tangox_sdio_dt_ids,

> 	},
> };

And that should be it.

> static int __init tangox_sdhci_drv_init(void) {
> 	return platform_driver_probe(&tangox_platform_sdio0, sdhci_tangox_probe);
> }
>
> static void __exit tangox_sdhci_drv_exit(void) {
> 	platform_driver_unregister(&tangox_platform_sdio0);
> }
>
> module_init(tangox_sdhci_drv_init);
> module_exit(tangox_sdhci_drv_exit);

You can replace those functions and module_init()/module_exit() with
module_platform_driver() if you want.

> The old way:
>
> 1) call platform_device_register() with a "struct platform_device"
> 2) call platform_driver_probe with a "struct platform_driver"
>
> The new way(?)
>
> The mess in 2) is hidden behind module_platform_driver?

module_platform_driver() is just a macro that creates standard driver
register/unregister functions much like the ones above.

> The platform_device_register() is done by the DT core?

Correct.

> The struct platform_driver requires a probe function?

That's the usual way.

-- 
M?ns Rullg?rd
mans at mansr.com

Grafting old platform drivers onto a new DT kernel

[Mason]

On 09/11/2015 18:13, M?ns Rullg?rd wrote:

> Add something like this:
> 
> static const struct of_device_id tangox_sdio_dt_ids[] = {
> 	{ .compatible = "sigma,tangox-sdio" },
> 	{ }
> };
> 
> static struct platform_driver tangox_platform_sdio0 = {
> 	.probe		= sdhci_tangox_probe,

It looks like one side effect of this transformation is that
the probe function cannot be __init anymore? Is that correct?

For this one particular driver, it weighs 944 bytes. (I guess
a few kilobytes wasted is no big deal...)

Regards.

Grafting old platform drivers onto a new DT kernel

[Arnd Bergmann]

On Tuesday 10 November 2015 13:44:48 Mason wrote:
> On 09/11/2015 18:13, M?ns Rullg?rd wrote:
> 
> > Add something like this:
> > 
> > static const struct of_device_id tangox_sdio_dt_ids[] = {
> >       { .compatible = "sigma,tangox-sdio" },
> >       { }
> > };
> > 
> > static struct platform_driver tangox_platform_sdio0 = {
> >       .probe          = sdhci_tangox_probe,
> 
> It looks like one side effect of this transformation is that
> the probe function cannot be __init anymore? Is that correct?
> 
> For this one particular driver, it weighs 944 bytes. (I guess
> a few kilobytes wasted is no big deal...)

Strictly speaking, it was already broken before. You can
detach and reattach a device from a driver through sysfs,
and that will call the probe function again, so it cannot
be marked as __init.

	Arnd

Grafting old platform drivers onto a new DT kernel

[Russell King - ARM Linux]

On Mon, Nov 09, 2015 at 04:15:03PM +0100, Mason wrote:
> I tried compiling an ancient SDHCI driver on a v4.2 system. It crashes
> all over init because several host->ops functions are required, but the
> old driver does not define them:
> .reset
> .set_clock
> .set_bus_width
> .set_uhs_signaling
> 
> So I downgraded to an older v3.14 kernel, and that problem vanished.

Nothing to do with DT.  Everything to do with improvements happening as
the kernel moves forward, and out of tree drivers suffer because they're
not _in_tree_.

Having stuff out of mainline is painful for this very reason, especially
if no one maintains the driver with each kernel revision - you end up
with a huge delta which is then very time consuming to fix.

However, in this case, all you need to resolve there is to set those
mandatory methods to the appropriate library functions - the changes
which created the ops methods was part of an effort to change the
horrid SDHCI core layer to be more of a library, and to stop it
becoming a driver where every single line is conditional on a quirk
bit being set or clear.

> But I am having a problem with the IRQ setup.
> 
> # cat /proc/interrupts 
>             CPU0       CPU1       
>  18:         93          0      irq0   1 Level     serial
>  55:       2832          0      irq0  38 Level     26000.ethernet
>  60:          0          0      irq0  43 Edge      mmc0
> 211:        319       2603       GIC  29 Edge      twd
> 
> Ethernet is using IRQ 38, as specified in the DT.
> mmc0 is supposed to use IRQ 60.
> 
> I see that the mmc0 has the index 60, so I must have messed up between
> the real irq (hwirq?) and the index Linux uses internally (virq?)
> 
> static struct resource sdhci_resources[] = {
> 	{
> 		.start	= TANGOX_SDIO0_BASE_ADDR,
> 		.end	= TANGOX_SDIO0_BASE_ADDR + 0x1ff,
> 		.flags	= IORESOURCE_MEM,
> 	},
> 	{
> 		.start	= 60,	/* SDHCI0 IRQ */
> 		.flags	= IORESOURCE_IRQ,
> 	},
> };

That tells the kernel to use interrupt 60, which is exactly what it
did.  To get interrupt 60 on the "irq0" controller, you need to look
up that interrupt - this would be a hack though:

+       struct device_node *np;
+
+       np = of_find_node_by_name(NULL, irq_controller_name);
+       if (np) {
+               struct of_phandle_args args;
+
+               args.np = np;
+               args.args[0] = irq - 1;
+               args.args_count = 1;
+
+               irq = irq_create_of_mapping(&args);
+               of_node_put(np);
+       }
+       return irq;

The _easiest_ way to get an old platform driver working at a basic level
with DT is:

1. Declare the device with that compatible value in your DT, with
   appropriate "reg" and "interrupts" properties.

2. If you need platform data for _any_ device, then use a .init_machine
   method in your platform code to call of_platform_populate() instead
   of using the generic version in customize_machine().  Pass the 3rd
   argument a struct of_dev_auxdata table, which allows you to bind
   platform data to the DT-created platform devices.  Set the bus-id
   for the device to the name of the device as it would have been
   originally created (in other words, the driver name, optionally
   followed by a period and a instance number.)

I don't think you have to modify the platform driver at all to make it
work (eg, you don't need an .of_match_table).  I've never used that
stuff, so there may be issues I don't know about.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.