Re: [PATCH v4 10/13] PCI: dw-rockchip: Add endpoint mode support

[Manivannan Sadhasivam <mani@kernel.org>]

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@kernel.org>
> > 
> > Couple of comments below. With those addressed,
> > 
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > > ---
> > >  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

-- 
மணிவண்ணன் சதாசிவம்