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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240913142420.675faf80@SWDEV2.connecttech.local>
Date: Fri, 13 Sep 2024 14:24:20 -0400
From: Parker Newman <parker@...est.io>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jiri Slaby
 <jirislaby@...nel.org>, Arnd Bergmann <arnd@...db.de>,
 linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org, Parker Newman
 <pnewman@...necttech.com>
Subject: Re: [PATCH v1 1/6] misc: eeprom: eeprom_93cx6: Add quirk for extra
 read clock cycle

On Fri, 13 Sep 2024 20:48:03 +0300
Andy Shevchenko <andriy.shevchenko@...ux.intel.com> wrote:

> On Fri, Sep 13, 2024 at 10:55:38AM -0400, Parker Newman wrote:
> > From: Parker Newman <pnewman@...necttech.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;
>