Amba bus definition (amba/bus.h)

[<Nick.Thompson@xxxxxxxxxxxx>]

Hi,

I'm porting Linux to an SoC device with a small number of amba peripherals on the chip.

While getting the SD-Card interface to work (mmci.c PL-180/1) I was surprised to find that the driver took irq numbers from the amba /device/ structure, but not the interrupt flags or the clock name. These where hard coded in the driver. This seems to be the case for all amba peripherals.

I extended the amba device structure to add three new fields at the end:

+#define AMBA_DEVICE_EXT_IRQF    (1 << 0)
+#define AMBA_DEVICE_EXT_CLK (1 << 1)

 struct amba_device {
     struct device       dev;
     struct resource     res;
     u64         dma_mask;
     unsigned int        periphid;
     unsigned int        irq[AMBA_NR_IRQS];
+    unsigned int        ext; /* bit map of extensions specified */
+    unsigned int        irqf[AMBA_NR_IRQS]; /* IRQF flags for each IRQ */
+    char *          clk; /* Name of the devices clock source */
 };

irqf allows the irq's flags to be defined (example: enabling entropy sampling with this device) and clk enables the device to specify which clock source the device is getting.

I then changed the mmci driver to use these new fields:

-    host->clk = clk_get(&dev->dev, "MCLK");
+    host->clk = clk_get(&dev->dev, dev->clk);

and

-    ret = request_irq(dev->irq[0], mmci_irq, IRQF_SHARED, DRIVER_NAME " (cmd)", host);
+    ret = request_irq(dev->irq[0], mmci_irq, dev->irqf[0], DRIVER_NAME " (cmd)", host);

-    ret = request_irq(dev->irq[1], mmci_pio_irq, IRQF_SHARED, DRIVER_NAME " (pio)", host);
+    ret = request_irq(dev->irq[1], mmci_pio_irq, dev->irqf[1], DRIVER_NAME " (pio)", host);

The device definition then becomes (in my case):

 static struct amba_device mmci0_device = {
     .periphid   = 0x00041180,
     .dev        = {
         .bus_id = "mmci-pl18x",
         .platform_data = &mmc0_plat_data,
     },
     .res        = {
         .start  = VX_MMC0_BASE,
         .end    = VX_MMC0_BASE + SZ_4K - 1,
         .flags  = IORESOURCE_MEM,
     },
     .irq        = { VXIRQ_MMC0_IRQ0, VXIRQ_MMC0_IRQ1 },
+    .ext        = (AMBA_DEVICE_EXT_IRQF | AMBA_DEVICE_EXT_CLK),
+    .irqf       = { IRQF_SAMPLE_RANDOM | IRQF_TRIGGER_HIGH,
+                IRQF_SAMPLE_RANDOM | IRQF_TRIGGER_HIGH },
+    .clk        = "CLK_PER1",
 };

I think this is a much clearer solution than having to define clock fakes/aliases for all the different clock names used by amba drivers, when the device definition already has similar hardware connectivity information (the irq numbers). It also enables the device to take *full* control of its interrupts. I think that is much better partitioning of driver from device.

The ext field is used for backward and forward compatibility. So the mmci driver restores its old defaults when the specific extension fields are not initialised:

+    /* amba devices extension backward compatibility checks */
+    if (!(dev->ext & AMBA_DEVICE_EXT_IRQF)) {
+        dev->irqf[0] = IRQF_SHARED;
+        dev->irqf[1] = IRQF_SHARED;
+    }
+    if (!(dev->ext & AMBA_DEVICE_EXT_CLK)) {
+        dev->clk = "MCLK";
+    }

A simpler backward only compatibility scheme would simply be a version counter. Devices using a newer version number would have to specify all the backward defined fields. Not as flexible, but can support 2^32-1 version updates rather than just 31.

This change doesn't affect drivers that don't want to use the extension fields - except for a small amount of extra memory used by the new fields in the device definition.

So, questions:

Does anybody here care about amba bus, or should I be approaching somebody else?

Do these changes look reasonable (comments welcome)?

Could they be submitted back to the Linux tree (if I generate a proper diff)?

Who would I send the patch to?

Sorry for the long e-mail,
Thanks,
Nick.

-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php