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

[PATCH v1 1/6] misc: eeprom: eeprom_93cx6: Add quirk for extra read clock cycle

Parker Newman posted 6 patches 2 months, 2 weeks ago
There is a newer version of this series
[PATCH v1 1/6] misc: eeprom: eeprom_93cx6: Add quirk for extra read clock cycle
Posted by Parker Newman 2 months, 2 weeks ago 
From: Parker Newman <pnewman@connecttech.com>

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. Similar notes
are found in other 93xx6 datasheets.
Link: https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-5193-SEEPROM-AT93C46D-Datasheet.pdf

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.

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
 drivers/misc/eeprom/eeprom_93cx6.c | 15 +++++++++++++++
 include/linux/eeprom_93cx6.h       |  7 +++++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/misc/eeprom/eeprom_93cx6.c b/drivers/misc/eeprom/eeprom_93cx6.c
index 9627294fe3e9..0f52d3c4bc1d 100644
--- a/drivers/misc/eeprom/eeprom_93cx6.c
+++ b/drivers/misc/eeprom/eeprom_93cx6.c
@@ -18,6 +18,11 @@ MODULE_VERSION("1.0");
 MODULE_DESCRIPTION("EEPROM 93cx6 chip driver");
 MODULE_LICENSE("GPL");

+static inline bool has_quirk_extra_read_cycle(struct eeprom_93cx6 *eeprom)
+{
+	return eeprom->quirks & PCI_EEPROM_QUIRK_EXTRA_READ_CYCLE;
+}
+
 static inline void eeprom_93cx6_pulse_high(struct eeprom_93cx6 *eeprom)
 {
 	eeprom->reg_data_clock = 1;
@@ -186,6 +191,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 +262,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.
 	 */
diff --git a/include/linux/eeprom_93cx6.h b/include/linux/eeprom_93cx6.h
index c860c72a921d..5274bcc7ca39 100644
--- a/include/linux/eeprom_93cx6.h
+++ b/include/linux/eeprom_93cx6.h
@@ -11,6 +11,8 @@
 	Supported chipsets: 93c46, 93c56 and 93c66.
  */

+#include <linux/bits.h>
+
 /*
  * EEPROM operation defines.
  */
@@ -25,6 +27,9 @@
 #define PCI_EEPROM_EWDS_OPCODE	0x10
 #define PCI_EEPROM_EWEN_OPCODE	0x13

+/* Some EEPROMs require an extra clock cycle before reading */
+#define PCI_EEPROM_QUIRK_EXTRA_READ_CYCLE	BIT(0)
+
 /**
  * struct eeprom_93cx6 - control structure for setting the commands
  * for reading the eeprom data.
@@ -34,6 +39,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 quirk bitfield
  * @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 +56,7 @@ struct eeprom_93cx6 {
 	void (*register_write)(struct eeprom_93cx6 *eeprom);

 	int width;
+	unsigned int quirks;

 	char drive_data;
 	char reg_data_in;
--
2.46.0
Re: [PATCH v1 1/6] misc: eeprom: eeprom_93cx6: Add quirk for extra read clock cycle
Posted by Andy Shevchenko 2 months, 2 weeks ago 
On Fri, Sep 13, 2024 at 10:55:38AM -0400, Parker Newman wrote:
> From: Parker Newman <pnewman@connecttech.com>
> 
> 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. Similar notes
> are found in other 93xx6 datasheets.

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

Make it a tag (i.e. locate just above your SoB tag)

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

The mixed TABs/space here (one extra space after :)

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

Also leading spaces, please remove them and use TAB, or use spaces only.

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

...

> +static inline bool has_quirk_extra_read_cycle(struct eeprom_93cx6 *eeprom)
> +{
> +	return eeprom->quirks & PCI_EEPROM_QUIRK_EXTRA_READ_CYCLE;
> +}

So, this makes sense to be in a header since everything else related to that
also in the header already.

...

> +	if (has_quirk_extra_read_cycle(eeprom)) {
> +		eeprom_93cx6_pulse_high(eeprom);

No additional delay is needed?

> +		eeprom_93cx6_pulse_low(eeprom);
> +	}

> +	if (has_quirk_extra_read_cycle(eeprom)) {
> +		eeprom_93cx6_pulse_high(eeprom);

Ditto.

> +		eeprom_93cx6_pulse_low(eeprom);
> +	}

...

> +/* Some EEPROMs require an extra clock cycle before reading */
> +#define PCI_EEPROM_QUIRK_EXTRA_READ_CYCLE	BIT(0)

I would move it directly into the structure definitions, just after quirk
field (the same way it's done in the other driver)...

...

>  	int width;
> +	unsigned int quirks;

...somewhere here.

>  	char drive_data;
>  	char reg_data_in;

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 1/6] misc: eeprom: eeprom_93cx6: Add quirk for extra read clock cycle
Posted by Parker Newman 2 months, 2 weeks ago 
On Fri, 13 Sep 2024 20:48:03 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Fri, Sep 13, 2024 at 10:55:38AM -0400, Parker Newman wrote:
> > From: Parker Newman <pnewman@connecttech.com>
> >
> > 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. Similar notes
> > are found in other 93xx6 datasheets.
>
> > Link: https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-5193-SEEPROM-AT93C46D-Datasheet.pdf
>
> Make it a tag (i.e. locate just above your SoB tag)
>

Sorry, not 100% sure what you mean by tag? Do I just need to move the Link: entry
to be above my Sign-off? Or is there something else? Thanks!

> > 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)
>
> The mixed TABs/space here (one extra space after :)
>
> > 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.
>
> Also leading spaces, please remove them and use TAB, or use spaces only.
>

