1. Patchew
  2. linux
  3. [PATCH v1 0/6] serial: 8250_exar: Replace custom EEPROM code with eeprom_93cx6
  4. View patch

[PATCH v1 4/6] serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6

Parker Newman posted 6 patches 2 months, 2 weeks ago
There is a newer version of this series
[PATCH v1 4/6] serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6
Posted by Parker Newman 2 months, 2 weeks ago 
From: Parker Newman <pnewman@connecttech.com>

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().

Link to discussion with Andy Shevchenko:
Link: https://lore.kernel.org/linux-serial/Ztr5u2wEt8VF1IdI@black.fi.intel.com/

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

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
 drivers/tty/serial/8250/8250_exar.c | 54 +++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index b7a75db15249..0edb5aeba199 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -21,6 +21,7 @@
 #include <linux/property.h>
 #include <linux/string.h>
 #include <linux/types.h>
+#include <linux/eeprom_93cx6.h>

 #include <linux/serial_8250.h>
 #include <linux/serial_core.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 = (struct exar8250 *)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 = (struct exar8250 *)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 = (void *)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,15 @@ 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;
+	__le32 osc_freq_le;

-	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,
+				(__le16 *)&osc_freq_le, 2);

-	upper_word = exar_ee_read(priv, (eeprom_offset + 1));
-	if (upper_word == 0xFFFF)
+	if (osc_freq_le == 0xFFFFFFFF)
 		return -EIO;

-	return FIELD_PREP(CTI_EE_MASK_OSC_FREQ_LOWER, lower_word) |
-	       FIELD_PREP(CTI_EE_MASK_OSC_FREQ_UPPER, upper_word);
+	return le32_to_cpu(osc_freq_le);
 }

 /**
@@ -833,7 +863,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 +1581,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) {
--
2.46.0
Re: [PATCH v1 4/6] serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6
Posted by kernel test robot 2 months, 2 weeks ago 
Hi Parker,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 5ed771f174726ae879945d4f148a9005ac909cb7]

url:    https://github.com/intel-lab-lkp/linux/commits/Parker-Newman/misc-eeprom-eeprom_93cx6-Add-quirk-for-extra-read-clock-cycle/20240913-230345
base:   5ed771f174726ae879945d4f148a9005ac909cb7
patch link:    https://lore.kernel.org/r/78dead78311ea619e0be99cc32ee0df1610a480d.1726237379.git.pnewman%40connecttech.com
patch subject: [PATCH v1 4/6] serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6
config: x86_64-randconfig-122-20240914 (https://download.01.org/0day-ci/archive/20240914/202409142138.yCOHBlL1-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409142138.yCOHBlL1-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409142138.yCOHBlL1-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/tty/serial/8250/8250_exar.c:739:13: sparse: sparse: restricted __le32 degrades to integer

vim +739 drivers/tty/serial/8250/8250_exar.c

   721	
   722	/**
   723	 * cti_read_osc_freq() - Read the UART oscillator frequency from EEPROM
   724	 * @priv: Device's private structure
   725	 * @eeprom_offset: Offset where the oscillator frequency is stored
   726	 *
   727	 * CTI XR17x15X and XR17V25X cards have the serial boards oscillator frequency
   728	 * stored in the EEPROM. FPGA and XR17V35X based cards use the PCI/PCIe clock.
   729	 *
   730	 * Return: frequency on success, negative error code on failure
   731	 */
   732	static int cti_read_osc_freq(struct exar8250 *priv, u8 eeprom_offset)
   733	{
   734		__le32 osc_freq_le;
   735	
   736		eeprom_93cx6_multiread(&priv->eeprom, eeprom_offset,
   737					(__le16 *)&osc_freq_le, 2);
   738	
 > 739		if (osc_freq_le == 0xFFFFFFFF)
   740			return -EIO;
   741	
   742		return le32_to_cpu(osc_freq_le);
   743	}
   744	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH v1 4/6] serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6
