| Message ID | 20181024152056.17813-1-marek.behun@nic.cz |
|---|---|
| State | Not Applicable |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | [RFC,v4.14] PCI: aadrvark: warm reset the cores and card | expand |
Hi Marek, On Wed, Oct 24, 2018 at 05:20:56PM +0200, Marek Behún wrote: > Add code to do a warm reset on the PHY and PCIE cores and if PERSTN GPIO > is specified in device tree (as reset-gpio), also reset the card. > > The reset-gpio is inspired by what is done in U-Boot and linux-marvell, > and is not final version: I am hoping this can be done via a PCIe register > rather than GPIO - bit 3 of CTRL_WARM_RESET_REG register (which is added > by this patch) is called PERSTN_GPIO_EN (Enable PERSTN from GPIO) and > I think this is the right register, but manipulating this register did > not have any effect on the PERSTN pin, even when pinctrl was correctly set. > > I asked Marvell about this and am awaiting their reply. > > The reset-gpio is needed for Compex 5 GHz wifi card model WLE900VX. Without > this patch the PCIe link never comes up in kernel (although U-Boot pci > command was able to enumerate the card). > > What is weird is that the link does not come up for this card when > pci-aardvark driver is probed in U-Boot. I haven't yet had time to discover > the problem there. My temporary solution is to compile out the pci-aardvark > driver from U-Boot. > > This patch is based on 4.14 kernel. Thanks for your research and the patch! As I'm sure you know, this would have to be applied to the latest kernel, e.g., v4.20-rc1 (when that comes out in about 10 days), so the best place to test it would be v4.19 + this patch, or v4.20-rc1 + this patch. After it's applied to the master branch, it's possible it could be backported to stable kernels if appropriate. > If you have time, please try it with some PCIe cards and let me know if they > work correctly. > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > Cc: Bjorn Helgaas <helgaas@kernel.org> > Cc: linux-pci@vger.kernel.org > Cc: Antoine Ténart <antoine.tenart@free-electrons.com> > Cc: Grégory Clement <gregory.clement@free-electrons.com> > Cc: Miquèl Raynal <miquel.raynal@free-electrons.com> > Cc: Victor Gu <xigu@marvell.com> Nit: some of the CC lines above have extra spaces.
On Wed, Oct 24, 2018 at 05:20:56PM +0200, Marek Behún wrote: > Add code to do a warm reset on the PHY and PCIE cores and if PERSTN GPIO > is specified in device tree (as reset-gpio), also reset the card. > > The reset-gpio is inspired by what is done in U-Boot and linux-marvell, > and is not final version: I am hoping this can be done via a PCIe register > rather than GPIO - bit 3 of CTRL_WARM_RESET_REG register (which is added > by this patch) is called PERSTN_GPIO_EN (Enable PERSTN from GPIO) and > I think this is the right register, but manipulating this register did > not have any effect on the PERSTN pin, even when pinctrl was correctly set. > > I asked Marvell about this and am awaiting their reply. > > The reset-gpio is needed for Compex 5 GHz wifi card model WLE900VX. Without > this patch the PCIe link never comes up in kernel (although U-Boot pci > command was able to enumerate the card). > > What is weird is that the link does not come up for this card when > pci-aardvark driver is probed in U-Boot. I haven't yet had time to discover > the problem there. My temporary solution is to compile out the pci-aardvark > driver from U-Boot. > > This patch is based on 4.14 kernel. This is not a commit log, these comments go either in a cover letter or below the log, prior to the diff. Patches should always be aimed at mainline, that's what is discussed and merged on linux-pci@vger.kernel.org Given its RFC status I consider this patch a proof of concept and won't consider it for upstreaming, not yet at least. > If you have time, please try it with some PCIe cards and let me know > if they work correctly. See above. For the time being I will drop this patch from the linux-pci patch queue, I really do not know what to do with it. Thanks, Lorenzo > Signed-off-by: Marek Behún <marek.behun@nic.cz> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > Cc: Bjorn Helgaas <helgaas@kernel.org> > Cc: linux-pci@vger.kernel.org > Cc: Antoine Ténart <antoine.tenart@free-electrons.com> > Cc: Grégory Clement <gregory.clement@free-electrons.com> > Cc: Miquèl Raynal <miquel.raynal@free-electrons.com> > Cc: Victor Gu <xigu@marvell.com> > --- > drivers/pci/host/pci-aardvark.c | 46 +++++++++++++++++++++++++++++++-- > 1 file changed, 44 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c > index 50e8addc22f9..5610fcb3f426 100644 > --- a/drivers/pci/host/pci-aardvark.c > +++ b/drivers/pci/host/pci-aardvark.c > @@ -21,6 +21,7 @@ > #include <linux/platform_device.h> > #include <linux/of_address.h> > #include <linux/of_pci.h> > +#include <linux/of_gpio.h> > > /* PCIe core registers */ > #define PCIE_CORE_CMD_STATUS_REG 0x4 > @@ -147,6 +148,9 @@ > #define CTRL_MODE_MASK 0x1 > #define PCIE_CORE_MODE_DIRECT 0x0 > #define PCIE_CORE_MODE_COMMAND 0x1 > +#define CTRL_WARM_RESET_REG (CTRL_CORE_BASE_ADDR + 0x4) > +#define CTRL_PCIE_CORE_WARM_RESET BIT(0) > +#define CTRL_PHY_CORE_WARM_RESET BIT(1) > > /* PCIe Central Interrupts Registers */ > #define CENTRAL_INT_BASE_ADDR 0x1b000 > @@ -270,8 +274,25 @@ static void advk_pcie_set_ob_win(struct advk_pcie *pcie, > advk_writel(pcie, match_ls | BIT(0), OB_WIN_MATCH_LS(win_num)); > } > > +static void advk_pcie_warm_reset(struct advk_pcie *pcie) > +{ > + u32 reg; > + > + reg = advk_readl(pcie, CTRL_WARM_RESET_REG); > + reg |= CTRL_PCIE_CORE_WARM_RESET | CTRL_PHY_CORE_WARM_RESET; > + advk_writel(pcie, reg, CTRL_WARM_RESET_REG); > + > + mdelay(100); > + > + reg = advk_readl(pcie, CTRL_WARM_RESET_REG); > + reg &= ~(CTRL_PCIE_CORE_WARM_RESET | CTRL_PHY_CORE_WARM_RESET); > + advk_writel(pcie, reg, CTRL_WARM_RESET_REG); > +} > + > static void advk_pcie_setup_hw(struct advk_pcie *pcie) > { > + struct device *dev = &pcie->pdev->dev; > + struct device_node *node = dev->of_node; > u32 reg; > int i; > > @@ -311,10 +332,15 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie) > PCIE_CORE_CTRL2_TD_ENABLE; > advk_writel(pcie, reg, PCIE_CORE_CTRL2_REG); > > - /* Set GEN2 */ > + /* Set GEN */ > reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG); > reg &= ~PCIE_GEN_SEL_MSK; > - reg |= SPEED_GEN_2; > + if (of_pci_get_max_link_speed(node) == 1) > + reg |= SPEED_GEN_1; > + if (of_pci_get_max_link_speed(node) == 3) > + reg |= SPEED_GEN_3; > + else > + reg |= SPEED_GEN_2; > advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG); > > /* Set lane X1 */ > @@ -948,6 +974,8 @@ static int advk_pcie_probe(struct platform_device *pdev) > struct pci_bus *bus, *child; > struct pci_host_bridge *bridge; > int ret, irq; > + int reset_gpio; > + enum of_gpio_flags flags; > > bridge = devm_pci_alloc_host_bridge(dev, sizeof(struct advk_pcie)); > if (!bridge) > @@ -970,6 +998,20 @@ static int advk_pcie_probe(struct platform_device *pdev) > return ret; > } > > + /* Config reset gpio for pcie if there is valid gpio setting in DTS */ > + reset_gpio = of_get_named_gpio_flags(pdev->dev.of_node, "reset-gpio", > + 0, &flags); > + if (gpio_is_valid(reset_gpio)) { > + struct gpio_desc *reset_gpiod; > + reset_gpiod = gpio_to_desc(reset_gpio); > + gpiod_direction_output(reset_gpiod, 0); > + mdelay(200); > + gpiod_direction_output(reset_gpiod, 1); > + mdelay(200); > + } > + > + advk_pcie_warm_reset(pcie); > + > ret = advk_pcie_parse_request_of_pci_ranges(pcie); > if (ret) { > dev_err(dev, "Failed to parse resources\n"); > -- > 2.18.1 >
Hi Lorenzo, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote on Mon, 19 Nov 2018 17:09:48 +0000: > On Wed, Oct 24, 2018 at 05:20:56PM +0200, Marek Behún wrote: > > Add code to do a warm reset on the PHY and PCIE cores and if PERSTN GPIO > > is specified in device tree (as reset-gpio), also reset the card. > > > > The reset-gpio is inspired by what is done in U-Boot and linux-marvell, > > and is not final version: I am hoping this can be done via a PCIe register > > rather than GPIO - bit 3 of CTRL_WARM_RESET_REG register (which is added > > by this patch) is called PERSTN_GPIO_EN (Enable PERSTN from GPIO) and > > I think this is the right register, but manipulating this register did > > not have any effect on the PERSTN pin, even when pinctrl was correctly set. > > > > I asked Marvell about this and am awaiting their reply. > > > > The reset-gpio is needed for Compex 5 GHz wifi card model WLE900VX. Without > > this patch the PCIe link never comes up in kernel (although U-Boot pci > > command was able to enumerate the card). > > > > What is weird is that the link does not come up for this card when > > pci-aardvark driver is probed in U-Boot. I haven't yet had time to discover > > the problem there. My temporary solution is to compile out the pci-aardvark > > driver from U-Boot. > > > > This patch is based on 4.14 kernel. > > This is not a commit log, these comments go either in a cover letter > or below the log, prior to the diff. > > Patches should always be aimed at mainline, that's what is discussed > and merged on linux-pci@vger.kernel.org > > Given its RFC status I consider this patch a proof of concept and > won't consider it for upstreaming, not yet at least. > > > If you have time, please try it with some PCIe cards and let me know > > if they work correctly. > > See above. For the time being I will drop this patch from the linux-pci > patch queue, I really do not know what to do with it. > > Thanks, > Lorenzo Just an FYI, I am currently working on S2RAM support on A3700, and doing this I added a few things to this driver including reset GPIO support. Patches will come soon. There is no warm-reset coming in though. Thanks, Miquèl
Hi Marek, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote on Mon, 19 Nov 2018 17:09:48 +0000: > On Wed, Oct 24, 2018 at 05:20:56PM +0200, Marek Behún wrote: > > Add code to do a warm reset on the PHY and PCIE cores and if PERSTN GPIO > > is specified in device tree (as reset-gpio), also reset the card. > > > > The reset-gpio is inspired by what is done in U-Boot and linux-marvell, > > and is not final version: I am hoping this can be done via a PCIe register > > rather than GPIO - bit 3 of CTRL_WARM_RESET_REG register (which is added > > by this patch) is called PERSTN_GPIO_EN (Enable PERSTN from GPIO) and > > I think this is the right register, but manipulating this register did > > not have any effect on the PERSTN pin, even when pinctrl was correctly set. Are you sure pinctrl was correctly set? Because in the current state of the pinctrl driver there is a mismatch in the registers layout. Gregory Clement will send a patch soon about it but basically, when you use the pin {func=pcie1,group=pcie}, the bit that is to be toggled is 5, not 4 (then the ptp bit is wrong, but this is a hot fix just for hour situation): --- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c @@ -195,7 +195,7 @@ static struct armada_37xx_pin_group armada_37xx_sb_groups[] = { PIN_GRP_GPIO("usb2_drvvbus1", 1, 1, BIT(1), "drvbus"), PIN_GRP_GPIO("sdio_sb", 24, 6, BIT(2), "sdio"), PIN_GRP_GPIO("rgmii", 6, 12, BIT(3), "mii"), - PIN_GRP_GPIO("pcie1", 3, 2, BIT(4), "pcie"), + PIN_GRP_GPIO("pcie1", 3, 2, BIT(5), "pcie"), PIN_GRP_GPIO("ptp", 20, 3, BIT(5), "ptp"), PIN_GRP("ptp_clk", 21, 1, BIT(6), "ptp", "mii"), PIN_GRP("ptp_trig", 22, 1, BIT(7), "ptp", "mii"), > > > > I asked Marvell about this and am awaiting their reply. > > > > The reset-gpio is needed for Compex 5 GHz wifi card model WLE900VX. Without > > this patch the PCIe link never comes up in kernel (although U-Boot pci > > command was able to enumerate the card). > > > > What is weird is that the link does not come up for this card when > > pci-aardvark driver is probed in U-Boot. I haven't yet had time to discover > > the problem there. My temporary solution is to compile out the pci-aardvark > > driver from U-Boot. > > [...] > > If you have time, please try it with some PCIe cards and let me know > > if they work correctly. While working on S2RAM, I checked how the reset pin works. I am on EspressoBin so no reset GPIO available but the "internal" pin that is controlled by the PCIe IP when "PERSTN_GPIO_EN" is set. After hours of testing, I could not come with a working setup, it seems like booting with any bit set in CTRL_WARM_RESET_REG leads to unstable behavior. AFAIK Marvell BSP does not provide a working example wrt this register. Could you actually use it on your platform? I decided to drop the reset patches from my series for now as it is not actually needed for S2RAM feature, but it would be great to understand how this works. Thanks, Miquèl
diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c index 50e8addc22f9..5610fcb3f426 100644 --- a/drivers/pci/host/pci-aardvark.c +++ b/drivers/pci/host/pci-aardvark.c @@ -21,6 +21,7 @@ #include <linux/platform_device.h> #include <linux/of_address.h> #include <linux/of_pci.h> +#include <linux/of_gpio.h> /* PCIe core registers */ #define PCIE_CORE_CMD_STATUS_REG 0x4 @@ -147,6 +148,9 @@ #define CTRL_MODE_MASK 0x1 #define PCIE_CORE_MODE_DIRECT 0x0 #define PCIE_CORE_MODE_COMMAND 0x1 +#define CTRL_WARM_RESET_REG (CTRL_CORE_BASE_ADDR + 0x4) +#define CTRL_PCIE_CORE_WARM_RESET BIT(0) +#define CTRL_PHY_CORE_WARM_RESET BIT(1) /* PCIe Central Interrupts Registers */ #define CENTRAL_INT_BASE_ADDR 0x1b000 @@ -270,8 +274,25 @@ static void advk_pcie_set_ob_win(struct advk_pcie *pcie, advk_writel(pcie, match_ls | BIT(0), OB_WIN_MATCH_LS(win_num)); } +static void advk_pcie_warm_reset(struct advk_pcie *pcie) +{ + u32 reg; + + reg = advk_readl(pcie, CTRL_WARM_RESET_REG); + reg |= CTRL_PCIE_CORE_WARM_RESET | CTRL_PHY_CORE_WARM_RESET; + advk_writel(pcie, reg, CTRL_WARM_RESET_REG); + + mdelay(100); + + reg = advk_readl(pcie, CTRL_WARM_RESET_REG); + reg &= ~(CTRL_PCIE_CORE_WARM_RESET | CTRL_PHY_CORE_WARM_RESET); + advk_writel(pcie, reg, CTRL_WARM_RESET_REG); +} + static void advk_pcie_setup_hw(struct advk_pcie *pcie) { + struct device *dev = &pcie->pdev->dev; + struct device_node *node = dev->of_node; u32 reg; int i; @@ -311,10 +332,15 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie) PCIE_CORE_CTRL2_TD_ENABLE; advk_writel(pcie, reg, PCIE_CORE_CTRL2_REG); - /* Set GEN2 */ + /* Set GEN */ reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG); reg &= ~PCIE_GEN_SEL_MSK; - reg |= SPEED_GEN_2; + if (of_pci_get_max_link_speed(node) == 1) + reg |= SPEED_GEN_1; + if (of_pci_get_max_link_speed(node) == 3) + reg |= SPEED_GEN_3; + else + reg |= SPEED_GEN_2; advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG); /* Set lane X1 */ @@ -948,6 +974,8 @@ static int advk_pcie_probe(struct platform_device *pdev) struct pci_bus *bus, *child; struct pci_host_bridge *bridge; int ret, irq; + int reset_gpio; + enum of_gpio_flags flags; bridge = devm_pci_alloc_host_bridge(dev, sizeof(struct advk_pcie)); if (!bridge) @@ -970,6 +998,20 @@ static int advk_pcie_probe(struct platform_device *pdev) return ret; } + /* Config reset gpio for pcie if there is valid gpio setting in DTS */ + reset_gpio = of_get_named_gpio_flags(pdev->dev.of_node, "reset-gpio", + 0, &flags); + if (gpio_is_valid(reset_gpio)) { + struct gpio_desc *reset_gpiod; + reset_gpiod = gpio_to_desc(reset_gpio); + gpiod_direction_output(reset_gpiod, 0); + mdelay(200); + gpiod_direction_output(reset_gpiod, 1); + mdelay(200); + } + + advk_pcie_warm_reset(pcie); + ret = advk_pcie_parse_request_of_pci_ranges(pcie); if (ret) { dev_err(dev, "Failed to parse resources\n");
Add code to do a warm reset on the PHY and PCIE cores and if PERSTN GPIO is specified in device tree (as reset-gpio), also reset the card. The reset-gpio is inspired by what is done in U-Boot and linux-marvell, and is not final version: I am hoping this can be done via a PCIe register rather than GPIO - bit 3 of CTRL_WARM_RESET_REG register (which is added by this patch) is called PERSTN_GPIO_EN (Enable PERSTN from GPIO) and I think this is the right register, but manipulating this register did not have any effect on the PERSTN pin, even when pinctrl was correctly set. I asked Marvell about this and am awaiting their reply. The reset-gpio is needed for Compex 5 GHz wifi card model WLE900VX. Without this patch the PCIe link never comes up in kernel (although U-Boot pci command was able to enumerate the card). What is weird is that the link does not come up for this card when pci-aardvark driver is probed in U-Boot. I haven't yet had time to discover the problem there. My temporary solution is to compile out the pci-aardvark driver from U-Boot. This patch is based on 4.14 kernel. If you have time, please try it with some PCIe cards and let me know if they work correctly. Signed-off-by: Marek Behún <marek.behun@nic.cz> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Cc: Bjorn Helgaas <helgaas@kernel.org> Cc: linux-pci@vger.kernel.org Cc: Antoine Ténart <antoine.tenart@free-electrons.com> Cc: Grégory Clement <gregory.clement@free-electrons.com> Cc: Miquèl Raynal <miquel.raynal@free-electrons.com> Cc: Victor Gu <xigu@marvell.com> --- drivers/pci/host/pci-aardvark.c | 46 +++++++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-)