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

[Hans Zhang]

On 4/7/26 20:29, Ilpo Järvinen wrote:
> 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@xxxxxxx>
> > > > Acked-by: Manivannan Sadhasivam <mani@xxxxxxxxxx>
> > > > ---
> > > >    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"):

Hi Ilpo,

Thank you. Will change.

Best regards,
Hans

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