| Message ID | 20231212053723.443772-2-Raju.Rangoju@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | amd-xgbe: add support for AMD Crater | expand |
On Tue, Dec 12, 2023 at 11:08 AM Raju Rangoju <Raju.Rangoju@amd.com> wrote: > The xgbe_{read/write}_mmd_regs_v* functions have common code which can > be moved to helper functions. Also, the xgbe_pci_probe() needs > reorganization. > > Add new helper functions to calculate the mmd_address for v1/v2 of xpcs > access. And, convert if/else statements in xgbe_pci_probe() to switch > case. This helps code look cleaner. > > Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com> > --- > drivers/net/ethernet/amd/xgbe/xgbe-dev.c | 43 ++++++++++++------------ > drivers/net/ethernet/amd/xgbe/xgbe-pci.c | 18 +++++++--- > 2 files changed, 34 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c > b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c > index f393228d41c7..6cd003c24a64 100644 > --- a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c > +++ b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c > @@ -1150,6 +1150,21 @@ static int xgbe_set_gpio(struct xgbe_prv_data > *pdata, unsigned int gpio) > return 0; > } > > +static unsigned int get_mmd_address(struct xgbe_prv_data *pdata, int > mmd_reg) > +{ > + return (mmd_reg & XGBE_ADDR_C45) ? > + mmd_reg & ~XGBE_ADDR_C45 : > + (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff); > +} > + > +static unsigned int get_index_offset(struct xgbe_prv_data *pdata, > unsigned int mmd_address, > + unsigned int *index) > +{ > + mmd_address <<= 1; > + *index = mmd_address & ~pdata->xpcs_window_mask; > + return pdata->xpcs_window + (mmd_address & > pdata->xpcs_window_mask); > +} > + > static int xgbe_read_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad, > int mmd_reg) > { > @@ -1157,10 +1172,7 @@ static int xgbe_read_mmd_regs_v2(struct > xgbe_prv_data *pdata, int prtad, > unsigned int mmd_address, index, offset; > int mmd_data; > > - if (mmd_reg & XGBE_ADDR_C45) > - mmd_address = mmd_reg & ~XGBE_ADDR_C45; > - else > - mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff); > + mmd_address = get_mmd_address(pdata, mmd_reg); > > /* The PCS registers are accessed using mmio. The underlying > * management interface uses indirect addressing to access the MMD > @@ -1171,9 +1183,7 @@ static int xgbe_read_mmd_regs_v2(struct > xgbe_prv_data *pdata, int prtad, > * register offsets must therefore be adjusted by left shifting the > * offset 1 bit and reading 16 bits of data. > */ > - mmd_address <<= 1; > - index = mmd_address & ~pdata->xpcs_window_mask; > - offset = pdata->xpcs_window + (mmd_address & > pdata->xpcs_window_mask); > + offset = get_index_offset(pdata, mmd_address, &index); > > spin_lock_irqsave(&pdata->xpcs_lock, flags); > XPCS32_IOWRITE(pdata, pdata->xpcs_window_sel_reg, index); > @@ -1189,10 +1199,7 @@ static void xgbe_write_mmd_regs_v2(struct > xgbe_prv_data *pdata, int prtad, > unsigned long flags; > unsigned int mmd_address, index, offset; > > - if (mmd_reg & XGBE_ADDR_C45) > - mmd_address = mmd_reg & ~XGBE_ADDR_C45; > - else > - mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff); > + mmd_address = get_mmd_address(pdata, mmd_reg); > > /* The PCS registers are accessed using mmio. The underlying > * management interface uses indirect addressing to access the MMD > @@ -1203,9 +1210,7 @@ static void xgbe_write_mmd_regs_v2(struct > xgbe_prv_data *pdata, int prtad, > * register offsets must therefore be adjusted by left shifting the > * offset 1 bit and writing 16 bits of data. > */ > - mmd_address <<= 1; > - index = mmd_address & ~pdata->xpcs_window_mask; > - offset = pdata->xpcs_window + (mmd_address & > pdata->xpcs_window_mask); > + offset = get_index_offset(pdata, mmd_address, &index); > > spin_lock_irqsave(&pdata->xpcs_lock, flags); > XPCS32_IOWRITE(pdata, pdata->xpcs_window_sel_reg, index); > @@ -1220,10 +1225,7 @@ static int xgbe_read_mmd_regs_v1(struct > xgbe_prv_data *pdata, int prtad, > unsigned int mmd_address; > int mmd_data; > > - if (mmd_reg & XGBE_ADDR_C45) > - mmd_address = mmd_reg & ~XGBE_ADDR_C45; > - else > - mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff); > + mmd_address = get_mmd_address(pdata, mmd_reg); > > /* The PCS registers are accessed using mmio. The underlying APB3 > * management interface uses indirect addressing to access the MMD > @@ -1248,10 +1250,7 @@ static void xgbe_write_mmd_regs_v1(struct > xgbe_prv_data *pdata, int prtad, > unsigned int mmd_address; > unsigned long flags; > > - if (mmd_reg & XGBE_ADDR_C45) > - mmd_address = mmd_reg & ~XGBE_ADDR_C45; > - else > - mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff); > + mmd_address = get_mmd_address(pdata, mmd_reg); > > /* The PCS registers are accessed using mmio. The underlying APB3 > * management interface uses indirect addressing to access the MMD > diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-pci.c > b/drivers/net/ethernet/amd/xgbe/xgbe-pci.c > index f409d7bd1f1e..8b0c1e450b7e 100644 > --- a/drivers/net/ethernet/amd/xgbe/xgbe-pci.c > +++ b/drivers/net/ethernet/amd/xgbe/xgbe-pci.c > @@ -274,12 +274,18 @@ static int xgbe_pci_probe(struct pci_dev *pdev, > const struct pci_device_id *id) > > /* Set the PCS indirect addressing definition registers */ > rdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0)); > - if (rdev && > - (rdev->vendor == PCI_VENDOR_ID_AMD) && (rdev->device == > 0x15d0)) { > + > + if (!(rdev && rdev->vendor == PCI_VENDOR_ID_AMD)) { > + ret = -ENODEV; > + goto err_pci_enable; > + } > + > + switch (rdev->device) { > + case 0x15d0: > pdata->xpcs_window_def_reg = PCS_V2_RV_WINDOW_DEF; > pdata->xpcs_window_sel_reg = PCS_V2_RV_WINDOW_SELECT; > - } else if (rdev && (rdev->vendor == PCI_VENDOR_ID_AMD) && > - (rdev->device == 0x14b5)) { > + break; > + case 0x14b5: > [Kalesh]: Can you use macros for 0x15d0 and 0x14b5 > pdata->xpcs_window_def_reg = PCS_V2_YC_WINDOW_DEF; > pdata->xpcs_window_sel_reg = PCS_V2_YC_WINDOW_SELECT; > > @@ -288,9 +294,11 @@ static int xgbe_pci_probe(struct pci_dev *pdev, const > struct pci_device_id *id) > > /* Yellow Carp devices do not need rrc */ > pdata->vdata->enable_rrc = 0; > - } else { > + break; > + default: > pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF; > pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT; > + break; > } > pci_dev_put(rdev); > > -- > 2.34.1 > > >
On 12/11/23 23:37, Raju Rangoju wrote: > The xgbe_{read/write}_mmd_regs_v* functions have common code which can > be moved to helper functions. Also, the xgbe_pci_probe() needs > reorganization. > > Add new helper functions to calculate the mmd_address for v1/v2 of xpcs > access. And, convert if/else statements in xgbe_pci_probe() to switch > case. This helps code look cleaner. > > Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com> > --- > drivers/net/ethernet/amd/xgbe/xgbe-dev.c | 43 ++++++++++++------------ > drivers/net/ethernet/amd/xgbe/xgbe-pci.c | 18 +++++++--- > 2 files changed, 34 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c > index f393228d41c7..6cd003c24a64 100644 > --- a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c > +++ b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c > @@ -1150,6 +1150,21 @@ static int xgbe_set_gpio(struct xgbe_prv_data *pdata, unsigned int gpio) > return 0; > } > > +static unsigned int get_mmd_address(struct xgbe_prv_data *pdata, int mmd_reg) > +{ > + return (mmd_reg & XGBE_ADDR_C45) ? > + mmd_reg & ~XGBE_ADDR_C45 : > + (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff); > +} > + > +static unsigned int get_index_offset(struct xgbe_prv_data *pdata, unsigned int mmd_address, > + unsigned int *index) Just my opinion, but this looks confusing to me by updating index and returning offset. And the name is confusing, too. I think it would read better as: static void get_pcs_index_and_offset(struct xgbe_prv_data *pdata, unsigned int mmd_address, unsigned int *index, unsigned int *offset) { mmd_address <<= 1; *index = mmd_address & ~pdata->xpcs_window_mask; *offset = pdata->xpcs_window + (mmd_address & pdata->xpcs_window_mask); } Or break this into two functions: static unsigned int get_pcs_index(struct xgbe_prv_data *pdata, unsigned int mmd_address) { return (mmd_address << 1) & ~pdata->xpcs_window_mask; } static unsigned int get_pcs_offset(struct xgbe_prv_data *pdata, unsigned int mmd_address) { return pdata->xpcs_window + ((mmd_address << 1) & pdata->xpcs_window_mask); } > +{ > + mmd_address <<= 1; > + *index = mmd_address & ~pdata->xpcs_window_mask; > + return pdata->xpcs_window + (mmd_address & pdata->xpcs_window_mask); > +} > + > static int xgbe_read_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad, > int mmd_reg) > { > @@ -1157,10 +1172,7 @@ static int xgbe_read_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad, > unsigned int mmd_address, index, offset; > int mmd_data; > > - if (mmd_reg & XGBE_ADDR_C45) > - mmd_address = mmd_reg & ~XGBE_ADDR_C45; > - else > - mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff); > + mmd_address = get_mmd_address(pdata, mmd_reg); > > /* The PCS registers are accessed using mmio. The underlying > * management interface uses indirect addressing to access the MMD > @@ -1171,9 +1183,7 @@ static int xgbe_read_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad, > * register offsets must therefore be adjusted by left shifting the > * offset 1 bit and reading 16 bits of data. > */ > - mmd_address <<= 1; > - index = mmd_address & ~pdata->xpcs_window_mask; > - offset = pdata->xpcs_window + (mmd_address & pdata->xpcs_window_mask); > + offset = get_index_offset(pdata, mmd_address, &index); The comment above this code should be moved into the new helper and then removed here and below. > > spin_lock_irqsave(&pdata->xpcs_lock, flags); > XPCS32_IOWRITE(pdata, pdata->xpcs_window_sel_reg, index); > @@ -1189,10 +1199,7 @@ static void xgbe_write_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad, > unsigned long flags; > unsigned int mmd_address, index, offset; > > - if (mmd_reg & XGBE_ADDR_C45) > - mmd_address = mmd_reg & ~XGBE_ADDR_C45; > - else > - mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff); > + mmd_address = get_mmd_address(pdata, mmd_reg); > > /* The PCS registers are accessed using mmio. The underlying > * management interface uses indirect addressing to access the MMD > @@ -1203,9 +1210,7 @@ static void xgbe_write_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad, > * register offsets must therefore be adjusted by left shifting the > * offset 1 bit and writing 16 bits of data. > */ > - mmd_address <<= 1; > - index = mmd_address & ~pdata->xpcs_window_mask; > - offset = pdata->xpcs_window + (mmd_address & pdata->xpcs_window_mask); > + offset = get_index_offset(pdata, mmd_address, &index); > > spin_lock_irqsave(&pdata->xpcs_lock, flags); > XPCS32_IOWRITE(pdata, pdata->xpcs_window_sel_reg, index); > @@ -1220,10 +1225,7 @@ static int xgbe_read_mmd_regs_v1(struct xgbe_prv_data *pdata, int prtad, > unsigned int mmd_address; > int mmd_data; > > - if (mmd_reg & XGBE_ADDR_C45) > - mmd_address = mmd_reg & ~XGBE_ADDR_C45; > - else > - mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff); > + mmd_address = get_mmd_address(pdata, mmd_reg); > > /* The PCS registers are accessed using mmio. The underlying APB3 > * management interface uses indirect addressing to access the MMD > @@ -1248,10 +1250,7 @@ static void xgbe_write_mmd_regs_v1(struct xgbe_prv_data *pdata, int prtad, > unsigned int mmd_address; > unsigned long flags; > > - if (mmd_reg & XGBE_ADDR_C45) > - mmd_address = mmd_reg & ~XGBE_ADDR_C45; > - else > - mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff); > + mmd_address = get_mmd_address(pdata, mmd_reg); > > /* The PCS registers are accessed using mmio. The underlying APB3 > * management interface uses indirect addressing to access the MMD > diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-pci.c b/drivers/net/ethernet/amd/xgbe/xgbe-pci.c > index f409d7bd1f1e..8b0c1e450b7e 100644 > --- a/drivers/net/ethernet/amd/xgbe/xgbe-pci.c > +++ b/drivers/net/ethernet/amd/xgbe/xgbe-pci.c > @@ -274,12 +274,18 @@ static int xgbe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > /* Set the PCS indirect addressing definition registers */ > rdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0)); > - if (rdev && > - (rdev->vendor == PCI_VENDOR_ID_AMD) && (rdev->device == 0x15d0)) { > + > + if (!(rdev && rdev->vendor == PCI_VENDOR_ID_AMD)) { > + ret = -ENODEV; > + goto err_pci_enable; > + } This is different behavior compared to today. Today, everything would default to the final "else" statement. With this patch you have a possibility of failing probe now. Thanks, Tom > + > + switch (rdev->device) { > + case 0x15d0: > pdata->xpcs_window_def_reg = PCS_V2_RV_WINDOW_DEF; > pdata->xpcs_window_sel_reg = PCS_V2_RV_WINDOW_SELECT; > - } else if (rdev && (rdev->vendor == PCI_VENDOR_ID_AMD) && > - (rdev->device == 0x14b5)) { > + break; > + case 0x14b5: > pdata->xpcs_window_def_reg = PCS_V2_YC_WINDOW_DEF; > pdata->xpcs_window_sel_reg = PCS_V2_YC_WINDOW_SELECT; > > @@ -288,9 +294,11 @@ static int xgbe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > /* Yellow Carp devices do not need rrc */ > pdata->vdata->enable_rrc = 0; > - } else { > + break; > + default: > pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF; > pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT; > + break; > } > pci_dev_put(rdev); >
On 12/12/2023 8:03 PM, Tom Lendacky wrote: > On 12/11/23 23:37, Raju Rangoju wrote: >> The xgbe_{read/write}_mmd_regs_v* functions have common code which can >> be moved to helper functions. Also, the xgbe_pci_probe() needs >> reorganization. >> >> Add new helper functions to calculate the mmd_address for v1/v2 of xpcs >> access. And, convert if/else statements in xgbe_pci_probe() to switch >> case. This helps code look cleaner. >> >> Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com> >> --- >> drivers/net/ethernet/amd/xgbe/xgbe-dev.c | 43 ++++++++++++------------ >> drivers/net/ethernet/amd/xgbe/xgbe-pci.c | 18 +++++++--- >> 2 files changed, 34 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c >> b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c >> index f393228d41c7..6cd003c24a64 100644 >> --- a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c >> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c >> @@ -1150,6 +1150,21 @@ static int xgbe_set_gpio(struct xgbe_prv_data >> *pdata, unsigned int gpio) >> return 0; >> } >> +static unsigned int get_mmd_address(struct xgbe_prv_data *pdata, int >> mmd_reg) >> +{ >> + return (mmd_reg & XGBE_ADDR_C45) ? >> + mmd_reg & ~XGBE_ADDR_C45 : >> + (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff); >> +} >> + >> +static unsigned int get_index_offset(struct xgbe_prv_data *pdata, >> unsigned int mmd_address, >> + unsigned int *index) > > Just my opinion, but this looks confusing to me by updating index and > returning > offset. And the name is confusing, too. I think it would read better as: > > static void get_pcs_index_and_offset(struct xgbe_prv_data *pdata, > unsigned int mmd_address, > unsigned int *index, unsigned int *offset) > { > mmd_address <<= 1; > *index = mmd_address & ~pdata->xpcs_window_mask; > *offset = pdata->xpcs_window + (mmd_address & > pdata->xpcs_window_mask); > } > Sure > Or break this into two functions: > > static unsigned int get_pcs_index(struct xgbe_prv_data *pdata, unsigned > int mmd_address) > { > return (mmd_address << 1) & ~pdata->xpcs_window_mask; > } > > static unsigned int get_pcs_offset(struct xgbe_prv_data *pdata, unsigned > int mmd_address) > { > return pdata->xpcs_window + ((mmd_address << 1) & > pdata->xpcs_window_mask); > } > >> +{ >> + mmd_address <<= 1; >> + *index = mmd_address & ~pdata->xpcs_window_mask; >> + return pdata->xpcs_window + (mmd_address & pdata->xpcs_window_mask); >> +} >> + >> static int xgbe_read_mmd_regs_v2(struct xgbe_prv_data *pdata, int >> prtad, >> int mmd_reg) >> { >> @@ -1157,10 +1172,7 @@ static int xgbe_read_mmd_regs_v2(struct >> xgbe_prv_data *pdata, int prtad, >> unsigned int mmd_address, index, offset; >> int mmd_data; >> - if (mmd_reg & XGBE_ADDR_C45) >> - mmd_address = mmd_reg & ~XGBE_ADDR_C45; >> - else >> - mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff); >> + mmd_address = get_mmd_address(pdata, mmd_reg); >> /* The PCS registers are accessed using mmio. The underlying >> * management interface uses indirect addressing to access the MMD >> @@ -1171,9 +1183,7 @@ static int xgbe_read_mmd_regs_v2(struct >> xgbe_prv_data *pdata, int prtad, >> * register offsets must therefore be adjusted by left shifting the >> * offset 1 bit and reading 16 bits of data. >> */ >> - mmd_address <<= 1; >> - index = mmd_address & ~pdata->xpcs_window_mask; >> - offset = pdata->xpcs_window + (mmd_address & >> pdata->xpcs_window_mask); >> + offset = get_index_offset(pdata, mmd_address, &index); > > The comment above this code should be moved into the new helper and then > removed here and below. Sure, I'll take care of this. > >> spin_lock_irqsave(&pdata->xpcs_lock, flags); >> XPCS32_IOWRITE(pdata, pdata->xpcs_window_sel_reg, index); >> @@ -1189,10 +1199,7 @@ static void xgbe_write_mmd_regs_v2(struct >> xgbe_prv_data *pdata, int prtad, >> unsigned long flags; >> unsigned int mmd_address, index, offset; >> - if (mmd_reg & XGBE_ADDR_C45) >> - mmd_address = mmd_reg & ~XGBE_ADDR_C45; >> - else >> - mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff); >> + mmd_address = get_mmd_address(pdata, mmd_reg); >> /* The PCS registers are accessed using mmio. The underlying >> * management interface uses indirect addressing to access the MMD >> @@ -1203,9 +1210,7 @@ static void xgbe_write_mmd_regs_v2(struct >> xgbe_prv_data *pdata, int prtad, >> * register offsets must therefore be adjusted by left shifting the >> * offset 1 bit and writing 16 bits of data. >> */ >> - mmd_address <<= 1; >> - index = mmd_address & ~pdata->xpcs_window_mask; >> - offset = pdata->xpcs_window + (mmd_address & >> pdata->xpcs_window_mask); >> + offset = get_index_offset(pdata, mmd_address, &index); >> spin_lock_irqsave(&pdata->xpcs_lock, flags); >> XPCS32_IOWRITE(pdata, pdata->xpcs_window_sel_reg, index); >> @@ -1220,10 +1225,7 @@ static int xgbe_read_mmd_regs_v1(struct >> xgbe_prv_data *pdata, int prtad, >> unsigned int mmd_address; >> int mmd_data; >> - if (mmd_reg & XGBE_ADDR_C45) >> - mmd_address = mmd_reg & ~XGBE_ADDR_C45; >> - else >> - mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff); >> + mmd_address = get_mmd_address(pdata, mmd_reg); >> /* The PCS registers are accessed using mmio. The underlying APB3 >> * management interface uses indirect addressing to access the MMD >> @@ -1248,10 +1250,7 @@ static void xgbe_write_mmd_regs_v1(struct >> xgbe_prv_data *pdata, int prtad, >> unsigned int mmd_address; >> unsigned long flags; >> - if (mmd_reg & XGBE_ADDR_C45) >> - mmd_address = mmd_reg & ~XGBE_ADDR_C45; >> - else >> - mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff); >> + mmd_address = get_mmd_address(pdata, mmd_reg); >> /* The PCS registers are accessed using mmio. The underlying APB3 >> * management interface uses indirect addressing to access the MMD >> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-pci.c >> b/drivers/net/ethernet/amd/xgbe/xgbe-pci.c >> index f409d7bd1f1e..8b0c1e450b7e 100644 >> --- a/drivers/net/ethernet/amd/xgbe/xgbe-pci.c >> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-pci.c >> @@ -274,12 +274,18 @@ static int xgbe_pci_probe(struct pci_dev *pdev, >> const struct pci_device_id *id) >> /* Set the PCS indirect addressing definition registers */ >> rdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0)); >> - if (rdev && >> - (rdev->vendor == PCI_VENDOR_ID_AMD) && (rdev->device == >> 0x15d0)) { >> + >> + if (!(rdev && rdev->vendor == PCI_VENDOR_ID_AMD)) { >> + ret = -ENODEV; >> + goto err_pci_enable; >> + } > > This is different behavior compared to today. Today, everything would > default to the final "else" statement. With this patch you have a > possibility of failing probe now. This was done to skip the cases where rdev is NULL or vendor != AMD. Not sure if I'm missing something. > > Thanks, > Tom > >> + >> + switch (rdev->device) { >> + case 0x15d0: >> pdata->xpcs_window_def_reg = PCS_V2_RV_WINDOW_DEF; >> pdata->xpcs_window_sel_reg = PCS_V2_RV_WINDOW_SELECT; >> - } else if (rdev && (rdev->vendor == PCI_VENDOR_ID_AMD) && >> - (rdev->device == 0x14b5)) { >> + break; >> + case 0x14b5: >> pdata->xpcs_window_def_reg = PCS_V2_YC_WINDOW_DEF; >> pdata->xpcs_window_sel_reg = PCS_V2_YC_WINDOW_SELECT; >> @@ -288,9 +294,11 @@ static int xgbe_pci_probe(struct pci_dev *pdev, >> const struct pci_device_id *id) >> /* Yellow Carp devices do not need rrc */ >> pdata->vdata->enable_rrc = 0; >> - } else { >> + break; >> + default: >> pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF; >> pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT; >> + break; >> } >> pci_dev_put(rdev);
On 12/13/23 09:35, Raju Rangoju wrote: > On 12/12/2023 8:03 PM, Tom Lendacky wrote: >> On 12/11/23 23:37, Raju Rangoju wrote: >>> The xgbe_{read/write}_mmd_regs_v* functions have common code which can >>> be moved to helper functions. Also, the xgbe_pci_probe() needs >>> reorganization. >>> >>> Add new helper functions to calculate the mmd_address for v1/v2 of xpcs >>> access. And, convert if/else statements in xgbe_pci_probe() to switch >>> case. This helps code look cleaner. >>> >>> Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com> >>> --- >>> drivers/net/ethernet/amd/xgbe/xgbe-dev.c | 43 ++++++++++++------------ >>> drivers/net/ethernet/amd/xgbe/xgbe-pci.c | 18 +++++++--- >>> 2 files changed, 34 insertions(+), 27 deletions(-) >>> >>> @@ -274,12 +274,18 @@ static int xgbe_pci_probe(struct pci_dev *pdev, >>> const struct pci_device_id *id) >>> /* Set the PCS indirect addressing definition registers */ >>> rdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0)); >>> - if (rdev && >>> - (rdev->vendor == PCI_VENDOR_ID_AMD) && (rdev->device == >>> 0x15d0)) { >>> + >>> + if (!(rdev && rdev->vendor == PCI_VENDOR_ID_AMD)) { >>> + ret = -ENODEV; >>> + goto err_pci_enable; >>> + } >> >> This is different behavior compared to today. Today, everything would >> default to the final "else" statement. With this patch you have a >> possibility of failing probe now. > > This was done to skip the cases where rdev is NULL or vendor != AMD. Not > sure if I'm missing something. Right, I'm just pointing out that if rdev == NULL or vendor != AMD then, today, the else path is taken. With this patch you'll fail the probe with -ENODEV. Thanks, Tom >
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c index f393228d41c7..6cd003c24a64 100644 --- a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c +++ b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c @@ -1150,6 +1150,21 @@ static int xgbe_set_gpio(struct xgbe_prv_data *pdata, unsigned int gpio) return 0; } +static unsigned int get_mmd_address(struct xgbe_prv_data *pdata, int mmd_reg) +{ + return (mmd_reg & XGBE_ADDR_C45) ? + mmd_reg & ~XGBE_ADDR_C45 : + (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff); +} + +static unsigned int get_index_offset(struct xgbe_prv_data *pdata, unsigned int mmd_address, + unsigned int *index) +{ + mmd_address <<= 1; + *index = mmd_address & ~pdata->xpcs_window_mask; + return pdata->xpcs_window + (mmd_address & pdata->xpcs_window_mask); +} + static int xgbe_read_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad, int mmd_reg) { @@ -1157,10 +1172,7 @@ static int xgbe_read_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad, unsigned int mmd_address, index, offset; int mmd_data; - if (mmd_reg & XGBE_ADDR_C45) - mmd_address = mmd_reg & ~XGBE_ADDR_C45; - else - mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff); + mmd_address = get_mmd_address(pdata, mmd_reg); /* The PCS registers are accessed using mmio. The underlying * management interface uses indirect addressing to access the MMD @@ -1171,9 +1183,7 @@ static int xgbe_read_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad, * register offsets must therefore be adjusted by left shifting the * offset 1 bit and reading 16 bits of data. */ - mmd_address <<= 1; - index = mmd_address & ~pdata->xpcs_window_mask; - offset = pdata->xpcs_window + (mmd_address & pdata->xpcs_window_mask); + offset = get_index_offset(pdata, mmd_address, &index); spin_lock_irqsave(&pdata->xpcs_lock, flags); XPCS32_IOWRITE(pdata, pdata->xpcs_window_sel_reg, index); @@ -1189,10 +1199,7 @@ static void xgbe_write_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad, unsigned long flags; unsigned int mmd_address, index, offset; - if (mmd_reg & XGBE_ADDR_C45) - mmd_address = mmd_reg & ~XGBE_ADDR_C45; - else - mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff); + mmd_address = get_mmd_address(pdata, mmd_reg); /* The PCS registers are accessed using mmio. The underlying * management interface uses indirect addressing to access the MMD @@ -1203,9 +1210,7 @@ static void xgbe_write_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad, * register offsets must therefore be adjusted by left shifting the * offset 1 bit and writing 16 bits of data. */ - mmd_address <<= 1; - index = mmd_address & ~pdata->xpcs_window_mask; - offset = pdata->xpcs_window + (mmd_address & pdata->xpcs_window_mask); + offset = get_index_offset(pdata, mmd_address, &index); spin_lock_irqsave(&pdata->xpcs_lock, flags); XPCS32_IOWRITE(pdata, pdata->xpcs_window_sel_reg, index); @@ -1220,10 +1225,7 @@ static int xgbe_read_mmd_regs_v1(struct xgbe_prv_data *pdata, int prtad, unsigned int mmd_address; int mmd_data; - if (mmd_reg & XGBE_ADDR_C45) - mmd_address = mmd_reg & ~XGBE_ADDR_C45; - else - mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff); + mmd_address = get_mmd_address(pdata, mmd_reg); /* The PCS registers are accessed using mmio. The underlying APB3 * management interface uses indirect addressing to access the MMD @@ -1248,10 +1250,7 @@ static void xgbe_write_mmd_regs_v1(struct xgbe_prv_data *pdata, int prtad, unsigned int mmd_address; unsigned long flags; - if (mmd_reg & XGBE_ADDR_C45) - mmd_address = mmd_reg & ~XGBE_ADDR_C45; - else - mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff); + mmd_address = get_mmd_address(pdata, mmd_reg); /* The PCS registers are accessed using mmio. The underlying APB3 * management interface uses indirect addressing to access the MMD diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-pci.c b/drivers/net/ethernet/amd/xgbe/xgbe-pci.c index f409d7bd1f1e..8b0c1e450b7e 100644 --- a/drivers/net/ethernet/amd/xgbe/xgbe-pci.c +++ b/drivers/net/ethernet/amd/xgbe/xgbe-pci.c @@ -274,12 +274,18 @@ static int xgbe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) /* Set the PCS indirect addressing definition registers */ rdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0)); - if (rdev && - (rdev->vendor == PCI_VENDOR_ID_AMD) && (rdev->device == 0x15d0)) { + + if (!(rdev && rdev->vendor == PCI_VENDOR_ID_AMD)) { + ret = -ENODEV; + goto err_pci_enable; + } + + switch (rdev->device) { + case 0x15d0: pdata->xpcs_window_def_reg = PCS_V2_RV_WINDOW_DEF; pdata->xpcs_window_sel_reg = PCS_V2_RV_WINDOW_SELECT; - } else if (rdev && (rdev->vendor == PCI_VENDOR_ID_AMD) && - (rdev->device == 0x14b5)) { + break; + case 0x14b5: pdata->xpcs_window_def_reg = PCS_V2_YC_WINDOW_DEF; pdata->xpcs_window_sel_reg = PCS_V2_YC_WINDOW_SELECT; @@ -288,9 +294,11 @@ static int xgbe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) /* Yellow Carp devices do not need rrc */ pdata->vdata->enable_rrc = 0; - } else { + break; + default: pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF; pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT; + break; } pci_dev_put(rdev);
The xgbe_{read/write}_mmd_regs_v* functions have common code which can be moved to helper functions. Also, the xgbe_pci_probe() needs reorganization. Add new helper functions to calculate the mmd_address for v1/v2 of xpcs access. And, convert if/else statements in xgbe_pci_probe() to switch case. This helps code look cleaner. Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com> --- drivers/net/ethernet/amd/xgbe/xgbe-dev.c | 43 ++++++++++++------------ drivers/net/ethernet/amd/xgbe/xgbe-pci.c | 18 +++++++--- 2 files changed, 34 insertions(+), 27 deletions(-)