Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1407775 > unrolled thread
| Started by | Julian Calaby <julian.calaby@gmail.com> |
|---|---|
| First post | 2016-05-27 04:10 +0200 |
| Last post | 2016-05-30 13:30 +0200 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout Julian Calaby <julian.calaby@gmail.com> - 2016-05-27 04:10 +0200
Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout Michal Suchanek <hramrach@gmail.com> - 2016-05-27 07:10 +0200
Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout Julian Calaby <julian.calaby@gmail.com> - 2016-05-27 07:20 +0200
Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout Mark Brown <broonie@kernel.org> - 2016-05-30 13:30 +0200
| From | Julian Calaby <julian.calaby@gmail.com> |
|---|---|
| Date | 2016-05-27 04:10 +0200 |
| Subject | Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout |
| Message-ID | <rDfnP-5MI-1@gated-at.bofh.it> |
Hi Michal,
On Fri, May 27, 2016 at 5:25 AM, Michal Suchanek <hramrach@gmail.com> wrote:
> The trasfer timeout is fixed at 1000 ms. Reading a 4Mbyte flash over
> 1MHz SPI bus takes way longer than that. Calculate the timeout from the
> actual time the transfer is supposed to take and multiply by 2 for good
> measure.
>
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
>
> v2:
> - fix build error
> - use unsigned instead of int in max_t
> - use tfr->speed_hz instead of sspi->max_speed_hz
> ---
> drivers/spi/spi-sun4i.c | 11 ++++++++++-
> drivers/spi/spi-sun6i.c | 11 ++++++++++-
> 2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index 1ddd9e2..fe63bbd 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,
> {
> struct sun4i_spi *sspi = spi_master_get_devdata(master);
> unsigned int mclk_rate, div, timeout;
> + unsigned int start, end, tx_time;
> unsigned int tx_len = 0;
> int ret = 0;
> u32 reg;
> @@ -279,9 +280,17 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
> reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
> sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
>
> + tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),
You should probably use "unsigned int" instead of just "unsigned" here.
> + 100);
> + start = jiffies;
> timeout = wait_for_completion_timeout(&sspi->done,
> - msecs_to_jiffies(1000));
> + msecs_to_jiffies(tx_time));
> + end = jiffies;
> if (!timeout) {
> + dev_warn(&master->dev,
> + "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
> + dev_name(&spi->dev), tfr->len, tfr->speed_hz,
> + jiffies_to_msecs(end - start), tx_time);
Should the debug changes be in a separate patch?
> ret = -ETIMEDOUT;
> goto out;
> }
> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
> index 42e2c4b..8be5c5c 100644
> --- a/drivers/spi/spi-sun6i.c
> +++ b/drivers/spi/spi-sun6i.c
> @@ -162,6 +162,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
> unsigned int mclk_rate, div, timeout;
> unsigned int tx_len = 0;
> int ret = 0;
> + unsigned int start, end, tx_time;
> u32 reg;
>
> /* We don't support transfer larger than the FIFO */
> @@ -269,9 +270,17 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
> reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
> sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg | SUN6I_TFR_CTL_XCH);
>
> + tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),
Ditto, "unsigned int" instead of "unsigned"?
> + 100);
> + start = jiffies;
> timeout = wait_for_completion_timeout(&sspi->done,
> - msecs_to_jiffies(1000));
> + msecs_to_jiffies(tx_time));
> + end = jiffies;
> if (!timeout) {
> + dev_warn(&master->dev,
> + "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
> + dev_name(&spi->dev), tfr->len, tfr->speed_hz,
> + jiffies_to_msecs(end - start), tx_time);
Ditto, separate patch?
Also, should the changes for the drivers be in two separate patches also?
Thanks,
--
Julian Calaby
Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
[toc] | [next] | [standalone]
| From | Michal Suchanek <hramrach@gmail.com> |
|---|---|
| Date | 2016-05-27 07:10 +0200 |
| Message-ID | <rDic1-7DV-1@gated-at.bofh.it> |
| In reply to | #1407775 |
On 27 May 2016 at 04:05, Julian Calaby <julian.calaby@gmail.com> wrote:
> Hi Michal,
>
> On Fri, May 27, 2016 at 5:25 AM, Michal Suchanek <hramrach@gmail.com> wrote:
>> The trasfer timeout is fixed at 1000 ms. Reading a 4Mbyte flash over
>> 1MHz SPI bus takes way longer than that. Calculate the timeout from the
>> actual time the transfer is supposed to take and multiply by 2 for good
>> measure.
>>
>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>> ---
>>
>> v2:
>> - fix build error
>> - use unsigned instead of int in max_t
>> - use tfr->speed_hz instead of sspi->max_speed_hz
>> ---
>> drivers/spi/spi-sun4i.c | 11 ++++++++++-
>> drivers/spi/spi-sun6i.c | 11 ++++++++++-
>> 2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
>> index 1ddd9e2..fe63bbd 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,
>> {
>> struct sun4i_spi *sspi = spi_master_get_devdata(master);
>> unsigned int mclk_rate, div, timeout;
>> + unsigned int start, end, tx_time;
>> unsigned int tx_len = 0;
>> int ret = 0;
>> u32 reg;
>> @@ -279,9 +280,17 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>> reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
>> sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
>>
>> + tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),
>
> You should probably use "unsigned int" instead of just "unsigned" here.
>
>> + 100);
Or just 100U constant and avoid max_t altogether.
>> + start = jiffies;
>> timeout = wait_for_completion_timeout(&sspi->done,
>> - msecs_to_jiffies(1000));
>> + msecs_to_jiffies(tx_time));
>> + end = jiffies;
>> if (!timeout) {
>> + dev_warn(&master->dev,
>> + "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
>> + dev_name(&spi->dev), tfr->len, tfr->speed_hz,
>> + jiffies_to_msecs(end - start), tx_time);
>
> Should the debug changes be in a separate patch?
Is this so big of a change that it needs to be split?
>
>> ret = -ETIMEDOUT;
>> goto out;
>> }
>> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
>> index 42e2c4b..8be5c5c 100644
>> --- a/drivers/spi/spi-sun6i.c
>> +++ b/drivers/spi/spi-sun6i.c
>> @@ -162,6 +162,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>> unsigned int mclk_rate, div, timeout;
>> unsigned int tx_len = 0;
>> int ret = 0;
>> + unsigned int start, end, tx_time;
>> u32 reg;
>>
>> /* We don't support transfer larger than the FIFO */
>> @@ -269,9 +270,17 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>> reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
>> sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg | SUN6I_TFR_CTL_XCH);
>>
>> + tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),
>
> Ditto, "unsigned int" instead of "unsigned"?
>
>> + 100);
>> + start = jiffies;
>> timeout = wait_for_completion_timeout(&sspi->done,
>> - msecs_to_jiffies(1000));
>> + msecs_to_jiffies(tx_time));
>> + end = jiffies;
>> if (!timeout) {
>> + dev_warn(&master->dev,
>> + "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
>> + dev_name(&spi->dev), tfr->len, tfr->speed_hz,
>> + jiffies_to_msecs(end - start), tx_time);
>
> Ditto, separate patch?
>
> Also, should the changes for the drivers be in two separate patches also?
That's basically the same driver with different constants so I guess not.
Thanks
Michal
[toc] | [prev] | [next] | [standalone]
| From | Julian Calaby <julian.calaby@gmail.com> |
|---|---|
| Date | 2016-05-27 07:20 +0200 |
| Message-ID | <rDilH-7Hr-1@gated-at.bofh.it> |
| In reply to | #1407814 |
Hi Michal,
On Fri, May 27, 2016 at 3:05 PM, Michal Suchanek <hramrach@gmail.com> wrote:
> On 27 May 2016 at 04:05, Julian Calaby <julian.calaby@gmail.com> wrote:
>> Hi Michal,
>>
>> On Fri, May 27, 2016 at 5:25 AM, Michal Suchanek <hramrach@gmail.com> wrote:
>>> The trasfer timeout is fixed at 1000 ms. Reading a 4Mbyte flash over
>>> 1MHz SPI bus takes way longer than that. Calculate the timeout from the
>>> actual time the transfer is supposed to take and multiply by 2 for good
>>> measure.
>>>
>>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>>> ---
>>>
>>> v2:
>>> - fix build error
>>> - use unsigned instead of int in max_t
>>> - use tfr->speed_hz instead of sspi->max_speed_hz
>>> ---
>>> drivers/spi/spi-sun4i.c | 11 ++++++++++-
>>> drivers/spi/spi-sun6i.c | 11 ++++++++++-
>>> 2 files changed, 20 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
>>> index 1ddd9e2..fe63bbd 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,
>>> {
>>> struct sun4i_spi *sspi = spi_master_get_devdata(master);
>>> unsigned int mclk_rate, div, timeout;
>>> + unsigned int start, end, tx_time;
>>> unsigned int tx_len = 0;
>>> int ret = 0;
>>> u32 reg;
>>> @@ -279,9 +280,17 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>>> reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
>>> sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
>>>
>>> + tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),
>>
>> You should probably use "unsigned int" instead of just "unsigned" here.
>>
>>> + 100);
>
> Or just 100U constant and avoid max_t altogether.
>
>>> + start = jiffies;
>>> timeout = wait_for_completion_timeout(&sspi->done,
>>> - msecs_to_jiffies(1000));
>>> + msecs_to_jiffies(tx_time));
>>> + end = jiffies;
>>> if (!timeout) {
>>> + dev_warn(&master->dev,
>>> + "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
>>> + dev_name(&spi->dev), tfr->len, tfr->speed_hz,
>>> + jiffies_to_msecs(end - start), tx_time);
>>
>> Should the debug changes be in a separate patch?
>
> Is this so big of a change that it needs to be split?
Some people get touchy about changes not being mentioned in the commit
log. Technically this is two changes: fixing the timeout and adding
the timeout debugging, however they're related, so maybe just mention
that you've added this debugging in the commit log.
>
>>
>>> ret = -ETIMEDOUT;
>>> goto out;
>>> }
>>> diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
>>> index 42e2c4b..8be5c5c 100644
>>> --- a/drivers/spi/spi-sun6i.c
>>> +++ b/drivers/spi/spi-sun6i.c
>>> @@ -162,6 +162,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>>> unsigned int mclk_rate, div, timeout;
>>> unsigned int tx_len = 0;
>>> int ret = 0;
>>> + unsigned int start, end, tx_time;
>>> u32 reg;
>>>
>>> /* We don't support transfer larger than the FIFO */
>>> @@ -269,9 +270,17 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
>>> reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
>>> sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg | SUN6I_TFR_CTL_XCH);
>>>
>>> + tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),
>>
>> Ditto, "unsigned int" instead of "unsigned"?
>>
>>> + 100);
>>> + start = jiffies;
>>> timeout = wait_for_completion_timeout(&sspi->done,
>>> - msecs_to_jiffies(1000));
>>> + msecs_to_jiffies(tx_time));
>>> + end = jiffies;
>>> if (!timeout) {
>>> + dev_warn(&master->dev,
>>> + "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
>>> + dev_name(&spi->dev), tfr->len, tfr->speed_hz,
>>> + jiffies_to_msecs(end - start), tx_time);
>>
>> Ditto, separate patch?
>>
>> Also, should the changes for the drivers be in two separate patches also?
>
> That's basically the same driver with different constants so I guess not.
Fair enough, I withdraw my comment then.
Thanks,
--
Julian Calaby
Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
[toc] | [prev] | [next] | [standalone]
| From | Mark Brown <broonie@kernel.org> |
|---|---|
| Date | 2016-05-30 13:30 +0200 |
| Message-ID | <rEtyp-3wi-5@gated-at.bofh.it> |
| In reply to | #1407816 |
[Multipart message — attachments visible in raw view] — view raw
On Fri, May 27, 2016 at 03:10:11PM +1000, Julian Calaby wrote: > On Fri, May 27, 2016 at 3:05 PM, Michal Suchanek <hramrach@gmail.com> wrote: > >> Also, should the changes for the drivers be in two separate patches also? > > That's basically the same driver with different constants so I guess not. > Fair enough, I withdraw my comment then. If it's the same driver with different constants it should really actually be the same driver - I did ask this when the drivers were originally merged...
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web