On Thu, Jun 06, 2024 at 12:01:28PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Jun 05, 2024 at 08:58:06PM +0200, Niklas Cassel wrote:
> > On Wed, Jun 05, 2024 at 01:47:53PM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, May 29, 2024 at 10:29:04AM +0200, Niklas Cassel wrote:
> > > > The PCIe controller in rk3568 and rk3588 can operate in endpoint mode.
> > > > This endpoint mode support heavily leverages the existing code in
> > > > pcie-designware-ep.c.
> > > >
> > > > Add support for endpoint mode to the existing pcie-dw-rockchip glue
> > > > driver.
> > > >
> > > > Signed-off-by: Niklas Cassel <cassel@xxxxxxxxxx>
> > >
> > > Couple of comments below. With those addressed,
> > >
> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > >
> > > > ---
> > > > drivers/pci/controller/dwc/Kconfig | 17 ++-
> > > > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 210 ++++++++++++++++++++++++++
> > > > 2 files changed, 224 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > > > index 8afacc90c63b..9fae0d977271 100644
> > > > --- a/drivers/pci/controller/dwc/Kconfig
> > > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > > @@ -311,16 +311,27 @@ config PCIE_RCAR_GEN4_EP
> > > > SoCs. To compile this driver as a module, choose M here: the module
> > > > will be called pcie-rcar-gen4.ko. This uses the DesignWare core.
> > > >
> > > > +config PCIE_ROCKCHIP_DW
> > > > + bool
> > >
> > > Where is this symbol used?
> >
> > It is supposed to be used by
> > drivers/pci/controller/dwc/Makefile
> >
> > such that the driver is compiled if either _EP or _HOST is selected, just
> > like how it is done for other drivers that support both in the same driver.
> > Looks like I missed to update Makefile...
> > Good catch, thank you!
> >
> >
> > > > +static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
> > > > +{
> > > > + struct rockchip_pcie *rockchip = arg;
> > > > + struct dw_pcie *pci = &rockchip->pci;
> > > > + struct device *dev = pci->dev;
> > > > + u32 reg, val;
> > > > +
> > > > + reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_MISC);
> > > > +
> > > > + dev_dbg(dev, "PCIE_CLIENT_INTR_STATUS_MISC: %#x\n", reg);
> > > > + dev_dbg(dev, "LTSSM_STATUS: %#x\n", rockchip_pcie_get_ltssm(rockchip));
> > > > +
> > > > + if (reg & PCIE_LINK_REQ_RST_NOT_INT) {
> > > > + dev_dbg(dev, "hot reset or link-down reset\n");
> > > > + dw_pcie_ep_linkdown(&pci->ep);
> > > > + }
> > > > +
> > > > + if (reg & PCIE_RDLH_LINK_UP_CHGED) {
> > > > + val = rockchip_pcie_get_ltssm(rockchip);
> > > > + if ((val & PCIE_LINKUP) == PCIE_LINKUP) {
> > > > + dev_dbg(dev, "link up\n");
> > > > + dw_pcie_ep_linkup(&pci->ep);
> > > > + }
> > > > + }
> > > > +
> > > > + rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC);
> > >
> > > It is recommended to clear the IRQs at the start of the handler (after status
> > > read).
> >
> > Can you quote a reference in the databook to back this recommendation?
> >
>
> It is just a general recommendation.
>
> > Otherwise I would lean towards keeping it like it is, since this is how
> > it looks in the downstream driver (that *should* be well proven), and it
> > also matches how it's done in dra7xx.
> >
> > (And since you ack only the events you read, you can not accidentally
> > clear another type of event.)
> >
>
> I haven't read the TRM, but if the IRQ line is level triggered, then if you do
> not clear the IRQs immediately, you will miss some events. So I always suggest
> to clear the IRQs at the start of the handler for all the platforms.
They are level triggered.
In this specific case, what could happen is that we fail to trigger an IRQ
if we get two succeeding hot/link-down resets, or two succeding link ups.
In neither case is this a serious case (compared to e.g. a host driver
missing a MSI irq), but I have incorporated your suggestion for v5,
thank you!
Kind regards,
Niklas
