| Message ID | 1446936065-15868-1-git-send-email-mweseloh42@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Nov 07, 2015 at 11:41:05PM +0100, Marcus Weseloh wrote: > + /* Transfer speed setup with fallback to board max_speed_hz */ > + if (tfr->speed_hz > 0 && tfr->speed_hz <= spi->max_speed_hz) > + speed = tfr->speed_hz; > + else > + speed = spi->max_speed_hz; This is not needed, the core will always ensure that the transfer speed is set per on each transfer.
Hi Mark, 2015-11-08 10:35 GMT+01:00 Mark Brown <broonie@kernel.org>: > On Sat, Nov 07, 2015 at 11:41:05PM +0100, Marcus Weseloh wrote: > >> + /* Transfer speed setup with fallback to board max_speed_hz */ >> + if (tfr->speed_hz > 0 && tfr->speed_hz <= spi->max_speed_hz) >> + speed = tfr->speed_hz; >> + else >> + speed = spi->max_speed_hz; > > This is not needed, the core will always ensure that the transfer speed > is set per on each transfer. Ah, so we could always use tfr->speed_hz and completely ignore the spi->max_speed_hz for the clock calculations, correct? I did notice however, that the SPI system doesn't validate the speed_hz value. So if I leave out the above quoted sanity checking, then tfr->speed_hz could also contain values like -400, which trips up the spi system in such a way that I could only repair with a reboot. I was testing this with spidev... so maybe spidev should be patched to check the speed value for validity? Cheers, Marcus -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2015-11-08 11:13 GMT+01:00 Marcus Weseloh <mweseloh42@gmail.com>: > I did notice however, that the SPI system doesn't validate the > speed_hz value. So if I leave out the above quoted sanity checking, > then tfr->speed_hz could also contain values like -400, which trips up > the spi system in such a way that I could only repair with a reboot. I > was testing this with spidev... so maybe spidev should be patched to > check the speed value for validity? I just had a look through the spi core and noticed that it does indeed validate the maximum and minium speed if set correctly on the spi master. So I guess it's just a matter of a missing minimum transfer speed in the DT. Sorry, should have done that earlier. Ok, I will prepare a new patch version that simply uses tfr->speed_hz. And I will also change sun6i-spi in the same patch, as sun4i and sun6i are using the same transfer logic. Cheers, Marcus -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Nov 08, 2015 at 11:13:33AM +0100, Marcus Weseloh wrote: > 2015-11-08 10:35 GMT+01:00 Mark Brown <broonie@kernel.org>: > > This is not needed, the core will always ensure that the transfer speed > > is set per on each transfer. > Ah, so we could always use tfr->speed_hz and completely ignore the > spi->max_speed_hz for the clock calculations, correct? Yes. > I did notice however, that the SPI system doesn't validate the > speed_hz value. So if I leave out the above quoted sanity checking, > then tfr->speed_hz could also contain values like -400, which trips up > the spi system in such a way that I could only repair with a reboot. I > was testing this with spidev... so maybe spidev should be patched to > check the speed value for validity? No, why would it make sense to have this validation in spidev - it's just one SPI device. It's better to add the validation to the SPI core.
Hi Mark, Maxime, 2015-11-07 23:41 GMT+01:00 Marcus Weseloh <mweseloh42@gmail.com>: > Allow transfers to set the transmission speed, only fall back to > board max_speed_hz if requested speed is not set or invalid. > > Signed-off-by: Marcus Weseloh <mweseloh42@gmail.com> > --- > drivers/spi/spi-sun4i.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) Just a quick follow-up: is there anything else I need to do to push this patch along? Or do I just need to be more patient? Best regards, Marcus -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 17, 2015 at 04:50:54PM +0100, Marcus Weseloh wrote: > 2015-11-07 23:41 GMT+01:00 Marcus Weseloh <mweseloh42@gmail.com>: > > Allow transfers to set the transmission speed, only fall back to > > board max_speed_hz if requested speed is not set or invalid. > Just a quick follow-up: is there anything else I need to do to push > this patch along? Or do I just need to be more patient? I don't have this patch, presumably there were review comments that I was expecting to see some action on.
Hi Mark, 2015-11-17 17:57 GMT+01:00 Mark Brown <broonie@kernel.org>: >> Just a quick follow-up: is there anything else I need to do to push >> this patch along? Or do I just need to be more patient? > > I don't have this patch, presumably there were review comments that I > was expecting to see some action on. Sorry, the mail I was quoting was actually the first version of the patch, which you did comment on. I had sent a v2 patch addressing your comments, sent on the 8th of November, 12:08 CET: http://marc.info/?l=linux-spi&m=144698062222607&w=2 Looks like I make a mistake when git send-email asked me for the message id that this second version refers to. The linux-spi mailing list didn't pick up on the reference and created a new thread for it... Best regards, Marcus -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 17, 2015 at 07:14:14PM +0100, Marcus Weseloh wrote: > Sorry, the mail I was quoting was actually the first version of the > patch, which you did comment on. I had sent a v2 patch addressing > your comments, sent on the 8th of November, 12:08 CET: > http://marc.info/?l=linux-spi&m=144698062222607&w=2 I have a form letter I usually send for this: Please don't send content free pings and please allow a reasonable time for review. People get busy, go on holiday, attend conferences and so on so unless there is some reason for urgency (like critical bug fixes) please allow at least a couple of weeks for review. Sending content free pings just adds to the mail volume (if they are seen at all) and if something has gone wrong you'll have to resend the patches anyway.
2015-11-17 19:34 GMT+01:00 Mark Brown <broonie@kernel.org>: > Please don't send content free pings and please allow a reasonable time > for review. People get busy, go on holiday, attend conferences and so > on so unless there is some reason for urgency (like critical bug fixes) > please allow at least a couple of weeks for review. Sending content > free pings just adds to the mail volume (if they are seen at all) and if > something has gone wrong you'll have to resend the patches anyway. OK, message received and understood. Please accept my apologies! This is my first code-changing contribution to mainline and I really was unsure if the second version of the patch came through alright. I guess I was spoiled by your very quick responses and review of the first version, so I got worried when I didn't hear anything for a few days. I'll be more patient in the future. Best regards, Marcus -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 17, 2015 at 11:06:54PM +0100, Marcus Weseloh wrote: > OK, message received and understood. Please accept my apologies! This > is my first code-changing contribution to mainline and I really was > unsure if the second version of the patch came through alright. I > guess I was spoiled by your very quick responses and review of the > first version, so I got worried when I didn't hear anything for a few > days. I'll be more patient in the future. I'd probably have been quicker but the merge window was open until Sunday so I've not been applying anything.
diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c index fbb0a4d..0630691 100644 --- a/drivers/spi/spi-sun4i.c +++ b/drivers/spi/spi-sun4i.c @@ -173,6 +173,7 @@ static int sun4i_spi_transfer_one(struct spi_master *master, unsigned int tx_len = 0; int ret = 0; u32 reg; + u32 speed; /* We don't support transfer larger than the FIFO */ if (tfr->len > SUN4I_FIFO_DEPTH) @@ -227,10 +228,16 @@ static int sun4i_spi_transfer_one(struct spi_master *master, sun4i_spi_write(sspi, SUN4I_CTL_REG, reg); + /* Transfer speed setup with fallback to board max_speed_hz */ + if (tfr->speed_hz > 0 && tfr->speed_hz <= spi->max_speed_hz) + speed = tfr->speed_hz; + else + speed = spi->max_speed_hz; + /* Ensure that we have a parent clock fast enough */ mclk_rate = clk_get_rate(sspi->mclk); - if (mclk_rate < (2 * spi->max_speed_hz)) { - clk_set_rate(sspi->mclk, 2 * spi->max_speed_hz); + if (mclk_rate < (2 * speed)) { + clk_set_rate(sspi->mclk, 2 * speed); mclk_rate = clk_get_rate(sspi->mclk); } @@ -248,14 +255,14 @@ static int sun4i_spi_transfer_one(struct spi_master *master, * First try CDR2, and if we can't reach the expected * frequency, fall back to CDR1. */ - div = mclk_rate / (2 * spi->max_speed_hz); + div = mclk_rate / (2 * speed); if (div <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) { if (div > 0) div--; reg = SUN4I_CLK_CTL_CDR2(div) | SUN4I_CLK_CTL_DRS; } else { - div = ilog2(mclk_rate) - ilog2(spi->max_speed_hz); + div = ilog2(mclk_rate) - ilog2(speed); reg = SUN4I_CLK_CTL_CDR1(div); }
Allow transfers to set the transmission speed, only fall back to board max_speed_hz if requested speed is not set or invalid. Signed-off-by: Marcus Weseloh <mweseloh42@gmail.com> --- drivers/spi/spi-sun4i.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)