[PATCH v1 net-next] lan743x: Provide Read/Write Access to on chip OTP

[PATCH v1 net-next] lan743x: Provide Read/Write Access to on chip OTP

[Bryan Whitehead]

The LAN743x includes on chip One-Time-Programmable (OTP) memory.

This patch extends the ethtool EEPROM read/write interface to
access OTP memory space.

This is done by adding the private flag OTP_ACCESS, which is used
to switch between EEPROM, and OTP modes.

The private flag OTP_ACCESS is configurable through the
  ethtool --set-priv-flags command.
And visible through the
  ethtool --show-priv-flags command.

Signed-off-by: Bryan Whitehead <Bryan.Whitehead@microchip.com>
---
 drivers/net/ethernet/microchip/lan743x_ethtool.c | 205 +++++++++++++++++------
 drivers/net/ethernet/microchip/lan743x_main.h    |  17 +-
 2 files changed, 170 insertions(+), 52 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index 07c1eb6..c69cc99 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -14,61 +14,132 @@
 #define EEPROM_INDICATOR_1		    (0xA5)
 #define EEPROM_INDICATOR_2		    (0xAA)
 #define EEPROM_MAC_OFFSET		    (0x01)
-#define MAX_EEPROM_SIZE			    512
+#define MAX_EEPROM_SIZE			    (512)
+#define MAX_OTP_SIZE			    (1024)
 #define OTP_INDICATOR_1			    (0xF3)
 #define OTP_INDICATOR_2			    (0xF7)
 
