| Message ID | 20241007041218.157516-13-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Manivannan Sadhasivam |
| Headers | show |
| Series | [v3,01/12] PCI: rockchip-ep: Fix address translation unit programming | expand |
Hi Damien,
kernel test robot noticed the following build errors:
[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus mani-mhi/mhi-next linus/master v6.12-rc2 next-20241009]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Damien-Le-Moal/PCI-rockchip-ep-Use-a-macro-to-define-EP-controller-align-feature/20241007-131224
base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link: https://lore.kernel.org/r/20241007041218.157516-13-dlemoal%40kernel.org
patch subject: [PATCH v3 12/12] PCI: rockchip-ep: Handle PERST# signal in endpoint mode
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20241010/202410101236.xqI8ZWNd-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241010/202410101236.xqI8ZWNd-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410101236.xqI8ZWNd-lkp@intel.com/
All errors (new ones prefixed by >>):
| ^~
drivers/pci/controller/pcie-rockchip-ep.c:255:44: error: invalid use of undefined type 'struct pci_epc_map'
255 | map->map_ofst = map->pci_addr - map->map_pci_addr;
| ^~
drivers/pci/controller/pcie-rockchip-ep.c:257:16: error: invalid use of undefined type 'struct pci_epc_map'
257 | if (map->map_ofst + map->pci_size > SZ_1M)
| ^~
drivers/pci/controller/pcie-rockchip-ep.c:257:32: error: invalid use of undefined type 'struct pci_epc_map'
257 | if (map->map_ofst + map->pci_size > SZ_1M)
| ^~
drivers/pci/controller/pcie-rockchip-ep.c:258:20: error: invalid use of undefined type 'struct pci_epc_map'
258 | map->pci_size = SZ_1M - map->map_ofst;
| ^~
drivers/pci/controller/pcie-rockchip-ep.c:258:44: error: invalid use of undefined type 'struct pci_epc_map'
258 | map->pci_size = SZ_1M - map->map_ofst;
| ^~
drivers/pci/controller/pcie-rockchip-ep.c:260:12: error: invalid use of undefined type 'struct pci_epc_map'
260 | map->map_size = ALIGN(map->map_ofst + map->pci_size,
| ^~
In file included from include/vdso/const.h:5,
from include/linux/const.h:4,
from include/uapi/linux/kernel.h:6,
from include/linux/cache.h:5,
from include/linux/time.h:5,
from include/linux/stat.h:19,
from include/linux/configfs.h:22,
from drivers/pci/controller/pcie-rockchip-ep.c:11:
drivers/pci/controller/pcie-rockchip-ep.c:260:34: error: invalid use of undefined type 'struct pci_epc_map'
260 | map->map_size = ALIGN(map->map_ofst + map->pci_size,
| ^~
include/uapi/linux/const.h:49:44: note: in definition of macro '__ALIGN_KERNEL_MASK'
49 | #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask))
| ^
include/linux/align.h:8:33: note: in expansion of macro '__ALIGN_KERNEL'
8 | #define ALIGN(x, a) __ALIGN_KERNEL((x), (a))
| ^~~~~~~~~~~~~~
drivers/pci/controller/pcie-rockchip-ep.c:260:25: note: in expansion of macro 'ALIGN'
260 | map->map_size = ALIGN(map->map_ofst + map->pci_size,
| ^~~~~
drivers/pci/controller/pcie-rockchip-ep.c:260:50: error: invalid use of undefined type 'struct pci_epc_map'
260 | map->map_size = ALIGN(map->map_ofst + map->pci_size,
| ^~
include/uapi/linux/const.h:49:44: note: in definition of macro '__ALIGN_KERNEL_MASK'
49 | #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask))
| ^
include/linux/align.h:8:33: note: in expansion of macro '__ALIGN_KERNEL'
8 | #define ALIGN(x, a) __ALIGN_KERNEL((x), (a))
| ^~~~~~~~~~~~~~
drivers/pci/controller/pcie-rockchip-ep.c:260:25: note: in expansion of macro 'ALIGN'
260 | map->map_size = ALIGN(map->map_ofst + map->pci_size,
| ^~~~~
drivers/pci/controller/pcie-rockchip-ep.c:260:34: error: invalid use of undefined type 'struct pci_epc_map'
260 | map->map_size = ALIGN(map->map_ofst + map->pci_size,
| ^~
include/uapi/linux/const.h:49:50: note: in definition of macro '__ALIGN_KERNEL_MASK'
49 | #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask))
| ^~~~
include/linux/align.h:8:33: note: in expansion of macro '__ALIGN_KERNEL'
8 | #define ALIGN(x, a) __ALIGN_KERNEL((x), (a))
| ^~~~~~~~~~~~~~
drivers/pci/controller/pcie-rockchip-ep.c:260:25: note: in expansion of macro 'ALIGN'
260 | map->map_size = ALIGN(map->map_ofst + map->pci_size,
| ^~~~~
drivers/pci/controller/pcie-rockchip-ep.c:260:50: error: invalid use of undefined type 'struct pci_epc_map'
260 | map->map_size = ALIGN(map->map_ofst + map->pci_size,
| ^~
include/uapi/linux/const.h:49:50: note: in definition of macro '__ALIGN_KERNEL_MASK'
49 | #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask))
| ^~~~
include/linux/align.h:8:33: note: in expansion of macro '__ALIGN_KERNEL'
8 | #define ALIGN(x, a) __ALIGN_KERNEL((x), (a))
| ^~~~~~~~~~~~~~
drivers/pci/controller/pcie-rockchip-ep.c:260:25: note: in expansion of macro 'ALIGN'
260 | map->map_size = ALIGN(map->map_ofst + map->pci_size,
| ^~~~~
drivers/pci/controller/pcie-rockchip-ep.c:260:34: error: invalid use of undefined type 'struct pci_epc_map'
260 | map->map_size = ALIGN(map->map_ofst + map->pci_size,
| ^~
include/uapi/linux/const.h:49:61: note: in definition of macro '__ALIGN_KERNEL_MASK'
49 | #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask))
| ^~~~
include/linux/align.h:8:33: note: in expansion of macro '__ALIGN_KERNEL'
8 | #define ALIGN(x, a) __ALIGN_KERNEL((x), (a))
| ^~~~~~~~~~~~~~
drivers/pci/controller/pcie-rockchip-ep.c:260:25: note: in expansion of macro 'ALIGN'
260 | map->map_size = ALIGN(map->map_ofst + map->pci_size,
| ^~~~~
drivers/pci/controller/pcie-rockchip-ep.c:260:50: error: invalid use of undefined type 'struct pci_epc_map'
260 | map->map_size = ALIGN(map->map_ofst + map->pci_size,
| ^~
include/uapi/linux/const.h:49:61: note: in definition of macro '__ALIGN_KERNEL_MASK'
49 | #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask))
| ^~~~
include/linux/align.h:8:33: note: in expansion of macro '__ALIGN_KERNEL'
8 | #define ALIGN(x, a) __ALIGN_KERNEL((x), (a))
| ^~~~~~~~~~~~~~
drivers/pci/controller/pcie-rockchip-ep.c:260:25: note: in expansion of macro 'ALIGN'
260 | map->map_size = ALIGN(map->map_ofst + map->pci_size,
| ^~~~~
drivers/pci/controller/pcie-rockchip-ep.c: In function 'rockchip_pcie_ep_perst_irq_thread':
>> drivers/pci/controller/pcie-rockchip-ep.c:631:9: error: implicit declaration of function 'irq_set_irq_type'; did you mean 'irq_set_irq_wake'? [-Wimplicit-function-declaration]
631 | irq_set_irq_type(ep->perst_irq,
| ^~~~~~~~~~~~~~~~
| irq_set_irq_wake
drivers/pci/controller/pcie-rockchip-ep.c: In function 'rockchip_pcie_ep_setup_irq':
>> drivers/pci/controller/pcie-rockchip-ep.c:655:9: error: implicit declaration of function 'irq_set_status_flags' [-Wimplicit-function-declaration]
655 | irq_set_status_flags(ep->perst_irq, IRQ_NOAUTOEN);
| ^~~~~~~~~~~~~~~~~~~~
>> drivers/pci/controller/pcie-rockchip-ep.c:655:45: error: 'IRQ_NOAUTOEN' undeclared (first use in this function); did you mean 'IRQF_NO_AUTOEN'?
655 | irq_set_status_flags(ep->perst_irq, IRQ_NOAUTOEN);
| ^~~~~~~~~~~~
| IRQF_NO_AUTOEN
drivers/pci/controller/pcie-rockchip-ep.c:655:45: note: each undeclared identifier is reported only once for each function it appears in
drivers/pci/controller/pcie-rockchip-ep.c: At top level:
drivers/pci/controller/pcie-rockchip-ep.c:685:10: error: 'const struct pci_epc_ops' has no member named 'map_align'
685 | .map_align = rockchip_pcie_ep_map_align,
| ^~~~~~~~~
drivers/pci/controller/pcie-rockchip-ep.c:685:27: error: initialization of 'int (*)(struct pci_epc *, u8, u8, phys_addr_t, u64, size_t)' {aka 'int (*)(struct pci_epc *, unsigned char, unsigned char, long long unsigned int, long long unsigned int, long unsigned int)'} from incompatible pointer type 'int (*)(struct pci_epc *, u8, u8, struct pci_epc_map *)' {aka 'int (*)(struct pci_epc *, unsigned char, unsigned char, struct pci_epc_map *)'} [-Wincompatible-pointer-types]
685 | .map_align = rockchip_pcie_ep_map_align,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/pci/controller/pcie-rockchip-ep.c:685:27: note: (near initialization for 'rockchip_pcie_epc_ops.map_addr')
drivers/pci/controller/pcie-rockchip-ep.c:686:27: warning: initialized field overwritten [-Woverride-init]
686 | .map_addr = rockchip_pcie_ep_map_addr,
| ^~~~~~~~~~~~~~~~~~~~~~~~~
drivers/pci/controller/pcie-rockchip-ep.c:686:27: note: (near initialization for 'rockchip_pcie_epc_ops.map_addr')
vim +631 drivers/pci/controller/pcie-rockchip-ep.c
618
619 static irqreturn_t rockchip_pcie_ep_perst_irq_thread(int irq, void *data)
620 {
621 struct pci_epc *epc = data;
622 struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
623 struct rockchip_pcie *rockchip = &ep->rockchip;
624 u32 perst = gpiod_get_value(rockchip->ep_gpio);
625
626 if (perst)
627 rockchip_pcie_ep_perst_assert(ep);
628 else
629 rockchip_pcie_ep_perst_deassert(ep);
630
> 631 irq_set_irq_type(ep->perst_irq,
632 (perst ? IRQF_TRIGGER_HIGH : IRQF_TRIGGER_LOW));
633
634 return IRQ_HANDLED;
635 }
636
637 static int rockchip_pcie_ep_setup_irq(struct pci_epc *epc)
638 {
639 struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
640 struct rockchip_pcie *rockchip = &ep->rockchip;
641 struct device *dev = rockchip->dev;
642 int ret;
643
644 if (!rockchip->ep_gpio)
645 return 0;
646
647 /* PCIe reset interrupt */
648 ep->perst_irq = gpiod_to_irq(rockchip->ep_gpio);
649 if (ep->perst_irq < 0) {
650 dev_err(dev, "No corresponding IRQ for PERST GPIO\n");
651 return ep->perst_irq;
652 }
653
654 ep->perst_asserted = true;
> 655 irq_set_status_flags(ep->perst_irq, IRQ_NOAUTOEN);
656 ret = devm_request_threaded_irq(dev, ep->perst_irq, NULL,
657 rockchip_pcie_ep_perst_irq_thread,
658 IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
659 "pcie-ep-perst", epc);
660 if (ret) {
661 dev_err(dev, "Request PERST GPIO IRQ failed %d\n", ret);
662 return ret;
663 }
664
665 return 0;
666 }
667
On Mon, Oct 07, 2024 at 01:12:18PM +0900, Damien Le Moal wrote: > Currently, the Rockchip PCIe endpoint controller driver does not handle > PERST# signal, which prevents detecting when link training should > actually be started or if the host reset the device. This however can > be supported using the controller ep_gpio, set as an input GPIO for > endpoint mode. > > Modify the endpoint rockchip driver to get the ep_gpio and its > associated interrupt which is serviced using a threaded IRQ with the > function rockchip_pcie_ep_perst_irq_thread() as handler. > > This handler function notifies a link down event corresponding to the RC > side asserting the PERST# signal using pci_epc_linkdown() when the gpio > is high. Once the gpio value goes down, corresponding to the RC > de-asserting the PERST# signal, link training is started. The polarity > of the gpio interrupt trigger is changed from high to low after the RC > asserted PERST#, and conversely changed from low to high after the RC > de-asserts PERST#. > > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> Just minor nits below. Overall LGTM. > --- > drivers/pci/controller/pcie-rockchip-ep.c | 118 +++++++++++++++++++++- > drivers/pci/controller/pcie-rockchip.c | 12 +-- > 2 files changed, 122 insertions(+), 8 deletions(-) > > diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c > index af50432525b4..c70a64c37a56 100644 > --- a/drivers/pci/controller/pcie-rockchip-ep.c > +++ b/drivers/pci/controller/pcie-rockchip-ep.c > @@ -18,6 +18,7 @@ > #include <linux/sizes.h> > #include <linux/workqueue.h> > #include <linux/iopoll.h> > +#include <linux/gpio/consumer.h> Please sort the includes. > > #include "pcie-rockchip.h" > > @@ -50,6 +51,9 @@ struct rockchip_pcie_ep { > u64 irq_pci_addr; > u8 irq_pci_fn; > u8 irq_pending; > + int perst_irq; > + bool perst_asserted; > + bool link_up; > struct delayed_work link_training; > }; > > @@ -462,13 +466,17 @@ static int rockchip_pcie_ep_start(struct pci_epc *epc) > > rockchip_pcie_write(rockchip, cfg, PCIE_CORE_PHY_FUNC_CFG); > > + if (rockchip->ep_gpio) > + enable_irq(ep->perst_irq); > + > /* Enable configuration and start link training */ > rockchip_pcie_write(rockchip, > PCIE_CLIENT_LINK_TRAIN_ENABLE | > PCIE_CLIENT_CONF_ENABLE, > PCIE_CLIENT_CONFIG); > > - schedule_delayed_work(&ep->link_training, 0); > + if (!rockchip->ep_gpio) > + schedule_delayed_work(&ep->link_training, 0); > > return 0; > } > @@ -478,6 +486,11 @@ static void rockchip_pcie_ep_stop(struct pci_epc *epc) > struct rockchip_pcie_ep *ep = epc_get_drvdata(epc); > struct rockchip_pcie *rockchip = &ep->rockchip; > > + if (rockchip->ep_gpio) { > + ep->perst_asserted = true; > + disable_irq(ep->perst_irq); > + } > + > cancel_delayed_work_sync(&ep->link_training); > > /* Stop link training and disable configuration */ > @@ -540,6 +553,13 @@ static void rockchip_pcie_ep_link_training(struct work_struct *work) > if (!rockchip_pcie_ep_link_up(rockchip)) > goto again; > > + /* > + * If PERST was asserted while polling the link, do not notify > + * the function. > + */ > + if (ep->perst_asserted) > + return; > + > val = rockchip_pcie_read(rockchip, PCIE_CLIENT_BASIC_STATUS0); > dev_info(dev, > "Link UP (Negociated speed: %sGT/s, width: x%lu)\n", > @@ -549,6 +569,7 @@ static void rockchip_pcie_ep_link_training(struct work_struct *work) > > /* Notify the function */ > pci_epc_linkup(ep->epc); > + ep->link_up = true; > > return; > > @@ -556,6 +577,94 @@ static void rockchip_pcie_ep_link_training(struct work_struct *work) > schedule_delayed_work(&ep->link_training, msecs_to_jiffies(5)); > } > > +static void rockchip_pcie_ep_perst_assert(struct rockchip_pcie_ep *ep) > +{ > + struct rockchip_pcie *rockchip = &ep->rockchip; > + struct device *dev = rockchip->dev; > + > + dev_dbg(dev, "PERST asserted, link down\n"); > + > + if (ep->perst_asserted) > + return; > + > + ep->perst_asserted = true; > + > + cancel_delayed_work_sync(&ep->link_training); > + > + if (ep->link_up) { > + pci_epc_linkdown(ep->epc); > + ep->link_up = false; > + } > +} > + > +static void rockchip_pcie_ep_perst_deassert(struct rockchip_pcie_ep *ep) > +{ > + struct rockchip_pcie *rockchip = &ep->rockchip; > + struct device *dev = rockchip->dev; > + > + dev_dbg(dev, "PERST de-asserted, starting link training\n"); > + > + if (!ep->perst_asserted) > + return; > + > + ep->perst_asserted = false; > + > + /* Enable link re-training */ > + rockchip_pcie_ep_retrain_link(rockchip); > + I hope that no registers are getting reset post PERST# assert. > + /* Start link training */ > + schedule_delayed_work(&ep->link_training, 0); > +} > + > +static irqreturn_t rockchip_pcie_ep_perst_irq_thread(int irq, void *data) > +{ > + struct pci_epc *epc = data; > + struct rockchip_pcie_ep *ep = epc_get_drvdata(epc); > + struct rockchip_pcie *rockchip = &ep->rockchip; > + u32 perst = gpiod_get_value(rockchip->ep_gpio); > + > + if (perst) > + rockchip_pcie_ep_perst_assert(ep); > + else > + rockchip_pcie_ep_perst_deassert(ep); > + > + irq_set_irq_type(ep->perst_irq, > + (perst ? IRQF_TRIGGER_HIGH : IRQF_TRIGGER_LOW)); > + > + return IRQ_HANDLED; > +} > + > +static int rockchip_pcie_ep_setup_irq(struct pci_epc *epc) > +{ > + struct rockchip_pcie_ep *ep = epc_get_drvdata(epc); > + struct rockchip_pcie *rockchip = &ep->rockchip; > + struct device *dev = rockchip->dev; > + int ret; > + > + if (!rockchip->ep_gpio) > + return 0; > + > + /* PCIe reset interrupt */ > + ep->perst_irq = gpiod_to_irq(rockchip->ep_gpio); > + if (ep->perst_irq < 0) { > + dev_err(dev, "No corresponding IRQ for PERST GPIO\n"); > + return ep->perst_irq; > + } > + > + ep->perst_asserted = true; How come? > + irq_set_status_flags(ep->perst_irq, IRQ_NOAUTOEN); > + ret = devm_request_threaded_irq(dev, ep->perst_irq, NULL, > + rockchip_pcie_ep_perst_irq_thread, > + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > + "pcie-ep-perst", epc); > + if (ret) { > + dev_err(dev, "Request PERST GPIO IRQ failed %d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > static const struct pci_epc_features rockchip_pcie_epc_features = { > .linkup_notifier = true, > .msi_capable = true, > @@ -719,6 +828,7 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev) > rockchip->is_rc = false; > rockchip->dev = dev; > INIT_DELAYED_WORK(&ep->link_training, rockchip_pcie_ep_link_training); > + ep->link_up = false; 'false' is the default state, isn't it? - Mani
On 10/10/24 19:49, Manivannan Sadhasivam wrote: >> +static int rockchip_pcie_ep_setup_irq(struct pci_epc *epc) >> +{ >> + struct rockchip_pcie_ep *ep = epc_get_drvdata(epc); >> + struct rockchip_pcie *rockchip = &ep->rockchip; >> + struct device *dev = rockchip->dev; >> + int ret; >> + >> + if (!rockchip->ep_gpio) >> + return 0; >> + >> + /* PCIe reset interrupt */ >> + ep->perst_irq = gpiod_to_irq(rockchip->ep_gpio); >> + if (ep->perst_irq < 0) { >> + dev_err(dev, "No corresponding IRQ for PERST GPIO\n"); >> + return ep->perst_irq; >> + } >> + >> + ep->perst_asserted = true; > > How come? Yeah, a bit confusing. This is because the gpio active low / inactive high, so as soon as we enable the IRQ, we are going to get one IRQ even though perst gpio signal has not changed yet. So to be consistent with this, perst_asserted is initialized to true so that on the first shot of rockchip_pcie_ep_perst_irq_thread() we end up calling rockchip_pcie_ep_perst_assert() as if we got an assert from the host (we have not yet). I will add a comment clarifying that.
On Fri, Oct 11, 2024 at 06:30:31PM +0900, Damien Le Moal wrote: > On 10/10/24 19:49, Manivannan Sadhasivam wrote: > >> +static int rockchip_pcie_ep_setup_irq(struct pci_epc *epc) > >> +{ > >> + struct rockchip_pcie_ep *ep = epc_get_drvdata(epc); > >> + struct rockchip_pcie *rockchip = &ep->rockchip; > >> + struct device *dev = rockchip->dev; > >> + int ret; > >> + > >> + if (!rockchip->ep_gpio) > >> + return 0; > >> + > >> + /* PCIe reset interrupt */ > >> + ep->perst_irq = gpiod_to_irq(rockchip->ep_gpio); > >> + if (ep->perst_irq < 0) { > >> + dev_err(dev, "No corresponding IRQ for PERST GPIO\n"); > >> + return ep->perst_irq; > >> + } > >> + > >> + ep->perst_asserted = true; > > > > How come? > > Yeah, a bit confusing. This is because the gpio active low / inactive high, so > as soon as we enable the IRQ, we are going to get one IRQ even though perst gpio > signal has not changed yet. Which means you are looking for a wrong level! What is the polarity of the PERST# gpio in DT? - Mani
On 10/12/24 21:31, Manivannan Sadhasivam wrote: > On Fri, Oct 11, 2024 at 06:30:31PM +0900, Damien Le Moal wrote: >> On 10/10/24 19:49, Manivannan Sadhasivam wrote: >>>> +static int rockchip_pcie_ep_setup_irq(struct pci_epc *epc) >>>> +{ >>>> + struct rockchip_pcie_ep *ep = epc_get_drvdata(epc); >>>> + struct rockchip_pcie *rockchip = &ep->rockchip; >>>> + struct device *dev = rockchip->dev; >>>> + int ret; >>>> + >>>> + if (!rockchip->ep_gpio) >>>> + return 0; >>>> + >>>> + /* PCIe reset interrupt */ >>>> + ep->perst_irq = gpiod_to_irq(rockchip->ep_gpio); >>>> + if (ep->perst_irq < 0) { >>>> + dev_err(dev, "No corresponding IRQ for PERST GPIO\n"); >>>> + return ep->perst_irq; >>>> + } >>>> + >>>> + ep->perst_asserted = true; >>> >>> How come? >> >> Yeah, a bit confusing. This is because the gpio active low / inactive high, so >> as soon as we enable the IRQ, we are going to get one IRQ even though perst gpio >> signal has not changed yet. > > Which means you are looking for a wrong level! What is the polarity of the > PERST# gpio in DT? It is not defined in the default DT with the kernel. I added an overlay file to define it together with the EP mode. And as I said above, the gpio is active low. If I reverse that to active high, it does not work.
diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c index af50432525b4..c70a64c37a56 100644 --- a/drivers/pci/controller/pcie-rockchip-ep.c +++ b/drivers/pci/controller/pcie-rockchip-ep.c @@ -18,6 +18,7 @@ #include <linux/sizes.h> #include <linux/workqueue.h> #include <linux/iopoll.h> +#include <linux/gpio/consumer.h> #include "pcie-rockchip.h" @@ -50,6 +51,9 @@ struct rockchip_pcie_ep { u64 irq_pci_addr; u8 irq_pci_fn; u8 irq_pending; + int perst_irq; + bool perst_asserted; + bool link_up; struct delayed_work link_training; }; @@ -462,13 +466,17 @@ static int rockchip_pcie_ep_start(struct pci_epc *epc) rockchip_pcie_write(rockchip, cfg, PCIE_CORE_PHY_FUNC_CFG); + if (rockchip->ep_gpio) + enable_irq(ep->perst_irq); + /* Enable configuration and start link training */ rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE | PCIE_CLIENT_CONF_ENABLE, PCIE_CLIENT_CONFIG); - schedule_delayed_work(&ep->link_training, 0); + if (!rockchip->ep_gpio) + schedule_delayed_work(&ep->link_training, 0); return 0; } @@ -478,6 +486,11 @@ static void rockchip_pcie_ep_stop(struct pci_epc *epc) struct rockchip_pcie_ep *ep = epc_get_drvdata(epc); struct rockchip_pcie *rockchip = &ep->rockchip; + if (rockchip->ep_gpio) { + ep->perst_asserted = true; + disable_irq(ep->perst_irq); + } + cancel_delayed_work_sync(&ep->link_training); /* Stop link training and disable configuration */ @@ -540,6 +553,13 @@ static void rockchip_pcie_ep_link_training(struct work_struct *work) if (!rockchip_pcie_ep_link_up(rockchip)) goto again; + /* + * If PERST was asserted while polling the link, do not notify + * the function. + */ + if (ep->perst_asserted) + return; + val = rockchip_pcie_read(rockchip, PCIE_CLIENT_BASIC_STATUS0); dev_info(dev, "Link UP (Negociated speed: %sGT/s, width: x%lu)\n", @@ -549,6 +569,7 @@ static void rockchip_pcie_ep_link_training(struct work_struct *work) /* Notify the function */ pci_epc_linkup(ep->epc); + ep->link_up = true; return; @@ -556,6 +577,94 @@ static void rockchip_pcie_ep_link_training(struct work_struct *work) schedule_delayed_work(&ep->link_training, msecs_to_jiffies(5)); } +static void rockchip_pcie_ep_perst_assert(struct rockchip_pcie_ep *ep) +{ + struct rockchip_pcie *rockchip = &ep->rockchip; + struct device *dev = rockchip->dev; + + dev_dbg(dev, "PERST asserted, link down\n"); + + if (ep->perst_asserted) + return; + + ep->perst_asserted = true; + + cancel_delayed_work_sync(&ep->link_training); + + if (ep->link_up) { + pci_epc_linkdown(ep->epc); + ep->link_up = false; + } +} + +static void rockchip_pcie_ep_perst_deassert(struct rockchip_pcie_ep *ep) +{ + struct rockchip_pcie *rockchip = &ep->rockchip; + struct device *dev = rockchip->dev; + + dev_dbg(dev, "PERST de-asserted, starting link training\n"); + + if (!ep->perst_asserted) + return; + + ep->perst_asserted = false; + + /* Enable link re-training */ + rockchip_pcie_ep_retrain_link(rockchip); + + /* Start link training */ + schedule_delayed_work(&ep->link_training, 0); +} + +static irqreturn_t rockchip_pcie_ep_perst_irq_thread(int irq, void *data) +{ + struct pci_epc *epc = data; + struct rockchip_pcie_ep *ep = epc_get_drvdata(epc); + struct rockchip_pcie *rockchip = &ep->rockchip; + u32 perst = gpiod_get_value(rockchip->ep_gpio); + + if (perst) + rockchip_pcie_ep_perst_assert(ep); + else + rockchip_pcie_ep_perst_deassert(ep); + + irq_set_irq_type(ep->perst_irq, + (perst ? IRQF_TRIGGER_HIGH : IRQF_TRIGGER_LOW)); + + return IRQ_HANDLED; +} + +static int rockchip_pcie_ep_setup_irq(struct pci_epc *epc) +{ + struct rockchip_pcie_ep *ep = epc_get_drvdata(epc); + struct rockchip_pcie *rockchip = &ep->rockchip; + struct device *dev = rockchip->dev; + int ret; + + if (!rockchip->ep_gpio) + return 0; + + /* PCIe reset interrupt */ + ep->perst_irq = gpiod_to_irq(rockchip->ep_gpio); + if (ep->perst_irq < 0) { + dev_err(dev, "No corresponding IRQ for PERST GPIO\n"); + return ep->perst_irq; + } + + ep->perst_asserted = true; + irq_set_status_flags(ep->perst_irq, IRQ_NOAUTOEN); + ret = devm_request_threaded_irq(dev, ep->perst_irq, NULL, + rockchip_pcie_ep_perst_irq_thread, + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, + "pcie-ep-perst", epc); + if (ret) { + dev_err(dev, "Request PERST GPIO IRQ failed %d\n", ret); + return ret; + } + + return 0; +} + static const struct pci_epc_features rockchip_pcie_epc_features = { .linkup_notifier = true, .msi_capable = true, @@ -719,6 +828,7 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev) rockchip->is_rc = false; rockchip->dev = dev; INIT_DELAYED_WORK(&ep->link_training, rockchip_pcie_ep_link_training); + ep->link_up = false; epc = devm_pci_epc_create(dev, &rockchip_pcie_epc_ops); if (IS_ERR(epc)) { @@ -751,7 +861,13 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev) pci_epc_init_notify(epc); + err = rockchip_pcie_ep_setup_irq(epc); + if (err < 0) + goto err_uninit_port; + return 0; +err_uninit_port: + rockchip_pcie_deinit_phys(rockchip); err_release_resources: rockchip_pcie_ep_release_resources(ep); err_disable_clocks: diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c index 154e78819e6e..bcb1c9266c56 100644 --- a/drivers/pci/controller/pcie-rockchip.c +++ b/drivers/pci/controller/pcie-rockchip.c @@ -119,13 +119,11 @@ int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip) return PTR_ERR(rockchip->aclk_rst); } - if (rockchip->is_rc) { - rockchip->ep_gpio = devm_gpiod_get_optional(dev, "ep", - GPIOD_OUT_LOW); - if (IS_ERR(rockchip->ep_gpio)) - return dev_err_probe(dev, PTR_ERR(rockchip->ep_gpio), - "failed to get ep GPIO\n"); - } + rockchip->ep_gpio = devm_gpiod_get_optional(dev, "ep", + rockchip->is_rc ? GPIOD_OUT_LOW : GPIOD_IN); + if (IS_ERR(rockchip->ep_gpio)) + return dev_err_probe(dev, PTR_ERR(rockchip->ep_gpio), + "failed to get ep GPIO\n"); rockchip->aclk_pcie = devm_clk_get(dev, "aclk"); if (IS_ERR(rockchip->aclk_pcie)) {
Currently, the Rockchip PCIe endpoint controller driver does not handle PERST# signal, which prevents detecting when link training should actually be started or if the host reset the device. This however can be supported using the controller ep_gpio, set as an input GPIO for endpoint mode. Modify the endpoint rockchip driver to get the ep_gpio and its associated interrupt which is serviced using a threaded IRQ with the function rockchip_pcie_ep_perst_irq_thread() as handler. This handler function notifies a link down event corresponding to the RC side asserting the PERST# signal using pci_epc_linkdown() when the gpio is high. Once the gpio value goes down, corresponding to the RC de-asserting the PERST# signal, link training is started. The polarity of the gpio interrupt trigger is changed from high to low after the RC asserted PERST#, and conversely changed from low to high after the RC de-asserts PERST#. Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/pci/controller/pcie-rockchip-ep.c | 118 +++++++++++++++++++++- drivers/pci/controller/pcie-rockchip.c | 12 +-- 2 files changed, 122 insertions(+), 8 deletions(-)