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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5398 bytes --]

On Tue, 7 Apr 2026, Hans Zhang wrote:
> On 4/7/26 16:25, Ilpo Järvinen wrote:
> > On Mon, 6 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@163.com>
> > > Acked-by: Manivannan Sadhasivam <mani@kernel.org>
> > > ---
> > >   drivers/pci/controller/dwc/pcie-designware.c | 18 +++---------------
> > >   1 file changed, 3 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c
> > > b/drivers/pci/controller/dwc/pcie-designware.c
> > > index 06792ba92aa7..ab8dee5d6c7a 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > @@ -861,24 +861,12 @@ 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;
> > >   -	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);
> > > +	link_speed = pci_bus_speed2lnkctl2(link_speed);
> > 
> > Its signature is:
> > 
> > u16 pci_bus_speed2lnkctl2(enum pci_bus_speed speed)
> > 
> > Using link_speed variable both for in and out does contradict with the
> > expected typing.
> > 
> > Maybe it would be beneficial to rename the current 'link_speed' to
> > 'ctrl2_speed' (or something along those lines) to differentiate Link
> > Control 2 speed representation from PCI core's internal link speed
> > representation. And then reintroduce link_speed variable as the
> > pci_bus_speed. ...But this is just my suggestion, there may be better ways
> > to untangle this type abuse.
> 
> Hi Ilpo,
> 
> 
> Thank you for your review comments. This way it will be clearer. Here are my
> revisions. If there are no issues, I will send the next version.
> 
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c
> b/drivers/pci/controller/dwc/pcie-designware.c
> index 06792ba92aa7..21a709a47e82 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -843,8 +843,9 @@ 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, pci_bus_speed;

I meant something like this (instead of introducing variable named 
"pci_bus_speed"):

	enum pci_bus_speed link_speed;

>         u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +       u16 ctrl2_speed;

...And keep this like you have it now.

-- 
 i.

>         cap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> 
> @@ -861,30 +862,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;
> 
> -       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:
> +       pci_bus_speed = pcie_get_link_speed(pci->max_link_speed);
> +       ctrl2_speed = pci_bus_speed2lnkctl2(pci_bus_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;
>         }
> 
> -       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);
> 
> 
> Best regards,
> Hans
> 
> > 
> > > +	if (link_speed == 0) {
> > >   		/* Use hardware capability */
> > >   		link_speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, cap);
> > >   		ctrl2 &= ~PCI_EXP_LNKCTL2_HASD;
> > > -		break;
> > >   	}
> > >     	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCTL2, ctrl2 |
> > > link_speed);
> > > 
> > 
>