linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Grafting old platform drivers onto a new DT kernel
@ 2015-11-05 11:02 Mason
  2015-11-05 15:15 ` Andrew Lunn
  0 siblings, 1 reply; 14+ messages in thread
From: Mason @ 2015-11-05 11:02 UTC (permalink / raw)
  To: Linux ARM, LKML

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Grafting old platform drivers onto a new DT kernel
  2015-11-05 11:02 Grafting old platform drivers onto a new DT kernel Mason
@ 2015-11-05 15:15 ` Andrew Lunn
  2015-11-05 15:42   ` Javier Martinez Canillas
  0 siblings, 1 reply; 14+ messages in thread
From: Andrew Lunn @ 2015-11-05 15:15 UTC (permalink / raw)
  To: Mason; +Cc: Linux ARM, LKML

> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Grafting old platform drivers onto a new DT kernel
  2015-11-05 15:15 ` Andrew Lunn
@ 2015-11-05 15:42   ` Javier Martinez Canillas
  2015-11-09 15:15     ` Mason
  0 siblings, 1 reply; 14+ messages in thread
From: Javier Martinez Canillas @ 2015-11-05 15:42 UTC (permalink / raw)
  To: Andrew Lunn; +Cc: Mason, LKML, Linux ARM

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Grafting old platform drivers onto a new DT kernel
  2015-11-05 15:42   ` Javier Martinez Canillas
@ 2015-11-09 15:15     ` Mason
  2015-11-09 15:36       ` Marc Zyngier
                         ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Mason @ 2015-11-09 15:15 UTC (permalink / raw)
  To: Javier Martinez Canillas, Andrew Lunn
  Cc: LKML, Linux ARM, Marc Zyngier, Jason Cooper, Thomas Gleixner,
	Ulf Hansson

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.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Grafting old platform drivers onto a new DT kernel
  2015-11-09 15:15     ` Mason
@ 2015-11-09 15:36       ` Marc Zyngier
  2016-02-03 15:33         ` Sebastian Frias
  2015-11-09 15:40       ` Måns Rullgård
  2015-11-09 16:26       ` Russell King - ARM Linux
  2 siblings, 1 reply; 14+ messages in thread
From: Marc Zyngier @ 2015-11-09 15:36 UTC (permalink / raw)
  To: Mason, Javier Martinez Canillas, Andrew Lunn
  Cc: LKML, Linux ARM, Jason Cooper, Thomas Gleixner, Ulf Hansson

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...

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Grafting old platform drivers onto a new DT kernel
  2015-11-09 15:15     ` Mason
  2015-11-09 15:36       ` Marc Zyngier
@ 2015-11-09 15:40       ` Måns Rullgård
  2015-11-09 16:07         ` Mason
  2015-11-09 16:26       ` Russell King - ARM Linux
  2 siblings, 1 reply; 14+ messages in thread
From: Måns Rullgård @ 2015-11-09 15:40 UTC (permalink / raw)
  To: Mason
  Cc: Javier Martinez Canillas, Andrew Lunn, LKML, Linux ARM,
	Marc Zyngier, Jason Cooper, Thomas Gleixner, Ulf Hansson

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@mansr.com

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Grafting old platform drivers onto a new DT kernel
  2015-11-09 15:40       ` Måns Rullgård
@ 2015-11-09 16:07         ` Mason
  2015-11-09 16:12           ` Måns Rullgård
  0 siblings, 1 reply; 14+ messages in thread
From: Mason @ 2015-11-09 16:07 UTC (permalink / raw)
  To: Mans Rullgard
  Cc: Javier Martinez Canillas, Andrew Lunn, LKML, Linux ARM,
	Marc Zyngier, Jason Cooper, Thomas Gleixner, Ulf Hansson

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.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Grafting old platform drivers onto a new DT kernel
  2015-11-09 16:07         ` Mason
@ 2015-11-09 16:12           ` Måns Rullgård
  2015-11-09 17:03             ` Mason
  0 siblings, 1 reply; 14+ messages in thread
From: Måns Rullgård @ 2015-11-09 16:12 UTC (permalink / raw)
  To: Mason
  Cc: Javier Martinez Canillas, Andrew Lunn, LKML, Linux ARM,
	Marc Zyngier, Jason Cooper, Thomas Gleixner, Ulf Hansson

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@mansr.com

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Grafting old platform drivers onto a new DT kernel
  2015-11-09 15:15     ` Mason
  2015-11-09 15:36       ` Marc Zyngier
  2015-11-09 15:40       ` Måns Rullgård