-static int lan743x_otp_write(struct lan743x_adapter *adapter, u32 offset,
-			     u32 length, u8 *data)
+static int lan743x_otp_power_up(struct lan743x_adapter *adapter)
+{
+	u32 reg_value;
+
+	reg_value = lan743x_csr_read(adapter, OTP_PWR_DN);
+
+	if (reg_value & OTP_PWR_DN_PWRDN_N_) {
+		/* clear it and wait to be cleared */
+		reg_value &= ~OTP_PWR_DN_PWRDN_N_;
+		lan743x_csr_write(adapter, OTP_PWR_DN, reg_value);
+
+		usleep_range(100, 20000);
+	}
+
+	return 0;
+}
+
+static void lan743x_otp_power_down(struct lan743x_adapter *adapter)
+{
+	u32 reg_value;
+
+	reg_value = lan743x_csr_read(adapter, OTP_PWR_DN);
+	if (!(reg_value & OTP_PWR_DN_PWRDN_N_)) {
+		/* set power down bit */
+		reg_value |= OTP_PWR_DN_PWRDN_N_;
+		lan743x_csr_write(adapter, OTP_PWR_DN, reg_value);
+	}
+}
+
+static void lan743x_otp_set_address(struct lan743x_adapter *adapter,
+				    u32 address)
+{
+	lan743x_csr_write(adapter, OTP_ADDR_HIGH, (address >> 8) & 0x03);
+	lan743x_csr_write(adapter, OTP_ADDR_LOW, address & 0xFF);
+}
+
+static void lan743x_otp_read_go(struct lan743x_adapter *adapter)
+{
+	lan743x_csr_write(adapter, OTP_FUNC_CMD, OTP_FUNC_CMD_READ_);
+	lan743x_csr_write(adapter, OTP_CMD_GO, OTP_CMD_GO_GO_);
+}
+
+static int lan743x_otp_wait_till_not_busy(struct lan743x_adapter *adapter)
 {
 	unsigned long timeout;
-	u32 buf;
+	u32 reg_val;
+
+	timeout = jiffies + HZ;
+	do {
+		if (time_after(jiffies, timeout)) {
+			netif_warn(adapter, drv, adapter->netdev,
+				   "Timeout on OTP_STATUS completion\n");
+			return -EIO;
+		}
+		udelay(1);
+		reg_val = lan743x_csr_read(adapter, OTP_STATUS);
+	} while (reg_val & OTP_STATUS_BUSY_);
+
+	return 0;
+}
+
+static int lan743x_otp_read(struct lan743x_adapter *adapter, u32 offset,
+			    u32 length, u8 *data)
+{
+	int ret;
 	int i;
 
-	buf = lan743x_csr_read(adapter, OTP_PWR_DN);
+	ret = lan743x_otp_power_up(adapter);
+	if (ret < 0)
+		return ret;
 
-	if (buf & OTP_PWR_DN_PWRDN_N_) {
-		/* clear it and wait to be cleared */
-		lan743x_csr_write(adapter, OTP_PWR_DN, 0);
-
-		timeout = jiffies + HZ;
-		do {
-			udelay(1);
-			buf = lan743x_csr_read(adapter, OTP_PWR_DN);
-			if (time_after(jiffies, timeout)) {
-				netif_warn(adapter, drv, adapter->netdev,
-					   "timeout on OTP_PWR_DN completion\n");
-				return -EIO;
-			}
-		} while (buf & OTP_PWR_DN_PWRDN_N_);
+	ret = lan743x_otp_wait_till_not_busy(adapter);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < length; i++) {
+		lan743x_otp_set_address(adapter, offset + i);
+
+		lan743x_otp_read_go(adapter);
+		ret = lan743x_otp_wait_till_not_busy(adapter);
+		if (ret < 0)
+			return ret;
+		data[i] = lan743x_csr_read(adapter, OTP_READ_DATA);
 	}
 
+	lan743x_otp_power_down(adapter);
+
+	return 0;
+}
+
+static int lan743x_otp_write(struct lan743x_adapter *adapter, u32 offset,
+			     u32 length, u8 *data)
+{
+	int ret;
+	int i;
+
+	ret = lan743x_otp_power_up(adapter);
+	if (ret < 0)
+		return ret;
+
+	ret = lan743x_otp_wait_till_not_busy(adapter);
+	if (ret < 0)
+		return ret;
+
 	/* set to BYTE program mode */
 	lan743x_csr_write(adapter, OTP_PRGM_MODE, OTP_PRGM_MODE_BYTE_);
 
 	for (i = 0; i < length; i++) {
-		lan743x_csr_write(adapter, OTP_ADDR1,
-				  ((offset + i) >> 8) &
-				  OTP_ADDR1_15_11_MASK_);
-		lan743x_csr_write(adapter, OTP_ADDR2,
-				  ((offset + i) &
-				  OTP_ADDR2_10_3_MASK_));
+		lan743x_otp_set_address(adapter, offset + i);
+
 		lan743x_csr_write(adapter, OTP_PRGM_DATA, data[i]);
 		lan743x_csr_write(adapter, OTP_TST_CMD, OTP_TST_CMD_PRGVRFY_);
 		lan743x_csr_write(adapter, OTP_CMD_GO, OTP_CMD_GO_GO_);
 
-		timeout = jiffies + HZ;
-		do {
-			udelay(1);
-			buf = lan743x_csr_read(adapter, OTP_STATUS);
-			if (time_after(jiffies, timeout)) {
-				netif_warn(adapter, drv, adapter->netdev,
-					   "Timeout on OTP_STATUS completion\n");
-				return -EIO;
-			}
-		} while (buf & OTP_STATUS_BUSY_);
+		ret = lan743x_otp_wait_till_not_busy(adapter);
+		if (ret < 0)
+			return ret;
 	}
 
+	lan743x_otp_power_down(adapter);
+
 	return 0;
 }
 