Ugh, copy/paste is hard! I will fix :).

> > 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.
>
> ...
>
> > +static inline bool has_quirk_extra_read_cycle(struct eeprom_93cx6 *eeprom)
> > +{
> > +	return eeprom->quirks & PCI_EEPROM_QUIRK_EXTRA_READ_CYCLE;
> > +}
>
> So, this makes sense to be in a header since everything else related to that
> also in the header already.

Makes sense, will do.

> ...
>
> > +	if (has_quirk_extra_read_cycle(eeprom)) {
> > +		eeprom_93cx6_pulse_high(eeprom);
>
> No additional delay is needed?
>

Should not need any extra delay as both pulse high/low functions have the worst case
450ns delay after the register write. It was working well on my test cards.

> > +		eeprom_93cx6_pulse_low(eeprom);
> > +	}
>
> > +	if (has_quirk_extra_read_cycle(eeprom)) {
> > +		eeprom_93cx6_pulse_high(eeprom);
>
> Ditto.
>
> > +		eeprom_93cx6_pulse_low(eeprom);
> > +	}
>
> ...
>
> > +/* Some EEPROMs require an extra clock cycle before reading */
> > +#define PCI_EEPROM_QUIRK_EXTRA_READ_CYCLE	BIT(0)
>
> I would move it directly into the structure definitions, just after quirk
> field (the same way it's done in the other driver)...
>

Will do, thanks!

> ...
>
> >  	int width;
> > +	unsigned int quirks;
>
> ...somewhere here.
>
> >  	char drive_data;
> >  	char reg_data_in;
>
Re: [PATCH v1 1/6] misc: eeprom: eeprom_93cx6: Add quirk for extra read clock cycle
Posted by Andy Shevchenko 2 months, 2 weeks ago 
On Fri, Sep 13, 2024 at 02:24:20PM -0400, Parker Newman wrote:
> On Fri, 13 Sep 2024 20:48:03 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Sep 13, 2024 at 10:55:38AM -0400, Parker Newman wrote:

...

> > > Link: https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-5193-SEEPROM-AT93C46D-Datasheet.pdf
> >
> > Make it a tag (i.e. locate just above your SoB tag)
> 
> Sorry, not 100% sure what you mean by tag? Do I just need to move the Link: entry
> to be above my Sign-off? Or is there something else? Thanks!

Make it like

  ...Summary...
  <blank line>
  ...commit message text...
  <blank line>
  Link: ...
  Signed-off-by: ...

...

> > > +	if (has_quirk_extra_read_cycle(eeprom)) {
> > > +		eeprom_93cx6_pulse_high(eeprom);
> >
> > No additional delay is needed?
> 
> Should not need any extra delay as both pulse high/low functions have the worst case
> 450ns delay after the register write. It was working well on my test cards.

OK!

> > > +		eeprom_93cx6_pulse_low(eeprom);
> > > +	}

-- 
With Best Regards,
Andy Shevchenko

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.