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@xxxxxxxx>
>
> 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
>
