serial: 8250_exar: Replace custom EEPROM code with eeprom_93cx6

PATCH Superseded Parker Newman 2 participants 15 messages Sep 20, 2024

Subsystems (2)

TTY layer and serial drivers Char and misc drivers

lore

Share & tools

lore

Parker Newman

author

2024-09-20 14:04 UTC #0 lore raw

v2 cover serial: 8250_exar: Replace custom EEPROM code with eeprom_93cx6

From: Parker Newman <[email protected]>

This series of patches replaces the custom 93cx6 EEPROM read functions in the 8250_exar driver with the eeprom_93cx6 driver. This removes duplicate code and improves code readability.

In order to use the eeprom_93cx6 driver a quirk needed to be added to add an extra clock cycle before reading from the EEPROM. This is similar to the quirk in the eeprom_93xx46 driver.

More details in associated patch and mailing list discussion with Andy Shevchenko about these changes:

Link https://lore.kernel.org/linux-serial/[email protected]/ open here

Changes in v2:
- Dropped patch 3 "misc: eeprom: eeprom_93cx6: Replace printk(KERN_ERR...) with pr_err()".
- Moved Kconfig change into main patch.
- Moved quirk define into struct eeprom_93cx6.
- Moved quirk check function into eeprom_93cx6.h.
- Refactored cti_read_osc_freq() based on feedback.
- Minor commit message formatting fixes.

Parker Newman (4):
misc: eeprom: eeprom_93cx6: Add quirk for extra read clock cycle
misc: eeprom: eeprom_93cx6: Switch to BIT() macro
serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6
serial: 8250_exar: Remove old exar_ee_read() and other unneeded code

 drivers/misc/eeprom/eeprom_93cx6.c  |  15 +++-
 drivers/tty/serial/8250/8250_exar.c | 121 ++++++++--------------------
 drivers/tty/serial/8250/Kconfig     |   1 +
 include/linux/eeprom_93cx6.h        |  12 +++
 4 files changed, 58 insertions(+), 91 deletions(-)

base-commit: 5ed771f174726ae879945d4f148a9005ac909cb7 --
2.46.0

Parker Newman

author

2024-09-20 14:04 UTC #1 lore raw

PATCH v2 1/4 misc: eeprom: eeprom_93cx6: Add quirk for extra read clock cycle

From: Parker Newman <[email protected]>

This patch adds a quirk similar to eeprom_93xx46 to add an extra clock cycle before reading data from the EEPROM.

The 93Cx6 family of EEPROMs output a "dummy 0 bit" between the writing of the op-code/address from the host to the EEPROM and the reading of the actual data from the EEPROM.

More info can be found on page 6 of the AT93C46 datasheet (linked below). Similar notes are found in other 93xx6 datasheets.

In summary the read operation for a 93Cx6 EEPROM is:
Write to EEPROM: 110[A5-A0] (9 bits)
Read from EEPROM: 0[D15-D0] (17 bits)

Where:
110 is the start bit and READ OpCode [A5-A0] is the address to read from 0 is a "dummy bit" preceding the actual data [D15-D0] is the actual data.

Looking at the READ timing diagrams in the 93Cx6 datasheets the dummy bit should be clocked out on the last address bit clock cycle meaning it should be discarded naturally.

However, depending on the hardware configuration sometimes this dummy bit is not discarded. This is the case with Exar PCI UARTs which require an extra clock cycle between sending the address and reading the data.

Link https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-5193-SEEPROM-AT93C46D-Datasheet.pdf

Signed-off-by Parker Newman <[email protected]>

---
Changes in v2:
- Moved quirk define into struct eeprom_93cx6.
- Moved has_quirk_extra_read_cycle() from eeprom_93cx6.c into eeprom_93cx6.h.
- Fixed commit message formatting.

 drivers/misc/eeprom/eeprom_93cx6.c | 10 ++++++++++
 include/linux/eeprom_93cx6.h       | 12 ++++++++++++
 2 files changed, 22 insertions(+)

