回覆: [PATCH] net: ftgmac100: Add Aspeed AST2700 support

[Jacky Chou]

Hello Joel,

Thanks for your reply.

> >
> > Add support of Aspeed AST2700 SoC.  AST2700 is based on ARM64 so
> > modify the DMA address related code to fit both ARM and ARM64.
> > Besides, the RMII/RGMII mode control register is moved from SCU500 to
> > MAC50 so initialize the register in ftgmac100_start correspondingly.
> 
> It looks like these changes will affect the older platforms. Have you tested on
> an ast2600?

Yes. We have verified on AST2600 and AST2700. These changes do not affect.

> 
> Is it okay to be writing to the new memory locations on the old platforms? If
> this is the case, please mention it in the commit message.

Yes. I will refine the commit message to add more information in the next version.

> 
> > diff --git a/drivers/net/ftgmac100.c b/drivers/net/ftgmac100.c index
> > 9fb8c12838..a909a0b309 100644
> > --- a/drivers/net/ftgmac100.c
> > +++ b/drivers/net/ftgmac100.c
> 
> > @@ -321,6 +333,7 @@ static int ftgmac100_start(struct udevice *dev)
> >         struct eth_pdata *plat = dev_get_plat(dev);
> >         struct ftgmac100_data *priv = dev_get_priv(dev);
> >         struct ftgmac100 *ftgmac100 = priv->iobase;
> > +       union ftgmac100_dma_addr dma_addr = {.hi = 0, .lo = 0};
> 
> You always set dma_addr before using it, so this zero-init is unnecessary.

Agree. I checked this part again. It does not need to initialize the variable to zero.
I will remove it in the next version. Thanks for your kindly reminder.

> 
> > @@ -352,7 +366,14 @@ static int ftgmac100_start(struct udevice *dev)
> >         flush_dcache_range(start, end);
> >
> >         for (i = 0; i < PKTBUFSRX; i++) {
> > -               priv->rxdes[i].rxdes3 = (unsigned int)net_rx_packets[i];
> > +               unsigned int ip_align = 0;
> > +
> > +               dma_addr.addr = (dma_addr_t)net_rx_packets[i];
> > +               priv->rxdes[i].rxdes2 =
> FIELD_PREP(FTGMAC100_RXDES2_RXBUF_BADR_HI, dma_addr.hi);
> > +               /* For IP alignment */
> > +               if ((dma_addr.lo & (PKTALIGN - 1)) == 0)
> > +                       ip_align = 2;
> > +               priv->rxdes[i].rxdes3 = dma_addr.lo + ip_align;
> 
> Please explain why the old code didn't need alignment added, and the new
> version does.

Sorry. I forgot the driver no longer need to support IP alignmen.
IP alignment was originally supported because the NCSI PHY driver would crash.
I have fixed the NCSI PHY driver and have submitted fixed patch to U-boot.
And it has accepted. Therefore, I will remove this part in the next version.

> 
> > @@ -394,6 +419,10 @@ static int ftgmac100_start(struct udevice *dev)
> >                 FTGMAC100_MACCR_RX_RUNT |
> >                 FTGMAC100_MACCR_RX_BROADPKT;
> >
> > +       if (priv->is_ast2700 && (priv->phydev->interface ==
> PHY_INTERFACE_MODE_RMII ||
> > +                                priv->phydev->interface ==
> > + PHY_INTERFACE_MODE_NCSI))
> 
> Use device_is_compatible(dev, "aspeed,ast2700-mac") instead of adding
> is_ast2700.

Agree. I will adjust this code using device_is_compatible() in the next version.

> 
> > +               maccr |= FTGMAC100_MACCR_RMII_ENABLE;
> > +
> >         writel(maccr, &ftgmac100->maccr);
> >
> >         ret = phy_startup(phydev);
> > @@ -443,9 +472,11 @@ static int ftgmac100_recv(struct udevice *dev, int
> flags, uchar **packetp)
> >         ulong des_start = ((ulong)curr_des) & ~(ARCH_DMA_MINALIGN -
> 1);
> >         ulong des_end = des_start +
> >                 roundup(sizeof(*curr_des), ARCH_DMA_MINALIGN);
> > -       ulong data_start = curr_des->rxdes3;
> > +       union ftgmac100_dma_addr data_start = { .lo = 0, .hi = 0 };
> 
> Same here.

I will remove it in the next version. Thanks for your kindly reminder.

> 
> >         ulong data_end;
> >
> > +       data_start.hi = FIELD_GET(FTGMAC100_RXDES2_RXBUF_BADR_HI,
> curr_des->rxdes2);
> > +       data_start.lo = curr_des->rxdes3;
> >         invalidate_dcache_range(des_start, des_end);

Thanks,
Jacky