Add LAN78XX OTP_ACCESS flag support

With this flag we can now use ethtool to access the OTP:
ethtool --set-priv-flags eth0 OTP_ACCESS on
ethtool -e eth0  # this will read OTP if OTP_ACCESS is on, else EEPROM

When writing to OTP we need to set OTP_ACCESS on and write with the correct magic 0x7873 for OTP
---
 drivers/net/usb/lan78xx.c | 55 ++++++++++++++++++++++++++++++++-------
 1 file changed, 45 insertions(+), 10 deletions(-)

On Sat, Oct 26, 2024 at 01:05:46AM +0200, Fabian Benschuh wrote:
> With this flag we can now use ethtool to access the OTP:
> ethtool --set-priv-flags eth0 OTP_ACCESS on
> ethtool -e eth0  # this will read OTP if OTP_ACCESS is on, else EEPROM
> 
> When writing to OTP we need to set OTP_ACCESS on and write with the correct magic 0x7873 for OTP


Please can you tell us more about OTP vs EEPROM? Is the OTP internal
while the EEPROM is external? What is contained in each? How does the
device decide which to use when it finds it has both?

I'm just wondering if we even need a private flag, if the hardware
will use one or the other exclusively?

	Andrew

> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Monday, October 28, 2024 8:38 AM
> 
> On Sat, Oct 26, 2024 at 01:05:46AM +0200, Fabian Benschuh wrote:
> > With this flag we can now use ethtool to access the OTP:
> > ethtool --set-priv-flags eth0 OTP_ACCESS on ethtool -e eth0  # this
> > will read OTP if OTP_ACCESS is on, else EEPROM
> >
> > When writing to OTP we need to set OTP_ACCESS on and write with the
> > correct magic 0x7873 for OTP
> 
> Please can you tell us more about OTP vs EEPROM? Is the OTP internal while the EEPROM is external?
> What is contained in each? How does the device decide which to use when it finds it has both?
> 
>         Andrew

This is pretty much the same implementation that is already in place for the Linux driver of the LAN743x PCIe device.

OTP (one time programmable) is a configuration memory internal to the device. EEPROM is external.
The internal OTP memory always exists but it may not be programmed. The EEPROM is optional, and if present can also be programmed or not.
A signature byte at offset 0 indicates whether the OTP or EEPROM device is programmed. 
If present, EEPROM has higher priority.
More info @ Chapter 10 here;
https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/LAN7800-Data-Sheet-DS00001992H.pdf

> I'm just wondering if we even need a private flag, if the hardware will use one or the other exclusively?
>
Yes, both can be present/used, so we need the flag so we can tell the destination of a write or source of a read.

Ronnie

On Mon, Oct 28, 2024 at 03:02:44PM +0000, Ronnie.Kunin@microchip.com wrote:
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Monday, October 28, 2024 8:38 AM
> > 
> > On Sat, Oct 26, 2024 at 01:05:46AM +0200, Fabian Benschuh wrote:
> > > With this flag we can now use ethtool to access the OTP:
> > > ethtool --set-priv-flags eth0 OTP_ACCESS on ethtool -e eth0  # this
> > > will read OTP if OTP_ACCESS is on, else EEPROM
> > >
> > > When writing to OTP we need to set OTP_ACCESS on and write with the
> > > correct magic 0x7873 for OTP
> > 
> > Please can you tell us more about OTP vs EEPROM? Is the OTP internal while the EEPROM is external?
> > What is contained in each? How does the device decide which to use when it finds it has both?
> > 
> >         Andrew
> 

> This is pretty much the same implementation that is already in place
> for the Linux driver of the LAN743x PCIe device.

That is good, it gives some degree of consistency. But i wounder if we
should go further. I doubt these are the only two devices which
support both EEPROM and OTP. It would be nicer to extend ethtool:

       ethtool -e|--eeprom-dump devname [raw on|off] [offset N] [length N] [otp] [eeprom]

There does not appear to be a netlink implementation of this ethtool
call. If we add one, we can add an additional optional attribute,
indicating OTP or EEPROM. We have done similar in the past. It
probably means within the kernel you replace struct ethtool_eeprom
with struct kernel_ethtool_eeprom which has the additional
parameter. The ioctl interface then never sees the new parameter,
which keeps with the kAPI. get_eeprom() and set_eeprom() probably have
all they need. get_eeprom_len() is more complex since it currently
only takes netdev. I think get_eeprom_len() needs its functionality
extended to indicate if the driver actually looks at the additional
parameter. We want the kAPI calls for get and set to failed with
-EOPNOTSUPP when otp or eeprom is not supported, which will initially
be 99% of the drivers. And we don't want to have to make proper code
changes to every driver. So maybe an additional parameter

	int	(*get_eeprom_len)(struct net_device *,
	                          struct kernel_eeprom_len *eeprom_len);

