[PATCH 0/6] Bridging PCI to amba

[PATCH 0/6] Bridging PCI to amba

[Alessandro Rubini]

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

[PATCH 1/6] sizes.h: move from asm-generic to <linux/sizes.h>

[Alessandro Rubini]

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 <rubini@gnudd.com>
Acked-by: Giancarlo Asnaghi <giancarlo.asnaghi@st.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
---
 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

[PATCH 2/6] amba: use the new linux/sizes.h

[Alessandro Rubini]

Signed-off-by: Alessandro Rubini <rubini@gnudd.com>
Acked-by: Giancarlo Asnaghi <giancarlo.asnaghi@st.com>
Cc: Russell King <linux@arm.linux.org.uk>
---
 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

[PATCH 3/6] ARM: use the new linux/sizes.h

[Alessandro Rubini]

Signed-off-by: Alessandro Rubini <rubini@gnudd.com>
Acked-by: Giancarlo Asnaghi <giancarlo.asnaghi@st.com>
Cc: Russell King <linux@arm.linux.org.uk>
---
 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

[PATCH 4/6] pl011: use the new linux/sizes.h

[Alessandro Rubini]

Signed-off-by: Alessandro Rubini <rubini@gnudd.com>
Acked-by: Giancarlo Asnaghi <giancarlo.asnaghi@st.com>
Cc: Russell King <linux@arm.linux.org.uk>
---
 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

[PATCH 5/6] x86: add CONFIG_ARM_AMBA, selected by STA2X11

[Alessandro Rubini]

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 <rubini@gnudd.com>
Acked-by: Giancarlo Asnaghi <giancarlo.asnaghi@st.com>
---
 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

[PATCH 6/6] serial: add amba-pl011-pci

[Alessandro Rubini]

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 <rubini@gnudd.com>
Acked-by: Giancarlo Asnaghi <giancarlo.asnaghi@st.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 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

[PATCH 6/6] serial: add amba-pl011-pci

[Arnd Bergmann]

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 <rubini@gnudd.com>
> Acked-by: Giancarlo Asnaghi <giancarlo.asnaghi@st.com>

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

[PATCH 6/6] serial: add amba-pl011-pci

[Alessandro Rubini]

> 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

[PATCH 6/6] serial: add amba-pl011-pci

[Arnd Bergmann]

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

[PATCH 2/6] amba: use the new linux/sizes.h

[Russell King - ARM Linux]

On Fri, May 25, 2012 at 05:48:12PM +0200, Alessandro Rubini wrote:
> Signed-off-by: Alessandro Rubini <rubini@gnudd.com>
> Acked-by: Giancarlo Asnaghi <giancarlo.asnaghi@st.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> ---
>  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.

[PATCH 6/6] serial: add amba-pl011-pci

[Russell King - ARM Linux]

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

[PATCH 6/6] serial: add amba-pl011-pci

[Russell King - ARM Linux]

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.

[PATCH 6/6] serial: add amba-pl011-pci

[Alessandro Rubini]

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