subject:"\[U\-Boot\] \[PATCH 1\/2\] PCI\: Add driver for a 'pci\-host\-ecam\-generic' host controller"

Re: [U-Boot] [PATCH 1/2] PCI: Add driver for a 'pci-host-ecam-generic' host controller

2017-09-01 Thread Tuomas Tynkkynen


Hi,

On 08/31/2017 09:51 AM, Bin Meng wrote:

Hi Tuomas,

On Wed, Aug 30, 2017 at 4:31 PM, Tuomas Tynkkynen
 wrote:

QEMU emulates such a device with '-machine virt,highmem=off' on ARM.
The 'highmem=off' part is required for things to work as the PCI code
in U-Boot doesn't seem to support 64-bit BARs.

This driver is basically a copy-paste of the Xilinx PCIE driver with the
Xilinx-specific bits removed and compatible string changed... The
generic code should probably be extracted into some sort of library
functions instead of duplicating them before committing this driver.

Signed-off-by: Tuomas Tynkkynen 
---
  drivers/pci/Kconfig |   8 ++
  drivers/pci/Makefile|   1 +
  drivers/pci/pcie_ecam_generic.c | 193 
  3 files changed, 202 insertions(+)
  create mode 100644 drivers/pci/pcie_ecam_generic.c

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index e2a1c0a409..745161fb9f 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -33,6 +33,14 @@ config PCI_PNP
 help
   Enable PCI memory and I/O space resource allocation and assignment.

+config PCIE_ECAM_GENERIC
+   bool "Generic PCI-E ECAM support"
+   default n


nits: default n is not needed as it is the default value



Seems I have copied from PCIE_DW_MVEBU below. Removed.


+   depends on DM_PCI
+   help
+ Say Y here if you want to enable support for generic ECAM-based
+ PCIe controllers, such as the one emulated by QEMU.
+
  config PCIE_DW_MVEBU
 bool "Enable Armada-8K PCIe driver (DesignWare core)"
 default n
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index ad44e83996..5eb12efbf5 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_PCI) += pci.o pci_auto_old.o
  endif
  obj-$(CONFIG_PCI) += pci_auto_common.o pci_common.o

+obj-$(CONFIG_PCIE_ECAM_GENERIC) += pcie_ecam_generic.o
  obj-$(CONFIG_FSL_PCI_INIT) += fsl_pci_init.o
  obj-$(CONFIG_PCI_INDIRECT_BRIDGE) += pci_indirect.o
  obj-$(CONFIG_PCI_GT64120) += pci_gt64120.o
