| Message ID | 20240802031822.1862030-3-jitendra.vegiraju@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: stmmac: Add PCI driver support for BCM8958x | expand |
On Thu, Aug 01, 2024 at 08:18:21PM -0700, jitendra.vegiraju@broadcom.com wrote: > +static u32 stmmac_get_user_version(struct stmmac_priv *priv, u32 id_reg) > +{ > + u32 reg = readl(priv->ioaddr + id_reg); > + > + if (!reg) { > + dev_info(priv->device, "User Version not available\n"); > + return 0x0; > + } > + > + return (reg & GENMASK(23, 16)) >> 16; return FIELD_GET(GENMASK(23, 16), reg); For even more bonus points, use a #define for the field mask. Thanks.
On Fri, Aug 2, 2024 at 1:23 AM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Thu, Aug 01, 2024 at 08:18:21PM -0700, jitendra.vegiraju@broadcom.com wrote: > > +static u32 stmmac_get_user_version(struct stmmac_priv *priv, u32 id_reg) > > +{ > > + u32 reg = readl(priv->ioaddr + id_reg); > > + > > + if (!reg) { > > + dev_info(priv->device, "User Version not available\n"); > > + return 0x0; > > + } > > + > > + return (reg & GENMASK(23, 16)) >> 16; > > return FIELD_GET(GENMASK(23, 16), reg); > > For even more bonus points, use a #define for the field mask. Thanks, I will make the change. > > Thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> + user_ver = stmmac_get_user_version(priv, GMAC4_VERSION); > + if (priv->synopsys_id == DWXGMAC_CORE_4_00 && > + user_ver == DWXGMAC_USER_VER_X22) > + mac->dma = &dwxgmac400_dma_ops; I know nothing about this hardware.... Does priv->synopsys_id == DWXGMAC_CORE_4_0 not imply dwxgmac400_dma_ops? Could a user synthesise DWXGMAC_CORE_4_00 without using dwxgmac400_dma_ops? Could dwxgmac500_dma_ops or dwxgmac100_dma_ops be used? Andrew
Hi Andrew, On Fri, Aug 2, 2024 at 3:59 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > + user_ver = stmmac_get_user_version(priv, GMAC4_VERSION); > > + if (priv->synopsys_id == DWXGMAC_CORE_4_00 && > > + user_ver == DWXGMAC_USER_VER_X22) > > + mac->dma = &dwxgmac400_dma_ops; > > I know nothing about this hardware.... > > Does priv->synopsys_id == DWXGMAC_CORE_4_0 not imply > dwxgmac400_dma_ops? > > Could a user synthesise DWXGMAC_CORE_4_00 without using > dwxgmac400_dma_ops? Could dwxgmac500_dma_ops or dwxgmac100_dma_ops be > used? Yes, the user can choose between Enhanced DMA , Hyper DMA , Normal DMA. This SoC support has chosen Hyper DMA for future expandability. > > Andrew
On Thu, Aug 01, 2024 at 08:18:21PM -0700, jitendra.vegiraju@broadcom.com wrote: > From: Jitendra Vegiraju <jitendra.vegiraju@broadcom.com> > > Integrate dwxgmac4 support into stmmac hardware interface handling. > A dwxgmac4 is an xgmac device and hence it inherits properties from > existing stmmac_hw table entry. > The quirks handling facility is used to update dma_ops field to > point to dwxgmac400_dma_ops when the user version field matches. > > Signed-off-by: Jitendra Vegiraju <jitendra.vegiraju@broadcom.com> > --- > drivers/net/ethernet/stmicro/stmmac/common.h | 4 +++ > drivers/net/ethernet/stmicro/stmmac/hwif.c | 26 +++++++++++++++++++- > drivers/net/ethernet/stmicro/stmmac/hwif.h | 1 + > 3 files changed, 30 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h > index cd36ff4da68c..9bf278e11704 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/common.h > +++ b/drivers/net/ethernet/stmicro/stmmac/common.h > @@ -37,11 +37,15 @@ > #define DWXGMAC_CORE_2_10 0x21 > #define DWXGMAC_CORE_2_20 0x22 > #define DWXLGMAC_CORE_2_00 0x20 > +#define DWXGMAC_CORE_4_00 0x40 DW25GMAC_CORE_4_00? > > /* Device ID */ > #define DWXGMAC_ID 0x76 What is the device ID in your case? Does it match to DWXGMAC_ID? > #define DWXLGMAC_ID 0x27 > > +/* User Version */ > +#define DWXGMAC_USER_VER_X22 0x22 > + > #define STMMAC_CHAN0 0 /* Always supported and default for all chips */ > > /* TX and RX Descriptor Length, these need to be power of two. > diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c > index 29367105df54..713cb5aa2c3e 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/hwif.c > +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c > @@ -36,6 +36,18 @@ static u32 stmmac_get_dev_id(struct stmmac_priv *priv, u32 id_reg) > return (reg & GENMASK(15, 8)) >> 8; > } > > +static u32 stmmac_get_user_version(struct stmmac_priv *priv, u32 id_reg) > +{ > + u32 reg = readl(priv->ioaddr + id_reg); > + > + if (!reg) { > + dev_info(priv->device, "User Version not available\n"); > + return 0x0; > + } > + > + return (reg & GENMASK(23, 16)) >> 16; > +} > + The User Version is purely a vendor-specific stuff defined on the IP-core synthesis stage. Moreover I don't see you'll need it anyway. > static void stmmac_dwmac_mode_quirk(struct stmmac_priv *priv) > { > struct mac_device_info *mac = priv->hw; > @@ -82,6 +94,18 @@ static int stmmac_dwmac4_quirks(struct stmmac_priv *priv) > return 0; > } > > +static int stmmac_dwxgmac_quirks(struct stmmac_priv *priv) > +{ > + struct mac_device_info *mac = priv->hw; > + u32 user_ver; > + > + user_ver = stmmac_get_user_version(priv, GMAC4_VERSION); > + if (priv->synopsys_id == DWXGMAC_CORE_4_00 && > + user_ver == DWXGMAC_USER_VER_X22) > + mac->dma = &dwxgmac400_dma_ops; > + return 0; > +} > + > static int stmmac_dwxlgmac_quirks(struct stmmac_priv *priv) > { > priv->hw->xlgmac = true; > @@ -256,7 +280,7 @@ static const struct stmmac_hwif_entry { > .mmc = &dwxgmac_mmc_ops, > .est = &dwmac510_est_ops, > .setup = dwxgmac2_setup, > - .quirks = NULL, > + .quirks = stmmac_dwxgmac_quirks, Why? You can just introduce a new stmmac_hw[] entry with the DW 25GMAC-specific stmmac_dma_ops instance specified. -Serge(y) > }, { > .gmac = false, > .gmac4 = false, > diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h > index e53c32362774..6213c496385c 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h > +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h > @@ -683,6 +683,7 @@ extern const struct stmmac_desc_ops dwxgmac210_desc_ops; > extern const struct stmmac_mmc_ops dwmac_mmc_ops; > extern const struct stmmac_mmc_ops dwxgmac_mmc_ops; > extern const struct stmmac_est_ops dwmac510_est_ops; > +extern const struct stmmac_dma_ops dwxgmac400_dma_ops; > > #define GMAC_VERSION 0x00000020 /* GMAC CORE Version */ > #define GMAC4_VERSION 0x00000110 /* GMAC4+ CORE Version */ > -- > 2.34.1 > >
On Mon, Aug 05, 2024 at 05:36:30PM -0700, Jitendra Vegiraju wrote: > Hi Andrew, > On Fri, Aug 2, 2024 at 3:59 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > + user_ver = stmmac_get_user_version(priv, GMAC4_VERSION); > > > + if (priv->synopsys_id == DWXGMAC_CORE_4_00 && > > > + user_ver == DWXGMAC_USER_VER_X22) > > > + mac->dma = &dwxgmac400_dma_ops; > > > > I know nothing about this hardware.... > > > > Does priv->synopsys_id == DWXGMAC_CORE_4_0 not imply > > dwxgmac400_dma_ops? > > > > Could a user synthesise DWXGMAC_CORE_4_00 without using > > dwxgmac400_dma_ops? Could dwxgmac500_dma_ops or dwxgmac100_dma_ops be > > used? > Yes, the user can choose between Enhanced DMA , Hyper DMA , Normal DMA. > This SoC support has chosen Hyper DMA for future expandability. Is there a register which describes the synthesis configuration? It is much better that the hardware tells us what it is, rather than having to expand this condition for every new devices which gets added. Also, what is the definition of user_ver. Can we guarantee this is unique and can actually be used to determine what DMA variant has been synthesised? Andrew
Hi Serge On Tue, Aug 6, 2024 at 3:14 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > On Thu, Aug 01, 2024 at 08:18:21PM -0700, jitendra.vegiraju@broadcom.com wrote: > > From: Jitendra Vegiraju <jitendra.vegiraju@broadcom.com> > > > > Integrate dwxgmac4 support into stmmac hardware interface handling. > > A dwxgmac4 is an xgmac device and hence it inherits properties from > > existing stmmac_hw table entry. > > The quirks handling facility is used to update dma_ops field to > > point to dwxgmac400_dma_ops when the user version field matches. > > > > Signed-off-by: Jitendra Vegiraju <jitendra.vegiraju@broadcom.com> > > --- > > drivers/net/ethernet/stmicro/stmmac/common.h | 4 +++ > > drivers/net/ethernet/stmicro/stmmac/hwif.c | 26 +++++++++++++++++++- > > drivers/net/ethernet/stmicro/stmmac/hwif.h | 1 + > > 3 files changed, 30 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h > > index cd36ff4da68c..9bf278e11704 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/common.h > > +++ b/drivers/net/ethernet/stmicro/stmmac/common.h > > @@ -37,11 +37,15 @@ > > #define DWXGMAC_CORE_2_10 0x21 > > #define DWXGMAC_CORE_2_20 0x22 > > #define DWXLGMAC_CORE_2_00 0x20 > > > +#define DWXGMAC_CORE_4_00 0x40 > > DW25GMAC_CORE_4_00? Will do. > > > > > /* Device ID */ > > #define DWXGMAC_ID 0x76 > > What is the device ID in your case? Does it match to DWXGMAC_ID? The early adopter 25MAC IP core used on this has 0x76. But, synopsis confirmed the 25GMAC is assigned with value 0x55. Will define DW25MAC_ID 0x55 and use it for 25GMAC hw_if entry. However, we would like to get a suggestion for dealing with this early adopter device_id number. Can we add override mechanism by defining device_id in stmmac_priv structure and let the hardware specific setup function in the glue driver update the device_id to 0x55 function? > > > #define DWXLGMAC_ID 0x27 > > > > +/* User Version */ > > +#define DWXGMAC_USER_VER_X22 0x22 > > + > > #define STMMAC_CHAN0 0 /* Always supported and default for all chips */ > > > > /* TX and RX Descriptor Length, these need to be power of two. > > diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c > > index 29367105df54..713cb5aa2c3e 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/hwif.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c > > @@ -36,6 +36,18 @@ static u32 stmmac_get_dev_id(struct stmmac_priv *priv, u32 id_reg) > > return (reg & GENMASK(15, 8)) >> 8; > > } > > > > > +static u32 stmmac_get_user_version(struct stmmac_priv *priv, u32 id_reg) > > +{ > > + u32 reg = readl(priv->ioaddr + id_reg); > > + > > + if (!reg) { > > + dev_info(priv->device, "User Version not available\n"); > > + return 0x0; > > + } > > + > > + return (reg & GENMASK(23, 16)) >> 16; > > +} > > + > > The User Version is purely a vendor-specific stuff defined on the > IP-core synthesis stage. Moreover I don't see you'll need it anyway. > Yes, we don't need this function with the 25GMAC entry. > > static void stmmac_dwmac_mode_quirk(struct stmmac_priv *priv) > > { > > struct mac_device_info *mac = priv->hw; > > @@ -82,6 +94,18 @@ static int stmmac_dwmac4_quirks(struct stmmac_priv *priv) > > return 0; > > } > > > > > +static int stmmac_dwxgmac_quirks(struct stmmac_priv *priv) > > +{ > > + struct mac_device_info *mac = priv->hw; > > + u32 user_ver; > > + > > + user_ver = stmmac_get_user_version(priv, GMAC4_VERSION); > > + if (priv->synopsys_id == DWXGMAC_CORE_4_00 && > > + user_ver == DWXGMAC_USER_VER_X22) > > + mac->dma = &dwxgmac400_dma_ops; > > + return 0; > > +} > > + Will remove this function. > > static int stmmac_dwxlgmac_quirks(struct stmmac_priv *priv) > > { > > priv->hw->xlgmac = true; > > @@ -256,7 +280,7 @@ static const struct stmmac_hwif_entry { > > .mmc = &dwxgmac_mmc_ops, > > .est = &dwmac510_est_ops, > > .setup = dwxgmac2_setup, > > - .quirks = NULL, > > + .quirks = stmmac_dwxgmac_quirks, > > Why? You can just introduce a new stmmac_hw[] entry with the DW > 25GMAC-specific stmmac_dma_ops instance specified. > Will do. > -Serge(y) > > > }, { > > .gmac = false, > > .gmac4 = false, > > diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h > > index e53c32362774..6213c496385c 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h > > +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h > > @@ -683,6 +683,7 @@ extern const struct stmmac_desc_ops dwxgmac210_desc_ops; > > extern const struct stmmac_mmc_ops dwmac_mmc_ops; > > extern const struct stmmac_mmc_ops dwxgmac_mmc_ops; > > extern const struct stmmac_est_ops dwmac510_est_ops; > > +extern const struct stmmac_dma_ops dwxgmac400_dma_ops; > > > > #define GMAC_VERSION 0x00000020 /* GMAC CORE Version */ > > #define GMAC4_VERSION 0x00000110 /* GMAC4+ CORE Version */ > > -- > > 2.34.1 > > > >
Hi Andrew On Tue, Aug 6, 2024 at 4:13 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Mon, Aug 05, 2024 at 05:36:30PM -0700, Jitendra Vegiraju wrote: > > Hi Andrew, > > On Fri, Aug 2, 2024 at 3:59 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > > + user_ver = stmmac_get_user_version(priv, GMAC4_VERSION); > > > > + if (priv->synopsys_id == DWXGMAC_CORE_4_00 && > > > > + user_ver == DWXGMAC_USER_VER_X22) > > > > + mac->dma = &dwxgmac400_dma_ops; > > > > > > I know nothing about this hardware.... > > > > > > Does priv->synopsys_id == DWXGMAC_CORE_4_0 not imply > > > dwxgmac400_dma_ops? > > > > > > Could a user synthesise DWXGMAC_CORE_4_00 without using > > > dwxgmac400_dma_ops? Could dwxgmac500_dma_ops or dwxgmac100_dma_ops be > > > used? > > Yes, the user can choose between Enhanced DMA , Hyper DMA , Normal DMA. > > This SoC support has chosen Hyper DMA for future expandability. > > Is there a register which describes the synthesis configuration? It is > much better that the hardware tells us what it is, rather than having > to expand this condition for every new devices which gets added. > Sorry, for the delayed response. We got confirmation that HDMA capability can be identified by the presence of DWXGMAC_CORE_4_xx and device_id 0x55. > Also, what is the definition of user_ver. Can we guarantee this is > unique and can actually be used to determine what DMA variant has been > synthesised? The initial information I got on this is that user versions are allocated and reserved by Synopsis. We probably don't need to key off this information now. > > Andrew
diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h index cd36ff4da68c..9bf278e11704 100644 --- a/drivers/net/ethernet/stmicro/stmmac/common.h +++ b/drivers/net/ethernet/stmicro/stmmac/common.h @@ -37,11 +37,15 @@ #define DWXGMAC_CORE_2_10 0x21 #define DWXGMAC_CORE_2_20 0x22 #define DWXLGMAC_CORE_2_00 0x20 +#define DWXGMAC_CORE_4_00 0x40 /* Device ID */ #define DWXGMAC_ID 0x76 #define DWXLGMAC_ID 0x27 +/* User Version */ +#define DWXGMAC_USER_VER_X22 0x22 + #define STMMAC_CHAN0 0 /* Always supported and default for all chips */ /* TX and RX Descriptor Length, these need to be power of two. diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c index 29367105df54..713cb5aa2c3e 100644 --- a/drivers/net/ethernet/stmicro/stmmac/hwif.c +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c @@ -36,6 +36,18 @@ static u32 stmmac_get_dev_id(struct stmmac_priv *priv, u32 id_reg) return (reg & GENMASK(15, 8)) >> 8; } +static u32 stmmac_get_user_version(struct stmmac_priv *priv, u32 id_reg) +{ + u32 reg = readl(priv->ioaddr + id_reg); + + if (!reg) { + dev_info(priv->device, "User Version not available\n"); + return 0x0; + } + + return (reg & GENMASK(23, 16)) >> 16; +} + static void stmmac_dwmac_mode_quirk(struct stmmac_priv *priv) { struct mac_device_info *mac = priv->hw; @@ -82,6 +94,18 @@ static int stmmac_dwmac4_quirks(struct stmmac_priv *priv) return 0; } +static int stmmac_dwxgmac_quirks(struct stmmac_priv *priv) +{ + struct mac_device_info *mac = priv->hw; + u32 user_ver; + + user_ver = stmmac_get_user_version(priv, GMAC4_VERSION); + if (priv->synopsys_id == DWXGMAC_CORE_4_00 && + user_ver == DWXGMAC_USER_VER_X22) + mac->dma = &dwxgmac400_dma_ops; + return 0; +} + static int stmmac_dwxlgmac_quirks(struct stmmac_priv *priv) { priv->hw->xlgmac = true; @@ -256,7 +280,7 @@ static const struct stmmac_hwif_entry { .mmc = &dwxgmac_mmc_ops, .est = &dwmac510_est_ops, .setup = dwxgmac2_setup, - .quirks = NULL, + .quirks = stmmac_dwxgmac_quirks, }, { .gmac = false, .gmac4 = false, diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h index e53c32362774..6213c496385c 100644 --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h @@ -683,6 +683,7 @@ extern const struct stmmac_desc_ops dwxgmac210_desc_ops; extern const struct stmmac_mmc_ops dwmac_mmc_ops; extern const struct stmmac_mmc_ops dwxgmac_mmc_ops; extern const struct stmmac_est_ops dwmac510_est_ops; +extern const struct stmmac_dma_ops dwxgmac400_dma_ops; #define GMAC_VERSION 0x00000020 /* GMAC CORE Version */ #define GMAC4_VERSION 0x00000110 /* GMAC4+ CORE Version */