| Message ID | 20250513091557.3660-2-rengarajan.s@microchip.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add GPIO Hot reset and Wakeup Support | expand |
> -----Original Message----- > From: Rengarajan S <rengarajan.s@microchip.com> > Sent: Tuesday, May 13, 2025 5:16 AM > To: VaibhaavRam TL - I69105 <VaibhaavRam.TL@microchip.com>; Kumaravel Thiagarajan - I21417 > <Kumaravel.Thiagarajan@microchip.com>; arnd@arndb.de; gregkh@linuxfoundation.org; > linus.walleij@linaro.org; brgl@bgdev.pl; linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org; > UNGLinuxDriver <UNGLinuxDriver@microchip.com> > Cc: Rengarajan S - I69107 <Rengarajan.S@microchip.com> > Subject: [PATCH v1 char-misc-next 1/2] misc: microchip: pci1xxxx: Add PCIe Hot reset disable support > for Rev C0 and later devices > > Systems that issue PCIe hot reset requests during a suspend/resume cycle cause PCI1XXXX device > revisions prior to C0 to get its GPIO configuration registers reset to hardware default values. This results > in device inaccessibility and GPIO read/write failure. Starting with Revision C0, support was added in the > device hardware (via the Hot Reset Disable Bit) to allow resetting only the PCIe interface and its > associated logic, but preserving the GPIO configurations during a hot reset. This patch enables the hot > reset disable feature during suspend/ resume for C0 and later revisions of the device. > > mchp_pci1xxxx_gpio is an auxiliary child of mchp_pci1xxxx_gp and does not have access to system > register address space for reading the device revision. Hence, the device revision is retrieved directly > from PCIe config space. > > Signed-off-by: Rengarajan S <rengarajan.s@microchip.com> > --- > .../misc/mchp_pci1xxxx/mchp_pci1xxxx_gpio.c | 31 +++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gpio.c > b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gpio.c > index 98d3d123004c..3a2a1a4ef612 100644 > --- a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gpio.c > +++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gpio.c > @@ -7,12 +7,14 @@ > #include <linux/gpio/driver.h> > #include <linux/bio.h> > #include <linux/mutex.h> > +#include <linux/pci.h> > #include <linux/kthread.h> > #include <linux/interrupt.h> > > #include "mchp_pci1xxxx_gp.h" > > #define PCI1XXXX_NR_PINS 93 > +#define PCI_DEV_REV_OFFSET 0x08 > #define PERI_GEN_RESET 0 > #define OUT_EN_OFFSET(x) ((((x) / 32) * 4) + 0x400) > #define INP_EN_OFFSET(x) ((((x) / 32) * 4) + 0x400 + 0x10) > @@ -41,8 +43,25 @@ struct pci1xxxx_gpio { > struct gpio_chip gpio; > spinlock_t lock; > int irq_base; > + u8 dev_rev; > }; > > +static int pci1xxxx_gpio_get_device_revision(struct pci1xxxx_gpio > +*priv) { > + struct device *parent = priv->aux_dev->dev.parent; > + struct pci_dev *pcidev = to_pci_dev(parent); > + int ret; > + u32 val; > + > + ret = pci_read_config_dword(pcidev, PCI_DEV_REV_OFFSET, &val); > + if (ret) > + return ret; > + > + priv->dev_rev = val; > + > + return 0; > +} > + Reiterate my comment close to an year ago for the SPI driver https://jira.microchip.com/browse/UNG_BRIDGEPORT-5468?focusedId=4336619&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-4336619 "Just so we are clear. using the PCI config Revision ID field is NOT as comprehensive/safe a solution for checking the chip revision as checking the internal BAR DEV_REV_REG register's REVID. You are only guaranteed Revision ID to match the DEFAULT value of DEV_REG REVID. While it would be highly unusual to do that, if a customer were to customize the (VID/PID) / Rev for his device they will not." If you are going to continue to check revision ID thru config space in more drivers for now, then for each one please open a new Jira so that whenever you decide to do it the proper way you do not forget any driver that needs to be updated. > static int pci1xxxx_gpio_get_direction(struct gpio_chip *gpio, unsigned int nr) { > struct pci1xxxx_gpio *priv = gpiochip_get_data(gpio); @@ -315,6 +334,10 @@ static int > pci1xxxx_gpio_suspend(struct device *dev) > pci1xxx_assign_bit(priv->reg_base, PIO_GLOBAL_CONFIG_OFFSET, > 17, false); > pci1xxx_assign_bit(priv->reg_base, PERI_GEN_RESET, 16, true); > + > + if (priv->dev_rev >= 0xC0) > + pci1xxx_assign_bit(priv->reg_base, PERI_GEN_RESET, 17, true); > + > spin_unlock_irqrestore(&priv->lock, flags); > > return 0; > @@ -331,6 +354,10 @@ static int pci1xxxx_gpio_resume(struct device *dev) > pci1xxx_assign_bit(priv->reg_base, PIO_GLOBAL_CONFIG_OFFSET, > 16, false); > pci1xxx_assign_bit(priv->reg_base, PERI_GEN_RESET, 16, false); > + > + if (priv->dev_rev >= 0xC0) > + pci1xxx_assign_bit(priv->reg_base, PERI_GEN_RESET, 17, false); > + > spin_unlock_irqrestore(&priv->lock, flags); > > return 0; > @@ -412,6 +439,10 @@ static int pci1xxxx_gpio_probe(struct auxiliary_device *aux_dev, > if (retval < 0) > return retval; > > + retval = pci1xxxx_gpio_get_device_revision(priv); > + if (retval) > + return retval; > + > dev_set_drvdata(&aux_dev->dev, priv); > > return devm_gpiochip_add_data(&aux_dev->dev, &priv->gpio, priv); > -- > 2.25.1
> -----Original Message----- > From: Ronnie Kunin - C21729 <Ronnie.Kunin@microchip.com> > Sent: Tuesday, May 13, 2025 10:26 AM > > ... > > +static int pci1xxxx_gpio_get_device_revision(struct pci1xxxx_gpio > > +*priv) { > > + struct device *parent = priv->aux_dev->dev.parent; > > + struct pci_dev *pcidev = to_pci_dev(parent); > > + int ret; > > + u32 val; > > + > > + ret = pci_read_config_dword(pcidev, PCI_DEV_REV_OFFSET, &val); > > + if (ret) > > + return ret; > > + > > + priv->dev_rev = val; > > + > > + return 0; > > +} > > + > > > Reiterate my comment close to an year ago for the SPI driver > https://jira.microchip.com/browse/UNG_BRIDGEPORT- > 5468?focusedId=4336619&page=com.atlassian.jira.plugin.system.issuetabpanels:comment- > tabpanel#comment-4336619 > > "Just so we are clear. using the PCI config Revision ID field is NOT as comprehensive/safe a solution for > checking the chip revision as checking the internal BAR DEV_REV_REG register's REVID. You are only > guaranteed Revision ID to match the DEFAULT value of DEV_REG REVID. > While it would be highly unusual to do that, if a customer were to customize the (VID/PID) / Rev for his > device they will not." > > If you are going to continue to check revision ID thru config space in more drivers for now, then for each > one please open a new Jira so that whenever you decide to do it the proper way you do not forget any > driver that needs to be updated. > My apologies that this comment with microchip private link went out to the community, it was meant to be an internal Microchip discussion. Just short FYI to the maintainers, my suggestion about a "proper way" which we use in the ethernet driver of the PCI11x1x devices for other purposes beyond ID check is not directly portable without other major changes in the architecture of the GPIO driver which is overkill just for this ID check purpose. Therefore, the implementation Rengarajan proposed in this patch is perfectly fine at this time and I have no reservations to merge it in. Thanks, Ronnie
diff --git a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gpio.c b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gpio.c index 98d3d123004c..3a2a1a4ef612 100644 --- a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gpio.c +++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gpio.c @@ -7,12 +7,14 @@ #include <linux/gpio/driver.h> #include <linux/bio.h> #include <linux/mutex.h> +#include <linux/pci.h> #include <linux/kthread.h> #include <linux/interrupt.h> #include "mchp_pci1xxxx_gp.h" #define PCI1XXXX_NR_PINS 93 +#define PCI_DEV_REV_OFFSET 0x08 #define PERI_GEN_RESET 0 #define OUT_EN_OFFSET(x) ((((x) / 32) * 4) + 0x400) #define INP_EN_OFFSET(x) ((((x) / 32) * 4) + 0x400 + 0x10) @@ -41,8 +43,25 @@ struct pci1xxxx_gpio { struct gpio_chip gpio; spinlock_t lock; int irq_base; + u8 dev_rev; }; +static int pci1xxxx_gpio_get_device_revision(struct pci1xxxx_gpio *priv) +{ + struct device *parent = priv->aux_dev->dev.parent; + struct pci_dev *pcidev = to_pci_dev(parent); + int ret; + u32 val; + + ret = pci_read_config_dword(pcidev, PCI_DEV_REV_OFFSET, &val); + if (ret) + return ret; + + priv->dev_rev = val; + + return 0; +} + static int pci1xxxx_gpio_get_direction(struct gpio_chip *gpio, unsigned int nr) { struct pci1xxxx_gpio *priv = gpiochip_get_data(gpio); @@ -315,6 +334,10 @@ static int pci1xxxx_gpio_suspend(struct device *dev) pci1xxx_assign_bit(priv->reg_base, PIO_GLOBAL_CONFIG_OFFSET, 17, false); pci1xxx_assign_bit(priv->reg_base, PERI_GEN_RESET, 16, true); + + if (priv->dev_rev >= 0xC0) + pci1xxx_assign_bit(priv->reg_base, PERI_GEN_RESET, 17, true); + spin_unlock_irqrestore(&priv->lock, flags); return 0; @@ -331,6 +354,10 @@ static int pci1xxxx_gpio_resume(struct device *dev) pci1xxx_assign_bit(priv->reg_base, PIO_GLOBAL_CONFIG_OFFSET, 16, false); pci1xxx_assign_bit(priv->reg_base, PERI_GEN_RESET, 16, false); + + if (priv->dev_rev >= 0xC0) + pci1xxx_assign_bit(priv->reg_base, PERI_GEN_RESET, 17, false); + spin_unlock_irqrestore(&priv->lock, flags); return 0; @@ -412,6 +439,10 @@ static int pci1xxxx_gpio_probe(struct auxiliary_device *aux_dev, if (retval < 0) return retval; + retval = pci1xxxx_gpio_get_device_revision(priv); + if (retval) + return retval; + dev_set_drvdata(&aux_dev->dev, priv); return devm_gpiochip_add_data(&aux_dev->dev, &priv->gpio, priv);
Systems that issue PCIe hot reset requests during a suspend/resume cycle cause PCI1XXXX device revisions prior to C0 to get its GPIO configuration registers reset to hardware default values. This results in device inaccessibility and GPIO read/write failure. Starting with Revision C0, support was added in the device hardware (via the Hot Reset Disable Bit) to allow resetting only the PCIe interface and its associated logic, but preserving the GPIO configurations during a hot reset. This patch enables the hot reset disable feature during suspend/ resume for C0 and later revisions of the device. mchp_pci1xxxx_gpio is an auxiliary child of mchp_pci1xxxx_gp and does not have access to system register address space for reading the device revision. Hence, the device revision is retrieved directly from PCIe config space. Signed-off-by: Rengarajan S <rengarajan.s@microchip.com> --- .../misc/mchp_pci1xxxx/mchp_pci1xxxx_gpio.c | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+)