[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a3462c68-ec1b-0b1a-fee7-612bd1109819@linux.intel.com>
Date: Mon, 24 Mar 2025 17:02:28 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Hans Zhang <18255117159@....com>
cc: lpieralisi@...nel.org, kw@...ux.com, manivannan.sadhasivam@...aro.org,
robh@...nel.org, bhelgaas@...gle.com, jingoohan1@...il.com,
thomas.richard@...tlin.com, linux-pci@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for
finding the capabilities
On Mon, 24 Mar 2025, Hans Zhang wrote:
> On 2025/3/24 21:44, Ilpo Järvinen wrote:
> > On Mon, 24 Mar 2025, Hans Zhang wrote:
> >
> > > Since the PCI core is now exposing generic APIs for the host bridges to
> >
> > No need to say "since ... is now exposing". Just say "Use ..." as if the
> > API has always existed even if you just added it.
> >
>
> Hi Ilpo,
>
> Thanks your for reply. Will change.
>
>
> > > search for the PCIe capabilities, make use of them in the CDNS driver.
> > >
> > > Signed-off-by: Hans Zhang <18255117159@....com>
> > > ---
> > > Changes since v5:
> > > https://lore.kernel.org/linux-pci/20250321163803.391056-4-18255117159@163.com
> > >
> > > - Kconfig add "select PCI_HOST_HELPERS"
> > > ---
> > > drivers/pci/controller/cadence/Kconfig | 1 +
> > > drivers/pci/controller/cadence/pcie-cadence.c | 25 +++++++++++++++++++
> > > drivers/pci/controller/cadence/pcie-cadence.h | 3 +++
> > > 3 files changed, 29 insertions(+)
> > >
> > > diff --git a/drivers/pci/controller/cadence/Kconfig
> > > b/drivers/pci/controller/cadence/Kconfig
> > > index 8a0044bb3989..0a4f245bbeb0 100644
> > > --- a/drivers/pci/controller/cadence/Kconfig
> > > +++ b/drivers/pci/controller/cadence/Kconfig
> > > @@ -5,6 +5,7 @@ menu "Cadence-based PCIe controllers"
> > > config PCIE_CADENCE
> > > bool
> > > + select PCI_HOST_HELPERS
> > > config PCIE_CADENCE_HOST
> > > bool
> > > diff --git a/drivers/pci/controller/cadence/pcie-cadence.c
> > > b/drivers/pci/controller/cadence/pcie-cadence.c
> > > index 204e045aed8c..329dab4ff813 100644
> > > --- a/drivers/pci/controller/cadence/pcie-cadence.c
> > > +++ b/drivers/pci/controller/cadence/pcie-cadence.c
> > > @@ -8,6 +8,31 @@
> > > #include "pcie-cadence.h"
> > > +static u32 cdns_pcie_read_cfg(void *priv, int where, int size)
> > > +{
> > > + struct cdns_pcie *pcie = priv;
> > > + u32 val;
> > > +
> > > + if (size == 4)
> > > + val = readl(pcie->reg_base + where);
> >
> > Should this use cdns_pcie_readl() ?
>
> pci_host_bridge_find_*capability required to read two or four bytes.
>
> reg = read_cfg(priv, cap_ptr, 2);
> or
> header = read_cfg(priv, pos, 4);
>
> Here I mainly want to write it the same way as size == 2 and size == 1.
> Or size == 4 should I write it as cdns_pcie_readl() ?
As is, it seems two functions are added for the same thing for the case
with size == 4 with different names which feels duplication. One could add
cdns_pcie_readw() and cdns_pcie_readb() too but perhaps cdns_pcie_readl()
should just call this new function instead?
> > > + else if (size == 2)
> > > + val = readw(pcie->reg_base + where);
> > > + else if (size == 1)
> > > + val = readb(pcie->reg_base + where);
> > > +
> > > + return val;
> > > +}
> > > +
> > > +u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap)
> > > +{
> > > + return pci_host_bridge_find_capability(pcie, cdns_pcie_read_cfg, cap);
> > > +}
> > > +
> > > +u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
> > > +{
> > > + return pci_host_bridge_find_ext_capability(pcie, cdns_pcie_read_cfg,
> > > cap);
> > > +}
> >
> > I'm really wondering why the read config function is provided directly as
> > an argument. Shouldn't struct pci_host_bridge have some ops that can read
> > config so wouldn't it make much more sense to pass it and use the func
> > from there? There seems to ops in pci_host_bridge that has read(), does
> > that work? If not, why?
> >
>
> No effect.
I'm not sure what you meant?
> Because we need to get the offset of the capability before PCIe
> enumerates the device.
Is this to say it is needed before the struct pci_host_bridge is created?
> I originally added a separate find capability related
> function for CDNS in the following patch. It's also copied directly from DWC.
> Mani felt there was too much duplicate code and also suggested passing a
> callback function that could manipulate the registers of the root port of DWC
> or CDNS.
I very much like the direction this patchset is moving (moving shared
part of controllers code to core), I just feel this doesn't go far enough
when it's passing function pointer to the read function.
I admit I've never written a controller driver so perhaps there's
something detail I lack knowledge of but I'd want to understand why
struct pci_ops (which exists both in pci_host_bridge and pci_bus) cannot
be used?
> https://patchwork.kernel.org/project/linux-pci/patch/20250308133903.322216-1-18255117159@163.com/
>
> The original function is in the following file:
> drivers/pci/controller/dwc/pcie-designware.c
> u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
> u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap)
>
> CDNS has the same need to find the offset of the capability.
>
> > > +
> > > void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie)
> > > {
> > > u32 delay = 0x3;
> > > diff --git a/drivers/pci/controller/cadence/pcie-cadence.h
> > > b/drivers/pci/controller/cadence/pcie-cadence.h
> > > index f5eeff834ec1..6f4981fccb94 100644
> > > --- a/drivers/pci/controller/cadence/pcie-cadence.h
> > > +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> > > @@ -557,6 +557,9 @@ static inline int cdns_pcie_ep_setup(struct
> > > cdns_pcie_ep *ep)
> > > }
> > > #endif
> > > +u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap);
> > > +u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap);
> > > +
> > > void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie);
> > > void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, u8 busnr,
> > > u8 fn,
> > >
> >
>
> Best regards,
> Hans
>
--
i.
Powered by blists - more mailing lists