* [PATCH] ASoC: RT5677-SPI: Disable 16Bit SPI Transfers
@ 2019-05-02 22:20 Curtis Malainey
2019-05-02 23:25 ` Ben Zhang
0 siblings, 1 reply; 3+ messages in thread
From: Curtis Malainey @ 2019-05-02 22:20 UTC (permalink / raw)
To: alsa-devel
Cc: Oder Chiou, Takashi Iwai, Liam Girdwood, Ben Zhang, Mark Brown,
Bard Liao, Curtis Malainey
The current algorithm allows 3 types of transfers, 16bit, 32bit and
burst. According to Realtek, 16bit transfers have a special restriction
in that it is restricted to the memory region of
0x18020000 ~ 0x18021000. This region is the memory location of the I2C
registers. The current algorithm does not uphold this restriction and
therefore fails to complete writes.
Since this has been broken for some time it likely no one is using it.
Better to simply disable the 16 bit writes. This will allow users to
properly load firmware over SPI without data corruption.
CC: Ben Zhang <benzh@chromium.org>
CC: Oder Chiou <oder_chiou@realtek.com>
Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
---
sound/soc/codecs/rt5677-spi.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/sound/soc/codecs/rt5677-spi.c b/sound/soc/codecs/rt5677-spi.c
index 167a02773a0b..b296e62fdbb4 100644
--- a/sound/soc/codecs/rt5677-spi.c
+++ b/sound/soc/codecs/rt5677-spi.c
@@ -58,13 +58,15 @@ static DEFINE_MUTEX(spi_mutex);
* RT5677_SPI_READ/WRITE_32: Transfer 4 bytes
* RT5677_SPI_READ/WRITE_BURST: Transfer any multiples of 8 bytes
*
- * For example, reading 260 bytes at 0x60030002 uses the following commands:
- * 0x60030002 RT5677_SPI_READ_16 2 bytes
+ * Note:
+ * 16 Bit writes and reads are restricted to the address range
+ * 0x18020000 ~ 0x18021000
+ *
+ * For example, reading 256 bytes at 0x60030004 uses the following commands:
* 0x60030004 RT5677_SPI_READ_32 4 bytes
* 0x60030008 RT5677_SPI_READ_BURST 240 bytes
* 0x600300F8 RT5677_SPI_READ_BURST 8 bytes
* 0x60030100 RT5677_SPI_READ_32 4 bytes
- * 0x60030104 RT5677_SPI_READ_16 2 bytes
*
* Input:
* @read: true for read commands; false for write commands
@@ -79,15 +81,13 @@ static u8 rt5677_spi_select_cmd(bool read, u32 align, u32 remain, u32 *len)
{
u8 cmd;
- if (align == 2 || align == 6 || remain == 2) {
- cmd = RT5677_SPI_READ_16;
- *len = 2;
- } else if (align == 4 || remain <= 6) {
+ if (align == 4 || remain <= 4) {
cmd = RT5677_SPI_READ_32;
*len = 4;
} else {
cmd = RT5677_SPI_READ_BURST;
- *len = min_t(u32, remain & ~7, RT5677_SPI_BURST_LEN);
+ *len = (((remain - 1) >> 3) + 1) << 3;
+ *len = min_t(u32, *len, RT5677_SPI_BURST_LEN);
}
return read ? cmd : cmd + 1;
}
@@ -108,7 +108,7 @@ static void rt5677_spi_reverse(u8 *dst, u32 dstlen, const u8 *src, u32 srclen)
}
}
-/* Read DSP address space using SPI. addr and len have to be 2-byte aligned. */
+/* Read DSP address space using SPI. addr and len have to be 4-byte aligned. */
int rt5677_spi_read(u32 addr, void *rxbuf, size_t len)
{
u32 offset;
@@ -124,7 +124,7 @@ int rt5677_spi_read(u32 addr, void *rxbuf, size_t len)
if (!g_spi)
return -ENODEV;
- if ((addr & 1) || (len & 1)) {
+ if ((addr & 3) || (len & 3)) {
dev_err(&g_spi->dev, "Bad read align 0x%x(%zu)\n", addr, len);
return -EACCES;
}
@@ -159,8 +159,8 @@ int rt5677_spi_read(u32 addr, void *rxbuf, size_t len)
}
EXPORT_SYMBOL_GPL(rt5677_spi_read);
-/* Write DSP address space using SPI. addr has to be 2-byte aligned.
- * If len is not 2-byte aligned, an extra byte of zero is written at the end
+/* Write DSP address space using SPI. addr has to be 4-byte aligned.
+ * If len is not 4-byte aligned, then extra zeros are written at the end
* as padding.
*/
int rt5677_spi_write(u32 addr, const void *txbuf, size_t len)
@@ -178,7 +178,7 @@ int rt5677_spi_write(u32 addr, const void *txbuf, size_t len)
if (!g_spi)
return -ENODEV;
- if (addr & 1) {
+ if (addr & 3) {
dev_err(&g_spi->dev, "Bad write align 0x%x(%zu)\n", addr, len);
return -EACCES;
}
--
2.21.0.593.g511ec345e18-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] ASoC: RT5677-SPI: Disable 16Bit SPI Transfers
2019-05-02 22:20 [PATCH] ASoC: RT5677-SPI: Disable 16Bit SPI Transfers Curtis Malainey
@ 2019-05-02 23:25 ` Ben Zhang
2019-05-02 23:58 ` Curtis Malainey
0 siblings, 1 reply; 3+ messages in thread
From: Ben Zhang @ 2019-05-02 23:25 UTC (permalink / raw)
To: Curtis Malainey
Cc: Oder Chiou, alsa-devel, Takashi Iwai, Liam Girdwood, Mark Brown,
Bard Liao
On Thu, May 2, 2019 at 6:20 PM Curtis Malainey
<cujomalainey@chromium.org> wrote:
>
> The current algorithm allows 3 types of transfers, 16bit, 32bit and
> burst. According to Realtek, 16bit transfers have a special restriction
> in that it is restricted to the memory region of
> 0x18020000 ~ 0x18021000. This region is the memory location of the I2C
> registers. The current algorithm does not uphold this restriction and
> therefore fails to complete writes.
>
> Since this has been broken for some time it likely no one is using it.
> Better to simply disable the 16 bit writes. This will allow users to
> properly load firmware over SPI without data corruption.
>
> CC: Ben Zhang <benzh@chromium.org>
> CC: Oder Chiou <oder_chiou@realtek.com>
> Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
> ---
> sound/soc/codecs/rt5677-spi.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/sound/soc/codecs/rt5677-spi.c b/sound/soc/codecs/rt5677-spi.c
> index 167a02773a0b..b296e62fdbb4 100644
> --- a/sound/soc/codecs/rt5677-spi.c
> +++ b/sound/soc/codecs/rt5677-spi.c
> @@ -58,13 +58,15 @@ static DEFINE_MUTEX(spi_mutex);
> * RT5677_SPI_READ/WRITE_32: Transfer 4 bytes
> * RT5677_SPI_READ/WRITE_BURST: Transfer any multiples of 8 bytes
> *
> - * For example, reading 260 bytes at 0x60030002 uses the following commands:
> - * 0x60030002 RT5677_SPI_READ_16 2 bytes
> + * Note:
> + * 16 Bit writes and reads are restricted to the address range
> + * 0x18020000 ~ 0x18021000
> + *
> + * For example, reading 256 bytes at 0x60030004 uses the following commands:
> * 0x60030004 RT5677_SPI_READ_32 4 bytes
> * 0x60030008 RT5677_SPI_READ_BURST 240 bytes
> * 0x600300F8 RT5677_SPI_READ_BURST 8 bytes
> * 0x60030100 RT5677_SPI_READ_32 4 bytes
> - * 0x60030104 RT5677_SPI_READ_16 2 bytes
> *
> * Input:
> * @read: true for read commands; false for write commands
> @@ -79,15 +81,13 @@ static u8 rt5677_spi_select_cmd(bool read, u32 align, u32 remain, u32 *len)
> {
> u8 cmd;
>
> - if (align == 2 || align == 6 || remain == 2) {
> - cmd = RT5677_SPI_READ_16;
> - *len = 2;
> - } else if (align == 4 || remain <= 6) {
> + if (align == 4 || remain <= 4) {
> cmd = RT5677_SPI_READ_32;
> *len = 4;
> } else {
> cmd = RT5677_SPI_READ_BURST;
> - *len = min_t(u32, remain & ~7, RT5677_SPI_BURST_LEN);
> + *len = (((remain - 1) >> 3) + 1) << 3;
> + *len = min_t(u32, *len, RT5677_SPI_BURST_LEN);
Since it already handles the case where remain&7 != 0 here, I think we
can remove len_with_pad in rt5677_spi_write and replace it with len in
the for loop to make it simpler.
> }
> return read ? cmd : cmd + 1;
> }
> @@ -108,7 +108,7 @@ static void rt5677_spi_reverse(u8 *dst, u32 dstlen, const u8 *src, u32 srclen)
> }
> }
>
> -/* Read DSP address space using SPI. addr and len have to be 2-byte aligned. */
> +/* Read DSP address space using SPI. addr and len have to be 4-byte aligned. */
> int rt5677_spi_read(u32 addr, void *rxbuf, size_t len)
> {
> u32 offset;
> @@ -124,7 +124,7 @@ int rt5677_spi_read(u32 addr, void *rxbuf, size_t len)
> if (!g_spi)
> return -ENODEV;
>
> - if ((addr & 1) || (len & 1)) {
> + if ((addr & 3) || (len & 3)) {
> dev_err(&g_spi->dev, "Bad read align 0x%x(%zu)\n", addr, len);
> return -EACCES;
> }
> @@ -159,8 +159,8 @@ int rt5677_spi_read(u32 addr, void *rxbuf, size_t len)
> }
> EXPORT_SYMBOL_GPL(rt5677_spi_read);
>
> -/* Write DSP address space using SPI. addr has to be 2-byte aligned.
> - * If len is not 2-byte aligned, an extra byte of zero is written at the end
> +/* Write DSP address space using SPI. addr has to be 4-byte aligned.
> + * If len is not 4-byte aligned, then extra zeros are written at the end
> * as padding.
> */
> int rt5677_spi_write(u32 addr, const void *txbuf, size_t len)
> @@ -178,7 +178,7 @@ int rt5677_spi_write(u32 addr, const void *txbuf, size_t len)
> if (!g_spi)
> return -ENODEV;
>
> - if (addr & 1) {
> + if (addr & 3) {
> dev_err(&g_spi->dev, "Bad write align 0x%x(%zu)\n", addr, len);
> return -EACCES;
> }
> --
> 2.21.0.593.g511ec345e18-goog
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ASoC: RT5677-SPI: Disable 16Bit SPI Transfers
2019-05-02 23:25 ` Ben Zhang
@ 2019-05-02 23:58 ` Curtis Malainey
0 siblings, 0 replies; 3+ messages in thread
From: Curtis Malainey @ 2019-05-02 23:58 UTC (permalink / raw)
To: Ben Zhang
Cc: Oder Chiou, alsa-devel, Takashi Iwai, Liam Girdwood, Mark Brown,
Bard Liao, Curtis Malainey
On Thu, May 2, 2019 at 4:25 PM Ben Zhang <benzh@chromium.org> wrote:
>
> On Thu, May 2, 2019 at 6:20 PM Curtis Malainey
> <cujomalainey@chromium.org> wrote:
> >
> > The current algorithm allows 3 types of transfers, 16bit, 32bit and
> > burst. According to Realtek, 16bit transfers have a special restriction
> > in that it is restricted to the memory region of
> > 0x18020000 ~ 0x18021000. This region is the memory location of the I2C
> > registers. The current algorithm does not uphold this restriction and
> > therefore fails to complete writes.
> >
> > Since this has been broken for some time it likely no one is using it.
> > Better to simply disable the 16 bit writes. This will allow users to
> > properly load firmware over SPI without data corruption.
> >
> > CC: Ben Zhang <benzh@chromium.org>
> > CC: Oder Chiou <oder_chiou@realtek.com>
> > Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
> > ---
> > sound/soc/codecs/rt5677-spi.c | 26 +++++++++++++-------------
> > 1 file changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/sound/soc/codecs/rt5677-spi.c b/sound/soc/codecs/rt5677-spi.c
> > index 167a02773a0b..b296e62fdbb4 100644
> > --- a/sound/soc/codecs/rt5677-spi.c
> > +++ b/sound/soc/codecs/rt5677-spi.c
> > @@ -58,13 +58,15 @@ static DEFINE_MUTEX(spi_mutex);
> > * RT5677_SPI_READ/WRITE_32: Transfer 4 bytes
> > * RT5677_SPI_READ/WRITE_BURST: Transfer any multiples of 8 bytes
> > *
> > - * For example, reading 260 bytes at 0x60030002 uses the following commands:
> > - * 0x60030002 RT5677_SPI_READ_16 2 bytes
> > + * Note:
> > + * 16 Bit writes and reads are restricted to the address range
> > + * 0x18020000 ~ 0x18021000
> > + *
> > + * For example, reading 256 bytes at 0x60030004 uses the following commands:
> > * 0x60030004 RT5677_SPI_READ_32 4 bytes
> > * 0x60030008 RT5677_SPI_READ_BURST 240 bytes
> > * 0x600300F8 RT5677_SPI_READ_BURST 8 bytes
> > * 0x60030100 RT5677_SPI_READ_32 4 bytes
> > - * 0x60030104 RT5677_SPI_READ_16 2 bytes
> > *
> > * Input:
> > * @read: true for read commands; false for write commands
> > @@ -79,15 +81,13 @@ static u8 rt5677_spi_select_cmd(bool read, u32 align, u32 remain, u32 *len)
> > {
> > u8 cmd;
> >
> > - if (align == 2 || align == 6 || remain == 2) {
> > - cmd = RT5677_SPI_READ_16;
> > - *len = 2;
> > - } else if (align == 4 || remain <= 6) {
> > + if (align == 4 || remain <= 4) {
> > cmd = RT5677_SPI_READ_32;
> > *len = 4;
> > } else {
> > cmd = RT5677_SPI_READ_BURST;
> > - *len = min_t(u32, remain & ~7, RT5677_SPI_BURST_LEN);
> > + *len = (((remain - 1) >> 3) + 1) << 3;
> > + *len = min_t(u32, *len, RT5677_SPI_BURST_LEN);
>
> Since it already handles the case where remain&7 != 0 here, I think we
> can remove len_with_pad in rt5677_spi_write and replace it with len in
> the for loop to make it simpler.
>
Tested that and it works, I'll send the updated patch tomorrow with
any more reviews that come in overnight. Thanks.
> > }
> > return read ? cmd : cmd + 1;
> > }
> > @@ -108,7 +108,7 @@ static void rt5677_spi_reverse(u8 *dst, u32 dstlen, const u8 *src, u32 srclen)
> > }
> > }
> >
> > -/* Read DSP address space using SPI. addr and len have to be 2-byte aligned. */
> > +/* Read DSP address space using SPI. addr and len have to be 4-byte aligned. */
> > int rt5677_spi_read(u32 addr, void *rxbuf, size_t len)
> > {
> > u32 offset;
> > @@ -124,7 +124,7 @@ int rt5677_spi_read(u32 addr, void *rxbuf, size_t len)
> > if (!g_spi)
> > return -ENODEV;
> >
> > - if ((addr & 1) || (len & 1)) {
> > + if ((addr & 3) || (len & 3)) {
> > dev_err(&g_spi->dev, "Bad read align 0x%x(%zu)\n", addr, len);
> > return -EACCES;
> > }
> > @@ -159,8 +159,8 @@ int rt5677_spi_read(u32 addr, void *rxbuf, size_t len)
> > }
> > EXPORT_SYMBOL_GPL(rt5677_spi_read);
> >
> > -/* Write DSP address space using SPI. addr has to be 2-byte aligned.
> > - * If len is not 2-byte aligned, an extra byte of zero is written at the end
> > +/* Write DSP address space using SPI. addr has to be 4-byte aligned.
> > + * If len is not 4-byte aligned, then extra zeros are written at the end
> > * as padding.
> > */
> > int rt5677_spi_write(u32 addr, const void *txbuf, size_t len)
> > @@ -178,7 +178,7 @@ int rt5677_spi_write(u32 addr, const void *txbuf, size_t len)
> > if (!g_spi)
> > return -ENODEV;
> >
> > - if (addr & 1) {
> > + if (addr & 3) {
> > dev_err(&g_spi->dev, "Bad write align 0x%x(%zu)\n", addr, len);
> > return -EACCES;
> > }
> > --
> > 2.21.0.593.g511ec345e18-goog
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-05-02 23:58 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-02 22:20 [PATCH] ASoC: RT5677-SPI: Disable 16Bit SPI Transfers Curtis Malainey
2019-05-02 23:25 ` Ben Zhang
2019-05-02 23:58 ` Curtis Malainey
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.