mbox series

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

Message ID 20240503171917.2921250-1-andriy.shevchenko@linux.intel.com
Headers show
Series serial: 8250_exar: Clean up the driver | expand

Message

Andy Shevchenko May 3, 2024, 5:15 p.m. UTC
After a rework for CONNTECH was done, the driver may need a bit of
love in order to become less verbose (in terms of indentation and
code duplication) and hence easier to read.

This clean up series fixes a couple of (not so critical) issues and
cleans up the recently added code. No functional change indented by
the cleaning up part.

Parker, please test this and give your formal Tested-by tag
(you may do it by replying to this message if all patches are
 successfully tested; more details about tags are available in
 the Submitting Patches documentation).

In v2:
- fixed the EEPROM reading data loop (Ilpo, Parker)

Andy Shevchenko (13):
  serial: 8250_exar: Don't return positive values as error codes
  serial: 8250_exar: Describe all parameters in kernel doc
  serial: 8250_exar: Kill CTI_PCI_DEVICE()
  serial: 8250_exar: Use PCI_SUBVENDOR_ID_IBM for subvendor ID
  serial: 8250_exar: Trivia typo fixes
  serial: 8250_exar: Extract cti_board_init_osc_freq() helper
  serial: 8250_exar: Kill unneeded ->board_init()
  serial: 8250_exar: Decrease indentation level
  serial: 8250_exar: Return directly from switch-cases
  serial: 8250_exar: Switch to use dev_err_probe()
  serial: 8250_exar: Use BIT() in exar_ee_read()
  serial: 8250_exar: Make type of bit the same in exar_ee_*_bit()
  serial: 8250_exar: Keep the includes sorted

 drivers/tty/serial/8250/8250_exar.c | 459 ++++++++++++----------------
 1 file changed, 203 insertions(+), 256 deletions(-)

Comments

Parker Newman May 3, 2024, 6:33 p.m. UTC | #1
On Fri,  3 May 2024 20:15:52 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> After a rework for CONNTECH was done, the driver may need a bit of
> love in order to become less verbose (in terms of indentation and
> code duplication) and hence easier to read.
>
> This clean up series fixes a couple of (not so critical) issues and
> cleans up the recently added code. No functional change indented by
> the cleaning up part.
>
> Parker, please test this and give your formal Tested-by tag
> (you may do it by replying to this message if all patches are
>  successfully tested; more details about tags are available in
>  the Submitting Patches documentation).
>

I was able to test the Connect Tech related code and everything is
work as expected. I can't test the non-CTI related changes but they
are pretty minor.

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

> In v2:
> - fixed the EEPROM reading data loop (Ilpo, Parker)
>
> Andy Shevchenko (13):
>   serial: 8250_exar: Don't return positive values as error codes
>   serial: 8250_exar: Describe all parameters in kernel doc
>   serial: 8250_exar: Kill CTI_PCI_DEVICE()
>   serial: 8250_exar: Use PCI_SUBVENDOR_ID_IBM for subvendor ID
>   serial: 8250_exar: Trivia typo fixes
>   serial: 8250_exar: Extract cti_board_init_osc_freq() helper
>   serial: 8250_exar: Kill unneeded ->board_init()
>   serial: 8250_exar: Decrease indentation level
>   serial: 8250_exar: Return directly from switch-cases
>   serial: 8250_exar: Switch to use dev_err_probe()
>   serial: 8250_exar: Use BIT() in exar_ee_read()
>   serial: 8250_exar: Make type of bit the same in exar_ee_*_bit()
>   serial: 8250_exar: Keep the includes sorted
>
>  drivers/tty/serial/8250/8250_exar.c | 459 ++++++++++++----------------
>  1 file changed, 203 insertions(+), 256 deletions(-)
>
Andy Shevchenko Sept. 6, 2024, 12:46 p.m. UTC | #2
On Fri, May 03, 2024 at 02:33:03PM -0400, Parker Newman wrote:
> On Fri,  3 May 2024 20:15:52 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > After a rework for CONNTECH was done, the driver may need a bit of
> > love in order to become less verbose (in terms of indentation and
> > code duplication) and hence easier to read.
> >
> > This clean up series fixes a couple of (not so critical) issues and
> > cleans up the recently added code. No functional change indented by
> > the cleaning up part.
> >
> > Parker, please test this and give your formal Tested-by tag
> > (you may do it by replying to this message if all patches are
> >  successfully tested; more details about tags are available in
> >  the Submitting Patches documentation).
> 
> I was able to test the Connect Tech related code and everything is
> work as expected. I can't test the non-CTI related changes but they
> are pretty minor.
> 
> Tested-by: Parker Newman <pnewman@connecttech.com>

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?
Parker Newman Sept. 6, 2024, 1:51 p.m. UTC | #3
On Fri, 6 Sep 2024 15:46:51 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Fri, May 03, 2024 at 02:33:03PM -0400, Parker Newman wrote:
> > On Fri,  3 May 2024 20:15:52 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >
> > > After a rework for CONNTECH was done, the driver may need a bit of
> > > love in order to become less verbose (in terms of indentation and
> > > code duplication) and hence easier to read.
> > >
> > > This clean up series fixes a couple of (not so critical) issues and
> > > cleans up the recently added code. No functional change indented by
> > > the cleaning up part.
> > >
> > > Parker, please test this and give your formal Tested-by tag
> > > (you may do it by replying to this message if all patches are
> > >  successfully tested; more details about tags are available in
> > >  the Submitting Patches documentation).
> >
> > I was able to test the Connect Tech related code and everything is
> > work as expected. I can't test the non-CTI related changes but they
> > are pretty minor.
> >
> > Tested-by: Parker Newman <pnewman@connecttech.com>
>
> 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?

