RE: [PATCH net-next 05/18] ravb: Replace chip type with a structure for driver data

Hi Sergei,

Thanks for the feedback.

Biju Das [off-list ref]; Prabhakar Mahadev Lad
[off-list ref]
Subject: Re: [PATCH net-next 05/18] ravb: Replace chip type with a
structure for driver data

Hello!

On 7/22/21 5:13 PM, Biju Das wrote:

quoted

The DMAC and EMAC blocks of Gigabit Ethernet IP is almost similar to
Ethernet AVB. With few changes in driver we can support both the IP.

This patch is in preparation for supporting the same by replacing chip
type by a structure with values, feature bits and function pointers.

Currently only values is added to structure and later patches will add
features and function pointers.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      | 14 +++++
 drivers/net/ethernet/renesas/ravb_main.c | 76
+++++++++++++++++-------
 2 files changed, 67 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h

b/drivers/net/ethernet/renesas/ravb.h
index 80e62ca2e3d3..0ed21262f26b 100644

--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h

@@ -988,6 +988,18 @@ enum ravb_chip_id {
 	RCAR_GEN3,
 };

+struct ravb_drv_data {

   I'd rather suggest *struct* ravb_hw_info... This is hardly a driver
data, more like hwrdware's one. :-)

OK.

quoted

+	netdev_features_t net_features;
+	netdev_features_t net_hw_features;
+	const char (*gstrings_stats)[ETH_GSTRING_LEN];
+	size_t gstrings_size;
+	size_t stats_len;
+	u32 num_gstat_queue;
+	size_t skb_sz;
+	u8 num_tx_desc;
+	enum ravb_chip_id chip_id;

   Mhm, I'd expect that chip_id is no longer needed with the feature
structs...

Yes,If you see the subsequent 3 patches, chip_id usage is completely removed from
ravb_main.c. but it is still required for ravb_ptp.c. Please let me know do you want
me take out from there as well. Then as part of [1], I can take out chipid completely.

[1]
https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210722141351.13668-7-biju.das.jz@bp.renesas.com/

[...]

quoted

@@ -1040,6 +1052,8 @@ struct ravb_private {
 	unsigned txcidm:1;		/* TX Clock Internal Delay Mode */
 	unsigned rgmii_override:1;	/* Deprecated rgmii-*id behavior */
 	int num_tx_desc;		/* TX descriptors per packet */
+
+	const struct ravb_drv_data *info;

   So data or info? :-)

As we are going to use ravb_hw_info, it will be info.

[...]

quoted

diff --git a/drivers/net/ethernet/renesas/ravb_main.c

b/drivers/net/ethernet/renesas/ravb_main.c
index 805397088850..84ebd6fef711 100644

--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c

[...]

quoted

@@ -1176,9 +1179,12 @@ static void ravb_get_ethtool_stats(struct

net_device *ndev,

 static void ravb_get_strings(struct net_device *ndev, u32 stringset,
u8 *data)  {
+	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
+
 	switch (stringset) {
 	case ETH_SS_STATS:
-		memcpy(data, ravb_gstrings_stats,

sizeof(ravb_gstrings_stats));

quoted

+		memcpy(data, info->gstrings_stats, info->gstrings_size);
 		break;
 	}
 }

@@ -1924,12 +1930,36 @@ static int ravb_mdio_release(struct ravb_private

*priv)

quoted

 	return 0;
 }

+static const struct ravb_drv_data ravb_gen3_data = {
+	.net_features = NETIF_F_RXCSUM,
+	.net_hw_features = NETIF_F_RXCSUM,
+	.gstrings_stats = ravb_gstrings_stats,
+	.gstrings_size = sizeof(ravb_gstrings_stats),
+	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),
+	.num_gstat_queue = NUM_RX_QUEUE,
+	.skb_sz = RX_BUF_SZ + RAVB_ALIGN - 1,
+	.num_tx_desc = NUM_TX_DESC_GEN3,
+	.chip_id = RCAR_GEN3,
+};
+
+static const struct ravb_drv_data ravb_gen2_data = {
+	.net_features = NETIF_F_RXCSUM,
+	.net_hw_features = NETIF_F_RXCSUM,

   Mhm, why have the fields that don't change from SoC to SoC anyway?
I do think they should be added when a new SoC support is added...

This is a preparation patch for supporting RZ/G2L and there is a difference w.r.to RZ/G2L
See [2]. That is the reason to isolate the new SoC changes w.r.to existing one it is added
Here. Andrew also suggested to make smaller changes.

[2]
https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210722141351.13668-18-biju.das.jz@bp.renesas.com/

quoted

+	.gstrings_stats = ravb_gstrings_stats,
+	.gstrings_size = sizeof(ravb_gstrings_stats),
+	.stats_len = ARRAY_SIZE(ravb_gstrings_stats),

   Same question...

Same as above.

quoted

+	.num_gstat_queue = NUM_RX_QUEUE,
+	.skb_sz = RX_BUF_SZ + RAVB_ALIGN - 1,

   Again why?

Same as above.

quoted

+	.num_tx_desc = NUM_TX_DESC_GEN2,
+	.chip_id = RCAR_GEN2,
+};
+

[...]

quoted

@@ -2052,15 +2082,15 @@ static int ravb_probe(struct platform_device

*pdev)

quoted

 	if (!ndev)
 		return -ENOMEM;

-	ndev->features = NETIF_F_RXCSUM;
-	ndev->hw_features = NETIF_F_RXCSUM;
+	info = of_device_get_match_data(&pdev->dev);
+
+	ndev->features = info->net_features;
+	ndev->hw_features = info->net_hw_features;

 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);

-	chip_id = (enum ravb_chip_id)of_device_get_match_data(&pdev->dev);
-
-	if (chip_id == RCAR_GEN3)
+	if (info->chip_id == RCAR_GEN3)

   Ugh...

This chip id is replaced in subsequent patch [3]

[3]
https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210722141351.13668-8-biju.das.jz@bp.renesas.com/

quoted

 		irq = platform_get_irq_byname(pdev, "ch22");
 	else
 		irq = platform_get_irq(pdev, 0);

[...]

quoted

@@ -2099,7 +2130,7 @@ static int ravb_probe(struct platform_device

*pdev)

quoted

 	priv->avb_link_active_low =
 		of_property_read_bool(np, "renesas,ether-link-active-low");

-	if (chip_id == RCAR_GEN3) {
+	if (info->chip_id == RCAR_GEN3) {

   Ugh...

Same as above.

quoted

 		irq = platform_get_irq_byname(pdev, "ch24");
 		if (irq < 0) {
 			error = irq;

[...]

quoted

@@ -2184,7 +2214,7 @@ static int ravb_probe(struct platform_device

*pdev)

quoted

 	INIT_LIST_HEAD(&priv->ts_skb_list);

 	/* Initialise PTP Clock driver */
-	if (chip_id != RCAR_GEN2)
+	if (info->chip_id != RCAR_GEN2)
 		ravb_ptp_init(ndev, pdev);

   Ugh...

Same as above.

quoted

 	/* Debug message level */

@@ -2232,7 +2262,7 @@ static int ravb_probe(struct platform_device

*pdev)

quoted

 			  priv->desc_bat_dma);

 	/* Stop PTP Clock driver */
-	if (chip_id != RCAR_GEN2)
+	if (info->chip_id != RCAR_GEN2)
 		ravb_ptp_stop(ndev);

  Ugh...

Same as above.

Regards,
Biju

quoted

 out_disable_refclk:
 	clk_disable_unprepare(priv->refclk);

MBR, Sergei