| Message ID | 20230823091153.2578417-6-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: rcar-gen4: Add R-Car Gen4 PCIe support | expand |
On Wed, Aug 23, 2023 at 06:11:39PM +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 dbi2 would > 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 .get_dbi2_offset() and > dw_pcie_ep_get_dbi2_offset(). > > Note: > - .func_conf_select() should be renamed later. That's the problem with half-measured solutions. The code now looks clumsy and thus weakly readable. > - dw_pcie_ep_get_dbi2_offset() will call .func_conf_select() > if .get_dbi2_offset() doesn't exist for backward compatibility. > - dw_pcie_writeX_{dbi/dbi2} APIs accepted the func_no argument, > so that these offset calculations are contained in the API > definitions itself as it should. I very much hope you'll get to do that after this patchset is merged in.) -Serge(y) > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > .../pci/controller/dwc/pcie-designware-ep.c | 32 ++++++++++++++----- > drivers/pci/controller/dwc/pcie-designware.h | 1 + > 2 files changed, 25 insertions(+), 8 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > index 4a8c116cdd4b..500b092984b0 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_get_dbi2_offset(struct dw_pcie_ep *ep, u8 func_no) > +{ > + unsigned int dbi2_offset = 0; > + > + if (ep->ops->get_dbi2_offset) > + dbi2_offset = ep->ops->get_dbi2_offset(ep, func_no); > + else if (ep->ops->func_conf_select) /* for backward compatibility */ > + dbi2_offset = ep->ops->func_conf_select(ep, func_no); > + > + return dbi2_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; > + unsigned int func_offset, dbi2_offset; > struct dw_pcie_ep *ep = &pci->ep; > + u32 reg, reg_dbi2; > > func_offset = dw_pcie_ep_func_select(ep, func_no); > + dbi2_offset = dw_pcie_ep_get_dbi2_offset(ep, func_no); > > reg = func_offset + PCI_BASE_ADDRESS_0 + (4 * bar); > + reg_dbi2 = dbi2_offset + 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); > @@ -229,16 +243,18 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, > { > struct dw_pcie_ep *ep = epc_get_drvdata(epc); > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > + unsigned int func_offset, dbi2_offset; > enum pci_barno bar = epf_bar->barno; > size_t size = epf_bar->size; > int flags = epf_bar->flags; > - unsigned int func_offset = 0; > + u32 reg, reg_dbi2; > int ret, type; > - u32 reg; > > func_offset = dw_pcie_ep_func_select(ep, func_no); > + dbi2_offset = dw_pcie_ep_get_dbi2_offset(ep, func_no); > > reg = PCI_BASE_ADDRESS_0 + (4 * bar) + func_offset; > + reg_dbi2 = PCI_BASE_ADDRESS_0 + (4 * bar) + dbi2_offset; > > 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 c17e5255fab6..7d02401fc783 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -343,6 +343,7 @@ struct dw_pcie_ep_ops { > * driver. > */ > unsigned int (*func_conf_select)(struct dw_pcie_ep *ep, u8 func_no); > + unsigned int (*get_dbi2_offset)(struct dw_pcie_ep *ep, u8 func_no); > }; > > struct dw_pcie_ep_func { > -- > 2.25.1 >
Hello Serge, > From: Serge Semin, Sent: Wednesday, August 23, 2023 7:46 PM > > On Wed, Aug 23, 2023 at 06:11:39PM +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 dbi2 would > > 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 .get_dbi2_offset() and > > dw_pcie_ep_get_dbi2_offset(). > > > > Note: > > > - .func_conf_select() should be renamed later. > > That's the problem with half-measured solutions. The code now looks > clumsy and thus weakly readable. Mani suggested that on v18 [1]. That's why I didn't rename the .func_conf_select() on this patchset. [1] https://lore.kernel.org/linux-pci/20230728023444.GA4433@thinkpad/ > > - dw_pcie_ep_get_dbi2_offset() will call .func_conf_select() > > if .get_dbi2_offset() doesn't exist for backward compatibility. > > > - dw_pcie_writeX_{dbi/dbi2} APIs accepted the func_no argument, > > so that these offset calculations are contained in the API > > definitions itself as it should. > > I very much hope you'll get to do that after this patchset is merged > in.) Yes, I'll do that after this patchset is merged in. Best regards, Yoshihiro Shimoda > -Serge(y) > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > .../pci/controller/dwc/pcie-designware-ep.c | 32 ++++++++++++++----- > > drivers/pci/controller/dwc/pcie-designware.h | 1 + > > 2 files changed, 25 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > > index 4a8c116cdd4b..500b092984b0 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_get_dbi2_offset(struct dw_pcie_ep *ep, u8 func_no) > > +{ > > + unsigned int dbi2_offset = 0; > > + > > + if (ep->ops->get_dbi2_offset) > > + dbi2_offset = ep->ops->get_dbi2_offset(ep, func_no); > > + else if (ep->ops->func_conf_select) /* for backward compatibility */ > > + dbi2_offset = ep->ops->func_conf_select(ep, func_no); > > + > > + return dbi2_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; > > + unsigned int func_offset, dbi2_offset; > > struct dw_pcie_ep *ep = &pci->ep; > > + u32 reg, reg_dbi2; > > > > func_offset = dw_pcie_ep_func_select(ep, func_no); > > + dbi2_offset = dw_pcie_ep_get_dbi2_offset(ep, func_no); > > > > reg = func_offset + PCI_BASE_ADDRESS_0 + (4 * bar); > > + reg_dbi2 = dbi2_offset + 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); > > @@ -229,16 +243,18 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, > > { > > struct dw_pcie_ep *ep = epc_get_drvdata(epc); > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > + unsigned int func_offset, dbi2_offset; > > enum pci_barno bar = epf_bar->barno; > > size_t size = epf_bar->size; > > int flags = epf_bar->flags; > > - unsigned int func_offset = 0; > > + u32 reg, reg_dbi2; > > int ret, type; > > - u32 reg; > > > > func_offset = dw_pcie_ep_func_select(ep, func_no); > > + dbi2_offset = dw_pcie_ep_get_dbi2_offset(ep, func_no); > > > > reg = PCI_BASE_ADDRESS_0 + (4 * bar) + func_offset; > > + reg_dbi2 = PCI_BASE_ADDRESS_0 + (4 * bar) + dbi2_offset; > > > > 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 c17e5255fab6..7d02401fc783 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware.h > > +++ b/drivers/pci/controller/dwc/pcie-designware.h > > @@ -343,6 +343,7 @@ struct dw_pcie_ep_ops { > > * driver. > > */ > > unsigned int (*func_conf_select)(struct dw_pcie_ep *ep, u8 func_no); > > + unsigned int (*get_dbi2_offset)(struct dw_pcie_ep *ep, u8 func_no); > > }; > > > > struct dw_pcie_ep_func { > > -- > > 2.25.1 > >
diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c index 4a8c116cdd4b..500b092984b0 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_get_dbi2_offset(struct dw_pcie_ep *ep, u8 func_no) +{ + unsigned int dbi2_offset = 0; + + if (ep->ops->get_dbi2_offset) + dbi2_offset = ep->ops->get_dbi2_offset(ep, func_no); + else if (ep->ops->func_conf_select) /* for backward compatibility */ + dbi2_offset = ep->ops->func_conf_select(ep, func_no); + + return dbi2_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; + unsigned int func_offset, dbi2_offset; struct dw_pcie_ep *ep = &pci->ep; + u32 reg, reg_dbi2; func_offset = dw_pcie_ep_func_select(ep, func_no); + dbi2_offset = dw_pcie_ep_get_dbi2_offset(ep, func_no); reg = func_offset + PCI_BASE_ADDRESS_0 + (4 * bar); + reg_dbi2 = dbi2_offset + 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); @@ -229,16 +243,18 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, { struct dw_pcie_ep *ep = epc_get_drvdata(epc); struct dw_pcie *pci = to_dw_pcie_from_ep(ep); + unsigned int func_offset, dbi2_offset; enum pci_barno bar = epf_bar->barno; size_t size = epf_bar->size; int flags = epf_bar->flags; - unsigned int func_offset = 0; + u32 reg, reg_dbi2; int ret, type; - u32 reg; func_offset = dw_pcie_ep_func_select(ep, func_no); + dbi2_offset = dw_pcie_ep_get_dbi2_offset(ep, func_no); reg = PCI_BASE_ADDRESS_0 + (4 * bar) + func_offset; + reg_dbi2 = PCI_BASE_ADDRESS_0 + (4 * bar) + dbi2_offset; 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 c17e5255fab6..7d02401fc783 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -343,6 +343,7 @@ struct dw_pcie_ep_ops { * driver. */ unsigned int (*func_conf_select)(struct dw_pcie_ep *ep, u8 func_no); + unsigned int (*get_dbi2_offset)(struct dw_pcie_ep *ep, u8 func_no); }; struct dw_pcie_ep_func {
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 dbi2 would 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 .get_dbi2_offset() and dw_pcie_ep_get_dbi2_offset(). Note: - .func_conf_select() should be renamed later. - dw_pcie_ep_get_dbi2_offset() will call .func_conf_select() if .get_dbi2_offset() doesn't exist for backward compatibility. - dw_pcie_writeX_{dbi/dbi2} APIs accepted the func_no argument, so that these offset calculations are contained in the API definitions itself as it should. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- .../pci/controller/dwc/pcie-designware-ep.c | 32 ++++++++++++++----- drivers/pci/controller/dwc/pcie-designware.h | 1 + 2 files changed, 25 insertions(+), 8 deletions(-)