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

[Ilpo Järvinen]

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.

>  
> -	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);
>  
>  }
>  
> 

-- 
 i.