[U-Boot] [PATCH v1] pci: add DM based mpc85xx driver

Z.q. Hou zhiqiang.hou at nxp.com
Fri Aug 30 07:20:29 UTC 2019


Hi Heiko,

> -----Original Message-----
> From: Heiko Schocher <hs at denx.de>
> Sent: 2019年8月30日 12:17
> To: Z.q. Hou <zhiqiang.hou at nxp.com>
> Cc: u-boot at lists.denx.de; Alexander Graf <agraf at suse.de>; Andrew F. Davis
> <afd at ti.com>; Xiaowei Bao <xiaowei.bao at nxp.com>; Bin Meng
> <bmeng.cn at gmail.com>; Eugen Hristev <eugen.hristev at microchip.com>;
> Heinrich Schuchardt <xypron.glpk at gmx.de>; Horatiu Vultur
> <horatiu.vultur at microchip.com>; Krzysztof Kozlowski <krzk at kernel.org>;
> Liviu Dudau <Liviu.Dudau at foss.arm.com>; Marek Vasut
> <marek.vasut at gmail.com>; Mario Six <mario.six at gdsys.cc>; Michal Simek
> <michal.simek at xilinx.com>; Neil Armstrong <narmstrong at baylibre.com>;
> Prabhakar Kushwaha <prabhakar.kushwaha at nxp.com>; Ryder Lee
> <ryder.lee at mediatek.com>; Simon Glass <sjg at chromium.org>; Stefan
> Roese <sr at denx.de>
> Subject: Re: [PATCH v1] pci: add DM based mpc85xx driver
> 
> Hello Z.q. Hou,
> 
> Am 29.08.2019 um 10:11 schrieb Z.q. Hou:
> > Hi Heiko,
> >
> >> -----Original Message-----
> >> From: Heiko Schocher <hs at denx.de>
> >> Sent: 2019年7月16日 11:36
> >> To: u-boot at lists.denx.de
> >> Cc: Heiko Schocher <hs at denx.de>; Alexander Graf <agraf at suse.de>;
> >> Andrew F. Davis <afd at ti.com>; Xiaowei Bao <xiaowei.bao at nxp.com>;
> Bin
> >> Meng <bmeng.cn at gmail.com>; Eugen Hristev
> >> <eugen.hristev at microchip.com>; Heinrich Schuchardt
> >> <xypron.glpk at gmx.de>; Horatiu Vultur <horatiu.vultur at microchip.com>;
> >> Z.q. Hou <zhiqiang.hou at nxp.com>; Krzysztof Kozlowski
> >> <krzk at kernel.org>; Liviu Dudau <Liviu.Dudau at foss.arm.com>; Marek
> >> Vasut <marek.vasut at gmail.com>; Mario Six <mario.six at gdsys.cc>;
> Michal
> >> Simek <michal.simek at xilinx.com>; Neil Armstrong
> >> <narmstrong at baylibre.com>; Prabhakar Kushwaha
> >> <prabhakar.kushwaha at nxp.com>; Ryder Lee
> <ryder.lee at mediatek.com>;
> >> Simon Glass <sjg at chromium.org>; Stefan Roese <sr at denx.de>
> >> Subject: [PATCH v1] pci: add DM based mpc85xx driver
> >>
> >> add DM based PCI Configuration space access support for MPC85xx PCI
> >> Bridge
> >
> > Seems you're converting the arch/powerpc/cpu/mpc85xx/pci.c to driver
> > model,
> 
> Yes, should have added this to commit message .. fixed.
> 
> > but not every mpc85xx platform uses this driver, platforms mpc8536,
> > 8544,
> > 8548 and 8568 use the drivers/pci/fsl_pci_init.c, please take them into
> account.
> 
> Oh, wasn;t aware of this! ... I have no idea why there are 2 drivers and
> where are the differences between them, nor I can test all the other
> plattforms, so this seems difficult to me.
> 
> Do you know, why there are 2 different drivers?

