| Message ID | 1426127476-18456-1-git-send-email-nobuhiro.iwamatsu.yj@renesas.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On Thu, Mar 12, 2015 at 11:31:16AM +0900, Nobuhiro Iwamatsu wrote: > This driver is the case of tx_buf was set or SPI_MASTER_MUST_TX is set to > master_flags, will be transmit mode. However, the current code, when > transmit mode and buffer is NULL, will be set to value of receive mode > size. Applied, thanks.
Hi Iwamatsu-san, On Thu, Mar 12, 2015 at 3:31 AM, Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> wrote: > This driver is the case of tx_buf was set or SPI_MASTER_MUST_TX is set to > master_flags, will be transmit mode. However, the current code, when > transmit mode and buffer is NULL, will be set to value of receive mode > size. If SPI_MASTER_MUST_TX is set, tx_buf will never be NULL. If an SPI slave driver submits a receive-only request, the SPI core will provide a dummy buffer (spi_master.dummy_tx). > This is when the SPI_MASTER_MUST_TX is set to master_flags, sets to > transmit and receive either small size of FIFO, so as not to set the > actual size larger than value of FIFO. > > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> > --- > drivers/spi/spi-sh-msiof.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c > index e57eec0..260f433 100644 > --- a/drivers/spi/spi-sh-msiof.c > +++ b/drivers/spi/spi-sh-msiof.c > @@ -606,12 +606,26 @@ static int sh_msiof_spi_txrx_once(struct sh_msiof_spi_priv *p, > { > int fifo_shift; > int ret; > + int rx_words = min_t(int, words, p->rx_fifo_size); > + int tx_words = min_t(int, words, p->tx_fifo_size); > > - /* limit maximum word transfer to rx/tx fifo size */ > - if (tx_buf) > - words = min_t(int, words, p->tx_fifo_size); > - if (rx_buf) > - words = min_t(int, words, p->rx_fifo_size); > + /* > + * limit maximum word transfer to rx/tx fifo size. > + * > + * If SPI_MASTER_MUST_TX was enabled in master_flags, words was > + * set to small value of FIFO. > + */ > + if (p->chipdata->master_flags & SPI_MASTER_MUST_TX) { > + if (rx_words > tx_words) > + words = tx_words; > + else > + words = rx_words; > + } else { > + if (tx_buf) > + words = tx_words; > + if (rx_buf) > + words = rx_words; > + } > > /* the fifo contents need shifting */ > fifo_shift = 32 - bits; Sorry, I fail to see what exactly this is fixing. If SPI_MASTER_MUST_TX is set, all hardware SPI transfers are either a) transmit-only. b) bidirectional (transmit buffer may be a dummy, provided by the SPI core). For case a, only the TX FIFO size matters. - The original code ignored the RX FIFO size (rx_buf == NULL), - After your change, it always uses the minimum of the TX and RX FIFO sizes (granted, the RX FIFO size is larger, so this doesn't make a difference on current hardware). For case b, both FIFO sizes matter, and the original code handled that fine (tx_buf != NULL, rx_buf != NULL). What am I missing? Are you using a backport with broken SPI_MASTER_MUST_TX handling in the SPI core? I've just verified that with today's tree (renesas-drivers-2015-03-12-v4.0-rc3), SPI_MASTER_MUST_TX works fine, and a dummy tx_buf is passed when needed. Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 12, 2015 at 01:35:10PM +0100, Geert Uytterhoeven wrote: > On Thu, Mar 12, 2015 at 3:31 AM, Nobuhiro Iwamatsu > > This driver is the case of tx_buf was set or SPI_MASTER_MUST_TX is set to > > master_flags, will be transmit mode. However, the current code, when > > transmit mode and buffer is NULL, will be set to value of receive mode > > size. > If SPI_MASTER_MUST_TX is set, tx_buf will never be NULL. > If an SPI slave driver submits a receive-only request, the SPI core will > provide a dummy buffer (spi_master.dummy_tx). Right, my reading of the changelog was that this was a red herring. > > + int rx_words = min_t(int, words, p->rx_fifo_size); > > + int tx_words = min_t(int, words, p->tx_fifo_size); > Sorry, I fail to see what exactly this is fixing. > If SPI_MASTER_MUST_TX is set, all hardware SPI transfers are either > a) transmit-only. > b) bidirectional (transmit buffer may be a dummy, provided by the SPI core). > For case a, only the TX FIFO size matters. > - The original code ignored the RX FIFO size (rx_buf == NULL), > - After your change, it always uses the minimum of the TX and RX FIFO sizes > (granted, the RX FIFO size is larger, so this doesn't make a difference on > current hardware). ... > What am I missing? > Are you using a backport with broken SPI_MASTER_MUST_TX handling in the SPI > core? My understanding was that it was just about making sure that the driver picked the minimum of the two FIFO sizes.
Hi, Thanks for your comment. 2015-03-12 21:35 GMT+09:00 Geert Uytterhoeven <geert@linux-m68k.org>: > Hi Iwamatsu-san, > > On Thu, Mar 12, 2015 at 3:31 AM, Nobuhiro Iwamatsu > <nobuhiro.iwamatsu.yj@renesas.com> wrote: >> This driver is the case of tx_buf was set or SPI_MASTER_MUST_TX is set to >> master_flags, will be transmit mode. However, the current code, when >> transmit mode and buffer is NULL, will be set to value of receive mode >> size. > > If SPI_MASTER_MUST_TX is set, tx_buf will never be NULL. > If an SPI slave driver submits a receive-only request, the SPI core will > provide a dummy buffer (spi_master.dummy_tx). I see. > >> This is when the SPI_MASTER_MUST_TX is set to master_flags, sets to >> transmit and receive either small size of FIFO, so as not to set the >> actual size larger than value of FIFO. >> >> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> >> --- >> drivers/spi/spi-sh-msiof.c | 24 +++++++++++++++++++----- >> 1 file changed, 19 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c >> index e57eec0..260f433 100644 >> --- a/drivers/spi/spi-sh-msiof.c >> +++ b/drivers/spi/spi-sh-msiof.c >> @@ -606,12 +606,26 @@ static int sh_msiof_spi_txrx_once(struct sh_msiof_spi_priv *p, >> { >> int fifo_shift; >> int ret; >> + int rx_words = min_t(int, words, p->rx_fifo_size); >> + int tx_words = min_t(int, words, p->tx_fifo_size); >> >> - /* limit maximum word transfer to rx/tx fifo size */ >> - if (tx_buf) >> - words = min_t(int, words, p->tx_fifo_size); >> - if (rx_buf) >> - words = min_t(int, words, p->rx_fifo_size); >> + /* >> + * limit maximum word transfer to rx/tx fifo size. >> + * >> + * If SPI_MASTER_MUST_TX was enabled in master_flags, words was >> + * set to small value of FIFO. >> + */ >> + if (p->chipdata->master_flags & SPI_MASTER_MUST_TX) { >> + if (rx_words > tx_words) >> + words = tx_words; >> + else >> + words = rx_words; >> + } else { >> + if (tx_buf) >> + words = tx_words; >> + if (rx_buf) >> + words = rx_words; >> + } >> >> /* the fifo contents need shifting */ >> fifo_shift = 32 - bits; > > Sorry, I fail to see what exactly this is fixing. > > If SPI_MASTER_MUST_TX is set, all hardware SPI transfers are either > a) transmit-only. > b) bidirectional (transmit buffer may be a dummy, provided by the SPI core). > > For case a, only the TX FIFO size matters. > - The original code ignored the RX FIFO size (rx_buf == NULL), > - After your change, it always uses the minimum of the TX and RX FIFO sizes > (granted, the RX FIFO size is larger, so this doesn't make a difference on > current hardware). Yes. > > For case b, both FIFO sizes matter, and the original code handled that fine > (tx_buf != NULL, rx_buf != NULL). > > What am I missing? > Are you using a backport with broken SPI_MASTER_MUST_TX handling in the SPI > core? When tx_buf != NULL and rx_buf != NULL, current code uses FIFO size of rx_buffer. Since TX FIFO size is smaller than RX FIFO size, corrent code set the wrong value to SITMDR2 register. Therefore, this patch selects a smaller FIFO size, adds the function to set. Does this has become a description? > > I've just verified that with today's tree (renesas-drivers-2015-03-12-v4.0-rc3), > SPI_MASTER_MUST_TX works fine, and a dummy tx_buf is passed when needed. I think correctly work on hardware. However, driver has been set to the correct register value? > > Thanks! > > Gr{oetje,eeting}s, > > Geert Best regards, Nobuhiro
Hi Iwamatsu-san, On Mon, Mar 16, 2015 at 2:18 AM, Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> wrote: > 2015-03-12 21:35 GMT+09:00 Geert Uytterhoeven <geert@linux-m68k.org>: >> On Thu, Mar 12, 2015 at 3:31 AM, Nobuhiro Iwamatsu >> <nobuhiro.iwamatsu.yj@renesas.com> wrote: >>> - /* limit maximum word transfer to rx/tx fifo size */ >>> - if (tx_buf) >>> - words = min_t(int, words, p->tx_fifo_size); >>> - if (rx_buf) >>> - words = min_t(int, words, p->rx_fifo_size); >> Sorry, I fail to see what exactly this is fixing. >> >> If SPI_MASTER_MUST_TX is set, all hardware SPI transfers are either >> a) transmit-only. >> b) bidirectional (transmit buffer may be a dummy, provided by the SPI core). >> >> For case a, only the TX FIFO size matters. >> - The original code ignored the RX FIFO size (rx_buf == NULL), >> - After your change, it always uses the minimum of the TX and RX FIFO sizes >> (granted, the RX FIFO size is larger, so this doesn't make a difference on >> current hardware). > > Yes. > >> >> For case b, both FIFO sizes matter, and the original code handled that fine >> (tx_buf != NULL, rx_buf != NULL). >> >> What am I missing? >> Are you using a backport with broken SPI_MASTER_MUST_TX handling in the SPI >> core? > > When tx_buf != NULL and rx_buf != NULL, current code uses FIFO size of > rx_buffer. > Since TX FIFO size is smaller than RX FIFO size, corrent code set the > wrong value to SITMDR2 register. if tx_buf != NULL and rx_buf != NULL, current code does: words = min_t(int, words, p->tx_fifo_size); words = min_t(int, words, p->rx_fifo_size); Hence words will be the minimum of the original value of words, tx_fifo_size, and rx_fifo_size. What's wrong about that? > Therefore, this patch selects a smaller FIFO size, adds the function to set. > > Does this has become a description? > >> >> I've just verified that with today's tree (renesas-drivers-2015-03-12-v4.0-rc3), >> SPI_MASTER_MUST_TX works fine, and a dummy tx_buf is passed when needed. > > I think correctly work on hardware. However, driver has been set to > the correct register value? I printed the value of words, which is passed to sh_msiof_spi_set_mode_regs(). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, (2015/03/16 16:50), Geert Uytterhoeven wrote: > Hi Iwamatsu-san, > > On Mon, Mar 16, 2015 at 2:18 AM, Nobuhiro Iwamatsu > <nobuhiro.iwamatsu.yj@renesas.com> wrote: >> 2015-03-12 21:35 GMT+09:00 Geert Uytterhoeven<geert@linux-m68k.org>: >>> On Thu, Mar 12, 2015 at 3:31 AM, Nobuhiro Iwamatsu >>> <nobuhiro.iwamatsu.yj@renesas.com> wrote: > >>>> - /* limit maximum word transfer to rx/tx fifo size */ >>>> - if (tx_buf) >>>> - words = min_t(int, words, p->tx_fifo_size); >>>> - if (rx_buf) >>>> - words = min_t(int, words, p->rx_fifo_size); > >>> Sorry, I fail to see what exactly this is fixing. >>> >>> If SPI_MASTER_MUST_TX is set, all hardware SPI transfers are either >>> a) transmit-only. >>> b) bidirectional (transmit buffer may be a dummy, provided by the SPI core). >>> >>> For case a, only the TX FIFO size matters. >>> - The original code ignored the RX FIFO size (rx_buf == NULL), >>> - After your change, it always uses the minimum of the TX and RX FIFO sizes >>> (granted, the RX FIFO size is larger, so this doesn't make a difference on >>> current hardware). >> >> Yes. >> >>> >>> For case b, both FIFO sizes matter, and the original code handled that fine >>> (tx_buf != NULL, rx_buf != NULL). >>> >>> What am I missing? >>> Are you using a backport with broken SPI_MASTER_MUST_TX handling in the SPI >>> core? >> >> When tx_buf != NULL and rx_buf != NULL, current code uses FIFO size of >> rx_buffer. >> Since TX FIFO size is smaller than RX FIFO size, corrent code set the >> wrong value to SITMDR2 register. > > if tx_buf != NULL and rx_buf != NULL, current code does: > > words = min_t(int, words, p->tx_fifo_size); > words = min_t(int, words, p->rx_fifo_size); > > Hence words will be the minimum of the original value of words, tx_fifo_size, > and rx_fifo_size. What's wrong about that? > I see. I understood about this. Sorry, my bad. Mark, if you do not apply this patch to your repository, could you ignore this? >> Therefore, this patch selects a smaller FIFO size, adds the function to set. >> >> Does this has become a description? >> >>> >>> I've just verified that with today's tree (renesas-drivers-2015-03-12-v4.0-rc3), >>> SPI_MASTER_MUST_TX works fine, and a dummy tx_buf is passed when needed. >> >> I think correctly work on hardware. However, driver has been set to >> the correct register value? > > I printed the value of words, which is passed to sh_msiof_spi_set_mode_regs(). > > Gr{oetje,eeting}s, > > Geert > Best regards, Nobuhiro -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 18, 2015 at 12:50:31PM +0900, Nobuhiro Iwamatsu wrote: > (2015/03/16 16:50), Geert Uytterhoeven wrote: > I see. I understood about this. > Sorry, my bad. > Mark, if you do not apply this patch to your repository, could you ignore this? OK, I'll drop it.
diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c index e57eec0..260f433 100644 --- a/drivers/spi/spi-sh-msiof.c +++ b/drivers/spi/spi-sh-msiof.c @@ -606,12 +606,26 @@ static int sh_msiof_spi_txrx_once(struct sh_msiof_spi_priv *p, { int fifo_shift; int ret; + int rx_words = min_t(int, words, p->rx_fifo_size); + int tx_words = min_t(int, words, p->tx_fifo_size); - /* limit maximum word transfer to rx/tx fifo size */ - if (tx_buf) - words = min_t(int, words, p->tx_fifo_size); - if (rx_buf) - words = min_t(int, words, p->rx_fifo_size); + /* + * limit maximum word transfer to rx/tx fifo size. + * + * If SPI_MASTER_MUST_TX was enabled in master_flags, words was + * set to small value of FIFO. + */ + if (p->chipdata->master_flags & SPI_MASTER_MUST_TX) { + if (rx_words > tx_words) + words = tx_words; + else + words = rx_words; + } else { + if (tx_buf) + words = tx_words; + if (rx_buf) + words = rx_words; + } /* the fifo contents need shifting */ fifo_shift = 32 - bits;
This driver is the case of tx_buf was set or SPI_MASTER_MUST_TX is set to master_flags, will be transmit mode. However, the current code, when transmit mode and buffer is NULL, will be set to value of receive mode size. This is when the SPI_MASTER_MUST_TX is set to master_flags, sets to transmit and receive either small size of FIFO, so as not to set the actual size larger than value of FIFO. Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com> --- drivers/spi/spi-sh-msiof.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)