RE: [PATCH v11 08/13] PCI: dwc: Add dw_pcie_num_lanes_setup()

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>]

Hi Serge,

> From: Serge Semin, Sent: Wednesday, March 22, 2023 3:57 PM
> 
> On Fri, Mar 10, 2023 at 09:35:05PM +0900, Yoshihiro Shimoda wrote:
> > Add dw_pcie_num_lanes_setup() to setup PCI_EXP_LNKCAP_MLW.
> 
> More details of why it's needed would be nice. For instance, in
> accordance with the DW PCIe RC/EP HW manuals [1,2,3,...] aside with the
> PORT_LINK_CTRL_OFF.LINK_CAPABLE and GEN2_CTRL_OFF.NUM_OF_LANES[8:0]
> field there is another one which needs to be update. It's
> LINK_CAPABILITIES_REG.PCIE_CAP_MAX_LINK_WIDTH. If it isn't done at the
> very least the maximum link-width capability CSR won't expose the
> actual maximum capability.

Thank you for your comments! I'll add them into the commit message on v12.

> [1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port,
>     Version 4.60a, March 2015, p.1032
> [2] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port,
>     Version 4.70a, March 2016, p.1065
> [3] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port,
>     Version 4.90a, March 2016, p.1057
> ...
> [X] DesignWare Cores PCI Express Controller Databook - DWC PCIe Endpoint,
>     Version 5.40a, March 2019, p.1396
> [X+1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port,
>     Version 5.40a, March 2019, p.1266
> 
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware.c | 12 ++++++++++++
> >  drivers/pci/controller/dwc/pcie-designware.h |  1 +
> >  drivers/pci/controller/dwc/pcie-tegra194.c   |  5 +----
> >  3 files changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 89b8ec29da7f..47860da5738e 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -696,6 +696,18 @@ void dw_pcie_upconfig_setup(struct dw_pcie *pci)
> >  }
> >  EXPORT_SYMBOL_GPL(dw_pcie_upconfig_setup);
> >
> 
> > +void dw_pcie_num_lanes_setup(struct dw_pcie *pci, int num_lanes)
> > +{
> > +	u8 cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > +	u32 val;
> > +
> > +	val = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP);
> > +	val &= ~PCI_EXP_LNKCAP_MLW;
> > +	val |= num_lanes << PCI_EXP_LNKSTA_NLW_SHIFT;
> > +	dw_pcie_writel_dbi(pci, cap + PCI_EXP_LNKCAP, val);
> > +}
> > +EXPORT_SYMBOL_GPL(dw_pcie_num_lanes_setup);
> 
> That's not what I meant. I meant to implement that functionality in
> the framework of dw_pcie_setup() function by moving all the
> link-width-related initializations into a dedicated static function
> dw_pcie_link_set_max_{link}_width() (thus the prototype would look
> like the dw_pcie_link_set_max_speed()) and _calling_ it from
> dw_pcie_setup() in place where the num-lanes initializations is
> performed if pci->num_lanes isn't zero. As I already mentioned in my
> comment above in accordance with the DW PCIe HW-manuals this should
> have been done for all DW PCIe IP-cores in the first place. I also
> checked the PCIE_CAP_MAX_LINK_WIDTH field access attribute. It turns
> out to be the same
> ■ Wire: No access.
> ■ Dbi: if (DBI_RO_WR_EN == 1) then R/W else R
> for all IP-cores which HW ref manuals I have at hands (v4.60a, v4.70a,
> v4.90a, v5.20a, v5.30a, v5.40a).
> 
> * Note even though the dw_pcie_setup() method currently permits the
> * 1, 2, 4 and 8 lanes only, in fact the x16 setup is also possible
> * judging by the CX_NL DW PCIe IP-core synthesize parameter.
> 
> It would also be more readable to place the dw_pcie_link_set_max_{link}_width()
> function below dw_pcie_link_set_max_speed() as per the calling order
> in the framework of dw_pcie_setup().
> 
> By doing as I suggested above you not only would be able to implement
> a correct link-width setup procedure for all IP-cores but also could
> get rid of the PATCH #5 since you'll be moving the respective code
> anyway and get rid of the dw_pcie_num_lanes_setup() method invocation
> from your and Tegra glue-drivers.

Thank you very much for your detailed explanation. I understood it.
I'll fix this on v12.

Best regards,
Yoshihiro Shimoda

> -Serge(y)
> 
> > +
> >  static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
> >  {
> >  	u32 cap, ctrl2, link_speed;
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 79713ce075cc..36f3e2c818fe 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -415,6 +415,7 @@ void dw_pcie_write_dbi(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
> >  void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
> >  int dw_pcie_link_up(struct dw_pcie *pci);
> >  void dw_pcie_upconfig_setup(struct dw_pcie *pci);
> > +void dw_pcie_num_lanes_setup(struct dw_pcie *pci, int num_lanes);
> >  int dw_pcie_wait_for_link(struct dw_pcie *pci);
> >  int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> >  			      u64 cpu_addr, u64 pci_addr, u64 size);
> > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> > index 09825b4a075e..befe17d57e2a 100644
> > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > @@ -902,10 +902,7 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp)
> >  	dw_pcie_writel_dbi(pci, PORT_LOGIC_AMBA_ERROR_RESPONSE_DEFAULT, val);
> >
> >  	/* Configure Max lane width from DT */
> > -	val = dw_pcie_readl_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKCAP);
> > -	val &= ~PCI_EXP_LNKCAP_MLW;
> > -	val |= (pcie->num_lanes << PCI_EXP_LNKSTA_NLW_SHIFT);
> > -	dw_pcie_writel_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKCAP, val);
> > +	dw_pcie_num_lanes_setup(pci, pcie->num_lanes);
> >
> >  	/* Clear Slot Clock Configuration bit if SRNS configuration */
> >  	if (pcie->enable_srns) {
> > --
> > 2.25.1
> >
> >