| Message ID | 20240503171917.2921250-1-andriy.shevchenko@linux.intel.com |
|---|---|
| Headers | show |
| Series | serial: 8250_exar: Clean up the driver | expand |
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(-) >
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?
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
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.
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
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")
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
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!
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
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...
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... >
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...