| Message ID | 20241112070310.757856-1-jpatel2@marvell.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] PCI: armada8k: add device reset to link-down handle | expand |
On Mon, Nov 11, 2024 at 11:03:10PM -0800, Jenishkumar Maheshbhai Patel wrote: > diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c > index b1b48c2016f7..9a48ef60be51 100644 > --- a/drivers/pci/controller/dwc/pcie-armada8k.c > +++ b/drivers/pci/controller/dwc/pcie-armada8k.c > @@ -23,6 +23,7 @@ > #include <linux/of_pci.h> > #include <linux/mfd/syscon.h> > #include <linux/regmap.h> > +#include <linux/of_gpio.h> > > #include "pcie-designware.h" > > @@ -37,6 +38,8 @@ struct armada8k_pcie { > struct regmap *sysctrl_base; > u32 mac_rest_bitmask; > struct work_struct recover_link_work; > + enum of_gpio_flags flags; > + struct gpio_desc *reset_gpio; > }; > > #define PCIE_VENDOR_REGS_OFFSET 0x8000 > @@ -238,9 +241,18 @@ static void armada8k_pcie_recover_link(struct work_struct *ws) > } > pci_lock_rescan_remove(); > pci_stop_and_remove_bus_device(root_port); > + /* Reset device if reset gpio is set */ > + if (pcie->reset_gpio) { > + /* assert and then deassert the reset signal */ > + gpiod_set_value_cansleep(pcie->reset_gpio, 0); > + msleep(100); > + gpiod_set_value_cansleep(pcie->reset_gpio, > + (pcie->flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1); This looks wrong. resets are normally active-low. gpiod_set_value_cansleep() should be called with '1' to indicate active state, and '0' to indicate de-active state. DT should specify whether the signal is active high or active low. > + /* Config reset gpio for pcie if the reset connected to gpio */ > + reset_gpio = of_get_named_gpio_flags(pdev->dev.of_node, > + "reset-gpios", 0, > + &pcie->flags); > + if (gpio_is_valid(reset_gpio)) > + pcie->reset_gpio = gpio_to_desc(reset_gpio); > + Just use devm_fwnode_gpiod_get() here, which will also handle the cleanup. To see how this should be done, look at drivers/pci/controller/pci-mvebu.c which gets the polarity settings correct too, as mentioned above. Lastly, as this patch is related to the breaking DT change, it _should_ have been sent as a patch series (which means the same Cc list on each patch) so that reviewers can see the full story and comments on the other patches.
In subject, follow capitalization convention. Use "git log --oneline". On Mon, Nov 11, 2024 at 11:03:10PM -0800, Jenishkumar Maheshbhai Patel wrote: > Added pcie reset via gpio support as described in the > designware-pcie.txt DT binding document. > In cases link down cause still exist in device. > The device need to be reset to reestablish the link. > If reset-gpio pin provided in the device tree, then the linkdown > handle resets the device before reestablishing link. s/pcie/PCIe/ s/gpio/GPIO/ Add blank lines between paragraphs. Rewrap to fill 75 columns. > Signed-off-by: Jenishkumar Maheshbhai Patel <jpatel2@marvell.com> > --- > drivers/pci/controller/dwc/pcie-armada8k.c | 24 ++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c > index b1b48c2016f7..9a48ef60be51 100644 > --- a/drivers/pci/controller/dwc/pcie-armada8k.c > +++ b/drivers/pci/controller/dwc/pcie-armada8k.c > @@ -23,6 +23,7 @@ > #include <linux/of_pci.h> > #include <linux/mfd/syscon.h> > #include <linux/regmap.h> > +#include <linux/of_gpio.h> Preserve (mostly) alpha sorted list of includes. > #include "pcie-designware.h" > > @@ -37,6 +38,8 @@ struct armada8k_pcie { > struct regmap *sysctrl_base; > u32 mac_rest_bitmask; > struct work_struct recover_link_work; > + enum of_gpio_flags flags; > + struct gpio_desc *reset_gpio; > }; > > #define PCIE_VENDOR_REGS_OFFSET 0x8000 > @@ -238,9 +241,18 @@ static void armada8k_pcie_recover_link(struct work_struct *ws) > } > pci_lock_rescan_remove(); > pci_stop_and_remove_bus_device(root_port); > + /* Reset device if reset gpio is set */ > + if (pcie->reset_gpio) { > + /* assert and then deassert the reset signal */ > + gpiod_set_value_cansleep(pcie->reset_gpio, 0); > + msleep(100); Needs some sort of #define for this 100 ms. > + gpiod_set_value_cansleep(pcie->reset_gpio, > + (pcie->flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1); > + } > /* > - * Sleep needed to make sure all pcie transactions and access > - * are flushed before resetting the mac > + * Sleep used for two reasons. > + * First make sure all pcie transactions and access are flushed before resetting the mac > + * and second to make sure pci device is ready in case we reset the device > */ > msleep(100); s/pcie/PCIe/ (throughout) s/mac/MAC/ Explain the 100ms. Hopefully this is something defined by PCIe base or CEM spec. Use or add #define as needed. > @@ -376,6 +388,7 @@ static int armada8k_pcie_probe(struct platform_device *pdev) > struct armada8k_pcie *pcie; > struct device *dev = &pdev->dev; > struct resource *base; > + int reset_gpio; > int ret; > > pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL); > @@ -420,6 +433,13 @@ static int armada8k_pcie_probe(struct platform_device *pdev) > goto fail_clkreg; > } > > + /* Config reset gpio for pcie if the reset connected to gpio */ > + reset_gpio = of_get_named_gpio_flags(pdev->dev.of_node, > + "reset-gpios", 0, > + &pcie->flags); > + if (gpio_is_valid(reset_gpio)) > + pcie->reset_gpio = gpio_to_desc(reset_gpio); > + > pcie->sysctrl_base = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, > "marvell,system-controller"); > if (IS_ERR(pcie->sysctrl_base)) { > -- > 2.25.1 >
diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c index b1b48c2016f7..9a48ef60be51 100644 --- a/drivers/pci/controller/dwc/pcie-armada8k.c +++ b/drivers/pci/controller/dwc/pcie-armada8k.c @@ -23,6 +23,7 @@ #include <linux/of_pci.h> #include <linux/mfd/syscon.h> #include <linux/regmap.h> +#include <linux/of_gpio.h> #include "pcie-designware.h" @@ -37,6 +38,8 @@ struct armada8k_pcie { struct regmap *sysctrl_base; u32 mac_rest_bitmask; struct work_struct recover_link_work; + enum of_gpio_flags flags; + struct gpio_desc *reset_gpio; }; #define PCIE_VENDOR_REGS_OFFSET 0x8000 @@ -238,9 +241,18 @@ static void armada8k_pcie_recover_link(struct work_struct *ws) } pci_lock_rescan_remove(); pci_stop_and_remove_bus_device(root_port); + /* Reset device if reset gpio is set */ + if (pcie->reset_gpio) { + /* assert and then deassert the reset signal */ + gpiod_set_value_cansleep(pcie->reset_gpio, 0); + msleep(100); + gpiod_set_value_cansleep(pcie->reset_gpio, + (pcie->flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1); + } /* - * Sleep needed to make sure all pcie transactions and access - * are flushed before resetting the mac + * Sleep used for two reasons. + * First make sure all pcie transactions and access are flushed before resetting the mac + * and second to make sure pci device is ready in case we reset the device */ msleep(100); @@ -376,6 +388,7 @@ static int armada8k_pcie_probe(struct platform_device *pdev) struct armada8k_pcie *pcie; struct device *dev = &pdev->dev; struct resource *base; + int reset_gpio; int ret; pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL); @@ -420,6 +433,13 @@ static int armada8k_pcie_probe(struct platform_device *pdev) goto fail_clkreg; } + /* Config reset gpio for pcie if the reset connected to gpio */ + reset_gpio = of_get_named_gpio_flags(pdev->dev.of_node, + "reset-gpios", 0, + &pcie->flags); + if (gpio_is_valid(reset_gpio)) + pcie->reset_gpio = gpio_to_desc(reset_gpio); + pcie->sysctrl_base = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "marvell,system-controller"); if (IS_ERR(pcie->sysctrl_base)) {
Added pcie reset via gpio support as described in the designware-pcie.txt DT binding document. In cases link down cause still exist in device. The device need to be reset to reestablish the link. If reset-gpio pin provided in the device tree, then the linkdown handle resets the device before reestablishing link. Signed-off-by: Jenishkumar Maheshbhai Patel <jpatel2@marvell.com> --- drivers/pci/controller/dwc/pcie-armada8k.c | 24 ++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)