Thanks,
Parker
Andy Shevchenko Sept. 6, 2024, 2:24 p.m. UTC | #4
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@linux.intel.com> wrote:
> > On Fri, May 03, 2024 at 02:33:03PM -0400, Parker Newman wrote:
> > > On Fri,  3 May 2024 20:15:52 +0300
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > > After a rework for CONNTECH was done, the driver may need a bit of
> > > > love in order to become less verbose (in terms of indentation and
> > > > code duplication) and hence easier to read.
> > > >
> > > > This clean up series fixes a couple of (not so critical) issues and
> > > > cleans up the recently added code. No functional change indented by
> > > > the cleaning up part.
> > > >
> > > > Parker, please test this and give your formal Tested-by tag
> > > > (you may do it by replying to this message if all patches are
> > > >  successfully tested; more details about tags are available in
> > > >  the Submitting Patches documentation).
> > >
> > > I was able to test the Connect Tech related code and everything is
> > > work as expected. I can't test the non-CTI related changes but they
> > > are pretty minor.
> > >
> > > Tested-by: Parker Newman <pnewman@connecttech.com>
> >
> > 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.
Parker Newman Sept. 6, 2024, 2:33 p.m. UTC | #5
On Fri, 6 Sep 2024 17:24:44 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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@linux.intel.com> wrote:
> > > On Fri, May 03, 2024 at 02:33:03PM -0400, Parker Newman wrote:
> > > > On Fri,  3 May 2024 20:15:52 +0300
> > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > > After a rework for CONNTECH was done, the driver may need a bit of
> > > > > love in order to become less verbose (in terms of indentation and
> > > > > code duplication) and hence easier to read.
> > > > >
> > > > > This clean up series fixes a couple of (not so critical) issues and
> > > > > cleans up the recently added code. No functional change indented by
> > > > > the cleaning up part.
> > > > >
> > > > > Parker, please test this and give your formal Tested-by tag
> > > > > (you may do it by replying to this message if all patches are
> > > > >  successfully tested; more details about tags are available in
> > > > >  the Submitting Patches documentation).
> > > >
> > > > I was able to test the Connect Tech related code and everything is
> > > > work as expected. I can't test the non-CTI related changes but they
> > > > are pretty minor.
> > > >
> > > > Tested-by: Parker Newman <pnewman@connecttech.com>
> > >
> > > 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!
Parker
Andy Shevchenko Sept. 6, 2024, 2:42 p.m. UTC | #6
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@linux.intel.com> 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@linux.intel.com> 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")
Parker Newman Sept. 6, 2024, 6:38 p.m. UTC | #7
On Fri, 6 Sep 2024 17:42:26 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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@linux.intel.com> 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@linux.intel.com> 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?

Thanks,
Parker
Andy Shevchenko Sept. 9, 2024, 10:06 a.m. UTC | #8
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@linux.intel.com> 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@linux.intel.com> 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@linux.intel.com> 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!
Parker Newman Sept. 11, 2024, 5:38 p.m. UTC | #9
On Mon, 9 Sep 2024 13:06:26 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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@linux.intel.com> 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@linux.intel.com> 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@linux.intel.com> 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 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?

Thanks,
Parker
Andy Shevchenko Sept. 11, 2024, 8:51 p.m. UTC | #10
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@linux.intel.com> 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@linux.intel.com> 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@linux.intel.com> 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@linux.intel.com> 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.

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

But yes, let's wait a bit for other opinions...
Parker Newman Sept. 12, 2024, 12:41 p.m. UTC | #11
On Wed, 11 Sep 2024 23:51:09 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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@linux.intel.com> 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@linux.intel.com> 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@linux.intel.com> 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@linux.intel.com> 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...
>
Andy Shevchenko Sept. 13, 2024, 9:37 a.m. UTC | #12
On Thu, Sep 12, 2024 at 08:41:47AM -0400, Parker Newman wrote:
> On Wed, 11 Sep 2024 23:51:09 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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@linux.intel.com> 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@linux.intel.com> 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@linux.intel.com> 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@linux.intel.com> 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.

Seems like a good plan to me, and thanks again for looking into this!

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