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

[Serge Semin <fancer.lancer@gmail.com>]

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.

[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.

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