I guess the mpc85xx/pci.c was added for the older platforms, which only
integrated PCI controllers, it can't support the newer platforms, which
integrated both PCI and PCIe controllers. So the fsl_pci_init.c was added.

> 
> I made this new driver, because I work on DM support for the socrates board,
> which is mpc8544 based ... !?
> 
> May I rename this driver from pci_mpc85xx.c to pci_mpc85xx_dm.c and it
> can be a base for DM support and others, who can test their changes, can
> add the other variants?

It's unnecessary to change the driver file name, I can help to verify on mpc8548,
Let others add other mpc85xx PCI DM driver base on this.

> 
> > Note: The platforms using arch/powerpc/cpu/mpc85xx/pci.c set up PCI
> > LAWs in file arch/powerpc/cpu/mpc8xxx/law.c, while other platforms set
> > up PCI LAWs in PCI driver. So I think you can move the PCI LAWs set-up
> > into this DM PCI driver And remove the PCI entries from law.c.
> 
> Yes good idea, add this in v2 for this driver.
> 
> >> Signed-off-by: Heiko Schocher <hs at denx.de>
> >>
> >> ---
> >> Travis build, see:
> >> https://travis
> >>
> -ci.org%2Fhsdenx%2Fu-boot-test%2Fbuilds%2F558855544&data=02%7
> >>
> C01%7CZhiqiang.Hou%40nxp.com%7C231ac8975b2242ea5bfb08d7099eccc
> >>
> 8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63698844995746
> >>
> 0734&sdata=lQcXtvh5a0G9r%2BFlXnnGQ5VgDYvcR%2FEGWpShJ324su
> >> U%3D&reserved=0
> >>
> >>   MAINTAINERS               |   5 ++
> >>   drivers/pci/Kconfig       |   7 ++
> >>   drivers/pci/Makefile      |   1 +
> >>   drivers/pci/pci_mpc85xx.c | 132
> >> ++++++++++++++++++++++++++++++++++++++
> >>   4 files changed, 145 insertions(+)
> >>   create mode 100644 drivers/pci/pci_mpc85xx.c
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS index
> e91684191f..01faa67a6d
> >> 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -639,6 +639,11 @@ S:	Maintained
> >>   F:	drivers/pci_endpoint/
> >>   F:  include/pci_ep.h
> >>
> >> +PCI MPC85xx
> >> +M:	Heiko Schocher <hs at denx.de>
> >> +S:	Maintained
> >> +F:	drivers/pci/pci_mpc85xx.c
> >> +
> >>   POWER
> >>   M:	Jaehoon Chung <jh80.chung at samsung.com>
> >>   S:	Maintained
> >> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig index
> >> 3fe38f7315..88db0067b4 100644
> >> --- a/drivers/pci/Kconfig
> >> +++ b/drivers/pci/Kconfig
> >> @@ -68,6 +68,13 @@ config PCIE_FSL
> >>   	  PowerPC MPC85xx, MPC86xx, B series, P series and T series
> SoCs.
> >>   	  This driver does not support SRIO_PCIE_BOOT feature.
> >>
> >> +config PCI_MPC85XX
> >> +	bool "MPC85XX PowerPC PCI support"
> >> +	depends on DM_PCI
> >> +	help
> >> +	  Say Y here if you want to enable PCI controller support on FSL
> >> +	  PowerPC MPC85xx SoC.
> >> +
> >>   config PCI_RCAR_GEN2
> >>   	bool "Renesas RCar Gen2 PCIe driver"
> >>   	depends on DM_PCI
> >> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile index
> >> b5ebd50c85..929f119fb3 100644
> >> --- a/drivers/pci/Makefile
> >> +++ b/drivers/pci/Makefile
> >> @@ -19,6 +19,7 @@ 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
> >> +obj-$(CONFIG_PCI_MPC85XX) += pci_mpc85xx.o
> >>   obj-$(CONFIG_PCI_MSC01) += pci_msc01.o
> >>   obj-$(CONFIG_PCIE_IMX) += pcie_imx.o
> >>   obj-$(CONFIG_FTPCI100) += pci_ftpci100.o diff --git
> >> a/drivers/pci/pci_mpc85xx.c b/drivers/pci/pci_mpc85xx.c new file mode
> >> 100644 index 0000000000..c0d35f7f2c
> >> --- /dev/null
> >> +++ b/drivers/pci/pci_mpc85xx.c
> >> @@ -0,0 +1,132 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * (C) Copyright 2019
> >> + * Heiko Schocher, DENX Software Engineering, hs at denx.de.
> >> + *
> >> + */
> >> +#include <common.h>
> >> +#include <asm/cpm_85xx.h>
> >> +#include <pci.h>
> >> +#include <dm.h>
> >> +
> >> +struct mpc85xx_pci_priv {
> >> +	void __iomem		*cfg_addr;
> >> +	void __iomem		*cfg_data;
> >> +};
> >> +
> >> +static int mpc85xx_pci_dm_read_config(struct udevice *dev, pci_dev_t
> bdf,
> >> +				      uint offset, ulong *value,
> >> +				      enum pci_size_t size)
> >> +{
> >> +	struct mpc85xx_pci_priv *priv = dev_get_priv(dev);
> >> +	u32 addr;
> >> +
> >> +	addr = bdf | (offset & 0xfc) | ((offset & 0xf00) << 16) | 0x80000000;
> >> +	out_be32(priv->cfg_addr, addr);
> >> +	sync();
> >> +	*value = pci_conv_32_to_size(in_le32(priv->cfg_data), offset,
> >> +size);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int mpc85xx_pci_dm_write_config(struct udevice *dev,
> >> +pci_dev_t
> >> bdf,
> >> +				       uint offset, ulong value,
> >> +				       enum pci_size_t size)
> >> +{
> >> +	struct mpc85xx_pci_priv *priv = dev_get_priv(dev);
> >> +	u32 addr;
> >> +
> >> +	addr = bdf | (offset & 0xfc) | ((offset & 0xf00) << 16) | 0x80000000;
> >> +	out_be32(priv->cfg_addr, addr);
> >> +	sync();
> >> +	out_le32(priv->cfg_data, pci_conv_size_to_32(0, value, offset,
> >> +size));
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int mpc85xx_pci_dm_probe(struct udevice *dev) {
> >> +	struct mpc85xx_pci_priv *priv = dev_get_priv(dev);
> >> +	struct pci_region *io;
> >> +	struct pci_region *mem;
> >> +	struct pci_region *pre;
> >> +	int ret;
> >> +	ccsr_pcix_t *pcix;
> >> +
> >> +	ret = pci_get_regions(dev, &io, &mem, &pre);
> >> +	if (ret != 2) {
> >> +		printf("%s: wrong count of regions %d only 2 allowed\n",
> >> +		       __func__, ret);
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	pcix = priv->cfg_addr;
> >> +	/* BAR 1: memory */
> >> +	out_be32(&pcix->potar1, (mem->bus_start >> 12) & 0x000fffff);
> >> +	out_be32(&pcix->potear1, 0);
> >> +	out_be32(&pcix->powbar1, (mem->phys_start >> 12) & 0x000fffff);
> >> +	out_be32(&pcix->powbear1, 0);
> >> +	out_be32(&pcix->powar1, (POWAR_EN | POWAR_MEM_READ |
> >> +		 POWAR_MEM_WRITE | (__ilog2(mem->size) - 1)));
> >> +
> >> +	/* BAR 1: IO */
> >> +	out_be32(&pcix->potar2, (io->bus_start >> 12) & 0x000fffff);
> >> +	out_be32(&pcix->potear2, 0);
> >> +	out_be32(&pcix->powbar2, (io->phys_start >> 12) & 0x000fffff);
> >> +	out_be32(&pcix->powbear2, 0);
> >> +	out_be32(&pcix->powar2, (POWAR_EN | POWAR_IO_READ |
> >> +		 POWAR_IO_WRITE | (__ilog2(io->size) - 1)));
> >> +
> >> +	out_be32(&pcix->pitar1, 0);
> >> +	out_be32(&pcix->piwbar1, 0);
> >> +	out_be32(&pcix->piwar1, (PIWAR_EN | PIWAR_PF | PIWAR_LOCAL |
> >> +		 PIWAR_READ_SNOOP | PIWAR_WRITE_SNOOP |
> >> PIWAR_MEM_2G));
> >> +
> >> +	out_be32(&pcix->powar3, 0);
> >> +	out_be32(&pcix->powar4, 0);
> >> +	out_be32(&pcix->piwar2, 0);
> >> +	out_be32(&pcix->piwar3, 0);
> >> +
> >
> > What about the HW issue below?
> > #if defined(CONFIG_TARGET_MPC8555CDS) ||
> defined(CONFIG_TARGET_MPC8541CDS)
> >          /*
> >           * This is a SW workaround for an apparent HW problem
> >           * in the PCI controller on the MPC85555/41 CDS boards.
> >           * The first config cycle must be to a valid, known
> >           * device on the PCI bus in order to trick the PCI
> >           * controller state machine into a known valid state.
> >           * Without this, the first config cycle has the chance
> >           * of hanging the controller permanently, just leaving
> >           * it in a semi-working state, or leaving it working.
> >           *
> >           * Pick on the Tundra, Device 17, to get it right.
> >           */
> >          {
> >                  u8 header_type;
> >
> >                  pci_hose_read_config_byte(hose,
> >
> PCI_BDF(0,BRIDGE_ID,0),
> >                                            PCI_HEADER_TYPE,
> >                                            &header_type);
> >          }
> > #endif
> 
> I have no such hw ... if the board maintainers switch to DM support, they can
> take a look at it ... I thought this is better than adding code I could not test ...
> I add a note in commit message.

OK

Thanks,
Zhiqiang

> 
> Thanks for your review!
> 
> bye,
> Heiko
> >
> > Thanks,
> > Zhiqiang
> >
> >> +	return 0;
> >> +}
> >> +
> >> +static int mpc85xx_pci_dm_remove(struct udevice *dev) {
> >> +	return 0;
> >> +}
> >> +
> >> +static int mpc85xx_pci_ofdata_to_platdata(struct udevice *dev) {
> >> +	struct mpc85xx_pci_priv *priv = dev_get_priv(dev);
> >> +	fdt_addr_t addr;
> >> +
> >> +	addr = devfdt_get_addr_index(dev, 0);
> >> +	if (addr == FDT_ADDR_T_NONE)
> >> +		return -EINVAL;
> >> +	priv->cfg_addr = (void __iomem *)addr;
> >> +	addr += 4;
> >> +	priv->cfg_data = (void __iomem *)addr;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static const struct dm_pci_ops mpc85xx_pci_ops = {
> >> +	.read_config	= mpc85xx_pci_dm_read_config,
> >> +	.write_config	= mpc85xx_pci_dm_write_config,
> >> +};
> >> +
> >> +static const struct udevice_id mpc85xx_pci_ids[] = {
> >> +	{ .compatible = "fsl,mpc8540-pci" },
> >> +	{ }
> >> +};
> >> +
> >> +U_BOOT_DRIVER(mpc85xx_pci) = {
> >> +	.name			= "mpc85xx_pci",
> >> +	.id			= UCLASS_PCI,
> >> +	.of_match		= mpc85xx_pci_ids,
> >> +	.ops			= &mpc85xx_pci_ops,
> >> +	.probe			= mpc85xx_pci_dm_probe,
> >> +	.remove			= mpc85xx_pci_dm_remove,
> >> +	.ofdata_to_platdata	= mpc85xx_pci_ofdata_to_platdata,
> >> +	.priv_auto_alloc_size	= sizeof(struct mpc85xx_pci_priv),
> >> +};
> >> --
> >> 2.21.0
> >
> 
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang
> Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email:
> hs at denx.de


More information about the U-Boot mailing list