Re: [PATCH v2 00/13] serial: 8250_exar: Clean up the driver

[Parker Newman]

On Wed, 11 Sep 2024 23:51:09 +0300
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:

> On Wed, Sep 11, 2024 at 01:38:48PM -0400, Parker Newman wrote:
> > On Mon, 9 Sep 2024 13:06:26 +0300
> > Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > > On Fri, Sep 06, 2024 at 02:38:51PM -0400, Parker Newman wrote:
> > > > On Fri, 6 Sep 2024 17:42:26 +0300
> > > > Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > > > > On Fri, Sep 06, 2024 at 10:33:54AM -0400, Parker Newman wrote:
> > > > > > On Fri, 6 Sep 2024 17:24:44 +0300
> > > > > > Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > > > > > > On Fri, Sep 06, 2024 at 09:51:41AM -0400, Parker Newman wrote:
> > > > > > > > On Fri, 6 Sep 2024 15:46:51 +0300
> > > > > > > > Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > > > > > > > > On Fri, May 03, 2024 at 02:33:03PM -0400, Parker Newman wrote:
>
> ...
>
> > > > > > > > > Sorry for blast from the past, but I have some instersting information
> > > > > > > > > for you. We now have spi-gpio and 93c46 eeprom drivers available to be
> > > > > > > > > used from others via software nodes, can you consider updating your code
> > > > > > > > > to replace custom bitbanging along with r/w ops by the instantiating the
> > > > > > > > > respective drivers?
> > > > > > > >
> > > > > > > > Hi Andy,
> > > > > > > > The Exar UARTs don't actually use MPIO/GPIO for the EEPROM.
> > > > > > > > They have a dedicated "EEPROM interface" which is accessed by the
> > > > > > > > REGB (0x8E) register. It is a very simple bit-bang interface though,
> > > > > > > > one bit per signal.
> > > > > > > >
> > > > > > > > I guess in theory I could either add  GPIO wrapper to toggle these bits
> > > > > > > > and use the spi-gpio driver but I am not sure if that really improves things?
> > > > > > > > Maybe using the spi-bitbang driver directly is more appropriate?
> > > > > > > > What do you think?
> > > > > > >
> > > > > > > Yes, spi-bitbang seems better in this case.
> > > > > >
> > > > > > I will try to make some time to implement this... Or if someone else from the
> > > > > > community wants to take this on in the mean time I am certainly happy to test
> > > > > > and help out!
> > > > >
> > > > > Sure, I shared this thought due to having lack of time to look myself,
> > > > > but I prepared the above mentioned drivers to make them work in this case.
> > > > > (If you are curios, see the Git history for the last few releases with
> > > > >  --author="Andy Shevchenko")
> > > > >
> > > >
> > > > Looking into it a bit more I think we could just use the eeprom_93cx6
> > > > driver without any SPI layer. Just need to add simple register_read()
> > > > and register_write() functions to read/write the REB register.
> > > >
> > > > That should be a pretty easy change to make, I can try to make that
> > > > change soon unless anyone has any objections to that method?
> > >
> > > Thank you, this is pretty wonderful news!
> > >
> >
> > I have this mostly working however there is one issue. The eeprom_93cx6
> > driver doesn't seem to discard the "dummy bit" the 93C46 EEPROM outputs
> > between the writing of the op-code/address to the EEPROM and the reading
> > of the data from the EEPROM.
> >
> > More info can be found on page 6 of the AT93C46 datasheet. I see similar
> > notes in other 93C46/93C56/93C66 datasheets.
> > Link: https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-5193-SEEPROM-AT93C46D-Datasheet.pdf
> >
> > In summary the read operation for the AT93C46 EEPROM is:
> > Write to EEPROM :	110[A5-A0]	(9 bits)
> > Read from EEPROM: 	0[D15-D0]	(17 bits)
> >
> > Where 110 is the READ OpCode, [A5-A0] is the address to read from,
> > 0 is a "dummy bit" and then [D15-D0] is the actual data.
> >
> > I am seeing the "correct" values being read from the EEPROM when using the
> > eeprom_93cx6 driver but they are all shifted right by one because the
> > dummy 0 bit is not being discarded.
> >
> > The confusing part is the eeprom_93cx6 driver has behaved the same since
> > at least 2009 and half a dozen or so other drivers use it. I am not sure
> > if they just work around and/or live with this bug or if they have
> > different HW that handles the extra dummy bit?
>
> I briefly looked at a few users and it seems to me:
> 1) either the Atmel chip has different HW protocol;
> 2) or all of them handle that in HW transparently to SW.

The 3 Exar cards I have handy actually use the ST M93C46 version but looking
through our BOMs I see AT/CAT/ST used on various cards over the years.

Looking at the READ timing diagrams in the Atmel and ST datasheets it looks
like the dummy bit should actually be clocked out on the last address bit
clock cycle. If this were so it would be ignored naturally.

This may just be a quirk of the Exar HW. All Exar code I have looked at
manually discards the dummy bit.

> > I am hesitant to "fix" the eeprom_93cx6 driver and potentially break the
> > other users of it. I could add a flag to the eeprom_93cx6 struct to work
> > around this issue... Unless anyone else has some ideas or input?
>
> In my opinion the 93c46 needs an additional configuration setting (in the
> respective data structure) and some code to implement what you need here.

I see the eeprom_93xx46 driver has the QUIRK_EXTRA_READ_CYCLE quirk to solve
this issue. I could add something similar.

> But yes, let's wait a bit for other opinions...
>