On Fri, Jul 14, 2023 at 02:01:16AM +0000, Yoshihiro Shimoda wrote:
> Hi,
>
> > From: Serge Semin, Sent: Wednesday, July 12, 2023 5:32 AM
> >
> > On Wed, Jul 05, 2023 at 08:41:53PM +0900, Yoshihiro Shimoda wrote:
> > > The commit 24ede430fa49 ("PCI: designware-ep: Add multiple PFs support
> > > for DWC") added .func_conf_select() to get the configuration space of
> > > different PFs and assumed that the offsets between dbi and dbi would
^
|
s/dbi/dbi2 -------------------------------------------------------+
> > > be the same. However, Renesas R-Car Gen4 PCIe controllers have different
> > > offsets of function 1: dbi (+0x1000) and dbi2 (+0x800). To get
> > > the offset for dbi2, add .func_conf_select2() and
> > > dw_pcie_ep_func_select2().
> > >
> > > Notes that dw_pcie_ep_func_select2() will call .func_conf_select()
> > > if .func_conf_select2() doesn't exist for backward compatibility.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> > > ---
> > > .../pci/controller/dwc/pcie-designware-ep.c | 32 ++++++++++++++-----
> > > drivers/pci/controller/dwc/pcie-designware.h | 3 +-
> > > 2 files changed, 26 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > index 1d24ebf9686f..bd57516d5313 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > @@ -54,21 +54,35 @@ static unsigned int dw_pcie_ep_func_select(struct dw_pcie_ep *ep, u8 func_no)
> > > return func_offset;
> > > }
> > >
> > > +static unsigned int dw_pcie_ep_func_select2(struct dw_pcie_ep *ep, u8 func_no)
> > > +{
> > > + unsigned int func_offset = 0;
> > > +
> > > + if (ep->ops->func_conf_select2)
> > > + func_offset = ep->ops->func_conf_select2(ep, func_no);
> > > + else if (ep->ops->func_conf_select) /* for backward compatibility */
> > > + func_offset = ep->ops->func_conf_select(ep, func_no);
> > > +
> > > + return func_offset;
> > > +}
> > > +
> > > static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, u8 func_no,
> > > enum pci_barno bar, int flags)
> > > {
> > > - u32 reg;
> > > - unsigned int func_offset = 0;
> > > + u32 reg, reg_dbi2;
> > > + unsigned int func_offset, func_offset_dbi2;
> > > struct dw_pcie_ep *ep = &pci->ep;
> > >
> > > func_offset = dw_pcie_ep_func_select(ep, func_no);
> > > + func_offset_dbi2 = dw_pcie_ep_func_select2(ep, func_no);
> >
> > IMO this will make the code even more complicated than it's already
> > with the offsets calculated and added here and there. What about
> > implementing a set of methods like this:
> >
> > +static void dw_pcie_ep_writeX_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg, uYZ val)
> > +{
> > + unsigned int ofs = dw_pcie_ep_func_select(ep, func_no);
> > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +
> > + dw_pcie_writeX_dbi(pci, reg + ofs, val);
> > +}
> > +
> > +static uYZ dw_pcie_ep_readX_dbi(struct dw_pcie_ep *ep, u8 func_no, u32 reg)
> > +{
> > + unsigned int ofs = dw_pcie_ep_func_select(ep, func_no);
> > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +
> > + return dw_pcie_readX_dbi(pci, reg + ofs);
> > +}
> >
> > and converting the entire DW PCIe EP core driver to using them instead
> > of always separately calculating the func_offset? Then in a subsequent
> > patch you can add a new method like this:
> >
> > +static void dw_pcie_ep_writel_dbi2(struct dw_pcie_ep *ep, u8 func_no, u32 reg, u32 val)
> > +{
> > + unsigned int ofs = dw_pcie_ep_func_select2(ep, func_no);
> > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +
> > + dw_pcie_writel_dbi2(pci, reg + ofs, val);
> > +}
> >
> > and have it utilized in the shadow registers update parts as you
> > originally intended. This will make the code much better readable with
> > no much harm to the performance since the most of setups are performed
> > once during the initial end-point configuration.
> >
> > Note my suggestion is quite heavy to implement and implies the code
> > cleanup. So I'd wait for the maintainers comment about this (Mani is
> > now responsible for the driver maintaining).
> > Mani, Krzysztof, Lorenzo, Rob, what do you think about that?
>
> To be honest, if possible, I would like to implement such clean up code
> after this patch series are applied (because this patch set had been
> developed for a year and more...).
I would be glad to review it then. Without the suggested modification
you have no other choice but to implement the change as is. No
objections against this patch from my side then.
Regarding the review timing. I fully understand your feelings. From my
experience the bigger series and the more various changes it implies,
the longer it takes to review especially if the corresponding driver
has no active maintainership. The later has been relevant to the DW
PCIe/eDMA core drivers up to recent Mani' assignment. Some of my even
smaller patchsets were even longer time in the review limbo. My very
first series was under review for about one year. I had to fully
rewrite it several times. It was very much irritating indeed. So the
kernel contributors must have a great deal of patience sometime in
order to finally have their patches taken. That gets to be even more
actual seeing the requirements may differ from subsystem to subsystem,
sometimes from maintainer to maintainer.
-Serge(y)
>
> Best regards,
> Yoshihiro Shimoda
>
> > -Serge(y)
> >
> > >
> > > reg = func_offset + PCI_BASE_ADDRESS_0 + (4 * bar);
> > > + reg_dbi2 = func_offset_dbi2 + PCI_BASE_ADDRESS_0 + (4 * bar);
> > > dw_pcie_dbi_ro_wr_en(pci);
> > > - dw_pcie_writel_dbi2(pci, reg, 0x0);
> > > + dw_pcie_writel_dbi2(pci, reg_dbi2, 0x0);
> > > dw_pcie_writel_dbi(pci, reg, 0x0);
> > > if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > > - dw_pcie_writel_dbi2(pci, reg + 4, 0x0);
> > > + dw_pcie_writel_dbi2(pci, reg_dbi2 + 4, 0x0);
> > > dw_pcie_writel_dbi(pci, reg + 4, 0x0);
> > > }
> > > dw_pcie_dbi_ro_wr_dis(pci);
> > > @@ -232,13 +246,15 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> > > enum pci_barno bar = epf_bar->barno;
> > > size_t size = epf_bar->size;
> > > int flags = epf_bar->flags;
> > > - unsigned int func_offset = 0;
> > > + unsigned int func_offset, func_offset_dbi2;
> > > int ret, type;
> > > - u32 reg;
> > > + u32 reg, reg_dbi2;
> > >
> > > func_offset = dw_pcie_ep_func_select(ep, func_no);
> > > + func_offset_dbi2 = dw_pcie_ep_func_select2(ep, func_no);
> > >
> > > reg = PCI_BASE_ADDRESS_0 + (4 * bar) + func_offset;
> > > + reg_dbi2 = PCI_BASE_ADDRESS_0 + (4 * bar) + func_offset_dbi2;
> > >
> > > if (!(flags & PCI_BASE_ADDRESS_SPACE))
> > > type = PCIE_ATU_TYPE_MEM;
> > > @@ -254,11 +270,11 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> > >
> > > dw_pcie_dbi_ro_wr_en(pci);
> > >
> > > - dw_pcie_writel_dbi2(pci, reg, lower_32_bits(size - 1));
> > > + dw_pcie_writel_dbi2(pci, reg_dbi2, lower_32_bits(size - 1));
> > > dw_pcie_writel_dbi(pci, reg, flags);
> > >
> > > if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > > - dw_pcie_writel_dbi2(pci, reg + 4, upper_32_bits(size - 1));
> > > + dw_pcie_writel_dbi2(pci, reg_dbi2 + 4, upper_32_bits(size - 1));
> > > dw_pcie_writel_dbi(pci, reg + 4, 0);
> > > }
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > index 812c221b3f7c..94bc20f5f600 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > @@ -340,9 +340,10 @@ struct dw_pcie_ep_ops {
> > > * access for different platform, if different func have different
> > > * offset, return the offset of func. if use write a register way
> > > * return a 0, and implement code in callback function of platform
> > > - * driver.
> > > + * driver. The func_conf_select2 is for dbi2.
> > > */
> > > unsigned int (*func_conf_select)(struct dw_pcie_ep *ep, u8 func_no);
> > > + unsigned int (*func_conf_select2)(struct dw_pcie_ep *ep, u8 func_no);
> > > };
> > >
> > > struct dw_pcie_ep_func {
> > > --
> > > 2.25.1
> > >