drivers/misc/eeprom/eeprom_93cx6.c +10 -0

index 9627294fe3e9..4c9827fe9217 100644
@@ -186,6 +186,11 @@ void eeprom_93cx6_read(struct eeprom_93cx6 *eeprom, const u8 word,
 	eeprom_93cx6_write_bits(eeprom, command,
 		PCI_EEPROM_WIDTH_OPCODE + eeprom->width);
 
+	if (has_quirk_extra_read_cycle(eeprom)) {
+		eeprom_93cx6_pulse_high(eeprom);
+		eeprom_93cx6_pulse_low(eeprom);
+	}
+
 	/*
 	 * Read the requested 16 bits.
 	 */

@@ -252,6 +257,11 @@ void eeprom_93cx6_readb(struct eeprom_93cx6 *eeprom, const u8 byte,
 	eeprom_93cx6_write_bits(eeprom, command,
 		PCI_EEPROM_WIDTH_OPCODE + eeprom->width + 1);
 
+	if (has_quirk_extra_read_cycle(eeprom)) {
+		eeprom_93cx6_pulse_high(eeprom);
+		eeprom_93cx6_pulse_low(eeprom);
+	}
+
 	/*
 	 * Read the requested 8 bits.
 	 */

include/linux/eeprom_93cx6.h +12 -0

index c860c72a921d..4d141345f4d4 100644
@@ -11,6 +11,8 @@
 	Supported chipsets: 93c46, 93c56 and 93c66.
  */
 
+#include <linux/bits.h>
+
 /*
  * EEPROM operation defines.
  */

@@ -34,6 +36,7 @@
  * @register_write(struct eeprom_93cx6 *eeprom): handler to
  * write to the eeprom register by using all reg_* fields.
  * @width: eeprom width, should be one of the PCI_EEPROM_WIDTH_* defines
+ * @quirks: eeprom or controller quirks
  * @drive_data: Set if we're driving the data line.
  * @reg_data_in: register field to indicate data input
  * @reg_data_out: register field to indicate data output

@@ -50,6 +53,9 @@ struct eeprom_93cx6 {
 	void (*register_write)(struct eeprom_93cx6 *eeprom);
 
 	int width;
+	unsigned int quirks;
+/* Some EEPROMs require an extra clock cycle before reading */
+#define PCI_EEPROM_QUIRK_EXTRA_READ_CYCLE	BIT(0)
 
 	char drive_data;
 	char reg_data_in;

@@ -71,3 +77,9 @@ extern void eeprom_93cx6_wren(struct eeprom_93cx6 *eeprom, bool enable);
 
 extern void eeprom_93cx6_write(struct eeprom_93cx6 *eeprom,
 			       u8 addr, u16 data);
+
+static inline bool has_quirk_extra_read_cycle(struct eeprom_93cx6 *eeprom)
+{
+	return eeprom->quirks & PCI_EEPROM_QUIRK_EXTRA_READ_CYCLE;
+}
+

--
2.46.0

Andy Shevchenko

Reviewed-by on v2 1/4

2024-09-20 14:45 UTC #2 lore raw

Re: [PATCH v2 1/4] misc: eeprom: eeprom_93cx6: Add quirk for extra read clock cycle

Quoted 26 lines · Parker Newman

From: Parker Newman <[email protected]>
This patch adds a quirk similar to eeprom_93xx46 to add an extra clock cycle before reading data from the EEPROM.
The 93Cx6 family of EEPROMs output a "dummy 0 bit" between the writing of the op-code/address from the host to the EEPROM and the reading of the actual data from the EEPROM.
More info can be found on page 6 of the AT93C46 datasheet (linked below). Similar notes are found in other 93xx6 datasheets.
In summary the read operation for a 93Cx6 EEPROM is:
Write to EEPROM: 110[A5-A0] (9 bits)
Read from EEPROM: 0[D15-D0] (17 bits)
Where:
110 is the start bit and READ OpCode [A5-A0] is the address to read from 0 is a "dummy bit" preceding the actual data [D15-D0] is the actual data.
Looking at the READ timing diagrams in the 93Cx6 datasheets the dummy bit should be clocked out on the last address bit clock cycle meaning it should be discarded naturally.

Quoted 5 lines

However, depending on the hardware configuration sometimes this dummy bit is not discarded. This is the case with Exar PCI UARTs which require an extra clock cycle between sending the address and reading the data.
Link https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-5193-SEEPROM-AT93C46D-Datasheet.pdf

JFYI: You may also convert this to Datasheet: tag (we have a history of it mostly in IIO subsystem), basically replacing word Link by Datasheet in the above line.

Quoted 1 line

Signed-off-by Parker Newman <[email protected]>

Code wise LGTM now,

Reviewed-by Andy Shevchenko <[email protected]>

(Do not forget to embed this tag into a new version. With this the b4 tool
is quite helpful, so the workflow is to checkout a new branch in your local Git tree, like git checkout -b exar-93xx46 v6.12-rc1 then taking a message ID from email of this thread — any should succeed, but cover letter's one for sure — run the following b4 am $<message ID>. It will print the hints what to do next, something like git am $<patch_title>.mbx. Then you can continue with git rebase --interactive v6.12-rc1 if the code or other stuff in the commit message needs to be updated.)

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Parker Newman

author

2024-09-20 15:03 UTC #3 lore raw

Re: [PATCH v2 1/4] misc: eeprom: eeprom_93cx6: Add quirk for extra read clock cycle

Quoted 31 lines · On Fri, 20 Sep 2024 17:45:15 +0300 Andy Shevchenko…

Quoted 30 lines · Parker Newman
From: Parker Newman <[email protected]>
This patch adds a quirk similar to eeprom_93xx46 to add an extra clock cycle before reading data from the EEPROM.
The 93Cx6 family of EEPROMs output a "dummy 0 bit" between the writing of the op-code/address from the host to the EEPROM and the reading of the actual data from the EEPROM.
More info can be found on page 6 of the AT93C46 datasheet (linked below). Similar notes are found in other 93xx6 datasheets.
In summary the read operation for a 93Cx6 EEPROM is:
Write to EEPROM: 110[A5-A0] (9 bits)
Read from EEPROM: 0[D15-D0] (17 bits)
Where:
110 is the start bit and READ OpCode [A5-A0] is the address to read from 0 is a "dummy bit" preceding the actual data [D15-D0] is the actual data.
Looking at the READ timing diagrams in the 93Cx6 datasheets the dummy bit should be clocked out on the last address bit clock cycle meaning it should be discarded naturally.
However, depending on the hardware configuration sometimes this dummy bit is not discarded. This is the case with Exar PCI UARTs which require an extra clock cycle between sending the address and reading the data.

Quoted 6 lines

Quoted 1 line
Link https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-5193-SEEPROM-AT93C46D-Datasheet.pdf
JFYI: You may also convert this to Datasheet: tag (we have a history of it mostly in IIO subsystem), basically replacing word Link by Datasheet in the above line.

Sounds good, I will change in v3.

Quoted 5 lines

Quoted 1 line
Signed-off-by Parker Newman <[email protected]>
Code wise LGTM now,
Reviewed-by Andy Shevchenko <[email protected]>

Quoted 9 lines

(Do not forget to embed this tag into a new version. With this the b4 tool
is quite helpful, so the workflow is to checkout a new branch in your local Git tree, like git checkout -b exar-93xx46 v6.12-rc1 then taking a message ID from email of this thread — any should succeed, but cover letter's one for sure — run the following b4 am $<message ID>. It will print the hints what to do next, something like git am $<patch_title>.mbx. Then you can continue with git rebase --interactive v6.12-rc1 if the code or other stuff in the commit message needs to be updated.)

Thanks! This is very helpful.

Andy Shevchenko

2024-09-20 14:48 UTC #4 lore raw

Re: [PATCH v2 1/4] misc: eeprom: eeprom_93cx6: Add quirk for extra read clock cycle

In reply to Parker Newman

Quoted 4 lines · Parker Newman

From: Parker Newman <[email protected]>
This patch adds a quirk similar to eeprom_93xx46 to add an extra clock cycle before reading data from the EEPROM.

Ah, sorry, forgot to mention the Submitting Patches documentation. It suggests to use imperative mood in the commit messages, hence the above should be like

Add a quirk similar to eeprom_93xx46 to add an extra clock cycle before reading data from the EEPROM.

Also check other messages in this series.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Parker Newman

author

2024-09-20 14:04 UTC #5 lore raw

PATCH v2 2/4 misc: eeprom: eeprom_93cx6: Switch to BIT() macro

In reply to Parker Newman

From: Parker Newman <[email protected]>

Use the BIT() macro rather than (1 << (i - 1)).

Signed-off-by Parker Newman <[email protected]>

---
 drivers/misc/eeprom/eeprom_93cx6.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/misc/eeprom/eeprom_93cx6.c +3 -2

index 4c9827fe9217..e6f0e0fc1ca2 100644
@@ -8,6 +8,7 @@
  * Supported chipsets: 93c46 & 93c66.
  */
 
+#include <linux/bits.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/delay.h>

@@ -102,7 +103,7 @@ static void eeprom_93cx6_write_bits(struct eeprom_93cx6 *eeprom,
 		/*
 		 * Check if this bit needs to be set.
 		 */
-		eeprom->reg_data_in = !!(data & (1 << (i - 1)));
+		eeprom->reg_data_in = !!(data & BIT(i - 1));
 
 		/*
 		 * Write the bit to the eeprom register.

@@ -152,7 +153,7 @@ static void eeprom_93cx6_read_bits(struct eeprom_93cx6 *eeprom,
 		 * Read if the bit has been set.
 		 */
 		if (eeprom->reg_data_out)
-			buf |= (1 << (i - 1));
+			buf |= BIT(i - 1);
 
 		eeprom_93cx6_pulse_low(eeprom);
 	}

--
2.46.0

Andy Shevchenko

Reviewed-by on v2 2/4

2024-09-20 14:46 UTC #6 lore raw

Re: [PATCH v2 2/4] misc: eeprom: eeprom_93cx6: Switch to BIT() macro

Quoted 3 lines · Parker Newman

From: Parker Newman <[email protected]>
Use the BIT() macro rather than (1 << (i - 1)).

Reviewed-by Andy Shevchenko <[email protected]>

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Parker Newman

author

2024-09-20 14:04 UTC #7 lore raw

PATCH v2 3/4 serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6

In reply to Parker Newman

From: Parker Newman <[email protected]>

Replace the custom 93cx6 EEPROM read functions with the eeprom_93cx6 driver. This removes duplicate code and improves code readability.

exar_ee_read() calls are replaced with eeprom_93cx6_read() or eeprom_93cx6_multiread().

Add "select EEPROM_93CX6" to config SERIAL_8250_EXAR to ensure eeprom_93cx6 driver is also compiled when 8250_exar driver is selected.

Note: Old exar_ee_read() and associated functions are removed in next patch in this series.

Link to mailing list discussion with Andy Shevchenko for reference.

Link https://lore.kernel.org/linux-serial/[email protected]/ open here

Signed-off-by Parker Newman <[email protected]>

---
Changes in v2:
- Refactored cti_read_osc_freq() based on feedback.
- Moved Kconfig change into this patch.
- Sorted headers.
- Fixed comment.

 drivers/tty/serial/8250/8250_exar.c | 55 +++++++++++++++++++++++------
 drivers/tty/serial/8250/Kconfig     |  1 +
 2 files changed, 45 insertions(+), 11 deletions(-)

drivers/tty/serial/8250/8250_exar.c +44 ~~-11~~

index b7a75db15249..c40e86920110 100644
@@ -11,6 +11,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/dmi.h>
+#include <linux/eeprom_93cx6.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/math.h>

@@ -252,6 +253,7 @@ struct exar8250 {
 	unsigned int		nr;
 	unsigned int		osc_freq;
 	struct exar8250_board	*board;
+	struct eeprom_93cx6	eeprom;
 	void __iomem		*virt;
 	int			line[];
 };

@@ -355,6 +357,39 @@ static u16 exar_ee_read(struct exar8250 *priv, u8 ee_addr)
 	return data;
 }
 
+static void exar_eeprom_93cx6_reg_read(struct eeprom_93cx6 *eeprom)
+{
+	struct exar8250 *priv = eeprom->data;
+	u8 regb = exar_read_reg(priv, UART_EXAR_REGB);
+
+	/* EECK and EECS always read 0 from REGB so only set EEDO */
+	eeprom->reg_data_out = regb & UART_EXAR_REGB_EEDO;
+}
+
+static void exar_eeprom_93cx6_reg_write(struct eeprom_93cx6 *eeprom)
+{
+	struct exar8250 *priv = eeprom->data;
+	u8 regb = 0;
+
+	if (eeprom->reg_data_in)
+		regb |= UART_EXAR_REGB_EEDI;
+	if (eeprom->reg_data_clock)
+		regb |= UART_EXAR_REGB_EECK;
+	if (eeprom->reg_chip_select)
+		regb |= UART_EXAR_REGB_EECS;
+
+	exar_write_reg(priv, UART_EXAR_REGB, regb);
+}
+
+static void exar_eeprom_init(struct exar8250 *priv)
+{
+	priv->eeprom.data = priv;
+	priv->eeprom.register_read = exar_eeprom_93cx6_reg_read;
+	priv->eeprom.register_write = exar_eeprom_93cx6_reg_write;
+	priv->eeprom.width = PCI_EEPROM_WIDTH_93C46;
+	priv->eeprom.quirks |= PCI_EEPROM_QUIRK_EXTRA_READ_CYCLE;
+}
+
 /**
  * exar_mpio_config_output() - Configure an Exar MPIO as an output
  * @priv: Device's private structure

@@ -696,20 +731,16 @@ static int cti_plx_int_enable(struct exar8250 *priv)
  */
 static int cti_read_osc_freq(struct exar8250 *priv, u8 eeprom_offset)
 {
-	u16 lower_word;
-	u16 upper_word;
+	__le16 ee_words[2];
+	u32 osc_freq;
 
-	lower_word = exar_ee_read(priv, eeprom_offset);
-	// Check if EEPROM word was blank
-	if (lower_word == 0xFFFF)
-		return -EIO;
+	eeprom_93cx6_multiread(&priv->eeprom, eeprom_offset, ee_words, ARRAY_SIZE(ee_words));
 
-	upper_word = exar_ee_read(priv, (eeprom_offset + 1));
-	if (upper_word == 0xFFFF)
+	osc_freq = le16_to_cpu(ee_words[0]) | (le16_to_cpu(ee_words[1]) << 16);
+	if (osc_freq == GENMASK(31, 0))
 		return -EIO;
 
-	return FIELD_PREP(CTI_EE_MASK_OSC_FREQ_LOWER, lower_word) |
-	       FIELD_PREP(CTI_EE_MASK_OSC_FREQ_UPPER, upper_word);
+	return osc_freq;
 }
 
 /**

@@ -833,7 +864,7 @@ static enum cti_port_type cti_get_port_type_xr17v35x(struct exar8250 *priv,
 	u8 offset;
 
 	offset = CTI_EE_OFF_XR17V35X_PORT_FLAGS + port_num;
-	port_flags = exar_ee_read(priv, offset);
+	eeprom_93cx6_read(&priv->eeprom, offset, &port_flags);
 
 	port_type = FIELD_GET(CTI_EE_MASK_PORT_FLAGS_TYPE, port_flags);
 	if (CTI_PORT_TYPE_VALID(port_type))

@@ -1551,6 +1582,8 @@ exar_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
 	if (rc)
 		return rc;
 
+	exar_eeprom_init(priv);
+
 	for (i = 0; i < nr_ports && i < maxnr; i++) {
 		rc = board->setup(priv, pcidev, &uart, i);
 		if (rc) {

drivers/tty/serial/8250/Kconfig +1 -0

index 47ff50763c04..94910ced8238 100644
@@ -150,6 +150,7 @@ config SERIAL_8250_EXAR
 	tristate "8250/16550 Exar/Commtech PCI/PCIe device support"
 	depends on SERIAL_8250 && PCI
 	select SERIAL_8250_PCILIB
+	select EEPROM_93CX6
 	default SERIAL_8250
 	help
 	  This builds support for XR17C1xx, XR17V3xx and some Commtech

--
2.46.0

Andy Shevchenko

Reviewed-by on v2 3/4

2024-09-20 14:55 UTC #8 lore raw

Re: [PATCH v2 3/4] serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6

Quoted 5 lines · Parker Newman

From: Parker Newman <[email protected]>
Replace the custom 93cx6 EEPROM read functions with the eeprom_93cx6 driver. This removes duplicate code and improves code readability.

Quoted 8 lines

exar_ee_read() calls are replaced with eeprom_93cx6_read() or eeprom_93cx6_multiread().
Add "select EEPROM_93CX6" to config SERIAL_8250_EXAR to ensure eeprom_93cx6 driver is also compiled when 8250_exar driver is selected.
Note: Old exar_ee_read() and associated functions are removed in next patch in this series.

Looks perfect!

Reviewed-by Andy Shevchenko <[email protected]>

Let's wait for others and CIs to test all this and then after v6.12-rc1 is out (presumably within ~ten days) rebase your series (as I suggested in another reply) and send a v3.

Good work!

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Parker Newman

author

2024-09-20 15:09 UTC #9 lore raw

Re: [PATCH v2 3/4] serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6

Quoted 15 lines · On Fri, 20 Sep 2024 17:55:33 +0300 Andy Shevchenko…

Quoted 5 lines · Parker Newman
From: Parker Newman <[email protected]>
Replace the custom 93cx6 EEPROM read functions with the eeprom_93cx6 driver. This removes duplicate code and improves code readability.
Quoted 8 lines
exar_ee_read() calls are replaced with eeprom_93cx6_read() or eeprom_93cx6_multiread().
Add "select EEPROM_93CX6" to config SERIAL_8250_EXAR to ensure eeprom_93cx6 driver is also compiled when 8250_exar driver is selected.
Note: Old exar_ee_read() and associated functions are removed in next patch in this series.

Quoted 7 lines

Looks perfect!
Reviewed-by Andy Shevchenko <[email protected]>
Let's wait for others and CIs to test all this and then after v6.12-rc1 is out (presumably within ~ten days) rebase your series (as I suggested in another reply) and send a v3.

Sounds good, I will wait for v6.12-rc1, rebase and send v3 with the changes you (or others) suggested. I promise I won't forget the Reviewed-by tags this time :).

Quoted 2 lines

Good work!

Thanks and thanks for the help!
-Parker

Andy Shevchenko

2024-09-20 15:26 UTC #10 lore raw

Re: [PATCH v2 3/4] serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6

In reply to Parker Newman

Quoted 1 line · Parker Newman

From: Parker Newman <[email protected]>

...

drivers/tty/serial/8250/8250_exar.c static int cti_plx_int_enable(struct exar8250 *priv) · line 741

Quoted 3 lines

drivers/tty/serial/8250/8250_exar.c +2 -0

+	osc_freq = le16_to_cpu(ee_words[0]) | (le16_to_cpu(ee_words[1]) << 16);
+	if (osc_freq == GENMASK(31, 0))
 		return -EIO;

Just noticed that you have
#define CTI_EE_MASK_OSC_FREQ_LOWER GENMASK(15, 0)
#define CTI_EE_MASK_OSC_FREQ_UPPER GENMASK(31, 16)

So, please modify them and the above check using these. Something like

#define CTI_EE_MASK_OSC_FREQ GENMASK(31, 0)

	osc_freq = le16_to_cpu(ee_words[0]) | (le16_to_cpu(ee_words[1]) << 16);
	if (osc_freq == CTI_EE_MASK_OSC_FREQ)
		return -EIO;

P.S> If I am not mistaken the definitions were only used in this function.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Parker Newman

author

2024-09-20 15:42 UTC #11 lore raw

Re: [PATCH v2 3/4] serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6

Quoted 13 lines · On Fri, 20 Sep 2024 18:26:08 +0300 Andy Shevchenko…

Quoted 1 line · Parker Newman
From: Parker Newman <[email protected]>
...
drivers/tty/serial/8250/8250_exar.c static int cti_plx_int_enable(struct exar8250 *priv) · line 741
Quoted 3 lines
drivers/tty/serial/8250/8250_exar.c +2 -0
+	osc_freq = le16_to_cpu(ee_words[0]) | (le16_to_cpu(ee_words[1]) << 16);
+	if (osc_freq == GENMASK(31, 0))
 		return -EIO;
Just noticed that you have
#define CTI_EE_MASK_OSC_FREQ_LOWER GENMASK(15, 0)
#define CTI_EE_MASK_OSC_FREQ_UPPER GENMASK(31, 16)

Quoted 3 lines

So, please modify them and the above check using these. Something like

Good catch, I debated using them again with FIELD_PREP() like the old code but it looked pretty ugly. I guess I missed removing them.

I will fix in v3. In this case should I drop your Reviewed-by tag? Or is this change small enough to keep it?

Quoted 8 lines

#define CTI_EE_MASK_OSC_FREQ GENMASK(31, 0)
	osc_freq = le16_to_cpu(ee_words[0]) | (le16_to_cpu(ee_words[1]) << 16);
	if (osc_freq == CTI_EE_MASK_OSC_FREQ)
		return -EIO;
P.S> If I am not mistaken the definitions were only used in this function.

Andy Shevchenko

2024-09-20 15:46 UTC #12 lore raw

Re: [PATCH v2 3/4] serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6

Quoted 2 lines · Parker Newman

On Fri, 20 Sep 2024 18:26:08 +0300
Andy Shevchenko <[email protected]> wrote:

...

Quoted 2 lines

I will fix in v3. In this case should I drop your Reviewed-by tag? Or is this change small enough to keep it?

I'm glad you asked. As you rightfully noted, this one is small, no need to drop tag.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Parker Newman

author

2024-09-20 14:04 UTC #13 lore raw

PATCH v2 4/4 serial: 8250_exar: Remove old exar_ee_read() and other unneeded code

In reply to Parker Newman

From: Parker Newman <[email protected]>

Remove the old exar_ee_read() and associated helper functions as well as some defines that are no longer needed after the switch to using the eeprom_93cx6 driver.

Signed-off-by Parker Newman <[email protected]>

---
 drivers/tty/serial/8250/8250_exar.c | 90 -----------------------------
 1 file changed, 90 deletions(-)

drivers/tty/serial/8250/8250_exar.c +0 ~~-90~~

index c40e86920110..659a44b52f8d 100644
@@ -136,8 +136,6 @@
 #define UART_EXAR_REGB_EECS		BIT(5)
 #define UART_EXAR_REGB_EEDI		BIT(6)
 #define UART_EXAR_REGB_EEDO		BIT(7)
-#define UART_EXAR_REGB_EE_ADDR_SIZE	6
-#define UART_EXAR_REGB_EE_DATA_SIZE	16
 
 #define UART_EXAR_XR17C15X_PORT_OFFSET	0x200
 #define UART_EXAR_XR17V25X_PORT_OFFSET	0x200

@@ -269,94 +267,6 @@ static inline u8 exar_read_reg(struct exar8250 *priv, unsigned int reg)
 	return readb(priv->virt + reg);
 }
 
-static inline void exar_ee_select(struct exar8250 *priv)
-{
-	// Set chip select pin high to enable EEPROM reads/writes
-	exar_write_reg(priv, UART_EXAR_REGB, UART_EXAR_REGB_EECS);
-	// Min ~500ns delay needed between CS assert and EEPROM access
-	udelay(1);
-}
-
-static inline void exar_ee_deselect(struct exar8250 *priv)
-{
-	exar_write_reg(priv, UART_EXAR_REGB, 0x00);
-}
-
-static inline void exar_ee_write_bit(struct exar8250 *priv, u8 bit)
-{
-	u8 value = UART_EXAR_REGB_EECS;
-
-	if (bit)
-		value |= UART_EXAR_REGB_EEDI;
-
-	// Clock out the bit on the EEPROM interface
-	exar_write_reg(priv, UART_EXAR_REGB, value);
-	// 2us delay = ~500khz clock speed
-	udelay(2);
-
-	value |= UART_EXAR_REGB_EECK;
-
-	exar_write_reg(priv, UART_EXAR_REGB, value);
-	udelay(2);
-}
-
-static inline u8 exar_ee_read_bit(struct exar8250 *priv)
-{
-	u8 regb;
-	u8 value = UART_EXAR_REGB_EECS;
-
-	// Clock in the bit on the EEPROM interface
-	exar_write_reg(priv, UART_EXAR_REGB, value);
-	// 2us delay = ~500khz clock speed
-	udelay(2);
-
-	value |= UART_EXAR_REGB_EECK;
-
-	exar_write_reg(priv, UART_EXAR_REGB, value);
-	udelay(2);
-
-	regb = exar_read_reg(priv, UART_EXAR_REGB);
-
-	return (regb & UART_EXAR_REGB_EEDO ? 1 : 0);
-}
-
-/**
- * exar_ee_read() - Read a word from the EEPROM
- * @priv: Device's private structure
- * @ee_addr: Offset of EEPROM to read word from
- *
- * Read a single 16bit word from an Exar UART's EEPROM.
- * The type of the EEPROM is AT93C46D.
- *
- * Return: EEPROM word
- */
-static u16 exar_ee_read(struct exar8250 *priv, u8 ee_addr)
-{
-	int i;
-	u16 data = 0;
-
-	exar_ee_select(priv);
-
-	// Send read command (opcode 110)
-	exar_ee_write_bit(priv, 1);
-	exar_ee_write_bit(priv, 1);
-	exar_ee_write_bit(priv, 0);
-
-	// Send address to read from
-	for (i = UART_EXAR_REGB_EE_ADDR_SIZE - 1; i >= 0; i--)
-		exar_ee_write_bit(priv, ee_addr & BIT(i));
-
-	// Read data 1 bit at a time starting with a dummy bit
-	for (i = UART_EXAR_REGB_EE_DATA_SIZE; i >= 0; i--) {
-		if (exar_ee_read_bit(priv))
-			data |= BIT(i);
-	}
-
-	exar_ee_deselect(priv);
-
-	return data;
-}
-
 static void exar_eeprom_93cx6_reg_read(struct eeprom_93cx6 *eeprom)
 {
 	struct exar8250 *priv = eeprom->data;

--
2.46.0

Andy Shevchenko

2024-09-20 14:37 UTC #14 lore raw

Re: [PATCH v2 4/4] serial: 8250_exar: Remove old exar_ee_read() and other unneeded code

Quoted 5 lines · Parker Newman

From: Parker Newman <[email protected]>
Remove the old exar_ee_read() and associated helper functions as well as some defines that are no longer needed after the switch to using the eeprom_93cx6 driver.

You missed my tag. If one sends a new version of the series, they are responsible to take care about previously given tags.

I'm going to comment on the rest, but something tells me that you might need a v3 of the series.

Signature · 2 lines

With Best Regards,
Andy Shevchenko