Posted by Andy Shevchenko 2 months, 2 weeks ago 
On Sat, Sep 14, 2024 at 09:26:27PM +0800, kernel test robot wrote:
> Hi Parker,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on 5ed771f174726ae879945d4f148a9005ac909cb7]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Parker-Newman/misc-eeprom-eeprom_93cx6-Add-quirk-for-extra-read-clock-cycle/20240913-230345
> base:   5ed771f174726ae879945d4f148a9005ac909cb7
> patch link:    https://lore.kernel.org/r/78dead78311ea619e0be99cc32ee0df1610a480d.1726237379.git.pnewman%40connecttech.com
> patch subject: [PATCH v1 4/6] serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6
> config: x86_64-randconfig-122-20240914 (https://download.01.org/0day-ci/archive/20240914/202409142138.yCOHBlL1-lkp@intel.com/config)
> compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409142138.yCOHBlL1-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202409142138.yCOHBlL1-lkp@intel.com/
> 
> sparse warnings: (new ones prefixed by >>)
> >> drivers/tty/serial/8250/8250_exar.c:739:13: sparse: sparse: restricted __le32 degrades to integer

Yes, and this should gone if Parker follows my suggestion on how to handle this.

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 4/6] serial: 8250_exar: Replace custom EEPROM read with eeprom_93cx6
Posted by Andy Shevchenko 2 months, 2 weeks ago 
On Fri, Sep 13, 2024 at 10:55:41AM -0400, Parker Newman wrote:
> From: Parker Newman <pnewman@connecttech.com>
> 
> 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().
> 
> Link to discussion with Andy Shevchenko:

(the above might need to be rephrased a bit, see below why)

> Link: https://lore.kernel.org/linux-serial/Ztr5u2wEt8VF1IdI@black.fi.intel.com/

Make it real tag by moving...

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

...somewhere here.

> Signed-off-by: Parker Newman <pnewman@connecttech.com>

...

>  #include <linux/property.h>
>  #include <linux/string.h>
>  #include <linux/types.h>
> +#include <linux/eeprom_93cx6.h>

Keep it sorted.

...

> +static void exar_eeprom_93cx6_reg_read(struct eeprom_93cx6 *eeprom)
> +{
> +	struct exar8250 *priv = (struct exar8250 *)eeprom->data;

Unneeded explicit cast.

> +	u8 regb = exar_read_reg(priv, UART_EXAR_REGB);
> +
> +	// EECK and EECS always read 0 from REGB so only set EEDO

Please, use C comment style as everywhere else in the file.

> +	eeprom->reg_data_out = regb & UART_EXAR_REGB_EEDO;
> +}

...

> +static void exar_eeprom_93cx6_reg_write(struct eeprom_93cx6 *eeprom)
> +{
> +	struct exar8250 *priv = (struct exar8250 *)eeprom->data;

Unneeded cast from void *.

> +	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);
> +}

...

> +       priv->eeprom.data = (void *)priv;

Unneeded cast.

...

> +	eeprom_93cx6_multiread(&priv->eeprom, eeprom_offset,
> +				(__le16 *)&osc_freq_le, 2);

Okay, this should be done better I believe:

	/* ...Find better names for variables... */
	__le16 f[2];
	u32 osc_freq;

	eeprom_93cx6_multiread(&priv->eeprom, eeprom_offset, &freq, ARRAY_SIZE(freq));

	osc_freq = le16_to_cpu(f[0]) | (le16_to_cpu(f[1]) << 16);
	if (osc_freq == GENMASK(31, 0))
		...

	return osc_freq;

(Also no need to break on 80 characters)

> +	if (osc_freq_le == 0xFFFFFFFF)
>  		return -EIO;
> 
> +	return le32_to_cpu(osc_freq_le);

-- 
With Best Regards,
Andy Shevchenko

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.