Re: [PATCH 10/11] net: macb: Add support for RP1's MACB variant

[Andrea della Porta <andrea.porta@suse.com>]

Hi Florian,

On 10:01 Wed 21 Aug     , Florian Fainelli wrote:
> On 8/20/24 07:36, Andrea della Porta wrote:
> > RaspberryPi RP1 contains Cadence's MACB core. Implement the
> > changes to be able to operate the customization in the RP1.
> > 
> > Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> 
> You are doing a lot of things, all at once, and you should consider
> extracting your change into a smaller subset with bug fixes first:
> 
> - one commit which writes to the RBQPH the upper 32-bits of the RX ring DMA
> address, that looks like a bug fix
> 
> - one commit which retriggers a buffer read, even though that appears to be
> RP1 specific maybe, if not, then this is also a bug fix
> 
> - one commit that adds support for macb_shutdown() to kill DMA operations
> 
> - one commit which adds support for a configurable PHY reset line + delay
> specified in milli seconds
> 
> - one commit which adds support for controling the interrupt coalescing
> settings
> 
> And then you can add all of the RP1 specific bits like the AXI bridge
> configuration.
> 
> [snip]
> 
> > @@ -1228,6 +1246,7 @@ struct macb_queue {
> >   	dma_addr_t		tx_ring_dma;
> >   	struct work_struct	tx_error_task;
> >   	bool			txubr_pending;
> > +	bool			tx_pending;
> >   	struct napi_struct	napi_tx;
> >   	dma_addr_t		rx_ring_dma;
> > @@ -1293,9 +1312,15 @@ struct macb {
> >   	u32			caps;
> >   	unsigned int		dma_burst_length;
> > +	u8			aw2w_max_pipe;
> > +	u8			ar2r_max_pipe;
> > +	bool			use_aw2b_fill;
> >   	phy_interface_t		phy_interface;
> > +	struct gpio_desc	*phy_reset_gpio;
> > +	int			phy_reset_ms;
> 
> The delay cannot be negative, so this needs to be unsigned int.
> 
> > +
> >   	/* AT91RM9200 transmit queue (1 on wire + 1 queued) */
> >   	struct macb_tx_skb	rm9200_txq[2];
> >   	unsigned int		max_tx_length;
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 11665be3a22c..5eb5be6c96fc 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -41,6 +41,9 @@
> >   #include <linux/inetdevice.h>
> >   #include "macb.h"
> > +static unsigned int txdelay = 35;
> > +module_param(txdelay, uint, 0644);
> > +
> >   /* This structure is only used for MACB on SiFive FU540 devices */
> >   struct sifive_fu540_macb_mgmt {
> >   	void __iomem *reg;
> > @@ -334,7 +337,7 @@ static int macb_mdio_wait_for_idle(struct macb *bp)
> >   	u32 val;
> >   	return readx_poll_timeout(MACB_READ_NSR, bp, val, val & MACB_BIT(IDLE),
> > -				  1, MACB_MDIO_TIMEOUT);
> > +				  100, MACB_MDIO_TIMEOUT);
> 
> Why do we need to increase how frequently we poll?

Thanks for your feedback, I will save all your precious suggestions for a future patch that
will enable the macb ethernet.
As stated in the cover letter, right now this specific patch is not intended to be upstreamed
as is but it's just here for testing purposes, hence its 'raw' state.
For sure the ethernet contained in RP1 will be one of the first device I will try to bring
upstream, so I'll apply your comments there. Maybe the next time I will also add a better
explanation about the state of a specific patch in the commit comment itself, and not only
in the cover letter, just to be more explicit.

Many thanks,
Andrea 

> -- 
> Florian
>