Re: [PATCH 3/3] net: j2: Introduce J-Core EMAC

[Andrew Lunn <andrew@lunn.ch>]

On Fri, Aug 15, 2025 at 11:14:08PM +0200, Artur Rojek wrote:
> On 2025-08-15 22:52, Artur Rojek wrote:
> > On 2025-08-15 22:16, Andrew Lunn wrote:
> > 
> > Hi Andrew,
> > thanks for the review!
> > 
> > > > +static irqreturn_t jcore_emac_irq(int irq, void *data)
> > > > +{
> > > > +	struct jcore_emac *priv = data;
> > > > +	struct net_device *ndev = priv->ndev;
> > > > +	struct sk_buff *skb;
> > > > +	struct {
> > > > +		int packets;
> > > > +		int bytes;
> > > > +		int dropped;
> > > > +		int crc_errors;
> > > > +	} stats = {};
> > > > +	unsigned int status, pkt_len, i;
> > > 
> > > netdev uses 'reverse christmas tree' for local variables. They should
> > > be sorted longest to shortest. This sometimes means you need to move
> > > assignments into the body of the function, in this case, ndev.
> 
> Should I move the struct stats members into stand alone variables as
> well? Or is below sorting acceptable with regards to stats vs skb:

I would pull the structure definition out of the function. Then just
create one instance of the structure on the stack.

> > > What support is there for MDIO? Normally the MAC driver would not be
> > > setting the carrier status, phylink or phylib would do that.
> > 
> > From what I can tell, none. This is a very simple FPGA RTL
> > implementation of a MAC, and looking at the VHDL, I don't see any MDIO
> > registers.
> 
> > Moreover, the MDIO pin on the PHY IC on my dev board also
> > appears unconnected.
> 
> I spoke too soon on that one. It appears to be connected through a trace
> that goes under the IC. Nevertheless, I don't think MDIO support is in
> the IP core design.

MDIO is actually two pins. MDC and MDIO.

It might be there is a second IP core which implements MDIO. There is
no reason it needs to be tightly integrated into the MAC. But it does
make the MAC driver slightly more complex. You then need a Linux MDIO
bus driver for it, and the DT for the MAC would include a phy-handle
property pointing to the PHY on the MDIO bus.

Is there an Ethernet PHY on your board?

	Andrew