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