| Message ID | 20230721074452.65545-8-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | PCI: rcar-gen4: Add R-Car Gen4 PCIe support | expand |
On Fri, Jul 21, 2023 at 04:44: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 .func_conf_select2() and > dw_pcie_ep_func_select2(). > How about, .get_dbi2_offset() and dw_pcie_ep_get_dbi2_offset()? This would've been much simpler if 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. Then the APIs could just do "func_offset * func_no" to get DBI base and "(func_offset * func_no) + dbi2_offset" to get DBI2 base, provided these offsets are passed by the vendor drivers. It can be done in a separate cleanup series later. > Notes that dw_pcie_ep_func_select2() will call .func_conf_select() s/Notes/Note > if .func_conf_select2() doesn't exist for backward compatibility. > > 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 | 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; Please maitain reverse Xmas tree order. - Mani > 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); > > 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 >
Hi Manivannan, > From: Manivannan Sadhasivam, Sent: Monday, July 24, 2023 6:25 PM > > On Fri, Jul 21, 2023 at 04:44: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 .func_conf_select2() and > > dw_pcie_ep_func_select2(). > > > > How about, > > .get_dbi2_offset() and dw_pcie_ep_get_dbi2_offset()? Thank you for your suggestion. I should have shared the following information in the commit log, but dbi2_offset is not depended on the DBI on my environment: +0x0000 : dbi Function 0 +0x1000 : dbi Function 1 +0x2000 : dbi2 Function 0 +0x2800 : dbi2 Function 1 So, on my environment: - the dbi_base is set to +0x0000.. -- And func_offset of func_no = 1 was 0x1000. - the dbi_base2 is set to +0x2000. -- And func_offset2 of function = 1 was 0x0800, not 0x1800. Perhaps, the name of new API should be .func_conf_select_dbi2 instead? ~~~~~ > This would've been much simpler if 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. Then the APIs could just do "func_offset * > func_no" to get DBI base and "(func_offset * func_no) + dbi2_offset" to get DBI2 > base, provided these offsets are passed by the vendor drivers. Serge suggested such implementation before [1] [1] https://lore.kernel.org/linux-pci/j4g4ijnxd7qyacszlwyi3tdztkw2nmnjwyhdqf2l2yj3h2mvje@iqsrqiodqbhq/ > It can be done in a separate cleanup series later. > > > Notes that dw_pcie_ep_func_select2() will call .func_conf_select() > > s/Notes/Note I'll fix it. > > if .func_conf_select2() doesn't exist for backward compatibility. > > > > 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 | 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; > > Please maitain reverse Xmas tree order. I got it. Best regards, Yoshihiro Shimoda > - Mani > > > 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); > > > > 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 > > > > -- > மணிவண்ணன் சதாசிவம்
On Tue, Jul 25, 2023 at 11:57:34AM +0000, Yoshihiro Shimoda wrote: > Hi Manivannan, > > > From: Manivannan Sadhasivam, Sent: Monday, July 24, 2023 6:25 PM > > > > On Fri, Jul 21, 2023 at 04:44: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 .func_conf_select2() and > > > dw_pcie_ep_func_select2(). > > > > > > > How about, > > > > .get_dbi2_offset() and dw_pcie_ep_get_dbi2_offset()? > > Thank you for your suggestion. I should have shared the following information > in the commit log, but dbi2_offset is not depended on the DBI on my environment: > > +0x0000 : dbi Function 0 > +0x1000 : dbi Function 1 > +0x2000 : dbi2 Function 0 > +0x2800 : dbi2 Function 1 > > So, on my environment: > - the dbi_base is set to +0x0000.. > -- And func_offset of func_no = 1 was 0x1000. > - the dbi_base2 is set to +0x2000. > -- And func_offset2 of function = 1 was 0x0800, not 0x1800. > > Perhaps, the name of new API should be .func_conf_select_dbi2 instead? > ~~~~~ "func_conf_select" doesn't look intuitive to me atleast. The idea behind this callback is to get the funcion offset based on the supplied function no. So this should've been something like, "get_func_offset" and the API should've been dw_pcie_ep_get_func_offset(). Since I do not want you to change the existing naming in this series, I suggested to get the next API naming right. > > > This would've been much simpler if 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. Then the APIs could just do "func_offset * > > func_no" to get DBI base and "(func_offset * func_no) + dbi2_offset" to get DBI2 > > base, provided these offsets are passed by the vendor drivers. > > Serge suggested such implementation before [1] > > [1] > https://lore.kernel.org/linux-pci/j4g4ijnxd7qyacszlwyi3tdztkw2nmnjwyhdqf2l2yj3h2mvje@iqsrqiodqbhq/ > Thanks for the link. I missed Serge's suggestion before. But I completely agree with him as you can see from my above suggestion. In addition, I also want to fix the "func_conf_select" naming as well. However, I do not want you to implement the suggestion in this series itself. It should be done as a separate cleanup series later. (I think you both agree to that as well). - Mani > > It can be done in a separate cleanup series later. > > > > > Notes that dw_pcie_ep_func_select2() will call .func_conf_select() > > > > s/Notes/Note > > I'll fix it. > > > > if .func_conf_select2() doesn't exist for backward compatibility. > > > > > > 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 | 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; > > > > Please maitain reverse Xmas tree order. > > I got it. > > Best regards, > Yoshihiro Shimoda > > > - Mani > > > > > 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); > > > > > > 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 > > > > > > > -- > > மணிவண்ணன் சதாசிவம்
Hi Manivannan, > From: Manivannan Sadhasivam, Sent: Friday, July 28, 2023 11:35 AM > > On Tue, Jul 25, 2023 at 11:57:34AM +0000, Yoshihiro Shimoda wrote: > > Hi Manivannan, > > > > > From: Manivannan Sadhasivam, Sent: Monday, July 24, 2023 6:25 PM > > > > > > On Fri, Jul 21, 2023 at 04:44: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 .func_conf_select2() and > > > > dw_pcie_ep_func_select2(). > > > > > > > > > > How about, > > > > > > .get_dbi2_offset() and dw_pcie_ep_get_dbi2_offset()? > > > > Thank you for your suggestion. I should have shared the following information > > in the commit log, but dbi2_offset is not depended on the DBI on my environment: > > > > +0x0000 : dbi Function 0 > > +0x1000 : dbi Function 1 > > +0x2000 : dbi2 Function 0 > > +0x2800 : dbi2 Function 1 > > > > So, on my environment: > > - the dbi_base is set to +0x0000.. > > -- And func_offset of func_no = 1 was 0x1000. > > - the dbi_base2 is set to +0x2000. > > -- And func_offset2 of function = 1 was 0x0800, not 0x1800. > > > > Perhaps, the name of new API should be .func_conf_select_dbi2 instead? > > ~~~~~ > > "func_conf_select" doesn't look intuitive to me atleast. The idea behind this > callback is to get the funcion offset based on the supplied function no. So this > should've been something like, "get_func_offset" and the API should've been > dw_pcie_ep_get_func_offset(). > > Since I do not want you to change the existing naming in this series, I > suggested to get the next API naming right. I got it. > > > > > This would've been much simpler if 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. Then the APIs could just do "func_offset * > > > func_no" to get DBI base and "(func_offset * func_no) + dbi2_offset" to get DBI2 > > > base, provided these offsets are passed by the vendor drivers. > > > > Serge suggested such implementation before [1] > > > > [1] > > <snip URL> > > > > Thanks for the link. I missed Serge's suggestion before. But I completely agree > with him as you can see from my above suggestion. In addition, I also want to > fix the "func_conf_select" naming as well. > > However, I do not want you to implement the suggestion in this series itself. > It should be done as a separate cleanup series later. I got it. > (I think you both agree to > that as well). Yes, I both agree to that. Best regards, Yoshihiro Shimoda > - Mani > > > > It can be done in a separate cleanup series later. > > > > > > > Notes that dw_pcie_ep_func_select2() will call .func_conf_select() > > > > > > s/Notes/Note > > > > I'll fix it. > > > > > > if .func_conf_select2() doesn't exist for backward compatibility. > > > > > > > > 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 | 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; > > > > > > Please maitain reverse Xmas tree order. > > > > I got it. > > > > Best regards, > > Yoshihiro Shimoda > > > > > - Mani > > > > > > > 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); > > > > > > > > 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 > > > > > > > > > > -- > > > மணிவண்ணன் சதாசிவம் > > -- > மணிவண்ணன் சதாசிவம்
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); 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 {
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 .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@renesas.com> --- .../pci/controller/dwc/pcie-designware-ep.c | 32 ++++++++++++++----- drivers/pci/controller/dwc/pcie-designware.h | 3 +- 2 files changed, 26 insertions(+), 9 deletions(-)