From: Ryder Lee <ryder.lee@mediatek.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 3/4] pci: mediatek: Add pci-driver for mt2701
Date: Wed, 31 Jul 2019 23:23:08 +0800 [thread overview]
Message-ID: <1564586588.18587.14.camel@mtkswgap22> (raw)
In-Reply-To: <1564583732.16285.17.camel@mtkswgap22>
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@mediatek.com> пише:
> >
> > + GSS_MTK_Uboot_upstream <GSS_MTK_Uboot_upstream@mediatek.com>
> >
> > On Wed, 2019-07-31 at 13:51 +0200, Frank Wunderlich wrote:
> > > From: Oleksandr Rybalko <ray@ddteam.net>
> > >
> > > this chip is used in MT7623 and some other Mediatek SoCs for
> > pcie
> > >
> > > Tested-by: Frank Wunderlich <frank-w@public-files.de>
> > > Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> > > Signed-off-by: Oleksandr Rybalko <ray@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@openwrt.org>
> > > + * Copyright (C) 2015 Ziv Huang <ziv.huang@mediatek.com>
> > > + * Copyright (C) 2019 Oleksandr Rybalko <ray@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.
>
> >
>
> >
> >
> >
>
>
next prev parent reply other threads:[~2019-07-31 15:23 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-31 11:51 [U-Boot] [PATCH 0/4] add pcie/ahci for mt7623/bpi-r2 Frank Wunderlich
2019-07-31 11:51 ` [U-Boot] [PATCH 1/4] ahci-pci: ASM1061 report wrong class, but support AHCI Frank Wunderlich
2019-07-31 11:51 ` [U-Boot] [PATCH 2/4] ata: ahci: Don't forget to clear upper address regs Frank Wunderlich
2019-07-31 11:51 ` [U-Boot] [PATCH 3/4] pci: mediatek: Add pci-driver for mt2701 Frank Wunderlich
2019-07-31 12:45 ` Ryder Lee
2019-07-31 14:13 ` Aleksandr Rybalko
2019-07-31 14:35 ` Ryder Lee
2019-07-31 15:23 ` Ryder Lee [this message]
2019-07-31 16:09 ` Frank Wunderlich
2019-08-01 7:32 ` Ryder Lee
2019-08-04 14:35 ` Frank Wunderlich
2019-07-31 11:51 ` [U-Boot] [PATCH 4/4] arm: dts: Add PCI-E controller for mt7623 Frank Wunderlich
2019-08-04 14:53 ` [U-Boot] [PATCH 0/4] add pcie/ahci for mt7623/bpi-r2 Frank Wunderlich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1564586588.18587.14.camel@mtkswgap22 \
--to=ryder.lee@mediatek.com \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.