@@ -207,6 +278,11 @@ static void lan743x_ethtool_set_msglevel(struct net_device *netdev,
 
 static int lan743x_ethtool_get_eeprom_len(struct net_device *netdev)
 {
+	struct lan743x_adapter *adapter = netdev_priv(netdev);
+
+	if (adapter->flags & LAN743X_ADAPTER_FLAG_OTP)
+		return MAX_OTP_SIZE;
+
 	return MAX_EEPROM_SIZE;
 }
 
@@ -214,8 +290,14 @@ static int lan743x_ethtool_get_eeprom(struct net_device *netdev,
 				      struct ethtool_eeprom *ee, u8 *data)
 {
 	struct lan743x_adapter *adapter = netdev_priv(netdev);
+	int ret = 0;
 
-	return lan743x_eeprom_read(adapter, ee->offset, ee->len, data);
+	if (adapter->flags & LAN743X_ADAPTER_FLAG_OTP)
+		ret = lan743x_otp_read(adapter, ee->offset, ee->len, data);
+	else
+		ret = lan743x_eeprom_read(adapter, ee->offset, ee->len, data);
+
+	return ret;
 }
 
 static int lan743x_ethtool_set_eeprom(struct net_device *netdev,
@@ -224,17 +306,18 @@ static int lan743x_ethtool_set_eeprom(struct net_device *netdev,
 	struct lan743x_adapter *adapter = netdev_priv(netdev);
 	int ret = -EINVAL;
 
-	if (ee->magic == LAN743X_EEPROM_MAGIC)
-		ret = lan743x_eeprom_write(adapter, ee->offset, ee->len,
-					   data);
-	/* Beware!  OTP is One Time Programming ONLY!
-	 * So do some strict condition check before messing up
-	 */
-	else if ((ee->magic == LAN743X_OTP_MAGIC) &&
-		 (ee->offset == 0) &&
-		 (ee->len == MAX_EEPROM_SIZE) &&
-		 (data[0] == OTP_INDICATOR_1))
-		ret = lan743x_otp_write(adapter, ee->offset, ee->len, data);
+	if (adapter->flags & LAN743X_ADAPTER_FLAG_OTP) {
+		/* Beware!  OTP is One Time Programming ONLY! */
+		if (ee->magic == LAN743X_OTP_MAGIC) {
+			ret = lan743x_otp_write(adapter, ee->offset,
+						ee->len, data);
+		}
+	} else {
+		if (ee->magic == LAN743X_EEPROM_MAGIC) {
+			ret = lan743x_eeprom_write(adapter, ee->offset,
+						   ee->len, data);
+		}
+	}
 
 	return ret;
 }
@@ -360,6 +443,10 @@ static const u32 lan743x_set2_hw_cnt_addr[] = {
 	STAT_TX_COUNTER_ROLLOVER_STATUS
 };
 
+static const char lan743x_priv_flags_strings[][ETH_GSTRING_LEN] = {
+	"OTP_ACCESS",
+};
+
 static void lan743x_ethtool_get_strings(struct net_device *netdev,
 					u32 stringset, u8 *data)
 {
@@ -375,6 +462,10 @@ static void lan743x_ethtool_get_strings(struct net_device *netdev,
 		       lan743x_set2_hw_cnt_strings,
 		       sizeof(lan743x_set2_hw_cnt_strings));
 		break;
+	case ETH_SS_PRIV_FLAGS:
+		memcpy(data, lan743x_priv_flags_strings,
+		       sizeof(lan743x_priv_flags_strings));
+		break;
 	}
 }
 
@@ -399,6 +490,22 @@ static void lan743x_ethtool_get_ethtool_stats(struct net_device *netdev,
 	}
 }
 