diff --git a/drivers/pci/pcie_ecam_generic.c b/drivers/pci/pcie_ecam_generic.c
new file mode 100644
index 00..039e378cb0
--- /dev/null
+++ b/drivers/pci/pcie_ecam_generic.c
@@ -0,0 +1,193 @@
+/*
+ * Generic PCIE host provided by e.g. QEMU
+ *
+ * Heavily based on drivers/pci/pcie_xilinx.c
+ *
+ * Copyright (C) 2016 Imagination Technologies
+ *
+ * SPDX-License-Identifier:GPL-2.0
+ */
+
+#include 
+#include 
+#include 
+
+#include 
+
+/**
+ * struct generic_ecam_pcie - generic_ecam PCIe controller state
+ * @hose: The parent classes PCI controller state
+ * @cfg_base: The base address of memory mapped configuration space
+ */
+struct generic_ecam_pcie {
+   struct pci_controller hose;


This sounds like a non-DM PCI driver stuff. I don't see it is
referenced in this driver.



Indeed, it appears to be leftover code also in the pcie_xilinx that I
copied from. Also a bunch of other drivers that have had a DM conversion
have this as leftovers. I will clean them up also.


+   void *cfg_base;
+};
+
+/**
+ * pcie_generic_ecam_config_address() - Calculate the address of a config 
access
+ * @pcie: Pointer to the PCI controller state
+ * @bdf: Identifies the PCIe device to access
+ * @offset: The offset into the device's configuration space
+ * @paddress: Pointer to the pointer to write the calculates address to
+ *
+ * Calculates the address that should be accessed to perform a PCIe
+ * configuration space access for a given device identified by the PCIe
+ * controller device @pcie and the bus, device & function numbers in @bdf. If
+ * access to the device is not valid then the function will return an error
+ * code. Otherwise the address to access will be written to the pointer pointed
+ * to by @paddress.
+ *
+ * Return: 0 on success, else -ENODEV


I see this driver always return 0.



Will fix the comment. I kept the same signature for config_address since 
I'm planning to have common parts of .write_config and .read_config 
abstracted (similar to pci_generic_config_{read,write} in Linux) instead 
of copy pasting the same code the 3rd time in U-Boot.



+ */
+static int pcie_generic_ecam_config_address(struct generic_ecam_pcie *pcie, 
pci_dev_t bdf,
+ uint offset, void **paddress)
+{
+   unsigned int bus = PCI_BUS(bdf);
+   unsigned int dev = PCI_DEV(bdf);
+   unsigned int func = PCI_FUNC(bdf);
+   void *addr;
+
+   addr = pcie->cfg_base;
+   addr += bus << 20;
+   addr += dev << 15;
+   addr += func << 12;
+   addr += offset;
+   *paddress = addr;
+
+   return 0;
+}
+
+/**
+ * pcie_generic_ecam_read_config() - Read from configuration space
+ * @pcie: Pointer to the PCI controller state


There is no pcie parameter, instead it's bus.



Again a problem inherited from pcie_xilinx... will fix there

Re: [U-Boot] [PATCH 1/2] PCI: Add driver for a 'pci-host-ecam-generic' host controller

2017-08-31 Thread Bin Meng

Hi Tuomas,

On Wed, Aug 30, 2017 at 4:31 PM, Tuomas Tynkkynen
 wrote:
> QEMU emulates such a device with '-machine virt,highmem=off' on ARM.
> The 'highmem=off' part is required for things to work as the PCI code
> in U-Boot doesn't seem to support 64-bit BARs.
>
> This driver is basically a copy-paste of the Xilinx PCIE driver with the
> Xilinx-specific bits removed and compatible string changed... The
> generic code should probably be extracted into some sort of library
> functions instead of duplicating them before committing this driver.
>
> Signed-off-by: Tuomas Tynkkynen 
> ---
>  drivers/pci/Kconfig |   8 ++
>  drivers/pci/Makefile|   1 +
>  drivers/pci/pcie_ecam_generic.c | 193 
> 
>  3 files changed, 202 insertions(+)
>  create mode 100644 drivers/pci/pcie_ecam_generic.c
>
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index e2a1c0a409..745161fb9f 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -33,6 +33,14 @@ config PCI_PNP
> help
>   Enable PCI memory and I/O space resource allocation and assignment.
>
> +config PCIE_ECAM_GENERIC
> +   bool "Generic PCI-E ECAM support"
> +   default n

nits: default n is not needed as it is the default value

> +   depends on DM_PCI
> +   help
> + Say Y here if you want to enable support for generic ECAM-based
> + PCIe controllers, such as the one emulated by QEMU.
> +
>  config PCIE_DW_MVEBU
> bool "Enable Armada-8K PCIe driver (DesignWare core)"
> default n
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index ad44e83996..5eb12efbf5 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_PCI) += pci.o pci_auto_old.o
>  endif
>  obj-$(CONFIG_PCI) += pci_auto_common.o pci_common.o
>
> +obj-$(CONFIG_PCIE_ECAM_GENERIC) += pcie_ecam_generic.o
>  obj-$(CONFIG_FSL_PCI_INIT) += fsl_pci_init.o
>  obj-$(CONFIG_PCI_INDIRECT_BRIDGE) += pci_indirect.o
>  obj-$(CONFIG_PCI_GT64120) += pci_gt64120.o
> diff --git a/drivers/pci/pcie_ecam_generic.c b/drivers/pci/pcie_ecam_generic.c
> new file mode 100644
> index 00..039e378cb0
> --- /dev/null
> +++ b/drivers/pci/pcie_ecam_generic.c
> @@ -0,0 +1,193 @@
> +/*
> + * Generic PCIE host provided by e.g. QEMU
> + *
> + * Heavily based on drivers/pci/pcie_xilinx.c
> + *
> + * Copyright (C) 2016 Imagination Technologies
> + *
> + * SPDX-License-Identifier:GPL-2.0
> + */
> +
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +/**
> + * struct generic_ecam_pcie - generic_ecam PCIe controller state
> + * @hose: The parent classes PCI controller state
> + * @cfg_base: The base address of memory mapped configuration space
> + */
> +struct generic_ecam_pcie {
> +   struct pci_controller hose;

This sounds like a non-DM PCI driver stuff. I don't see it is
referenced in this driver.

> +   void *cfg_base;
> +};
> +
> +/**
> + * pcie_generic_ecam_config_address() - Calculate the address of a config 
> access
> + * @pcie: Pointer to the PCI controller state
> + * @bdf: Identifies the PCIe device to access
> + * @offset: The offset into the device's configuration space
> + * @paddress: Pointer to the pointer to write the calculates address to
> + *
> + * Calculates the address that should be accessed to perform a PCIe
> + * configuration space access for a given device identified by the PCIe
> + * controller device @pcie and the bus, device & function numbers in @bdf. If
> + * access to the device is not valid then the function will return an error
> + * code. Otherwise the address to access will be written to the pointer 
> pointed
> + * to by @paddress.
> + *
> + * Return: 0 on success, else -ENODEV

I see this driver always return 0.

> + */
> +static int pcie_generic_ecam_config_address(struct generic_ecam_pcie *pcie, 
> pci_dev_t bdf,
> + uint offset, void **paddress)
> +{
> +   unsigned int bus = PCI_BUS(bdf);
> +   unsigned int dev = PCI_DEV(bdf);
> +   unsigned int func = PCI_FUNC(bdf);
> +   void *addr;
> +
> +   addr = pcie->cfg_base;
> +   addr += bus << 20;
> +   addr += dev << 15;
> +   addr += func << 12;
> +   addr += offset;
> +   *paddress = addr;
> +
> +   return 0;
> +}
> +
> +/**
> + * pcie_generic_ecam_read_config() - Read from configuration space
> + * @pcie: Pointer to the PCI controller state

There is no pcie parameter, instead it's bus.

> + * @bdf: Identifies the PCIe device to access
> + * @offset: The offset into the device's configuration space
> + * @valuep: A pointer at which to store the read value
> + * @size: Indicates the size of access to perform
> + *
> + * Read a value of size @size from offset @offset within the configuration
> + * space of the device identified by the bus, device & function numbers in 
> @bdf
> + * on the PCI bus @bus.
>

[U-Boot] [PATCH 1/2] PCI: Add driver for a 'pci-host-ecam-generic' host controller

2017-08-30 Thread Tuomas Tynkkynen

QEMU emulates such a device with '-machine virt,highmem=off' on ARM.
The 'highmem=off' part is required for things to work as the PCI code
in U-Boot doesn't seem to support 64-bit BARs.

This driver is basically a copy-paste of the Xilinx PCIE driver with the
Xilinx-specific bits removed and compatible string changed... The
generic code should probably be extracted into some sort of library
functions instead of duplicating them before committing this driver.

Signed-off-by: Tuomas Tynkkynen 
---
 drivers/pci/Kconfig |   8 ++
 drivers/pci/Makefile|   1 +
 drivers/pci/pcie_ecam_generic.c | 193 
 3 files changed, 202 insertions(+)
 create mode 100644 drivers/pci/pcie_ecam_generic.c

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index e2a1c0a409..745161fb9f 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -33,6 +33,14 @@ config PCI_PNP
help
  Enable PCI memory and I/O space resource allocation and assignment.
 
+config PCIE_ECAM_GENERIC
+   bool "Generic PCI-E ECAM support"
+   default n
+   depends on DM_PCI
+   help
+ Say Y here if you want to enable support for generic ECAM-based
+ PCIe controllers, such as the one emulated by QEMU.
+
 config PCIE_DW_MVEBU
bool "Enable Armada-8K PCIe driver (DesignWare core)"
default n
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index ad44e83996..5eb12efbf5 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_PCI) += pci.o pci_auto_old.o
 endif
 obj-$(CONFIG_PCI) += pci_auto_common.o pci_common.o
 
+obj-$(CONFIG_PCIE_ECAM_GENERIC) += pcie_ecam_generic.o
 obj-$(CONFIG_FSL_PCI_INIT) += fsl_pci_init.o
 obj-$(CONFIG_PCI_INDIRECT_BRIDGE) += pci_indirect.o
 obj-$(CONFIG_PCI_GT64120) += pci_gt64120.o
diff --git a/drivers/pci/pcie_ecam_generic.c b/drivers/pci/pcie_ecam_generic.c
new file mode 100644
index 00..039e378cb0
--- /dev/null
+++ b/drivers/pci/pcie_ecam_generic.c
@@ -0,0 +1,193 @@
+/*
+ * Generic PCIE host provided by e.g. QEMU
+ *
+ * Heavily based on drivers/pci/pcie_xilinx.c
+ *
+ * Copyright (C) 2016 Imagination Technologies
+ *
+ * SPDX-License-Identifier:GPL-2.0
+ */
+
+#include 
+#include 
+#include 
+
+#include 
+
+/**
+ * struct generic_ecam_pcie - generic_ecam PCIe controller state
+ * @hose: The parent classes PCI controller state
+ * @cfg_base: The base address of memory mapped configuration space
+ */
+struct generic_ecam_pcie {
+   struct pci_controller hose;
+   void *cfg_base;
+};
+
+/**
+ * pcie_generic_ecam_config_address() - Calculate the address of a config 
access
+ * @pcie: Pointer to the PCI controller state
+ * @bdf: Identifies the PCIe device to access
+ * @offset: The offset into the device's configuration space
+ * @paddress: Pointer to the pointer to write the calculates address to
+ *
+ * Calculates the address that should be accessed to perform a PCIe
+ * configuration space access for a given device identified by the PCIe
+ * controller device @pcie and the bus, device & function numbers in @bdf. If
+ * access to the device is not valid then the function will return an error
+ * code. Otherwise the address to access will be written to the pointer pointed
+ * to by @paddress.
+ *
+ * Return: 0 on success, else -ENODEV
+ */
+static int pcie_generic_ecam_config_address(struct generic_ecam_pcie *pcie, 
pci_dev_t bdf,
+ uint offset, void **paddress)
+{
+   unsigned int bus = PCI_BUS(bdf);
+   unsigned int dev = PCI_DEV(bdf);
+   unsigned int func = PCI_FUNC(bdf);
+   void *addr;
+
+   addr = pcie->cfg_base;
+   addr += bus << 20;
+   addr += dev << 15;
+   addr += func << 12;
+   addr += offset;
+   *paddress = addr;
+
+   return 0;
+}
+
+/**
+ * pcie_generic_ecam_read_config() - Read from configuration space
+ * @pcie: Pointer to the PCI controller state
+ * @bdf: Identifies the PCIe device to access
+ * @offset: The offset into the device's configuration space
+ * @valuep: A pointer at which to store the read value
+ * @size: Indicates the size of access to perform
+ *
+ * Read a value of size @size from offset @offset within the configuration
+ * space of the device identified by the bus, device & function numbers in @bdf
+ * on the PCI bus @bus.
+ *
+ * Return: 0 on success, else -ENODEV or -EINVAL
+ */
+static int pcie_generic_ecam_read_config(struct udevice *bus, pci_dev_t bdf,
+  uint offset, ulong *valuep,
+  enum pci_size_t size)
+{
+   struct generic_ecam_pcie *pcie = dev_get_priv(bus);
+   void *address;
+   int err;
+
+   err = pcie_generic_ecam_config_address(pcie, bdf, offset, );
+   if (err < 0) {
+   *valuep = pci_get_ff(size);
+   return 0;
+   }
+
+   switch (size) {
+   case PCI_SIZE_8:
+

Re: [U-Boot] [PATCH 1/2] PCI: Add driver for a 'pci-host-ecam-generic' host controller

Re: [U-Boot] [PATCH 1/2] PCI: Add driver for a 'pci-host-ecam-generic' host controller

[U-Boot] [PATCH 1/2] PCI: Add driver for a 'pci-host-ecam-generic' host controller

3 matches

Site Navigation

Mail list logo

Footer information