Hi Jitendra

On Mon, Aug 26, 2024 at 11:53:13AM -0700, Jitendra Vegiraju wrote:

Hi Serge(y)
Thank you for reviewing the patch.

On Fri, Aug 23, 2024 at 6:49 AM Serge Semin [off-list ref] wrote:

quoted

Hi Jitendra

On Wed, Aug 14, 2024 at 03:18:16PM -0700, jitendra.vegiraju@broadcom.com wrote:

quoted

From: Jitendra Vegiraju <redacted>

Integrate dw25gmac support into stmmac hardware interface handling.
Added a new entry to the stmmac_hw table in hwif.c.
Define new macros DW25GMAC_CORE_4_00 and DW25GMAC_ID to identify 25GMAC
device.
Since BCM8958x is an early adaptor device, the synopsis_id reported in HW
is 0x32 and device_id is DWXGMAC_ID. Provide override support by defining
synopsys_dev_id member in struct stmmac_priv so that driver specific setup
functions can override the hardware reported values.

Signed-off-by: Jitendra Vegiraju <redacted>
---
 drivers/net/ethernet/stmicro/stmmac/common.h |  2 ++
 drivers/net/ethernet/stmicro/stmmac/hwif.c   | 25 ++++++++++++++++++--
 drivers/net/ethernet/stmicro/stmmac/stmmac.h |  1 +
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index 684489156dce..46edbe73a124 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h

@@ -38,9 +38,11 @@
 #define DWXGMAC_CORE_2_10    0x21
 #define DWXGMAC_CORE_2_20    0x22
 #define DWXLGMAC_CORE_2_00   0x20
+#define DW25GMAC_CORE_4_00   0x40

 /* Device ID */
 #define DWXGMAC_ID           0x76
+#define DW25GMAC_ID          0x55
 #define DWXLGMAC_ID          0x27

 #define STMMAC_CHAN0 0       /* Always supported and default for all chips */

diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c
index 29367105df54..97e5594ddcda 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.c
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c

@@ -278,6 +278,27 @@ static const struct stmmac_hwif_entry {
              .est = &dwmac510_est_ops,
              .setup = dwxlgmac2_setup,
              .quirks = stmmac_dwxlgmac_quirks,

quoted

+     }, {
+             .gmac = false,
+             .gmac4 = false,
+             .xgmac = true,
+             .min_id = DW25GMAC_CORE_4_00,
+             .dev_id = DW25GMAC_ID,
+             .regs = {
+                     .ptp_off = PTP_XGMAC_OFFSET,
+                     .mmc_off = MMC_XGMAC_OFFSET,
+                     .est_off = EST_XGMAC_OFFSET,
+             },
+             .desc = &dwxgmac210_desc_ops,
+             .dma = &dw25gmac400_dma_ops,
+             .mac = &dwxgmac210_ops,
+             .hwtimestamp = &stmmac_ptp,
+             .mode = NULL,
+             .tc = &dwmac510_tc_ops,
+             .mmc = &dwxgmac_mmc_ops,
+             .est = &dwmac510_est_ops,
+             .setup = dwxgmac2_setup,
+             .quirks = NULL,
      },

This can be replaced with just:

+       }, {
+               .gmac = false,
+               .gmac4 = false,
+               .xgmac = true,
+               .min_id = DW25GMAC_CORE_4_00,
+               .dev_id = DWXGMAC_ID, /* Early DW 25GMAC IP-core had XGMAC ID */
+               .regs = {
+                       .ptp_off = PTP_XGMAC_OFFSET,
+                       .mmc_off = MMC_XGMAC_OFFSET,
+                       .est_off = EST_XGMAC_OFFSET,
+               },
+               .desc = &dwxgmac210_desc_ops,
+               .dma = &dw25gmac400_dma_ops,
+               .mac = &dwxgmac210_ops,
+               .hwtimestamp = &stmmac_ptp,
+               .mode = NULL,
+               .tc = &dwmac510_tc_ops,
+               .mmc = &dwxgmac_mmc_ops,
+               .est = &dwmac510_est_ops,
+               .setup = dw25gmac_setup,
+               .quirks = NULL,
        }

and you won't need to pre-define the setup() method in the
glue driver. Instead you can define a new dw25xgmac_setup() method in
the dwxgmac2_core.c as it's done for the DW XGMAC/LXGMAC IP-cores.

Note if your device is capable to work with up to 10Gbps speed, then
just set the plat_stmmacenet_data::max_speed field to SPEED_10000.
Alternatively if you really need to specify the exact MAC
capabilities, then you can implement what Russell suggested here
sometime ago:
https://lore.kernel.org/netdev/Zf3ifH%2FCjyHtmXE3@shell.armlinux.org.uk/ (local)

I like your suggestion to add one stmmac_hw[] array entry (entry_a) for this
"early release" DW25GMAC IP and another entry (entry_b) for final DW25MAC
IP, in the process eliminate the need for a new member variable in struct
stmmac_priv.

However, I would like to bring to your attention that this device requires
special handling for both synopsys_id and dev_id.
This device is reporting 0x32 for synopsys_id and 0x76(XGMAC) for dev_id.
The final 25GMAC spec will have 0x40 for synopsys_id and 0x55(25GMAC) for
dev_id.

For some reason I was thinking that your device had only the device ID
pre-defined with the XGMAC value meanwhile the Synopsys ID was 0x40.
Indeed you get to override both of these data in the platform-specific
setup() method.

So, in order to avoid falsely qualifying other XGMAC devices with
synopsis_id >=0x32 as DW25GMAC, I am thinking we will have to overwrite the
synopsys_id to 0x40 (DW25GMAC_CORE_4_00) in glue driver using existing
glue driver override mechanism.

We can implement dw25gmac_setup() in dwxgmac2_core.c for generic 25GMAC
case. But, this glue driver will have to rely on its own setup function
to override the synopsys_id as DW25GMAC_CORE_4_00.

Do you think it looks reasonable?

What I was trying to avoid was the setup() method re-definition just
for the sake of the IP-core version override. Because if not for that
you could have created and used the generic DW 25GMAC dw25gmac_setup()
function.

One of the possible solutions I was thinking was to introduce the
plat_stmmacenet_data::{snps_id,dev_id} fields and use their values in
the stmmac_hwif_init() procedure instead of the data read from the
MAC.VERSION CSR.

Another solution could be to add the plat_stmmacenet_data::has_25gmac
field and fix the generic driver code to using it where it's relevant.
Then you won't need to think about what actual Synopsys ID/Device ID
since it would mean a whole new IP-core.

-Serge(y)

If yes, do you want me to add the generic 25GMAC stmmac_hw[] entry
("entry_b") now or when
such a device becomes available for testing?

quoted

If you also have a DW 25GMAC-based device with 0x55 device ID, then
just add another stmmac_hw[] array entry.

quoted

quoted

 };

      int hw_cap_support;
      int synopsys_id;

quoted

+     int synopsys_dev_id;

With the suggestion above implemented you won't need this.

Got it. Thanks.

quoted

-Serge(y)

quoted

      u32 msg_enable;
      int wolopts;
      int wol_irq;
--
2.34.1