struct kernel_eeprom_len {
	int otp;
	int eeprom;
}

Have the core zero this. After the call, if they are still zero, they
are not supported.

I know this is a lot more work than your current patch, but it is a
better solution, should be well documented, easy to find and should
work for everybody, against a device private parameter which is not
obvious and unlikely to be consistent across drivers.

	Andrew

Thanks Andrew.

Adding Rakesh who has been working on this patch internally at Microchip (though for an earlier kernel version) and after completing that was going to be upstreaming it for net-next (Fabian beat him to it). 

Ronnie

-----Original Message-----
From: Andrew Lunn <andrew@lunn.ch> 
Sent: Monday, October 28, 2024 3:19 PM
To: Ronnie Kunin - C21729 <Ronnie.Kunin@microchip.com>
Cc: Fabi.Benschuh@fau.de; Woojung Huh - C21699 <Woojung.Huh@microchip.com>; UNGLinuxDriver <UNGLinuxDriver@microchip.com>; andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; netdev@vger.kernel.org; linux-usb@vger.kernel.org
Subject: Re: [PATCH] Add LAN78XX OTP_ACCESS flag support

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On Mon, Oct 28, 2024 at 03:02:44PM +0000, Ronnie.Kunin@microchip.com wrote:
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Monday, October 28, 2024 8:38 AM
> >
> > On Sat, Oct 26, 2024 at 01:05:46AM +0200, Fabian Benschuh wrote:
> > > With this flag we can now use ethtool to access the OTP:
> > > ethtool --set-priv-flags eth0 OTP_ACCESS on ethtool -e eth0  # 
> > > this will read OTP if OTP_ACCESS is on, else EEPROM
> > >
> > > When writing to OTP we need to set OTP_ACCESS on and write with 
> > > the correct magic 0x7873 for OTP
> >
> > Please can you tell us more about OTP vs EEPROM? Is the OTP internal while the EEPROM is external?
> > What is contained in each? How does the device decide which to use when it finds it has both?
> >
> >         Andrew
>

> This is pretty much the same implementation that is already in place 
> for the Linux driver of the LAN743x PCIe device.

That is good, it gives some degree of consistency. But i wounder if we should go further. I doubt these are the only two devices which support both EEPROM and OTP. It would be nicer to extend ethtool:

       ethtool -e|--eeprom-dump devname [raw on|off] [offset N] [length N] [otp] [eeprom]

There does not appear to be a netlink implementation of this ethtool call. If we add one, we can add an additional optional attribute, indicating OTP or EEPROM. We have done similar in the past. It probably means within the kernel you replace struct ethtool_eeprom with struct kernel_ethtool_eeprom which has the additional parameter. The ioctl interface then never sees the new parameter, which keeps with the kAPI. get_eeprom() and set_eeprom() probably have all they need. get_eeprom_len() is more complex since it currently only takes netdev. I think get_eeprom_len() needs its functionality extended to indicate if the driver actually looks at the additional parameter. We want the kAPI calls for get and set to failed with -EOPNOTSUPP when otp or eeprom is not supported, which will initially be 99% of the drivers. And we don't want to have to make proper code changes to every driver. So maybe an additional parameter

        int     (*get_eeprom_len)(struct net_device *,
                                  struct kernel_eeprom_len *eeprom_len);

struct kernel_eeprom_len {
        int otp;
        int eeprom;
}

Have the core zero this. After the call, if they are still zero, they are not supported.

I know this is a lot more work than your current patch, but it is a better solution, should be well documented, easy to find and should work for everybody, against a device private parameter which is not obvious and unlikely to be consistent across drivers.

        Andrew

On Tue, 29 Oct 2024 22:14:12 +0100 Andrew Lunn wrote:
> > > That is good, it gives some degree of consistency. But i wounder if we
> > > should go further. I doubt these are the only two devices which
> > > support both EEPROM and OTP. It would be nicer to extend ethtool:
> > > 
> > >        ethtool -e|--eeprom-dump devname [raw on|off] [offset N] [length N] [otp] [eeprom]  
> > 
> > After a cursory look at the conversation I wonder if it wouldn't 
> > be easier to register devlink regions for eeprom and otp?  
> 
> devlink regions don't allow write. ethtool does.

Sorry I missed the write part.
I see you already asked the "why" but I don't think the answer 
is entirely to the point. We need to know more - netdev focuses on
production use cases. Burning an OTP seems like a manufacturing action.