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