Hi Sergei,
Thanks for the feedback.
> Biju Das <biju.das@xxxxxxxxxxxxxx>; Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> 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:
>
> > 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@xxxxxxxxxxxxxx>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > 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.
>
> > + 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@xxxxxxxxxxxxxx/
>
> [...]
> > @@ -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.
>
> [...]
> > 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
> [...]
> > @@ -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));
> > + memcpy(data, info->gstrings_stats, info->gstrings_size);
> > break;
> > }
> > }
> > @@ -1924,12 +1930,36 @@ static int ravb_mdio_release(struct ravb_private
> *priv)
> > 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@xxxxxxxxxxxxxx/
>
> > + .gstrings_stats = ravb_gstrings_stats,
> > + .gstrings_size = sizeof(ravb_gstrings_stats),
> > + .stats_len = ARRAY_SIZE(ravb_gstrings_stats),
>
> Same question...
Same as above.
> > + .num_gstat_queue = NUM_RX_QUEUE,
> > + .skb_sz = RX_BUF_SZ + RAVB_ALIGN - 1,
>
> Again why?
Same as above.
>
> > + .num_tx_desc = NUM_TX_DESC_GEN2,
> > + .chip_id = RCAR_GEN2,
> > +};
> > +
> [...]
> > @@ -2052,15 +2082,15 @@ static int ravb_probe(struct platform_device
> *pdev)
> > 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@xxxxxxxxxxxxxx/
>
> > irq = platform_get_irq_byname(pdev, "ch22");
> > else
> > irq = platform_get_irq(pdev, 0);
> [...]
> > @@ -2099,7 +2130,7 @@ static int ravb_probe(struct platform_device
> *pdev)
> > 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.
>
> > irq = platform_get_irq_byname(pdev, "ch24");
> > if (irq < 0) {
> > error = irq;
> [...]
> > @@ -2184,7 +2214,7 @@ static int ravb_probe(struct platform_device
> *pdev)
> > 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.
>
> >
> > /* Debug message level */
> > @@ -2232,7 +2262,7 @@ static int ravb_probe(struct platform_device
> *pdev)
> > 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
>
>
> > out_disable_refclk:
> > clk_disable_unprepare(priv->refclk);
>
> MBR, Sergei
