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?
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.)
Kind regards,
Niklas
