From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ryder Lee Date: Wed, 31 Jul 2019 23:23:08 +0800 Subject: [U-Boot] [PATCH 3/4] pci: mediatek: Add pci-driver for mt2701 In-Reply-To: <1564583732.16285.17.camel@mtkswgap22> References: <20190731115145.22095-1-frank-w@public-files.de> <20190731115145.22095-4-frank-w@public-files.de> <1564577142.11003.20.camel@mtkswgap22> <1564583732.16285.17.camel@mtkswgap22> Message-ID: <1564586588.18587.14.camel@mtkswgap22> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 пише: > > > > + GSS_MTK_Uboot_upstream > > > > On Wed, 2019-07-31 at 13:51 +0200, Frank Wunderlich wrote: > > > From: Oleksandr Rybalko > > > > > > this chip is used in MT7623 and some other Mediatek SoCs for > > pcie > > > > > > Tested-by: Frank Wunderlich > > > Signed-off-by: Frank Wunderlich > > > Signed-off-by: Oleksandr Rybalko > > > --- > > > 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 > > > + * Copyright (C) 2015 Ziv Huang > > > + * Copyright (C) 2019 Oleksandr Rybalko > > > + */ > > > + > > > +#include > > > +#include > > > + > > > +#include > > > +#include > > > + > > > +#include > > > +#include > > > +#include > > > +#include > > > + > > > +#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. > > > > > > > > > > > >