Grafting old platform drivers onto a new DT kernel
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.
Quoted 3 lines
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.
Quoted 1 line
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
Hello,
Quoted 10 lines · Andrew Lunn
Quoted 3 lines
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
Quoted 14 lines · Javier Martinez Canillas
Hello,
Quoted 10 lines · Andrew Lunn
Quoted 3 lines
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.
Quoted 3 lines
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.cto 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 twdEthernet 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.
Quoted 57 lines · Mason
Quoted 14 lines · Javier Martinez Canillas
Hello,
Quoted 10 lines · Andrew Lunn
Quoted 3 lines
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.Quoted 3 lines
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.cto 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->opsfunctions are required, but the old driver does not define them:
.reset
.set_clock
.set_bus_width
.set_uhs_signalingSo 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 twdEthernet 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, }, };
Quoted 5 lines
Both ethernet and sdhci driver call
platform_get_irq(pdev, 0); but the eth driver is DT, so callsof_irq_get()while sdhci is legacy, so callsplatform_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.
Signature · 1 line
Jazz is not dead. It just smells funny...
Hi Marc,
Quoted 11 lines · Marc Zyngier <marc.zyngier <at> arm.com> writes:
Quoted 5 lines
Both ethernet and sdhci driver call
platform_get_irq(pdev, 0); but the eth driver is DT, so callsof_irq_get()while sdhci is legacy, so callsplatform_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.
Quoted 6 lines
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.
One question about commit 0fb22a8 ("ARM: OMAP: Work around hardcoded interrupts"), there is a call to irq_create_of_mapping(), but no call to irq_dispose_mapping(), is that intended?
In previous API iterations, we had request_irq() and free_irq()
$ git grep irq_dispose_mapping drivers/ | wc -l 105 $ git grep irq_dispose_mapping arch/arm/mach-omap2/ | wc -l 0
Regards,
Sebastian
Quoted 57 lines · Mason
Quoted 14 lines · Javier Martinez Canillas
Hello,
Quoted 10 lines · Andrew Lunn
Quoted 3 lines
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.Quoted 3 lines
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.cto 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->opsfunctions are required, but the old driver does not define them:
.reset
.set_clock
.set_bus_width
.set_uhs_signalingSo 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 twdEthernet 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, }, };
Quoted 5 lines
Both ethernet and sdhci driver call
platform_get_irq(pdev, 0); but the eth driver is DT, so callsof_irq_get()while sdhci is legacy, so callsplatform_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.
Signature · 2 lines
M?ns Rullg?rd mans at mansr.com
Quoted 6 lines · M?ns Rullg?rd
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.
Quoted 2 lines
To get those drivers accepted upstream will obviously take a bit more work.
AKA "rewrite from scratch" :-)
Regards.
Quoted 11 lines · Mason
Quoted 6 lines · M?ns Rullg?rd
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.
Signature · 2 lines
M?ns Rullg?rd mans at mansr.com
Quoted 11 lines · M?ns Rullg?rd
Quoted 9 lines · Mason
Quoted 6 lines · M?ns Rullg?rd
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.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 41 lines · Mason
Quoted 11 lines · M?ns Rullg?rd
Quoted 9 lines · Mason
Quoted 6 lines · M?ns Rullg?rd
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.Quoted 5 lines
Quoted 2 lines
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.cstatic struct
platform_devicetangox_sdhci0_device= { ... }; static structplatform_devicetangox_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); }
Quoted 1 line
called from
tangox_init_devices()which is markedarch_initcall.
Delete all of that. The generic DT code will create the platform devices based on the device tree.
Quoted 1 line
In the driver
Add something like this:
static const struct of_device_id tangox_sdio_dt_ids[] = {
{ .compatible = "sigma,tangox-sdio" },
{ }
};Quoted 1 line
static struct
platform_drivertangox_platform_sdio0= {
.probe = sdhci_tangox_probe,
Quoted 6 lines
.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,
Quoted 2 lines
}, };
And that should be it.
Quoted 10 lines
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.
Quoted 8 lines
The old way:
1) call
platform_device_register()with a "structplatform_device"
2) callplatform_driver_probewith a "structplatform_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.
Quoted 1 line
The
platform_device_register()is done by the DT core?
Correct.
Quoted 1 line
The struct
platform_driverrequires a probe function?
That's the usual way.
Signature · 2 lines
M?ns Rullg?rd mans at mansr.com
Quoted 9 lines · M?ns Rullg?rd
Add something like this:
static const struct of_device_id tangox_sdio_dt_ids[] = { { .compatible = "sigma,tangox-sdio" }, { } };static struct
platform_drivertangox_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.
Quoted 9 lines · On Tuesday 10 November 2015 13:44:48 Mason wrote:
Quoted 7 lines · M?ns Rullg?rd
Add something like this:
static const struct
of_device_idtangox_sdio_dt_ids[] = {
{ .compatible = "sigma,tangox-sdio" },
{ }
};
Quoted 8 lines
Quoted 2 lines
static struct
platform_drivertangox_platform_sdio0= {
.probe =sdhci_tangox_probe,It looks like one side effect of this transformation is that the probe function cannot be
__initanymore? 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
Quoted 9 lines · Mason
I tried compiling an ancient SDHCI driver on a v4.2 system. It crashes all over init because several
host->opsfunctions are required, but the old driver does not define them:
.reset
.set_clock
.set_bus_width
.set_uhs_signalingSo 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.
Quoted 15 lines
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 twdEthernet 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?)
Quoted 11 lines
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.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.