Re: [PATCH v7 3/3] PCI: dwc: Use common speed conversion function

[Hans Zhang]

On 6/12/26 01:55, Ilpo Järvinen wrote:
> On Tue, 7 Apr 2026, Hans Zhang wrote:
>
> > Replace the private switch-based speed conversion in
> > dw_pcie_link_set_max_speed() with the public pci_bus_speed2lnkctl2()
> > function.
> >
> > This eliminates duplicate conversion logic and ensures consistency with
> > other PCIe drivers, while handling invalid speeds by falling back to
> > hardware capabilities.
> >
> > Signed-off-by: Hans Zhang <18255117159@xxxxxxx>
> > Reviewed-by: Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>
> > Acked-by: Manivannan Sadhasivam <mani@xxxxxxxxxx>
> > ---
> >   drivers/pci/controller/dwc/pcie-designware.c | 28 +++++++-------------
> >   1 file changed, 9 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 06792ba92aa7..10895f6a8e6e 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -843,8 +843,10 @@ EXPORT_SYMBOL_GPL(dw_pcie_upconfig_setup);
> >   
> >   static void dw_pcie_link_set_max_speed(struct dw_pcie *pci)
> >   {
> > -	u32 cap, ctrl2, link_speed;
> > +	u32 cap, ctrl2;
> > +	enum pci_bus_speed link_speed;
> >   	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > +	u16 ctrl2_speed;
> >   
> >   	cap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> >   
> > @@ -861,30 +863,18 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci)
> >   	ctrl2 = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCTL2);
> >   	ctrl2 &= ~PCI_EXP_LNKCTL2_TLS;
>
> Not directly related to this patch but I wonder why this function gets the
> speed from LNKCTL2 instead of taking it from LNKCAP2_SLS.

Hi Ilpo,

Thank you for your review and the helpful observations.

Regarding the use of LNKCTL2 vs LNKCAP2_SLS – I will investigate the 
current design choice in dw_pcie_link_set_max_speed() and see if there 
is room for improvement in a future patch.

Since you have no objection to this change, I will keep this patch
as is for now. I'll put the points you raised on my TODO list for
potential follow-up cleanups.

Thanks again for your time.

Best regards,
Hans

> >   
> > -	switch (pcie_get_link_speed(pci->max_link_speed)) {
> > -	case PCIE_SPEED_2_5GT:
> > -		link_speed = PCI_EXP_LNKCTL2_TLS_2_5GT;
> > -		break;
> > -	case PCIE_SPEED_5_0GT:
> > -		link_speed = PCI_EXP_LNKCTL2_TLS_5_0GT;
> > -		break;
> > -	case PCIE_SPEED_8_0GT:
> > -		link_speed = PCI_EXP_LNKCTL2_TLS_8_0GT;
> > -		break;
> > -	case PCIE_SPEED_16_0GT:
> > -		link_speed = PCI_EXP_LNKCTL2_TLS_16_0GT;
> > -		break;
> > -	default:
> > +	link_speed = pcie_get_link_speed(pci->max_link_speed);
> > +	ctrl2_speed = pci_bus_speed2lnkctl2(link_speed);
> > +	if (ctrl2_speed == 0) {
> >   		/* Use hardware capability */
> > -		link_speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, cap);
> > +		ctrl2_speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, cap);
> >   		ctrl2 &= ~PCI_EXP_LNKCTL2_HASD;
> > -		break;
> >   	}
>
> I again lament a bit that pcie_capability_read_*() cannot be used early in
> the controller drivers, which might allow using e.g.
> pcie_get_supported_speeds() here (depending on whether the small
> differences this function has compared it are really meaningful or not).
>
> But this is not really a problem this series is trying to address so as
> stated in my comment to the other patch, no objection to this change.
>
> > -	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL2, ctrl2 | link_speed);
> > +	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL2, ctrl2 | ctrl2_speed);
> >   
> >   	cap &= ~((u32)PCI_EXP_LNKCAP_SLS);
> > -	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, cap | link_speed);
> > +	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, cap | ctrl2_speed);
> >   
> >   }
> >