[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20251015172904.GH439570@ragnatech.se>
Date: Wed, 15 Oct 2025 19:29:04 +0200
From: Niklas Söderlund <niklas.soderlund@...natech.se>
To: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
Cc: Paul Barker <paul@...rker.dev>, Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Sergei Shtylyov <sergei.shtylyov@...entembedded.com>,
netdev@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
linux-kernel@...r.kernel.org, Biju Das <biju.das.jz@...renesas.com>,
Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
stable@...r.kernel.org
Subject: Re: [PATCH 1/3] net: ravb: Make DBAT entry count configurable per-SoC
On 2025-10-15 18:05:34 +0100, Lad, Prabhakar wrote:
> Hi Niklas,
>
> Thank you for the review.
>
> On Wed, Oct 15, 2025 at 4:36 PM Niklas Söderlund
> <niklas.soderlund@...natech.se> wrote:
> >
> > Hi Prabhakar,
> >
> > Thanks for your work.
> >
> > On 2025-10-15 16:00:24 +0100, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> > >
> > > The number of CDARq (Current Descriptor Address Register) registers is not
> > > fixed to 22 across all SoC variants. For example, the GBETH implementation
> > > uses only two entries. Hardcoding the value leads to incorrect resource
> > > allocation on such platforms.
> > >
> > > Pass the DBAT entry count through the per-SoC hardware info struct and use
> > > it during probe instead of relying on a fixed constant. This ensures
> > > correct descriptor table sizing and initialization across different SoCs.
> > >
> > > Fixes: feab85c7ccea ("ravb: Add support for RZ/G2L SoC")
> > > Cc: stable@...r.kernel.org
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> >
> > I have not verified with documentation the setting of 2 for
> > gbeth_hw_info. But the change itself is good.
> >
> > > ---
> > > drivers/net/ethernet/renesas/ravb.h | 2 +-
> > > drivers/net/ethernet/renesas/ravb_main.c | 9 +++++++--
> > > 2 files changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> > > index 7b48060c250b..d65cd83ddd16 100644
> > > --- a/drivers/net/ethernet/renesas/ravb.h
> > > +++ b/drivers/net/ethernet/renesas/ravb.h
> > > @@ -1017,7 +1017,6 @@ enum CSR2_BIT {
> > > #define CSR2_CSUM_ENABLE (CSR2_RTCP4 | CSR2_RUDP4 | CSR2_RICMP4 | \
> > > CSR2_RTCP6 | CSR2_RUDP6 | CSR2_RICMP6)
> > >
> > > -#define DBAT_ENTRY_NUM 22
> > > #define RX_QUEUE_OFFSET 4
> > > #define NUM_RX_QUEUE 2
> > > #define NUM_TX_QUEUE 2
> > > @@ -1062,6 +1061,7 @@ struct ravb_hw_info {
> > > u32 rx_max_frame_size;
> > > u32 rx_buffer_size;
> > > u32 rx_desc_size;
> > > + u32 dbat_entry_num;
> >
> > I have been wondering for some time if we shall not start to document
> > these fields as they always take so much time to get back to what each
> > of them represent. How do you feel about starting a header?
> >
> > /**
> > * dbat_entry_num: Describe me here.
> > */
> >
> I agree, let's take this separately into a different patch as it will
> make things easier to backport. What do you think?
Works for me.
>
> Cheers,
> Prabhakar
>
> > Without, but preferably with, this added.
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
> >
> > > unsigned aligned_tx: 1;
> > > unsigned coalesce_irqs:1; /* Needs software IRQ coalescing */
> > >
> > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > > index 9d3bd65b85ff..69d382e8757d 100644
> > > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > > @@ -2694,6 +2694,7 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
> > > .rx_buffer_size = SZ_2K +
> > > SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> > > .rx_desc_size = sizeof(struct ravb_ex_rx_desc),
> > > + .dbat_entry_num = 22,
> > > .aligned_tx = 1,
> > > .gptp = 1,
> > > .nc_queues = 1,
> > > @@ -2717,6 +2718,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
> > > .rx_buffer_size = SZ_2K +
> > > SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> > > .rx_desc_size = sizeof(struct ravb_ex_rx_desc),
> > > + .dbat_entry_num = 22,
> > > .internal_delay = 1,
> > > .tx_counters = 1,
> > > .multi_irqs = 1,
> > > @@ -2743,6 +2745,7 @@ static const struct ravb_hw_info ravb_gen4_hw_info = {
> > > .rx_buffer_size = SZ_2K +
> > > SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> > > .rx_desc_size = sizeof(struct ravb_ex_rx_desc),
> > > + .dbat_entry_num = 22,
> > > .internal_delay = 1,
> > > .tx_counters = 1,
> > > .multi_irqs = 1,
> > > @@ -2769,6 +2772,7 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
> > > .rx_buffer_size = SZ_2K +
> > > SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> > > .rx_desc_size = sizeof(struct ravb_ex_rx_desc),
> > > + .dbat_entry_num = 22,
> > > .multi_irqs = 1,
> > > .err_mgmt_irqs = 1,
> > > .gptp = 1,
> > > @@ -2794,6 +2798,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
> > > .rx_max_frame_size = SZ_8K,
> > > .rx_buffer_size = SZ_2K,
> > > .rx_desc_size = sizeof(struct ravb_rx_desc),
> > > + .dbat_entry_num = 2,
> > > .aligned_tx = 1,
> > > .coalesce_irqs = 1,
> > > .tx_counters = 1,
> > > @@ -3025,7 +3030,7 @@ static int ravb_probe(struct platform_device *pdev)
> > > ravb_parse_delay_mode(np, ndev);
> > >
> > > /* Allocate descriptor base address table */
> > > - priv->desc_bat_size = sizeof(struct ravb_desc) * DBAT_ENTRY_NUM;
> > > + priv->desc_bat_size = sizeof(struct ravb_desc) * info->dbat_entry_num;
> > > priv->desc_bat = dma_alloc_coherent(ndev->dev.parent, priv->desc_bat_size,
> > > &priv->desc_bat_dma, GFP_KERNEL);
> > > if (!priv->desc_bat) {
> > > @@ -3035,7 +3040,7 @@ static int ravb_probe(struct platform_device *pdev)
> > > error = -ENOMEM;
> > > goto out_rpm_put;
> > > }
> > > - for (q = RAVB_BE; q < DBAT_ENTRY_NUM; q++)
> > > + for (q = RAVB_BE; q < info->dbat_entry_num; q++)
> > > priv->desc_bat[q].die_dt = DT_EOS;
> > >
> > > /* Initialise HW timestamp list */
> > > --
> > > 2.43.0
> > >
> >
> > --
> > Kind Regards,
> > Niklas Söderlund
--
Kind Regards,
Niklas Söderlund
Powered by blists - more mailing lists