@ 2015-11-09 16:26       ` Russell King - ARM Linux
  2 siblings, 0 replies; 14+ messages in thread
From: Russell King - ARM Linux @ 2015-11-09 16:26 UTC (permalink / raw)
  To: Mason
  Cc: Javier Martinez Canillas, Andrew Lunn, Ulf Hansson, Jason Cooper,
	Marc Zyngier, LKML, Thomas Gleixner, Linux ARM

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Grafting old platform drivers onto a new DT kernel
  2015-11-09 16:12           ` Måns Rullgård
@ 2015-11-09 17:03             ` Mason
  2015-11-09 17:13               ` Måns Rullgård
  0 siblings, 1 reply; 14+ messages in thread
From: Mason @ 2015-11-09 17:03 UTC (permalink / raw)
  To: Mans Rullgard
  Cc: Javier Martinez Canillas, Andrew Lunn, LKML, Linux ARM,
	Marc Zyngier, Jason Cooper, Thomas Gleixner, Ulf Hansson

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.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Grafting old platform drivers onto a new DT kernel
  2015-11-09 17:03             ` Mason
@ 2015-11-09 17:13               ` Måns Rullgård
  2015-11-10 12:44                 ` Mason
  0 siblings, 1 reply; 14+ messages in thread
From: Måns Rullgård @ 2015-11-09 17:13 UTC (permalink / raw)
  To: Mason
  Cc: Javier Martinez Canillas, Andrew Lunn, LKML, Linux ARM,
	Marc Zyngier, Jason Cooper, Thomas Gleixner, Ulf Hansson

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@mansr.com

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Grafting old platform drivers onto a new DT kernel
  2015-11-09 17:13               ` Måns Rullgård
@ 2015-11-10 12:44                 ` Mason
  2015-11-10 12:56                   ` Arnd Bergmann
  0 siblings, 1 reply; 14+ messages in thread
From: Mason @ 2015-11-10 12:44 UTC (permalink / raw)
  To: Mans Rullgard
  Cc: Javier Martinez Canillas, Andrew Lunn, LKML, Linux ARM,
	Marc Zyngier, Jason Cooper, Thomas Gleixner, Ulf Hansson

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.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Grafting old platform drivers onto a new DT kernel
  2015-11-10 12:44                 ` Mason
@ 2015-11-10 12:56                   ` Arnd Bergmann
  0 siblings, 0 replies; 14+ messages in thread
From: Arnd Bergmann @ 2015-11-10 12:56 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Mason, Mans Rullgard, Andrew Lunn, Ulf Hansson, Jason Cooper,
	Marc Zyngier, LKML, Javier Martinez Canillas, Thomas Gleixner

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Grafting old platform drivers onto a new DT kernel
  2015-11-09 15:36       ` Marc Zyngier
@ 2016-02-03 15:33         ` Sebastian Frias
  0 siblings, 0 replies; 14+ messages in thread
From: Sebastian Frias @ 2016-02-03 15:33 UTC (permalink / raw)
  To: linux-kernel

Hi Marc,

Marc Zyngier <marc.zyngier <at> arm.com> writes:
> > 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.
> 

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2016-02-03 15:35 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-05 11:02 Grafting old platform drivers onto a new DT kernel Mason
2015-11-05 15:15 ` Andrew Lunn
2015-11-05 15:42   ` Javier Martinez Canillas
2015-11-09 15:15     ` Mason
2015-11-09 15:36       ` Marc Zyngier
2016-02-03 15:33         ` Sebastian Frias
2015-11-09 15:40       ` Måns Rullgård
2015-11-09 16:07         ` Mason
2015-11-09 16:12           ` Måns Rullgård
2015-11-09 17:03             ` Mason
2015-11-09 17:13               ` Måns Rullgård
2015-11-10 12:44                 ` Mason
2015-11-10 12:56                   ` Arnd Bergmann
2015-11-09 16:26       ` Russell King - ARM Linux

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).