[U-Boot] [PATCH 3/4] pci: mediatek: Add pci-driver for mt2701
Ryder Lee
ryder.lee at mediatek.com
Wed Jul 31 15:23:08 UTC 2019
On Wed, 2019-07-31 at 22:35 +0800, Ryder Lee wrote:
> On Wed, 2019-07-31 at 17:13 +0300, Aleksandr Rybalko wrote:
> > Hello Ryder.
> >
> >
> > ср, 31 лип. 2019 о 15:45 Ryder Lee <ryder.lee at mediatek.com> пише:
> >
> > + GSS_MTK_Uboot_upstream <GSS_MTK_Uboot_upstream at mediatek.com>
> >
> > On Wed, 2019-07-31 at 13:51 +0200, Frank Wunderlich wrote:
> > > From: Oleksandr Rybalko <ray at ddteam.net>
> > >
> > > this chip is used in MT7623 and some other Mediatek SoCs for
> > pcie
> > >
> > > Tested-by: Frank Wunderlich <frank-w at public-files.de>
> > > Signed-off-by: Frank Wunderlich <frank-w at public-files.de>
> > > Signed-off-by: Oleksandr Rybalko <ray at ddteam.net>
> > > ---
> > > drivers/pci/Kconfig | 6 +
> > > drivers/pci/Makefile | 1 +
> > > drivers/pci/pci-mt2701.c | 490
> > +++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 497 insertions(+)
> > > create mode 100644 drivers/pci/pci-mt2701.c
> >
> > Rename 'pci-mt2701.c' to 'pcie-mediatek.c' and then change the
> > subject.
> >
> >
> > So you promise us, that Mediatek will never produce PCI-E controller
> > which will be totally different from that one? :)
>
> imho we can use single driver for different IP generation and that is
> what we did in linux now.
>
> MT7623/MT2701 - mtk_pcie_soc_v1
> MT7622/MT7629 - v2
> gen3 IP - TBD
> >
> >
> > Obviously, this is an intermediate version of Linux patch, so
> > I suggest
> > to take a look at the latest version of vanilla Kernel:
> > https://github.com/torvalds/linux/blob/master/drivers/pci/controller/pcie-mediatek.c
> >
> > Please see my comments inline:
> >
> > > diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> > > index 3fe38f7315..cfe8ba5e52 100644
> > > --- a/drivers/pci/Kconfig
> > > +++ b/drivers/pci/Kconfig
> > > @@ -145,4 +145,10 @@ config PCI_MVEBU
> > > Say Y here if you want to enable PCIe controller
> > support on
> > > Armada XP/38x SoCs.
> > >
> > > +config PCIE_MT2701
> > > + bool "Mediatek 2701 PCI-E"
> > > + help
> > > + Say Y here if you want to enable PCIe controller
> > support on
> > > + Mediatek MT7623
> > > +
> > > endif
> > > diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> > > index b5ebd50c85..a4c4002b9c 100644
> > > --- a/drivers/pci/Makefile
> > > +++ b/drivers/pci/Makefile
> > > @@ -38,3 +38,4 @@ obj-$(CONFIG_PCIE_LAYERSCAPE_GEN4) +=
> > pcie_layerscape_gen4.o \
> > > pcie_layerscape_gen4_fixup.o
> > > obj-$(CONFIG_PCI_XILINX) += pcie_xilinx.o
> > > obj-$(CONFIG_PCIE_INTEL_FPGA) += pcie_intel_fpga.o
> > > +obj-$(CONFIG_PCIE_MT2701) += pci-mt2701.o
> > > diff --git a/drivers/pci/pci-mt2701.c
> > b/drivers/pci/pci-mt2701.c
> > > new file mode 100644
> > > index 0000000000..5904f15330
> > > --- /dev/null
> > > +++ b/drivers/pci/pci-mt2701.c
> > > @@ -0,0 +1,490 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Mediatek MT7623 SoC PCIE support
> > > + *
> > > + * Copyright (C) 2015 Mediatek
> > > + * Copyright (C) 2015 John Crispin <blogic at openwrt.org>
> > > + * Copyright (C) 2015 Ziv Huang <ziv.huang at mediatek.com>
> > > + * Copyright (C) 2019 Oleksandr Rybalko <ray at ddteam.net>
> > > + */
> > > +
> > > +#include <common.h>
> > > +#include <malloc.h>
> > > +
> > > +#include <pci.h>
> > > +#include <asm/io.h>
> > > +
> > > +#include <dm.h>
> > > +#include <fdtdec.h>
> > > +#include <linux/libfdt.h>
> > > +#include <dm/lists.h>
> > > +
> > > +#define iowrite32(v, a) writel(v, a)
> > > +#define iowrite16(v, a) writew(v, a)
> > > +#define iowrite8(v, a) writeb(v, a)
> > > +#define ioread32(a) readl(a)
> > > +#define ioread16(a) readw(a)
> > > +#define ioread8(a) readb(a)
> >
> > Remove these defines.
> >
> > > +#define RT_HIFSYS_BASE 0x1a000000
> > > +#define RT_PCIE_BASE 0x1a140000
> > > +#define RT_PCIE_IOWIN_BASE 0x1a160000
> > > +#define RT_PCIE_IOWIN_SIZE 0x00010000
> > > +#define RT_PCIE_MEMWIN_BASE 0x60000000
> > > +#define RT_PCIE_MEMWIN_SIZE 0x10000000
> >
> > Move these base to dts.
> >
> >
> > Already there (ranges), just not used yet.
>
> so it's better remove unused parts, right?
> >
> > > +#define RD(x) readl(RT_PCIE_BASE | (x))
> > > +#define WR(x, v) writel(v, RT_PCIE_BASE | (x))
> > > +
> > > +#define SYSCFG1 0x14
> > > +#define RSTCTL 0x34
> > > +#define RSTSTAT 0x38
> > > +#define PCICFG 0x00
> > > +#define PCIINT 0x08
> > > +#define PCIENA 0x0c
> > > +#define CFGADDR 0x20
> > > +#define CFGDATA 0x24
> > > +#define MEMBASE 0x28
> > > +#define IOBASE 0x2c
> > > +
> > > +#define BAR0SETUP 0x10
> > > +#define IMBASEBAR0 0x18
> > > +#define PCIE_CLASS 0x34
> > > +#define PCIE_SISTAT 0x50
> > > +
> > > +#define MTK_PCIE_HIGH_PERF BIT(14)
> > > +#define PCIEP0_BASE 0x2000
> > > +#define PCIEP1_BASE 0x3000
> > > +#define PCIEP2_BASE 0x4000
> > > +
> > > +#define PHY_P0_CTL 0x9000
> > > +#define PHY_P1_CTL 0xa000
> > > +#define PHY_P2_CTL 0x4000 /* in USB space */
> > > +
> > > +#define RSTCTL_PCIE0_RST BIT(24)
> > > +#define RSTCTL_PCIE1_RST BIT(25)
> > > +#define RSTCTL_PCIE2_RST BIT(26)
> > > +#define MAX_PORT_NUM 3
> > > +
> > > +struct resource {
> > > + char *name;
> > > + u32 start;
> > > + u32 end;
> > > +};
> > > +
> > > +struct mt_pcie {
> > > + char name[16];
> > > +};
> > > +
> > > +static struct mtk_pcie_port {
> > > + int id;
> > > + int enable;
> > > + u32 base;
> > > + u32 phy_base;
> > > + u32 perst_n;
> > > + u32 reset;
> > > + u32 interrupt_en;
> > > + int irq;
> > > + u32 link;
> > > +} mtk_pcie_port[] = {
> > > + { 0, 1, PCIEP0_BASE, PHY_P0_CTL, BIT(1),
> > RSTCTL_PCIE0_RST, BIT(20) },
> > > + { 1, 1, PCIEP1_BASE, PHY_P1_CTL, BIT(2),
> > RSTCTL_PCIE1_RST, BIT(21) },
> > > + { 2, 0, PCIEP2_BASE, PHY_P2_CTL, BIT(3),
> > RSTCTL_PCIE2_RST, BIT(22) },
> > > +};
> >
> > move some of mtk_pcie_port[] to dts.
> >
> >
> > Main point, at the end use orginal linux kernel DTS files. So it can
> > be passed into kernel, instead of hardcode it inside kernel.
> >
> > Just like we have ACPI blob on PC.
> >
> From what I see "[PATCH 4/4] arm: dts: Add PCI-E controller for mt7623"
> has enough information about "base/offset/port/phy" (that dts is same as
> linux one), but this driver does not use them.
> >
> > > +struct mtk_pcie {
> > > + struct device *dev;
> > > + void __iomem *sys_base; /* HIF SYSCTL
> > registers */
> > > + void __iomem *pcie_base; /* PCIE registers */
> > > + void __iomem *usb_base; /* USB registers */
> > > +
> > > + struct resource io;
> > > + struct resource pio;
> > > + struct resource mem;
> > > + struct resource prefetch;
> > > + struct resource busn;
> > > +
> > > + u32 io_bus_addr;
> > > + u32 mem_bus_addr;
> > > +
> > > + struct clk *clk;
> > > + int pcie_card_link;
> > > +};
> > > +
> > > +static const struct mtk_phy_init {
> > > + u32 reg;
> > > + u32 mask;
> > > + u32 val;
> > > +} mtk_phy_init[] = {
> > > + { 0xc00, 0x33000, 0x22000 },
> > > + { 0xb04, 0xe0000000, 0x40000000 },
> > > + { 0xb00, 0xe, 0x4 },
> > > + { 0xc3C, 0xffff0000, 0x3c0000 },
> > > + { 0xc48, 0xffff, 0x36 },
> > > + { 0xc0c, 0x30000000, 0x10000000 },
> > > + { 0xc08, 0x3800c0, 0xc0 },
> > > + { 0xc10, 0xf0000, 0x20000 },
> > > + { 0xc0c, 0xf000, 0x1000 },
> > > + { 0xc14, 0xf0000, 0xa0000 },
> > > + { 0xa28, 0x3fe00, 0x2000 },
> > > + { 0xa2c, 0x1ff, 0x10 },
> > > +};
> >
> > We should add another PHY driver (drivers/phy/) for these
> > settings.
> > Like what we did for Linux:
> > https://github.com/torvalds/linux/blob/master/drivers/phy/mediatek/phy-mtk-tphy.c
> >
> >
> >
> > Yes, but it more complex way for future releases.
> >
> phy driver is used for configuring basic settings, there is no complex
> logic in it. we can even copy and paste the entire block from that
> driver.
>
> .version = MTK_PHY_V1
> case PHY_TYPE_PCIE:
> pcie_phy_instance_init(tphy, instance);
>
>
> > > +/*
> > > + * Globals.
> > > + */
> > > +
> > > +struct mtk_pcie pcie;
> > > +struct mtk_pcie *pcie0;
> > > +
> > > +static int mtk_pcie_probe(void);
> > > +static int mt_pcie_read_config(struct udevice *, pci_dev_t,
> > uint, ulong *,
> > > + enum pci_size_t);
> > > +static int mt_pcie_write_config(struct udevice *,
> > pci_dev_t, uint, ulong,
> > > + enum pci_size_t);
> > > +
> > > +#define mtk_foreach_port(p) \
> > > + for ((p) = mtk_pcie_port; \
> > > + (p) !=
> > &mtk_pcie_port[ARRAY_SIZE(mtk_pcie_port)]; (p)++)
> >
> > Use dts for each port.
> >
> > > +#define BITS(m, n) (~(BIT(m) - 1) & ((BIT(n) - 1) |
> > BIT(n)))
> > > +
> > > +static void
> > > +mt7623_pcie_pinmux_set(void)
> > > +{
> > > + u32 regValue;
> > > +
> > > + /* Pin208: PCIE0_PERST_N (3) */
> > > + regValue = le32_to_cpu(ioread32(0x100059f0));
> > > + regValue &= ~(BITS(9, 11));
> > > + regValue |= 3 << 9;
> > > + iowrite32(regValue, 0x100059f0);
> > > +
> > > + /* Pin208: PCIE1_PERST_N (3) */
> > > + regValue = le32_to_cpu(ioread32(0x100059f0));
> > > + regValue &= ~(BITS(12, 14));
> > > + regValue |= 3 << 12;
> > > + iowrite32(regValue, 0x100059f0);
> > > +}
> >
> > Configure pinmux in dts.
> > Will be, when we reach point of full tree.
>
> now we have mt7623 pinctrl driver that should be enough.
>
> > > +static void
> > > +sys_w32(struct mtk_pcie *pcie, u32 val, unsigned int reg)
> > > +{
> > > + iowrite32(val, pcie->sys_base + reg);
> > > +}
> > > +
> > > +static u32
> > > +sys_r32(struct mtk_pcie *pcie, unsigned int reg)
> > > +{
> > > + return ioread32(pcie->sys_base + reg);
> > > +}
> > > +
> > > +static void
> > > +pcie_w32(struct mtk_pcie *pcie, u32 val, unsigned int reg)
> > > +{
> > > + iowrite32(val, pcie->pcie_base + reg);
> > > +}
> > > +
> > > +static void
> > > +pcie_w16(struct mtk_pcie *pcie, u16 val, unsigned int reg)
> > > +{
> > > + iowrite16(val, pcie->pcie_base + reg);
> > > +}
> > > +
> > > +static void
> > > +pcie_w8(struct mtk_pcie *pcie, u8 val, unsigned int reg)
> > > +{
> > > + iowrite8(val, pcie->pcie_base + reg);
> > > +}
> > > +
> > > +static u32
> > > +pcie_r32(struct mtk_pcie *pcie, unsigned int reg)
> > > +{
> > > + return ioread32(pcie->pcie_base + reg);
> > > +}
> > > +
> > > +static u32
> > > +pcie_r16(struct mtk_pcie *pcie, unsigned int reg)
> > > +{
> > > + return ioread16(pcie->pcie_base + reg);
> > > +}
> > > +
> > > +static u32
> > > +pcie_r8(struct mtk_pcie *pcie, unsigned int reg)
> > > +{
> > > + return ioread8(pcie->pcie_base + reg);
> > > +}
More specifically, we should use pci_generic_mmap_write/read_config()
for standard set of confi read/write operations so that we can get rid
of the helpers here.
I think the uboot driver end up dealing with nothing but just doing some
initial parts (< 300 lines I guess), or you can take pcie_xilinx or
pci_tegra as examples.
> > > +static void
> > > +pcie_m32(struct mtk_pcie *pcie, u32 mask, u32 val, unsigned
> > int reg)
> > > +{
> > > + u32 v;
> > > +
> > > + v = pcie_r32(pcie, reg);
> > > + v &= mask;
> > > + v |= val;
> > > + pcie_w32(pcie, v, reg);
> > > +}
> > > +
> > > +static void
> > > +mtk_pcie_configure_phy(struct mtk_pcie_port *port)
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(mtk_phy_init); i++) {
> > > + void __iomem *phy_addr = (void __iomem
> > *)port->phy_base +
> > > + mtk_phy_init[i].reg;
> > > + u32 val = ioread32(phy_addr);
> > > +
> > > + val &= ~mtk_phy_init[i].mask;
> > > + val |= mtk_phy_init[i].val;
> > > + iowrite32(val, phy_addr);
> > > + udelay(100);
> > > + }
> > > + mdelay(16);
> > > +}
> >
> > Use PHY driver.
> >
> > > +static void
> > > +mtk_pcie_configure_rc(struct mtk_pcie *pcie, struct
> > mtk_pcie_port *port)
> > > +{
> > > + ulong val = 0;
> > > +
> > > + mt_pcie_write_config(NULL, (port->id) << 3,
> > PCI_BASE_ADDRESS_0,
> > > + 0x80000000, PCI_SIZE_32);
> > > + mt_pcie_read_config(NULL, (port->id) << 3,
> > PCI_BASE_ADDRESS_0, &val,
> > > + PCI_SIZE_32);
> > > +
> > > + /* Configre RC Credit */
> > > + val = 0;
> > > + mt_pcie_read_config(NULL, (port->id) << 3, 0x73c,
> > &val, PCI_SIZE_32);
> > > + val &= ~(0x9fffUL) << 16;
> > > + val |= 0x806c << 16;
> > > + mt_pcie_write_config(NULL, (port->id) << 3, 0x73c,
> > val, PCI_SIZE_32);
> > > + mt_pcie_read_config(NULL, (port->id) << 3, 0x73c,
> > &val, PCI_SIZE_32);
> > > +
> > > + /* Configre RC FTS number */
> > > + mt_pcie_read_config(NULL, (port->id) << 3, 0x70c,
> > &val, PCI_SIZE_32);
> > > + val &= ~(0xff3) << 8;
> > > + val |= 0x50 << 8;
> > > + mt_pcie_write_config(NULL, (port->id) << 3, 0x70c,
> > val, PCI_SIZE_32);
> > > + mt_pcie_read_config(NULL, (port->id) << 3, 0x70c,
> > &val, PCI_SIZE_32);
> > > +}
> > > +
> > > +static void
> > > +mtk_pcie_preinit(struct mtk_pcie *pcie)
> > > +{
> > > + struct mtk_pcie_port *port;
> > > + u32 val = 0;
> > > + int i;
> > > +
> > > + mt7623_pcie_pinmux_set();
> > > +
> > > + /* PCIe RC Reset */
> > > + val = 0;
> > > + mtk_foreach_port(port)
> > > + if (port->enable)
> > > + val |= port->reset;
> > > + sys_w32(pcie, sys_r32(pcie, RSTCTL) | val, RSTCTL);
> > > + mdelay(12);
> > > + sys_w32(pcie, sys_r32(pcie, RSTCTL) & ~val, RSTCTL);
> > > + mdelay(12);
> > > +
> > > + i = 100000;
> > > + while (i--) {
> > > + if (sys_r32(pcie, RSTSTAT) == 0)
> > > + break;
> > > + udelay(10);
> > > + }
> >
> > We should avoid this while loop.
> >
> > > + /* Configure PCIe PHY */
> > > +
> > > + mtk_foreach_port(port)
> > > + if (port->enable)
> > > + mtk_pcie_configure_phy(port);
> > > +
> > > + /* PCIe EP reset */
> > > + val = 0;
> > > + mtk_foreach_port(port)
> > > + if (port->enable)
> > > + val |= port->perst_n;
> > > + pcie_w32(pcie, pcie_r32(pcie, PCICFG) | val, PCICFG);
> > > + mdelay(12);
> > > + pcie_w32(pcie, pcie_r32(pcie, PCICFG) & ~val, PCICFG);
> > > + mdelay(200);
> > > +
> > > + /* check the link status */
> > > + val = 0;
> > > + mtk_foreach_port(port) {
> > > + if (port->enable) {
> > > + if ((pcie_r32(pcie, port->base +
> > PCIE_SISTAT) & 0x1))
> > > + port->link = 1;
> > > + else
> > > + val |= port->reset;
> > > + }
> > > + }
> > > + sys_w32(pcie, sys_r32(pcie, RSTCTL) | val, RSTCTL);
> > > + mdelay(200);
> > > +
> > > + i = 100000;
> > > + while (i--) {
> > > + if (sys_r32(pcie, RSTSTAT) == 0)
> > > + break;
> > > + udelay(10);
> > > + }
> > > +
> > > + mtk_foreach_port(port) {
> > > + if (port->link)
> > > + pcie->pcie_card_link++;
> > > + }
> > > + printf("%s: PCIe Link count=%d\n", __func__,
> > pcie->pcie_card_link);
> > > + if (!pcie->pcie_card_link)
> > > + return;
> > > +
> > > + pcie_w32(pcie, pcie->mem_bus_addr, MEMBASE);
> > > + pcie_w32(pcie, pcie->io_bus_addr, IOBASE);
> > > +
> > > + mtk_foreach_port(port) {
> > > + if (port->link) {
> > > + pcie_m32(pcie, 0xffffffff,
> > port->interrupt_en, PCIENA);
> > > + pcie_w32(pcie, 0x7fff0001, port->base
> > + BAR0SETUP);
> > > + pcie_w32(pcie, 0x80000000, port->base
> > + IMBASEBAR0);
> > > + pcie_w32(pcie, 0x06040001, port->base
> > + PCIE_CLASS);
> > magic number.
> > Descriptive by reg names.
>
> we can directly replace these chunks with mtk_pcie_startup_port()
> (pcie-mediatek.c) and there are many defines for the registers/bit
> field.
>
> >
>
> >
> >
> >
>
>
More information about the U-Boot
mailing list