This patch set introduces use of the pl011 AMBA serial port under a
PCI bridge. To compile AMBA under x86, though I need <asm/sizes.h>,
which is moved to <linux/sizes.h> as suggested earlier.
I'm hereby volunteering to handle the moving of the various users
of <asm/sizes.h> to <linux/sizes.h>; this set only moves the ARM core
files and the ones that I need under x86.
The whole patch set is sent to the same set of recipients:
all relevant lists, Russell King (for arm), Greg-KH (for uart) and
Arnd Bergmann (for generic include).
With this set in place (plus a clok API not included here) I have
4 serial ports working. We have a number of other devices that can
use existing drivers, but we definitely need <linux/sizes.h> first.
spusa.root# uname -r
3.4.0-next-20120524-00014-gae0c129
spusa.root# dmesg | grep ttyA
pl011-pci-03:0005: ttyAMA0 at MMIO 0xcf400000 (irq = 46) is a PL011 rev3
pl011-pci-03:0006: ttyAMA1 at MMIO 0xcec00000 (irq = 47) is a PL011 rev3
pl011-pci-03:0007: ttyAMA2 at MMIO 0xce400000 (irq = 48) is a PL011 rev3
pl011-pci-04:0005: ttyAMA3 at MMIO 0xd3400000 (irq = 49) is a PL011 rev3
spusa.root# grep -C1 pl011 /proc/iomem
ce400000-ce7fffff : 0000:03:00.7
ce400000-ce400fff : pl011-pci-03:0007
ce400000-ce400fff : uart-pl011
ce800000-cebfffff : 0000:03:00.6
cec00000-ceffffff : 0000:03:00.6
cec00000-cec00fff : pl011-pci-03:0006
cec00000-cec00fff : uart-pl011
cf000000-cf3fffff : 0000:03:00.5
cf400000-cf7fffff : 0000:03:00.5
cf400000-cf400fff : pl011-pci-03:0005
cf400000-cf400fff : uart-pl011
cf800000-cfbfffff : 0000:03:00.4
--
d3400000-d37fffff : 0000:04:00.5
d3400000-d3400fff : pl011-pci-04:0005
d3400000-d3400fff : uart-pl011
d3800000-d3bfffff : 0000:04:00.4
Alessandro Rubini (6):
sizes.h: move from asm-generic to <linux/sizes.h>
amba: use the new linux/sizes.h
ARM: use the new linux/sizes.h
serial: use the new linux/sizes.h
x86: add CONFIG_ARM_AMBA, selected by STA2X11
serial: add amba-pl011-pci
arch/arm/include/asm/memory.h | 2 +-
arch/arm/mm/dma-mapping.c | 2 +-
arch/arm/mm/init.c | 2 +-
arch/arm/mm/ioremap.c | 2 +-
arch/arm/mm/mmu.c | 2 +-
arch/x86/Kconfig | 4 ++
drivers/amba/bus.c | 2 +-
drivers/tty/serial/Kconfig | 10 +++-
drivers/tty/serial/Makefile | 1 +
drivers/tty/serial/amba-pl011-pci.c | 101 +++++++++++++++++++++++++++++++++++
drivers/tty/serial/amba-pl011.c | 2 +-
include/asm-generic/sizes.h | 49 +----------------
include/linux/sizes.h | 47 ++++++++++++++++
13 files changed, 171 insertions(+), 55 deletions(-)
create mode 100644 drivers/tty/serial/amba-pl011-pci.c
create mode 100644 include/linux/sizes.h
--
1.7.7.2
Signed-off-by: Alessandro Rubini <[email protected]>
Acked-by: Giancarlo Asnaghi <[email protected]>
Cc: Russell King <[email protected]>
---
drivers/amba/bus.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index b7e7285..e29bfa7 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -18,7 +18,7 @@
#include <linux/amba/bus.h>
#include <asm/irq.h>
-#include <asm/sizes.h>
+#include <linux/sizes.h>
#define to_amba_driver(d) container_of(d, struct amba_driver, drv)
--
1.7.7.2
Signed-off-by: Alessandro Rubini <[email protected]>
Acked-by: Giancarlo Asnaghi <[email protected]>
Cc: Russell King <[email protected]>
---
arch/arm/include/asm/memory.h | 2 +-
arch/arm/mm/dma-mapping.c | 2 +-
arch/arm/mm/init.c | 2 +-
arch/arm/mm/ioremap.c | 2 +-
arch/arm/mm/mmu.c | 2 +-
5 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index fcb5757..e965f1b 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h
@@ -16,7 +16,7 @@
#include <linux/compiler.h>
#include <linux/const.h>
#include <linux/types.h>
-#include <asm/sizes.h>
+#include <linux/sizes.h>
#ifdef CONFIG_NEED_MACH_MEMORY_H
#include <mach/memory.h>
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index ea6b431..30a031c 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -23,12 +23,12 @@
#include <linux/slab.h>
#include <linux/iommu.h>
#include <linux/vmalloc.h>
+#include <linux/sizes.h>
#include <asm/memory.h>
#include <asm/highmem.h>
#include <asm/cacheflush.h>
#include <asm/tlbflush.h>
-#include <asm/sizes.h>
#include <asm/mach/arch.h>
#include <asm/dma-iommu.h>
#include <asm/mach/map.h>
diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index c21d06c..ad7fd8a 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -21,13 +21,13 @@
#include <linux/gfp.h>
#include <linux/memblock.h>
#include <linux/dma-contiguous.h>
+#include <linux/sizes.h>
#include <asm/mach-types.h>
#include <asm/memblock.h>
#include <asm/prom.h>
#include <asm/sections.h>
#include <asm/setup.h>
-#include <asm/sizes.h>
#include <asm/tlb.h>
#include <asm/fixmap.h>
diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index 4f55f50..566750f 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -25,6 +25,7 @@
#include <linux/mm.h>
#include <linux/vmalloc.h>
#include <linux/io.h>
+#include <linux/sizes.h>
#include <asm/cp15.h>
#include <asm/cputype.h>
@@ -32,7 +33,6 @@
#include <asm/mmu_context.h>
#include <asm/pgalloc.h>
#include <asm/tlbflush.h>
-#include <asm/sizes.h>
#include <asm/system_info.h>
#include <asm/mach/map.h>
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index e5dad60..2196116 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -16,13 +16,13 @@
#include <linux/memblock.h>
#include <linux/fs.h>
#include <linux/vmalloc.h>
+#include <linux/sizes.h>
#include <asm/cp15.h>
#include <asm/cputype.h>
#include <asm/sections.h>
#include <asm/cachetype.h>
#include <asm/setup.h>
-#include <asm/sizes.h>
#include <asm/smp_plat.h>
#include <asm/tlb.h>
#include <asm/highmem.h>
--
1.7.7.2
Signed-off-by: Alessandro Rubini <[email protected]>
Acked-by: Giancarlo Asnaghi <[email protected]>
Cc: Russell King <[email protected]>
---
drivers/tty/serial/amba-pl011.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 4ad721f..d394b93 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -53,9 +53,9 @@
#include <linux/delay.h>
#include <linux/types.h>
#include <linux/pinctrl/consumer.h>
+#include <linux/sizes.h>
#include <asm/io.h>
-#include <asm/sizes.h>
#define UART_NR 14
--
1.7.7.2
This is a simple PCI driver that registers an amba device
in its probe function. It successfully drives the 4 serial
ports of the sta2x11 I/O Hub.
Signed-off-by: Alessandro Rubini <[email protected]>
Acked-by: Giancarlo Asnaghi <[email protected]>
Cc: Russell King <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
---
drivers/tty/serial/Kconfig | 10 +++-
drivers/tty/serial/Makefile | 1 +
drivers/tty/serial/amba-pl011-pci.c | 101 +++++++++++++++++++++++++++++++++++
3 files changed, 111 insertions(+), 1 deletions(-)
create mode 100644 drivers/tty/serial/amba-pl011-pci.c
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 070b442..e5e5ef6 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -39,14 +39,22 @@ config SERIAL_AMBA_PL010_CONSOLE
config SERIAL_AMBA_PL011
tristate "ARM AMBA PL011 serial port support"
depends on ARM_AMBA
+ default y if STA2X11
select SERIAL_CORE
help
This selects the ARM(R) AMBA(R) PrimeCell PL011 UART. If you have
an Integrator/PP2, Integrator/CP or Versatile platform, say Y or M
- here.
+ here. This is also needed to use the sta2x11 I/O Hub for Atom.
If unsure, say N.
+config SERIAL_AMBA_PL011_PCI
+ tristate "ARM AMBA PL011 behind a PCI-to-AMBA bridge"
+ depends on SERIAL_AMBA_PL011 && PCI
+ default y if STA2X11
+ help
+ Say Y if your AMBA bus is behind a PCI bridge (e.g.: sta2x11)
+
config SERIAL_AMBA_PL011_CONSOLE
bool "Support for console on AMBA serial port"
depends on SERIAL_AMBA_PL011=y
diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
index 7257c5d..b8cd14b 100644
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_SERIAL_8250) += 8250/
obj-$(CONFIG_SERIAL_AMBA_PL010) += amba-pl010.o
obj-$(CONFIG_SERIAL_AMBA_PL011) += amba-pl011.o
+obj-$(CONFIG_SERIAL_AMBA_PL011_PCI) += amba-pl011-pci.o
obj-$(CONFIG_SERIAL_CLPS711X) += clps711x.o
obj-$(CONFIG_SERIAL_PXA) += pxa.o
obj-$(CONFIG_SERIAL_PNX8XXX) += pnx8xxx_uart.o
diff --git a/drivers/tty/serial/amba-pl011-pci.c b/drivers/tty/serial/amba-pl011-pci.c
new file mode 100644
index 0000000..b3aa0f1
--- /dev/null
+++ b/drivers/tty/serial/amba-pl011-pci.c
@@ -0,0 +1,101 @@
+/*
+ * Support for AMBA pl011 uart behind a PCI bridge
+ * Copyright 2012 ST Microelectronics (Alessandro Rubini)
+ * GNU GPL version 2.
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/amba/bus.h>
+#include <linux/pci.h>
+#include <linux/pci_ids.h>
+#include <linux/slab.h>
+#include <linux/irq.h>
+#include <linux/sizes.h>
+#include <linux/amba/serial.h>
+
+/* This is a template, copied every time a new pci device appears */
+static AMBA_APB_DEVICE(pl011_pci_template, "pl011-pci", 0,
+ 0 /* base */, {0} /* irqs */, NULL /* data */);
+
+static int __devinit pl011_pci_probe(struct pci_dev *pdev,
+ const struct pci_device_id *id)
+{
+ struct amba_device *adev;
+ struct amba_pl011_data *data;
+ int ret;
+
+ /* Simply build an amba device and register it */
+ adev = kmemdup(&pl011_pci_template_device,
+ sizeof(pl011_pci_template_device), GFP_KERNEL);
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!adev || !data) {
+ kfree(adev);
+ kfree(data);
+ return -ENOMEM;
+ }
+ pci_set_master(pdev);
+ pci_enable_msi(pdev);
+ adev->irq[0] = pdev->irq;
+ if (pdev->vendor == PCI_VENDOR_ID_STMICRO) {
+ /* Under sta2x11, DMA is there but limited to 512M */
+ adev->dma_mask = SZ_512M - 1;
+ adev->dev.coherent_dma_mask = SZ_512M - 1;
+ }
+
+ /* Link the pci to amba and the amba to pci */
+ adev->dev.platform_data = data;
+ //data->pdev = pdev;
+ pci_set_drvdata(pdev, adev);
+
+ /* Create a new resource, to be registered as child of the PCI one */
+ adev->res.flags = pdev->resource[0].flags;
+ adev->res.start = pdev->resource[0].start;
+ adev->res.end = adev->res.start + SZ_4K - 1;
+
+ /* change name */
+ adev->dev.init_name = kasprintf(GFP_ATOMIC, "pl011-pci-%02x:%04x",
+ pdev->bus->number, pdev->devfn);
+
+ printk(KERN_INFO "%s %i\n", __func__, __LINE__);
+ if ((ret = amba_device_register(adev, &pdev->resource[0])) < 0) {
+ kfree(adev);
+ return ret;
+ }
+ return 0;
+};
+
+static void __devexit pl011_pci_remove(struct pci_dev *pdev)
+{
+ struct amba_device *adev = pci_get_drvdata(pdev);
+ amba_device_unregister(adev);
+ kfree(adev->dev.platform_data);
+ kfree(adev);
+}
+
+static DEFINE_PCI_DEVICE_TABLE(pl011_pci_table) = {
+ {PCI_VDEVICE(STMICRO, PCI_DEVICE_ID_STMICRO_UART_HWFC)},
+ {PCI_VDEVICE(STMICRO, PCI_DEVICE_ID_STMICRO_UART_NO_HWFC)},
+ {0,}
+};
+
+static struct pci_driver pl011_pci_driver = {
+ .name = "pl011-pci",
+ .id_table = pl011_pci_table,
+ .probe = pl011_pci_probe,
+ .remove = __devexit_p(pl011_pci_remove),
+};
+
+static int __init pl011_pci_init(void)
+{
+ return pci_register_driver(&pl011_pci_driver);
+}
+
+static void __exit pl011_pci_exit(void)
+{
+ pci_unregister_driver(&pl011_pci_driver);
+}
+
+module_init(pl011_pci_init);
+module_exit(pl011_pci_exit);
+
--
1.7.7.2
The sta2x11 I/O Hub is a bridge from PCIe to AMBA. It reuses a number
of amba drivers and needs to activate core bus support.
Signed-off-by: Alessandro Rubini <[email protected]>
Acked-by: Giancarlo Asnaghi <[email protected]>
---
arch/x86/Kconfig | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 4732997..1f3938a 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -499,6 +499,7 @@ config STA2X11
select SWIOTLB
select MFD_STA2X11
select ARCH_REQUIRE_GPIOLIB
+ select ARM_AMBA
default n
---help---
This adds support for boards based on the STA2X11 IO-Hub,
@@ -2154,6 +2155,9 @@ config GEOS
endif # X86_32
+config ARM_AMBA
+ bool
+
config AMD_NB
def_bool y
depends on CPU_SUP_AMD && PCI
--
1.7.7.2
sizes.h is used throughout the AMBA code and drivers, so the header
should be available to everyone in order to driver AMBA/PrimeCell
peripherals behind a PCI bridge where the host can be any platform
(I'm doing it under x86).
At this step <asm-generic/sizes.h> includes <linux/sizes.h>,
to allow a grace period for both in-tree and out-of-tree drivers.
Signed-off-by: Alessandro Rubini <[email protected]>
Acked-by: Giancarlo Asnaghi <[email protected]>
Cc: Russell King <[email protected]>
Cc: Arnd Bergmann <[email protected]>
---
include/asm-generic/sizes.h | 49 +-----------------------------------------
include/linux/sizes.h | 47 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 49 insertions(+), 47 deletions(-)
create mode 100644 include/linux/sizes.h
diff --git a/include/asm-generic/sizes.h b/include/asm-generic/sizes.h
index ea5d4ef..1dcfad9 100644
--- a/include/asm-generic/sizes.h
+++ b/include/asm-generic/sizes.h
@@ -1,47 +1,2 @@
-/*
- * linux/include/asm-generic/sizes.h
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-#ifndef __ASM_GENERIC_SIZES_H__
-#define __ASM_GENERIC_SIZES_H__
-
-#define SZ_1 0x00000001
-#define SZ_2 0x00000002
-#define SZ_4 0x00000004
-#define SZ_8 0x00000008
-#define SZ_16 0x00000010
-#define SZ_32 0x00000020
-#define SZ_64 0x00000040
-#define SZ_128 0x00000080
-#define SZ_256 0x00000100
-#define SZ_512 0x00000200
-
-#define SZ_1K 0x00000400
-#define SZ_2K 0x00000800
-#define SZ_4K 0x00001000
-#define SZ_8K 0x00002000
-#define SZ_16K 0x00004000
-#define SZ_32K 0x00008000
-#define SZ_64K 0x00010000
-#define SZ_128K 0x00020000
-#define SZ_256K 0x00040000
-#define SZ_512K 0x00080000
-
-#define SZ_1M 0x00100000
-#define SZ_2M 0x00200000
-#define SZ_4M 0x00400000
-#define SZ_8M 0x00800000
-#define SZ_16M 0x01000000
-#define SZ_32M 0x02000000
-#define SZ_64M 0x04000000
-#define SZ_128M 0x08000000
-#define SZ_256M 0x10000000
-#define SZ_512M 0x20000000
-
-#define SZ_1G 0x40000000
-#define SZ_2G 0x80000000
-
-#endif /* __ASM_GENERIC_SIZES_H__ */
+/* This is a placeholder, to be removed over time */
+#include <linux/sizes.h>
diff --git a/include/linux/sizes.h b/include/linux/sizes.h
new file mode 100644
index 0000000..ce3e815
--- /dev/null
+++ b/include/linux/sizes.h
@@ -0,0 +1,47 @@
+/*
+ * include/linux/sizes.h
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef __LINUX_SIZES_H__
+#define __LINUX_SIZES_H__
+
+#define SZ_1 0x00000001
+#define SZ_2 0x00000002
+#define SZ_4 0x00000004
+#define SZ_8 0x00000008
+#define SZ_16 0x00000010
+#define SZ_32 0x00000020
+#define SZ_64 0x00000040
+#define SZ_128 0x00000080
+#define SZ_256 0x00000100
+#define SZ_512 0x00000200
+
+#define SZ_1K 0x00000400
+#define SZ_2K 0x00000800
+#define SZ_4K 0x00001000
+#define SZ_8K 0x00002000
+#define SZ_16K 0x00004000
+#define SZ_32K 0x00008000
+#define SZ_64K 0x00010000
+#define SZ_128K 0x00020000
+#define SZ_256K 0x00040000
+#define SZ_512K 0x00080000
+
+#define SZ_1M 0x00100000
+#define SZ_2M 0x00200000
+#define SZ_4M 0x00400000
+#define SZ_8M 0x00800000
+#define SZ_16M 0x01000000
+#define SZ_32M 0x02000000
+#define SZ_64M 0x04000000
+#define SZ_128M 0x08000000
+#define SZ_256M 0x10000000
+#define SZ_512M 0x20000000
+
+#define SZ_1G 0x40000000
+#define SZ_2G 0x80000000
+
+#endif /* __LINUX_SIZES_H__ */
--
1.7.7.2
On Friday 25 May 2012, Alessandro Rubini wrote:
>
> This is a simple PCI driver that registers an amba device
> in its probe function. It successfully drives the 4 serial
> ports of the sta2x11 I/O Hub.
>
> Signed-off-by: Alessandro Rubini <[email protected]>
> Acked-by: Giancarlo Asnaghi <[email protected]>
Aside from the dma mask, this looks almost entirely generic. Would it
be possible to make this a generic pci-amba driver that lives under
drivers/amba/ and does not care about the type of device behind it?
Arnd
> Aside from the dma mask, this looks almost entirely generic. Would it
> be possible to make this a generic pci-amba driver that lives under
> drivers/amba/ and does not care about the type of device behind it?
Yes, it's possible. Actually, this was my longer-time plan: propose a
factorization when more of those were ready. So I'll remove the
special name in the device and offer an implementation as generic
as possible.
BTW, are the prerequisite patches of with you?
thanks
/alessandro
On Saturday 26 May 2012, Alessandro Rubini wrote:
> > Aside from the dma mask, this looks almost entirely generic. Would it
> > be possible to make this a generic pci-amba driver that lives under
> > drivers/amba/ and does not care about the type of device behind it?
>
> Yes, it's possible. Actually, this was my longer-time plan: propose a
> factorization when more of those were ready. So I'll remove the
> special name in the device and offer an implementation as generic
> as possible.
>
> BTW, are the prerequisite patches of with you?
>
Yes, they all look good to me, although patch 5 would have to change
if you do patch 6 the way we discussed here.
Arnd
On Fri, May 25, 2012 at 05:48:12PM +0200, Alessandro Rubini wrote:
> Signed-off-by: Alessandro Rubini <[email protected]>
> Acked-by: Giancarlo Asnaghi <[email protected]>
> Cc: Russell King <[email protected]>
> ---
> drivers/amba/bus.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index b7e7285..e29bfa7 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -18,7 +18,7 @@
> #include <linux/amba/bus.h>
>
> #include <asm/irq.h>
> -#include <asm/sizes.h>
> +#include <linux/sizes.h>
Please move this up alongside the other linux/ includes.
On Fri, May 25, 2012 at 05:48:57PM +0200, Alessandro Rubini wrote:
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 070b442..e5e5ef6 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -39,14 +39,22 @@ config SERIAL_AMBA_PL010_CONSOLE
> config SERIAL_AMBA_PL011
> tristate "ARM AMBA PL011 serial port support"
> depends on ARM_AMBA
> + default y if STA2X11
I don't think we want to encourage an ever growing list of platforms
here. If we did this on ARM, this would be hellishly long.
> +/* This is a template, copied every time a new pci device appears */
> +static AMBA_APB_DEVICE(pl011_pci_template, "pl011-pci", 0,
> + 0 /* base */, {0} /* irqs */, NULL /* data */);
> +
> +static int __devinit pl011_pci_probe(struct pci_dev *pdev,
> + const struct pci_device_id *id)
> +{
> + struct amba_device *adev;
> + struct amba_pl011_data *data;
> + int ret;
> +
> + /* Simply build an amba device and register it */
> + adev = kmemdup(&pl011_pci_template_device,
> + sizeof(pl011_pci_template_device), GFP_KERNEL);
NAK. We have interfaces in the AMBA code for dynamically allocating
AMBA devices now - please use them instead of coding your own. They
avoid bugs.
> + /* change name */
> + adev->dev.init_name = kasprintf(GFP_ATOMIC, "pl011-pci-%02x:%04x",
> + pdev->bus->number, pdev->devfn);
> +
> + printk(KERN_INFO "%s %i\n", __func__, __LINE__);
This looks like debugging.
> + if ((ret = amba_device_register(adev, &pdev->resource[0])) < 0) {
> + kfree(adev);
> + return ret;
> + }
> + return 0;
> +};
> +
> +static void __devexit pl011_pci_remove(struct pci_dev *pdev)
> +{
> + struct amba_device *adev = pci_get_drvdata(pdev);
> + amba_device_unregister(adev);
> + kfree(adev->dev.platform_data);
> + kfree(adev);
This comes nowhere close to being sane with the driver model. This is
an oops waiting to happen. You can't guarantee that 'adev' will be
unused by the time amba_device_unregister() (or in fact any such call
into the driver model) returns.
This is because these suckers (and all devices) are refcounted. If you
use the correct interfaces, the devices will be freed for you automatically
at the correct time.
The only thing that won't be is adev->dev.platform_data - the correct way
to do this is the following along with the correct allocation interfaces:
struct amba_device *adev = pci_get_drvdata(pdev);
void *priv = adev->dev.platform_data;
amba_device_unregister(adev);
kfree(priv);
In other words, save off the platform data pointer, unregister the struct
device, and then free the platform data (it will not be used at that point.)
One other point.
On Fri, May 25, 2012 at 05:48:57PM +0200, Alessandro Rubini wrote:
> +/* This is a template, copied every time a new pci device appears */
> +static AMBA_APB_DEVICE(pl011_pci_template, "pl011-pci", 0,
> + 0 /* base */, {0} /* irqs */, NULL /* data */);
APB device. It's a _peripheral_, it only has a _slave_ port which can't
initiate DMA, so:
> + if (pdev->vendor == PCI_VENDOR_ID_STMICRO) {
> + /* Under sta2x11, DMA is there but limited to 512M */
> + adev->dma_mask = SZ_512M - 1;
> + adev->dev.coherent_dma_mask = SZ_512M - 1;
> + }
This is pointless and unnecessary. The PL011 driver itself doesn't use
_this_ struct device for anything to do with DMA at all. That's all
handled by the DMA engine's struct device.
That's true of all APB devices. Only AHB devices with a master port can
initiate bus transactions, and hence cause accesses to other parts of the
system.
>> + default y if STA2X11
>
> I don't think we want to encourage an ever growing list of platforms
> here. If we did this on ARM, this would be hellishly long.
Ok.
> NAK. We have interfaces in the AMBA code for dynamically allocating
> AMBA devices now - please use them instead of coding your own. They
> avoid bugs.
Sure. Thanks for noting. Maybe it wasn't there when I coded this
initially. Will do.
>> + printk(KERN_INFO "%s %i\n", __func__, __LINE__);
>
> This looks like debugging.
Yes. After sending I noted this and another point. I apologize.
Version 2 will be fixed in all respects.
> This comes nowhere close to being sane with the driver model. [...]
> In other words, save off the platform data pointer, unregister the struct
> device, and then free the platform data (it will not be used at that point.)
Thanks a lot for this and also the comments on APB/AHB in the other message.
/alessandro