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
> 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?
-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
>