+static u32 lan743x_ethtool_get_priv_flags(struct net_device *netdev)
+{
+	struct lan743x_adapter *adapter = netdev_priv(netdev);
+
+	return adapter->flags;
+}
+
+static int lan743x_ethtool_set_priv_flags(struct net_device *netdev, u32 flags)
+{
+	struct lan743x_adapter *adapter = netdev_priv(netdev);
+
+	adapter->flags = flags;
+
+	return 0;
+}
+
 static int lan743x_ethtool_get_sset_count(struct net_device *netdev, int sset)
 {
 	switch (sset) {
@@ -411,6 +518,8 @@ static int lan743x_ethtool_get_sset_count(struct net_device *netdev, int sset)
 		ret += ARRAY_SIZE(lan743x_set2_hw_cnt_strings);
 		return ret;
 	}
+	case ETH_SS_PRIV_FLAGS:
+		return ARRAY_SIZE(lan743x_priv_flags_strings);
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -705,6 +814,8 @@ const struct ethtool_ops lan743x_ethtool_ops = {
 	.set_eeprom = lan743x_ethtool_set_eeprom,
 	.get_strings = lan743x_ethtool_get_strings,
 	.get_ethtool_stats = lan743x_ethtool_get_ethtool_stats,
+	.get_priv_flags = lan743x_ethtool_get_priv_flags,
+	.set_priv_flags = lan743x_ethtool_set_priv_flags,
 	.get_sset_count = lan743x_ethtool_get_sset_count,
 	.get_rxnfc = lan743x_ethtool_get_rxnfc,
 	.get_rxfh_key_size = lan743x_ethtool_get_rxfh_key_size,
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index 2d6eea1..3b02eea 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -26,6 +26,8 @@
 #define FPGA_REV_GET_MAJOR_(fpga_rev)	((fpga_rev) & 0x000000FF)
 
 #define HW_CFG					(0x010)
+#define HW_CFG_RELOAD_TYPE_ALL_			(0x00000FC0)
+#define HW_CFG_EE_OTP_RELOAD_			BIT(4)
 #define HW_CFG_LRST_				BIT(1)
 
 #define PMT_CTL					(0x014)
@@ -453,17 +455,19 @@
 #define OTP_PWR_DN				(0x1000)
 #define OTP_PWR_DN_PWRDN_N_			BIT(0)
 
-#define OTP_ADDR1				(0x1004)
-#define OTP_ADDR1_15_11_MASK_			(0x1F)
-
-#define OTP_ADDR2				(0x1008)
-#define OTP_ADDR2_10_3_MASK_			(0xFF)
+#define OTP_ADDR_HIGH				(0x1004)
+#define OTP_ADDR_LOW				(0x1008)
 
 #define OTP_PRGM_DATA				(0x1010)
 
 #define OTP_PRGM_MODE				(0x1014)
 #define OTP_PRGM_MODE_BYTE_			BIT(0)
 
+#define OTP_READ_DATA				(0x1018)
+
+#define OTP_FUNC_CMD				(0x1020)
+#define OTP_FUNC_CMD_READ_			BIT(0)
+
 #define OTP_TST_CMD				(0x1024)
 #define OTP_TST_CMD_PRGVRFY_			BIT(3)
 
@@ -713,6 +717,9 @@ struct lan743x_adapter {
 	struct lan743x_phy      phy;
 	struct lan743x_tx       tx[LAN743X_MAX_TX_CHANNELS];
 	struct lan743x_rx       rx[LAN743X_MAX_RX_CHANNELS];
+
+#define LAN743X_ADAPTER_FLAG_OTP		BIT(0)
+	u32			flags;
 };
 
 #define LAN743X_COMPONENT_FLAG_RX(channel)  BIT(20 + (channel))
-- 
2.7.4

Re: [PATCH v1 net-next] lan743x: Provide Read/Write Access to on chip OTP

[Andrew Lunn]

On Fri, Jan 18, 2019 at 04:54:53PM -0500, Bryan Whitehead wrote:
> The LAN743x includes on chip One-Time-Programmable (OTP) memory.
> 
> This patch extends the ethtool EEPROM read/write interface to
> access OTP memory space.
> 
> This is done by adding the private flag OTP_ACCESS, which is used
> to switch between EEPROM, and OTP modes.
> 
> The private flag OTP_ACCESS is configurable through the
>   ethtool --set-priv-flags command.
> And visible through the
>   ethtool --show-priv-flags command.

Hi Bryan

It would be good to explain what is wrong with the current code, which
allows you to select between the OTP and the EEPROM at write time.

> +static int lan743x_ethtool_set_priv_flags(struct net_device *netdev, u32 flags)
> +{
> +	struct lan743x_adapter *adapter = netdev_priv(netdev);
> +
> +	adapter->flags = flags;
> +
> +	return 0;
> +}

Is the EEPROM mandatory? Could there be designs without an EEPROM?
When setting the private flag here, should you probe to see if there
actually is an EEPROM and return -ENODEV if it is missing.

	 Andrew

RE: [PATCH v1 net-next] lan743x: Provide Read/Write Access to on chip OTP

[Bryan.Whitehead]

> On Fri, Jan 18, 2019 at 04:54:53PM -0500, Bryan Whitehead wrote:
> > The LAN743x includes on chip One-Time-Programmable (OTP) memory.
> >
> > This patch extends the ethtool EEPROM read/write interface to access
> > OTP memory space.
> >
> > This is done by adding the private flag OTP_ACCESS, which is used to
> > switch between EEPROM, and OTP modes.
> >
> > The private flag OTP_ACCESS is configurable through the
> >   ethtool --set-priv-flags command.
> > And visible through the
> >   ethtool --show-priv-flags command.
> 
> Hi Bryan
> 
> It would be good to explain what is wrong with the current code, which
> allows you to select between the OTP and the EEPROM at write time.

Hi Andrew,

The current code does not allow OTP read access. 
Plus the current code places unreasonable restrictions on OTP write operations.
Such as requiring offset == 0, length == 512, 
    and data[0] == 0xF3, which is the signature used to indicate the first OTP block is valid.
    However if the user wanted to use the second block, then data[0] should be 0xF7,
        And data should start at offset 513.
    Also if you want to permanently invalidate the OTP then data[0] should be 0xFF.
This patch allows the user the program OTP with any offset, length and data.
And read access is necessary to confirm what is currently in the OTP.

Would you like me to submit a new patch containing this information?

> > +static int lan743x_ethtool_set_priv_flags(struct net_device *netdev,
> > +u32 flags) {
> > +	struct lan743x_adapter *adapter = netdev_priv(netdev);
> > +
> > +	adapter->flags = flags;
> > +
> > +	return 0;
> > +}
> 
> Is the EEPROM mandatory? Could there be designs without an EEPROM?
> When setting the private flag here, should you probe to see if there actually
> is an EEPROM and return -ENODEV if it is missing.

EEPROM is not mandatory. If EEPROM is not attached or contains invalid data,
the lan743x will attempt to load configuration from on chip OTP memory.

To be honest, I'm not sure how to do a EEPROM probe with out doing a 
Write and read back test. And it is not recommended to do that due to limited
write cycles which cause EEPROM damage.

The current code does not do an EEPROM probe, so I don't see why the new
code should. And we assume that the user can diagnose EEPROM presence
using existing read/write operations.

Bryan

Re: [PATCH v1 net-next] lan743x: Provide Read/Write Access to on chip OTP

[Andrew Lunn]

On Mon, Jan 21, 2019 at 06:06:20PM +0000, Bryan.Whitehead@microchip.com wrote:
> > On Fri, Jan 18, 2019 at 04:54:53PM -0500, Bryan Whitehead wrote:
> > > The LAN743x includes on chip One-Time-Programmable (OTP) memory.
> > >
> > > This patch extends the ethtool EEPROM read/write interface to access
> > > OTP memory space.
> > >
> > > This is done by adding the private flag OTP_ACCESS, which is used to
> > > switch between EEPROM, and OTP modes.
> > >
> > > The private flag OTP_ACCESS is configurable through the
> > >   ethtool --set-priv-flags command.
> > > And visible through the
> > >   ethtool --show-priv-flags command.
> > 
> > Hi Bryan
> > 
> > It would be good to explain what is wrong with the current code, which
> > allows you to select between the OTP and the EEPROM at write time.
> 
> Hi Andrew,
> 
> The current code does not allow OTP read access. 
> Plus the current code places unreasonable restrictions on OTP write operations.
> Such as requiring offset == 0, length == 512, 
>     and data[0] == 0xF3, which is the signature used to indicate the first OTP block is valid.
>     However if the user wanted to use the second block, then data[0] should be 0xF7,
>         And data should start at offset 513.
>     Also if you want to permanently invalidate the OTP then data[0] should be 0xFF.
> This patch allows the user the program OTP with any offset, length and data.
> And read access is necessary to confirm what is currently in the OTP.
> 
> Would you like me to submit a new patch containing this information?

Hi Bryan

Yes, please indicate why the patch is needed.

> > Is the EEPROM mandatory? Could there be designs without an EEPROM?
> > When setting the private flag here, should you probe to see if there actually
> > is an EEPROM and return -ENODEV if it is missing.
> 
> EEPROM is not mandatory. If EEPROM is not attached or contains invalid data,
> the lan743x will attempt to load configuration from on chip OTP memory.
> 
> To be honest, I'm not sure how to do a EEPROM probe with out doing a 
> Write and read back test. And it is not recommended to do that due to limited
> write cycles which cause EEPROM damage.
> 
> The current code does not do an EEPROM probe, so I don't see why the new
> code should. And we assume that the user can diagnose EEPROM presence
> using existing read/write operations.

I would also put some text in the commit message to it is possible to
swap to the EEPROM using the private setting, even when the EEPROM is
not present. The failure will only happen later. With the current
code, the failure is going to happen immediately.

      Andrew

Re: [PATCH v1 net-next] lan743x: Provide Read/Write Access to on chip OTP

[Andrew Lunn]

> +static int lan743x_otp_write(struct lan743x_adapter *adapter, u32 offset,
> +			     u32 length, u8 *data)
> +{
> +	int ret;
> +	int i;
> +
> +	ret = lan743x_otp_power_up(adapter);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = lan743x_otp_wait_till_not_busy(adapter);
> +	if (ret < 0)
> +		return ret;
> +
>  	/* set to BYTE program mode */
>  	lan743x_csr_write(adapter, OTP_PRGM_MODE, OTP_PRGM_MODE_BYTE_);
>  
>  	for (i = 0; i < length; i++) {

I think you now need to verify length. It might be possible to change
from EEPROM to OTP after lan743x_ethtool_get_eeprom_len() has been
called, so you have been asked to write EEPROM length into the OTP.
The same is also true for read.

> -		lan743x_csr_write(adapter, OTP_ADDR1,
> -				  ((offset + i) >> 8) &
> -				  OTP_ADDR1_15_11_MASK_);
> -		lan743x_csr_write(adapter, OTP_ADDR2,
> -				  ((offset + i) &
> -				  OTP_ADDR2_10_3_MASK_));
> +		lan743x_otp_set_address(adapter, offset + i);
> +
>  		lan743x_csr_write(adapter, OTP_PRGM_DATA, data[i]);
>  		lan743x_csr_write(adapter, OTP_TST_CMD, OTP_TST_CMD_PRGVRFY_);
>  		lan743x_csr_write(adapter, OTP_CMD_GO, OTP_CMD_GO_GO_);
>  
> -		timeout = jiffies + HZ;
> -		do {
> -			udelay(1);
> -			buf = lan743x_csr_read(adapter, OTP_STATUS);
> -			if (time_after(jiffies, timeout)) {
> -				netif_warn(adapter, drv, adapter->netdev,
> -					   "Timeout on OTP_STATUS completion\n");
> -				return -EIO;
> -			}
> -		} while (buf & OTP_STATUS_BUSY_);
> +		ret = lan743x_otp_wait_till_not_busy(adapter);
> +		if (ret < 0)
> +			return ret;
>  	}
>  
> +	lan743x_otp_power_down(adapter);
> +
>  	return 0;
>  }
>  
>  static int lan743x_ethtool_set_eeprom(struct net_device *netdev,
> @@ -224,17 +306,18 @@ static int lan743x_ethtool_set_eeprom(struct net_device *netdev,
>  	struct lan743x_adapter *adapter = netdev_priv(netdev);
>  	int ret = -EINVAL;
>  
> -	if (ee->magic == LAN743X_EEPROM_MAGIC)
> -		ret = lan743x_eeprom_write(adapter, ee->offset, ee->len,
> -					   data);
> -	/* Beware!  OTP is One Time Programming ONLY!
> -	 * So do some strict condition check before messing up
> -	 */
> -	else if ((ee->magic == LAN743X_OTP_MAGIC) &&
> -		 (ee->offset == 0) &&
> -		 (ee->len == MAX_EEPROM_SIZE) &&
> -		 (data[0] == OTP_INDICATOR_1))
> -		ret = lan743x_otp_write(adapter, ee->offset, ee->len, data);
> +	if (adapter->flags & LAN743X_ADAPTER_FLAG_OTP) {
> +		/* Beware!  OTP is One Time Programming ONLY! */
> +		if (ee->magic == LAN743X_OTP_MAGIC) {
> +			ret = lan743x_otp_write(adapter, ee->offset,
> +						ee->len, data);
> +		}
> +	} else {
> +		if (ee->magic == LAN743X_EEPROM_MAGIC) {
> +			ret = lan743x_eeprom_write(adapter, ee->offset,
> +						   ee->len, data);
> +		}
> +	}

This is breaking backwards compatibility. I think you need to respect
the magic value, independent of how adapter->flags.
  
  Andrew

RE: [PATCH v1 net-next] lan743x: Provide Read/Write Access to on chip OTP

[Bryan.Whitehead]

> > > Hi Bryan
> > >
> > > It would be good to explain what is wrong with the current code,
> > > which allows you to select between the OTP and the EEPROM at write
> time.
> >
> > Hi Andrew,
> >
> > The current code does not allow OTP read access.
> > Plus the current code places unreasonable restrictions on OTP write
> operations.
> > Such as requiring offset == 0, length == 512,
> >     and data[0] == 0xF3, which is the signature used to indicate the first OTP
> block is valid.
> >     However if the user wanted to use the second block, then data[0] should
> be 0xF7,
> >         And data should start at offset 513.
> >     Also if you want to permanently invalidate the OTP then data[0] should
> be 0xFF.
> > This patch allows the user the program OTP with any offset, length and
> data.
> > And read access is necessary to confirm what is currently in the OTP.
> >
> > Would you like me to submit a new patch containing this information?
> 
> Hi Bryan
> 
> Yes, please indicate why the patch is needed.

OK

> 
> > > Is the EEPROM mandatory? Could there be designs without an EEPROM?
> > > When setting the private flag here, should you probe to see if there
> > > actually is an EEPROM and return -ENODEV if it is missing.
> >
> > EEPROM is not mandatory. If EEPROM is not attached or contains invalid
> > data, the lan743x will attempt to load configuration from on chip OTP
> memory.
> >
> > To be honest, I'm not sure how to do a EEPROM probe with out doing a
> > Write and read back test. And it is not recommended to do that due to
> > limited write cycles which cause EEPROM damage.
> >
> > The current code does not do an EEPROM probe, so I don't see why the
> > new code should. And we assume that the user can diagnose EEPROM
> > presence using existing read/write operations.
> 
> I would also put some text in the commit message to it is possible to swap to
> the EEPROM using the private setting, even when the EEPROM is not
> present. The failure will only happen later. With the current code, the failure
> is going to happen immediately.

OK, I'll submit a new patch soon.

RE: [PATCH v1 net-next] lan743x: Provide Read/Write Access to on chip OTP

[Bryan.Whitehead]

> > +static int lan743x_otp_write(struct lan743x_adapter *adapter, u32 offset,
> > +			     u32 length, u8 *data)
> > +{
> > +	int ret;
> > +	int i;
> > +
> > +	ret = lan743x_otp_power_up(adapter);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = lan743x_otp_wait_till_not_busy(adapter);
> > +	if (ret < 0)
> > +		return ret;
> > +
> >  	/* set to BYTE program mode */
> >  	lan743x_csr_write(adapter, OTP_PRGM_MODE,
> OTP_PRGM_MODE_BYTE_);
> >
> >  	for (i = 0; i < length; i++) {
> 
> I think you now need to verify length. It might be possible to change from
> EEPROM to OTP after lan743x_ethtool_get_eeprom_len() has been called,
> so you have been asked to write EEPROM length into the OTP.
> The same is also true for read.

OK, will do.

> >  static int lan743x_ethtool_set_eeprom(struct net_device *netdev, @@
> > -224,17 +306,18 @@ static int lan743x_ethtool_set_eeprom(struct
> net_device *netdev,
> >  	struct lan743x_adapter *adapter = netdev_priv(netdev);
> >  	int ret = -EINVAL;
> >
> > -	if (ee->magic == LAN743X_EEPROM_MAGIC)
> > -		ret = lan743x_eeprom_write(adapter, ee->offset, ee->len,
> > -					   data);
> > -	/* Beware!  OTP is One Time Programming ONLY!
> > -	 * So do some strict condition check before messing up
> > -	 */
> > -	else if ((ee->magic == LAN743X_OTP_MAGIC) &&
> > -		 (ee->offset == 0) &&
> > -		 (ee->len == MAX_EEPROM_SIZE) &&
> > -		 (data[0] == OTP_INDICATOR_1))
> > -		ret = lan743x_otp_write(adapter, ee->offset, ee->len, data);
> > +	if (adapter->flags & LAN743X_ADAPTER_FLAG_OTP) {
> > +		/* Beware!  OTP is One Time Programming ONLY! */
> > +		if (ee->magic == LAN743X_OTP_MAGIC) {
> > +			ret = lan743x_otp_write(adapter, ee->offset,
> > +						ee->len, data);
> > +		}
> > +	} else {
> > +		if (ee->magic == LAN743X_EEPROM_MAGIC) {
> > +			ret = lan743x_eeprom_write(adapter, ee->offset,
> > +						   ee->len, data);
> > +		}
> > +	}
> 
> This is breaking backwards compatibility. I think you need to respect the
> magic value, independent of how adapter->flags.

Is backwards compatibility a requirement?

If so, this only breaks OTP backward compatibility, which was extremely
  restrictive and ultimately not very useful.
In exchange for looser restrictions, we believe it is a good
  idea to make the user take the extra step to set the OTP_ACCESS flag
  to true. The reason for this is the OTP is One Time Programming only,
  and we want to make sure the user really intends to program it.

Since the OTP_ACCESS/LAN743X_ADAPTER_FLAG_OTP is false/0 by default,
  previously used EEPROM programming commands will work exactly the same.

We realize the magic value is also intended to "make sure the user really
  intends to program it".
But the magic value is not available for the read command, or length reporting.
And we believe it is easier for the user to understand the mechanics
  when one flag is used to control reading, writing, and length reporting.
If I did not respect the OTP_ACCESS flag for writing, then the length reporting
  could be invalid for writing operations.

Do you agree?

Re: [PATCH v1 net-next] lan743x: Provide Read/Write Access to on chip OTP

[Andrew Lunn]

> > This is breaking backwards compatibility. I think you need to respect the
> > magic value, independent of how adapter->flags.
> 
> Is backwards compatibility a requirement?

Hi Bryan

You should not change the ABI. And this is an ABI. So yes, backwards
compatibility should be maintained. Maybe there is a manufacturer out
there that has a script in their factory programming MAC addresses
into their devices using the current API. This change is going to
break their system. You really should be trying to avoid that.
 
> If so, this only breaks OTP backward compatibility, which was
> extremely restrictive and ultimately not very useful.

Was it sufficiently useful to actually program something into the OTP
to make the hardware work? If yes, somebody is probably using it. Or
is there some other way you expect a manufacturer to be programming
the OTP? Do you tell them the current API is horribly broken, don't
use it, always use JTAG?

You also need to think about the new ABI you are putting into
place. Is it really what you want? You cannot change it
afterwards. You need to support this forever.

	    Andrew

RE: [PATCH v1 net-next] lan743x: Provide Read/Write Access to on chip OTP

[Bryan.Whitehead]

> > > This is breaking backwards compatibility. I think you need to
> > > respect the magic value, independent of how adapter->flags.
> >
> > Is backwards compatibility a requirement?
> 
> Hi Bryan
> 
> You should not change the ABI. And this is an ABI. So yes, backwards
> compatibility should be maintained. Maybe there is a manufacturer out there
> that has a script in their factory programming MAC addresses into their
> devices using the current API. This change is going to break their system. You
> really should be trying to avoid that.
> 
> > If so, this only breaks OTP backward compatibility, which was
> > extremely restrictive and ultimately not very useful.
> 
> Was it sufficiently useful to actually program something into the OTP to make
> the hardware work? If yes, somebody is probably using it. Or is there some
> other way you expect a manufacturer to be programming the OTP? Do you
> tell them the current API is horribly broken, don't use it, always use JTAG?
> 
> You also need to think about the new ABI you are putting into place. Is it
> really what you want? You cannot change it afterwards. You need to support
> this forever.
> 
Hi Andrew,

Maybe it will help to provide you a little more background on our reasons for
doing this change.
First this product is very new. It only just went into production last December (2018).
And we recently received a support request regarding how to program the OTP. 
Unfortunately our interface was too restrictive for their needs. Which is why
we need to change it. 
So at this stage, before demand ramps up, we believe this change to the ABI is
worth it, since most customers who try to use OTP as is, will likely generate
more support requests, which we would naturally like to avoid.
I've spoken with the architect on the project and he agrees this new ABI is the
one we want going forward.
We apologize for needing to change the ABI, but we are prepared to support
the few early customers in their transition to the new ABI.
We hope you understand.

Regards,
Bryan