RE: [PATCH net-next v2 2/2] net: macb: Add support for partial store and forward

[Somisetty, Pranavi]

> -----Original Message-----
> From: Claudiu.Beznea@xxxxxxxxxxxxx <Claudiu.Beznea@xxxxxxxxxxxxx>
> Sent: Friday, May 12, 2023 1:28 PM
> To: Somisetty, Pranavi <pranavi.somisetty@xxxxxxx>;
> Nicolas.Ferre@xxxxxxxxxxxxx; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> richardcochran@xxxxxxxxx; linux@xxxxxxxxxxxxxxx; palmer@xxxxxxxxxxx
> Cc: git (AMD-Xilinx) <git@xxxxxxx>; Simek, Michal
> <michal.simek@xxxxxxx>; Katakam, Harini <harini.katakam@xxxxxxx>;
> Pandey, Radhey Shyam <radhey.shyam.pandey@xxxxxxx>;
> netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> riscv@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next v2 2/2] net: macb: Add support for partial store
> and forward
> 
> On 11.05.2023 10:12, Pranavi Somisetty wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > the content is safe
> >
> > When the receive partial store and forward mode is activated, the
> > receiver will only begin to forward the packet to the external AHB or
> > AXI slave when enough packet data is stored in the packet buffer.
> > The amount of packet data required to activate the forwarding process
> > is programmable via watermark registers which are located at the same
> > address as the partial store and forward enable bits. Adding support
> > to read this rx-watermark value from device-tree, to program the
> > watermark registers and enable partial store and forwarding.
> >
> > Signed-off-by: Maulik Jodhani <maulik.jodhani@xxxxxxxxxx>
> > Signed-off-by: Michal Simek <michal.simek@xxxxxxxxxx>
> > Signed-off-by: Harini Katakam <harini.katakam@xxxxxxxxxx>
> > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xxxxxxxxxx>
> > Signed-off-by: Pranavi Somisetty <pranavi.somisetty@xxxxxxx>
> > ---

<snip>
> > +       /* Disable RX partial store and forward and reset watermark value */
> > +       if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD) {
> > +               watermark_reset_value = (1 << (GEM_BFEXT(RX_PBUF_ADDR,
> > + gem_readl(bp, DCFG2)))) - 1;
> 
> Is this block needed? Maybe all you need here is just to disable the rx partial
> store and forward?

Yes, the value doesn’t need to be written, disabling rx partial store and forward
is enough, will take care.
<snip>
> 
> > +                       retval = of_property_read_u16(bp->pdev->dev.of_node,
> > +                                                     "rx-watermark",
> > +
> > + &bp->rx_watermark);
> 
> E.g. SAMA7G5 has PBUFRXCUT.watermark on 10 bits. Is it the same on
> Xynqmp?

On ZynqMP its 12 bits.

> For compatibility with future implementations and stable DT interface it
> would be better to just keep rx-watermark DT property on 32 bits.
> 

I don’t think it can extend beyond u16, considering, the maximum pbuf depth
field in DCFG2, is only 4 bits (22:25). 

I'll do a re-spin addressing your and other reviewer's comments.

Thanks
Pranavi