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. - Mani -- மணிவண்ணன் சதாசிவம்