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

[Artur Rojek <contact@artur-rojek.eu>]

On 2025-08-16 00:38, Andrew Lunn wrote:
> 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.

Makes sense, thanks.

> 
>> > > 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?

Yes, it's an IC+ IP101ALF 10/100 Ethernet PHY [1]. It does have both MDC
and MDIO pins connected, however I suspect that nothing really
configures it, and it simply runs on default register values (which
allow for valid operation in 100Mb/s mode, it seems). I doubt there is
another IP core to handle MDIO, as this SoC design is optimized for
minimal utilization of FPGA blocks. Does it make sense to you that a MAC
could run without any access to an MDIO bus?

If neither Rob L. or Jeff clarify on this topic, I'll hook up a logic
analyzer to the MDIO bus and see if anything (e.g. loader firmware)
touches it at any point.

Cheers,
Artur

[1] https://www.micros.com.pl/mediaserver/info-uiip101a.pdf

> 
> 	Andrew