* [PATCH 1/5] spi: sunxi: fix transfer timeout
2016-05-24 22:56 ` Michal Suchanek
@ 2016-05-26 19:25 ` Michal Suchanek
-1 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-26 19:25 UTC (permalink / raw)
To: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Mark Brown, Maxime Ripard,
Chen-Yu Tsai, linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
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),
+ 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);
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),
+ 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);
ret = -ETIMEDOUT;
goto out;
}
--
2.8.1
^ permalink raw reply related [flat|nested] 95+ messages in thread* [PATCH 1/5] spi: sunxi: fix transfer timeout
@ 2016-05-26 19:25 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-26 19:25 UTC (permalink / raw)
To: linux-arm-kernel
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),
+ 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);
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),
+ 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);
ret = -ETIMEDOUT;
goto out;
}
--
2.8.1
^ permalink raw reply related [flat|nested] 95+ messages in thread[parent not found: <cb90b922caa6ac07c1425d726ea19709ee5284f4.1464130597.git.hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout
2016-05-26 19:25 ` Michal Suchanek
(?)
@ 2016-05-27 2:05 ` Julian Calaby
-1 siblings, 0 replies; 95+ messages in thread
From: Julian Calaby @ 2016-05-27 2:05 UTC (permalink / raw)
To: Michal Suchanek
Cc: linux-sunxi, Mark Brown, Maxime Ripard, Chen-Yu Tsai,
linux-spi-u79uwXL29TY76Z2rM5mHXA, Mailing List, Arm,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Hi Michal,
On Fri, May 27, 2016 at 5:25 AM, Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>
> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 95+ messages in thread* Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout
@ 2016-05-27 2:05 ` Julian Calaby
0 siblings, 0 replies; 95+ messages in thread
From: Julian Calaby @ 2016-05-27 2:05 UTC (permalink / raw)
To: Michal Suchanek
Cc: linux-sunxi, Mark Brown, Maxime Ripard, Chen-Yu Tsai, linux-spi,
Mailing List, Arm, linux-kernel@vger.kernel.org
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/
^ permalink raw reply [flat|nested] 95+ messages in thread* [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout
@ 2016-05-27 2:05 ` Julian Calaby
0 siblings, 0 replies; 95+ messages in thread
From: Julian Calaby @ 2016-05-27 2:05 UTC (permalink / raw)
To: linux-arm-kernel
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 at gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
^ permalink raw reply [flat|nested] 95+ messages in thread[parent not found: <CAGRGNgWDrVHG26umTSHHAxFC3KXjwmpDejBAfKqvuik0_2-Jcw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 1/5] spi: sunxi: fix transfer timeout
2016-05-27 2:05 ` Julian Calaby
(?)
@ 2016-05-27 5:05 ` Michal Suchanek
-1 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-27 5:05 UTC (permalink / raw)
To: Julian Calaby
Cc: linux-sunxi, Mark Brown, Maxime Ripard, Chen-Yu Tsai, linux-spi,
Mailing List, Arm,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On 27 May 2016 at 04:05, Julian Calaby <julian.calaby-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Michal,
>
> On Fri, May 27, 2016 at 5:25 AM, Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>
>> 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
^ permalink raw reply [flat|nested] 95+ messages in thread* Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout
@ 2016-05-27 5:05 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-27 5:05 UTC (permalink / raw)
To: Julian Calaby
Cc: linux-sunxi, Mark Brown, Maxime Ripard, Chen-Yu Tsai, linux-spi,
Mailing List, Arm, linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 95+ messages in thread* [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout
@ 2016-05-27 5:05 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-27 5:05 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 95+ messages in thread[parent not found: <CAOMqctS0AD6rHGt3wjxxyidee=_5xqby95t5TGuozAA12DdPjQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 1/5] spi: sunxi: fix transfer timeout
2016-05-27 5:05 ` Michal Suchanek
(?)
@ 2016-05-27 5:10 ` Julian Calaby
-1 siblings, 0 replies; 95+ messages in thread
From: Julian Calaby @ 2016-05-27 5:10 UTC (permalink / raw)
To: Michal Suchanek
Cc: linux-sunxi, Mark Brown, Maxime Ripard, Chen-Yu Tsai, linux-spi,
Mailing List, Arm,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Hi Michal,
On Fri, May 27, 2016 at 3:05 PM, Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 27 May 2016 at 04:05, Julian Calaby <julian.calaby-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Hi Michal,
>>
>> On Fri, May 27, 2016 at 5:25 AM, Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> ---
>>>
>>> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Profile: http://www.google.com/profiles/julian.calaby/
^ permalink raw reply [flat|nested] 95+ messages in thread* Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout
@ 2016-05-27 5:10 ` Julian Calaby
0 siblings, 0 replies; 95+ messages in thread
From: Julian Calaby @ 2016-05-27 5:10 UTC (permalink / raw)
To: Michal Suchanek
Cc: linux-sunxi, Mark Brown, Maxime Ripard, Chen-Yu Tsai, linux-spi,
Mailing List, Arm, linux-kernel@vger.kernel.org
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/
^ permalink raw reply [flat|nested] 95+ messages in thread* [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout
@ 2016-05-27 5:10 ` Julian Calaby
0 siblings, 0 replies; 95+ messages in thread
From: Julian Calaby @ 2016-05-27 5:10 UTC (permalink / raw)
To: linux-arm-kernel
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 at gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
^ permalink raw reply [flat|nested] 95+ messages in thread[parent not found: <CAGRGNgW=UnmwwiREV+nVcws6sk18-4fKz3qf-WKf4LY9-VqBKg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout
2016-05-27 5:10 ` Julian Calaby
(?)
@ 2016-05-30 11:23 ` Mark Brown
-1 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-05-30 11:23 UTC (permalink / raw)
To: Julian Calaby
Cc: Michal Suchanek, linux-sunxi, Maxime Ripard, Chen-Yu Tsai,
linux-spi, Mailing List, Arm,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
[-- Attachment #1: Type: text/plain, Size: 530 bytes --]
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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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...
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout
@ 2016-05-30 11:23 ` Mark Brown
0 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-05-30 11:23 UTC (permalink / raw)
To: Julian Calaby
Cc: Michal Suchanek, linux-sunxi, Maxime Ripard, Chen-Yu Tsai,
linux-spi, Mailing List, Arm, linux-kernel@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 500 bytes --]
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...
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 95+ messages in thread
* [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout
@ 2016-05-30 11:23 ` Mark Brown
0 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-05-30 11:23 UTC (permalink / raw)
To: linux-arm-kernel
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...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160530/fafb72c7/attachment.sig>
^ permalink raw reply [flat|nested] 95+ messages in thread
[parent not found: <20160530112350.GI29837-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>]
* Re: [PATCH 1/5] spi: sunxi: fix transfer timeout
2016-05-30 11:23 ` Mark Brown
(?)
@ 2016-05-31 11:52 ` Michal Suchanek
-1 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-31 11:52 UTC (permalink / raw)
To: Mark Brown
Cc: Julian Calaby, linux-sunxi, Maxime Ripard, Chen-Yu Tsai,
linux-spi, Mailing List, Arm,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Hello
On 30 May 2016 at 13:23, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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...
There are some slight differences and the constants really are
different for each driver.
You would need a register remap adaptation layer and a few qirks for
each revision of the driver.
It's certainly possible to merge them but I am not sure if that gives
easier to maintain code.
On the other hand, comparing the drivers there is different comment
anout clock calculation arithmetic and the code is the same :S
Thanks
Michal
--- spi-sun4i.c 2016-05-31 13:19:27.076510421 +0200
+++ spi-sun6i.c 2016-05-31 13:26:58.123580382 +0200
@@ -19,65 +19,72 @@
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
+#include <linux/reset.h>
#include <linux/spi/spi.h>
-#define SUNXI_FIFO_DEPTH 64
+#define SUNXI_FIFO_DEPTH 128
-#define SUNXI_RXDATA_REG 0x00
+#define SUNXI_TXDATA_REG 0x200
-#define SUNXI_TXDATA_REG 0x04
+#define SUNXI_RXDATA_REG 0x300
#define SUNXI_TFR_CTL_REG 0x08
-#define SUNXI_TFR_CTL_ENABLE BIT(0)
-#define SUNXI_TFR_CTL_MASTER BIT(1)
-#define SUNXI_TFR_CTL_CPHA BIT(2)
-#define SUNXI_TFR_CTL_CPOL BIT(3)
-#define SUNXI_TFR_CTL_CS_ACTIVE_LOW BIT(4)
-#define SUNXI_TFR_CTL_LMTF BIT(6)
-#define SUNXI_TFR_CTL_TF_RST BIT(8)
-#define SUNXI_TFR_CTL_RF_RST BIT(9)
-#define SUNXI_TFR_CTL_XCH BIT(10)
-#define SUNXI_TFR_CTL_CS_MASK 0x3000
-#define SUNXI_TFR_CTL_CS(cs) (((cs) << 12) & SUNXI_TFR_CTL_CS_MASK)
-#define SUNXI_TFR_CTL_DHB BIT(15)
-#define SUNXI_TFR_CTL_CS_MANUAL BIT(16)
-#define SUNXI_TFR_CTL_CS_LEVEL BIT(17)
-#define SUNXI_TFR_CTL_TP BIT(18)
+#define SUNXI_TFR_CTL_CPHA BIT(0)
+#define SUNXI_TFR_CTL_CPOL BIT(1)
+#define SUNXI_TFR_CTL_SPOL BIT(2)
+#define SUNXI_TFR_CTL_CS_MASK 0x30
+#define SUNXI_TFR_CTL_CS(cs) (((cs) << 4) & SUNXI_TFR_CTL_CS_MASK)
+#define SUNXI_TFR_CTL_CS_MANUAL BIT(6)
+#define SUNXI_TFR_CTL_CS_LEVEL BIT(7)
+#define SUNXI_TFR_CTL_DHB BIT(8)
+#define SUNXI_TFR_CTL_FBS BIT(12)
+#define SUNXI_TFR_CTL_XCH BIT(31)
-#define SUNXI_INT_CTL_REG 0x0c
-#define SUNXI_INT_CTL_TC BIT(16)
+#define SUNXI_INT_CTL_REG 0x10
+#define SUNXI_INT_CTL_RF_OVF BIT(8)
+#define SUNXI_INT_CTL_TC BIT(12)
-#define SUNXI_INT_STA_REG 0x10
+#define SUNXI_INT_STA_REG 0x14
-#define SUNXI_DMA_CTL_REG 0x14
+#define SUNXI_FIFO_CTL_REG 0x18
+#define SUNXI_FIFO_CTL_RF_RST BIT(15)
+#define SUNXI_FIFO_CTL_TF_RST BIT(31)
-#define SUNXI_CLK_CTL_REG 0x1c
+#define SUNXI_CLK_CTL_REG 0x24
#define SUNXI_CLK_CTL_CDR2_MASK 0xff
#define SUNXI_CLK_CTL_CDR2(div) (((div) &
SUNXI_CLK_CTL_CDR2_MASK) << 0)
#define SUNXI_CLK_CTL_CDR1_MASK 0xf
#define SUNXI_CLK_CTL_CDR1(div) (((div) &
SUNXI_CLK_CTL_CDR1_MASK) << 8)
#define SUNXI_CLK_CTL_DRS BIT(12)
-#define SUNXI_BURST_CNT_REG 0x20
+#define SUNXI_BURST_CNT_REG 0x30
#define SUNXI_BURST_CNT(cnt) ((cnt) & 0xffffff)
-#define SUNXI_XMIT_CNT_REG 0x24
+#define SUNXI_XMIT_CNT_REG 0x34
#define SUNXI_XMIT_CNT(cnt) ((cnt) & 0xffffff)
-#define SUNXI_FIFO_STA_REG 0x28
+#define SUNXI_FIFO_STA_REG 0x1c
#define SUNXI_FIFO_STA_RF_CNT_MASK 0x7f
#define SUNXI_FIFO_STA_RF_CNT_BITS 0
#define SUNXI_FIFO_STA_TF_CNT_MASK 0x7f
#define SUNXI_FIFO_STA_TF_CNT_BITS 16
-#define SUNXI_WAIT_REG 0x18
+#define SUNXI_BURST_CTL_CNT_REG 0x38
+#define SUNXI_BURST_CTL_CNT_STC(cnt) ((cnt) & 0xffffff)
+
+#define SUNXI_GBL_CTL_REG 0x04
+#define SUNXI_GBL_CTL_BUS_ENABLE BIT(0)
+#define SUNXI_GBL_CTL_MASTER BIT(1)
+#define SUNXI_GBL_CTL_TP BIT(7)
+#define SUNXI_GBL_CTL_RST BIT(31)
struct sunxi_spi {
struct spi_master *master;
void __iomem *base_addr;
struct clk *hclk;
struct clk *mclk;
+ struct reset_control *rstc;
struct completion done;
@@ -136,37 +143,18 @@
u32 reg;
reg = sunxi_spi_read(sspi, SUNXI_TFR_CTL_REG);
-
reg &= ~SUNXI_TFR_CTL_CS_MASK;
reg |= SUNXI_TFR_CTL_CS(spi->chip_select);
- /* We want to control the chip select manually */
- reg |= SUNXI_TFR_CTL_CS_MANUAL;
-
if (enable)
reg |= SUNXI_TFR_CTL_CS_LEVEL;
else
reg &= ~SUNXI_TFR_CTL_CS_LEVEL;
- /*
- * Even though this looks irrelevant since we are supposed to
- * be controlling the chip select manually, this bit also
- * controls the levels of the chip select for inactive
- * devices.
- *
- * If we don't set it, the chip select level will go low by
- * default when the device is idle, which is not really
- * expected in the common case where the chip select is active
- * low.
- */
- if (spi->mode & SPI_CS_HIGH)
- reg &= ~SUNXI_TFR_CTL_CS_ACTIVE_LOW;
- else
- reg |= SUNXI_TFR_CTL_CS_ACTIVE_LOW;
-
sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG, reg);
}
+
static int sunxi_spi_transfer_one(struct spi_master *master,
struct spi_device *spi,
struct spi_transfer *tfr)
@@ -189,17 +177,16 @@
/* Clear pending interrupts */
sunxi_spi_write(sspi, SUNXI_INT_STA_REG, ~0);
-
- reg = sunxi_spi_read(sspi, SUNXI_TFR_CTL_REG);
-
/* Reset FIFOs */
- sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
- reg | SUNXI_TFR_CTL_RF_RST | SUNXI_TFR_CTL_TF_RST);
+ sunxi_spi_write(sspi, SUNXI_FIFO_CTL_REG,
+ SUNXI_FIFO_CTL_RF_RST | SUNXI_FIFO_CTL_TF_RST);
/*
* Setup the transfer control register: Chip Select,
* polarities, etc.
*/
+ reg = sunxi_spi_read(sspi, SUNXI_TFR_CTL_REG);
+
if (spi->mode & SPI_CPOL)
reg |= SUNXI_TFR_CTL_CPOL;
else
@@ -211,10 +198,9 @@
reg &= ~SUNXI_TFR_CTL_CPHA;
if (spi->mode & SPI_LSB_FIRST)
- reg |= SUNXI_TFR_CTL_LMTF;
+ reg |= SUNXI_TFR_CTL_FBS;
else
- reg &= ~SUNXI_TFR_CTL_LMTF;
-
+ reg &= ~SUNXI_TFR_CTL_FBS;
/*
* If it's a TX only transfer, we don't want to fill the RX
@@ -225,6 +211,9 @@
else
reg |= SUNXI_TFR_CTL_DHB;
+ /* We want to control the chip select manually */
+ reg |= SUNXI_TFR_CTL_CS_MANUAL;
+
sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG, reg);
/* Ensure that we have a parent clock fast enough */
@@ -239,7 +228,7 @@
*
* We have two choices there. Either we can use the clock
* divide rate 1, which is calculated thanks to this formula:
- * SPI_CLK = MOD_CLK / (2 ^ (cdr + 1))
+ * SPI_CLK = MOD_CLK / (2 ^ cdr)
* Or we can use CDR2, which is calculated with the formula:
* SPI_CLK = MOD_CLK / (2 * (cdr + 1))
* Wether we use the former or the latter is set through the
@@ -268,6 +257,8 @@
/* Setup the counters */
sunxi_spi_write(sspi, SUNXI_BURST_CNT_REG, SUNXI_BURST_CNT(tfr->len));
sunxi_spi_write(sspi, SUNXI_XMIT_CNT_REG, SUNXI_XMIT_CNT(tx_len));
+ sunxi_spi_write(sspi, SUNXI_BURST_CTL_CNT_REG,
+ SUNXI_BURST_CTL_CNT_STC(tx_len));
/* Fill the TX FIFO */
sunxi_spi_fill_fifo(sspi, SUNXI_FIFO_DEPTH);
@@ -327,11 +318,19 @@
goto err;
}
- sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
- SUNXI_TFR_CTL_ENABLE | SUNXI_TFR_CTL_MASTER | SUNXI_TFR_CTL_TP);
+ ret = reset_control_deassert(sspi->rstc);
+ if (ret) {
+ dev_err(dev, "Couldn't deassert the device from reset\n");
+ goto err2;
+ }
+
+ sunxi_spi_write(sspi, SUNXI_GBL_CTL_REG,
+ SUNXI_GBL_CTL_BUS_ENABLE | SUNXI_GBL_CTL_MASTER |
SUNXI_GBL_CTL_TP);
return 0;
+err2:
+ clk_disable_unprepare(sspi->mclk);
err:
clk_disable_unprepare(sspi->hclk);
out:
@@ -343,6 +342,7 @@
struct spi_master *master = dev_get_drvdata(dev);
struct sunxi_spi *sspi = spi_master_get_devdata(master);
+ reset_control_assert(sspi->rstc);
clk_disable_unprepare(sspi->mclk);
clk_disable_unprepare(sspi->hclk);
@@ -411,6 +411,13 @@
init_completion(&sspi->done);
+ sspi->rstc = devm_reset_control_get(&pdev->dev, NULL);
+ if (IS_ERR(sspi->rstc)) {
+ dev_err(&pdev->dev, "Couldn't get reset controller\n");
+ ret = PTR_ERR(sspi->rstc);
+ goto err_free_master;
+ }
+
/*
* This wake-up/shutdown pattern is to be able to have the
* device woken up, even if runtime_pm is disabled
@@ -449,7 +456,7 @@
}
static const struct of_device_id sunxi_spi_match[] = {
- { .compatible = "allwinner,sun4i-a10-spi", },
+ { .compatible = "allwinner,sun6i-a31-spi", },
{}
};
MODULE_DEVICE_TABLE(of, sunxi_spi_match);
@@ -472,5 +479,5 @@
MODULE_AUTHOR("Pan Nan <pannan-0TFLnhJekD6UEPyfVivIlAC/G2K4zDHf@public.gmane.org>");
MODULE_AUTHOR("Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>");
-MODULE_DESCRIPTION("Allwinner A1X/A20 SPI controller driver");
+MODULE_DESCRIPTION("Allwinner A31 SPI controller driver");
MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 95+ messages in thread* Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout
@ 2016-05-31 11:52 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-31 11:52 UTC (permalink / raw)
To: Mark Brown
Cc: Julian Calaby, linux-sunxi, Maxime Ripard, Chen-Yu Tsai,
linux-spi, Mailing List, Arm, linux-kernel@vger.kernel.org
Hello
On 30 May 2016 at 13:23, Mark Brown <broonie@kernel.org> wrote:
> 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...
There are some slight differences and the constants really are
different for each driver.
You would need a register remap adaptation layer and a few qirks for
each revision of the driver.
It's certainly possible to merge them but I am not sure if that gives
easier to maintain code.
On the other hand, comparing the drivers there is different comment
anout clock calculation arithmetic and the code is the same :S
Thanks
Michal
--- spi-sun4i.c 2016-05-31 13:19:27.076510421 +0200
+++ spi-sun6i.c 2016-05-31 13:26:58.123580382 +0200
@@ -19,65 +19,72 @@
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
+#include <linux/reset.h>
#include <linux/spi/spi.h>
-#define SUNXI_FIFO_DEPTH 64
+#define SUNXI_FIFO_DEPTH 128
-#define SUNXI_RXDATA_REG 0x00
+#define SUNXI_TXDATA_REG 0x200
-#define SUNXI_TXDATA_REG 0x04
+#define SUNXI_RXDATA_REG 0x300
#define SUNXI_TFR_CTL_REG 0x08
-#define SUNXI_TFR_CTL_ENABLE BIT(0)
-#define SUNXI_TFR_CTL_MASTER BIT(1)
-#define SUNXI_TFR_CTL_CPHA BIT(2)
-#define SUNXI_TFR_CTL_CPOL BIT(3)
-#define SUNXI_TFR_CTL_CS_ACTIVE_LOW BIT(4)
-#define SUNXI_TFR_CTL_LMTF BIT(6)
-#define SUNXI_TFR_CTL_TF_RST BIT(8)
-#define SUNXI_TFR_CTL_RF_RST BIT(9)
-#define SUNXI_TFR_CTL_XCH BIT(10)
-#define SUNXI_TFR_CTL_CS_MASK 0x3000
-#define SUNXI_TFR_CTL_CS(cs) (((cs) << 12) & SUNXI_TFR_CTL_CS_MASK)
-#define SUNXI_TFR_CTL_DHB BIT(15)
-#define SUNXI_TFR_CTL_CS_MANUAL BIT(16)
-#define SUNXI_TFR_CTL_CS_LEVEL BIT(17)
-#define SUNXI_TFR_CTL_TP BIT(18)
+#define SUNXI_TFR_CTL_CPHA BIT(0)
+#define SUNXI_TFR_CTL_CPOL BIT(1)
+#define SUNXI_TFR_CTL_SPOL BIT(2)
+#define SUNXI_TFR_CTL_CS_MASK 0x30
+#define SUNXI_TFR_CTL_CS(cs) (((cs) << 4) & SUNXI_TFR_CTL_CS_MASK)
+#define SUNXI_TFR_CTL_CS_MANUAL BIT(6)
+#define SUNXI_TFR_CTL_CS_LEVEL BIT(7)
+#define SUNXI_TFR_CTL_DHB BIT(8)
+#define SUNXI_TFR_CTL_FBS BIT(12)
+#define SUNXI_TFR_CTL_XCH BIT(31)
-#define SUNXI_INT_CTL_REG 0x0c
-#define SUNXI_INT_CTL_TC BIT(16)
+#define SUNXI_INT_CTL_REG 0x10
+#define SUNXI_INT_CTL_RF_OVF BIT(8)
+#define SUNXI_INT_CTL_TC BIT(12)
-#define SUNXI_INT_STA_REG 0x10
+#define SUNXI_INT_STA_REG 0x14
-#define SUNXI_DMA_CTL_REG 0x14
+#define SUNXI_FIFO_CTL_REG 0x18
+#define SUNXI_FIFO_CTL_RF_RST BIT(15)
+#define SUNXI_FIFO_CTL_TF_RST BIT(31)
-#define SUNXI_CLK_CTL_REG 0x1c
+#define SUNXI_CLK_CTL_REG 0x24
#define SUNXI_CLK_CTL_CDR2_MASK 0xff
#define SUNXI_CLK_CTL_CDR2(div) (((div) &
SUNXI_CLK_CTL_CDR2_MASK) << 0)
#define SUNXI_CLK_CTL_CDR1_MASK 0xf
#define SUNXI_CLK_CTL_CDR1(div) (((div) &
SUNXI_CLK_CTL_CDR1_MASK) << 8)
#define SUNXI_CLK_CTL_DRS BIT(12)
-#define SUNXI_BURST_CNT_REG 0x20
+#define SUNXI_BURST_CNT_REG 0x30
#define SUNXI_BURST_CNT(cnt) ((cnt) & 0xffffff)
-#define SUNXI_XMIT_CNT_REG 0x24
+#define SUNXI_XMIT_CNT_REG 0x34
#define SUNXI_XMIT_CNT(cnt) ((cnt) & 0xffffff)
-#define SUNXI_FIFO_STA_REG 0x28
+#define SUNXI_FIFO_STA_REG 0x1c
#define SUNXI_FIFO_STA_RF_CNT_MASK 0x7f
#define SUNXI_FIFO_STA_RF_CNT_BITS 0
#define SUNXI_FIFO_STA_TF_CNT_MASK 0x7f
#define SUNXI_FIFO_STA_TF_CNT_BITS 16
-#define SUNXI_WAIT_REG 0x18
+#define SUNXI_BURST_CTL_CNT_REG 0x38
+#define SUNXI_BURST_CTL_CNT_STC(cnt) ((cnt) & 0xffffff)
+
+#define SUNXI_GBL_CTL_REG 0x04
+#define SUNXI_GBL_CTL_BUS_ENABLE BIT(0)
+#define SUNXI_GBL_CTL_MASTER BIT(1)
+#define SUNXI_GBL_CTL_TP BIT(7)
+#define SUNXI_GBL_CTL_RST BIT(31)
struct sunxi_spi {
struct spi_master *master;
void __iomem *base_addr;
struct clk *hclk;
struct clk *mclk;
+ struct reset_control *rstc;
struct completion done;
@@ -136,37 +143,18 @@
u32 reg;
reg = sunxi_spi_read(sspi, SUNXI_TFR_CTL_REG);
-
reg &= ~SUNXI_TFR_CTL_CS_MASK;
reg |= SUNXI_TFR_CTL_CS(spi->chip_select);
- /* We want to control the chip select manually */
- reg |= SUNXI_TFR_CTL_CS_MANUAL;
-
if (enable)
reg |= SUNXI_TFR_CTL_CS_LEVEL;
else
reg &= ~SUNXI_TFR_CTL_CS_LEVEL;
- /*
- * Even though this looks irrelevant since we are supposed to
- * be controlling the chip select manually, this bit also
- * controls the levels of the chip select for inactive
- * devices.
- *
- * If we don't set it, the chip select level will go low by
- * default when the device is idle, which is not really
- * expected in the common case where the chip select is active
- * low.
- */
- if (spi->mode & SPI_CS_HIGH)
- reg &= ~SUNXI_TFR_CTL_CS_ACTIVE_LOW;
- else
- reg |= SUNXI_TFR_CTL_CS_ACTIVE_LOW;
-
sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG, reg);
}
+
static int sunxi_spi_transfer_one(struct spi_master *master,
struct spi_device *spi,
struct spi_transfer *tfr)
@@ -189,17 +177,16 @@
/* Clear pending interrupts */
sunxi_spi_write(sspi, SUNXI_INT_STA_REG, ~0);
-
- reg = sunxi_spi_read(sspi, SUNXI_TFR_CTL_REG);
-
/* Reset FIFOs */
- sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
- reg | SUNXI_TFR_CTL_RF_RST | SUNXI_TFR_CTL_TF_RST);
+ sunxi_spi_write(sspi, SUNXI_FIFO_CTL_REG,
+ SUNXI_FIFO_CTL_RF_RST | SUNXI_FIFO_CTL_TF_RST);
/*
* Setup the transfer control register: Chip Select,
* polarities, etc.
*/
+ reg = sunxi_spi_read(sspi, SUNXI_TFR_CTL_REG);
+
if (spi->mode & SPI_CPOL)
reg |= SUNXI_TFR_CTL_CPOL;
else
@@ -211,10 +198,9 @@
reg &= ~SUNXI_TFR_CTL_CPHA;
if (spi->mode & SPI_LSB_FIRST)
- reg |= SUNXI_TFR_CTL_LMTF;
+ reg |= SUNXI_TFR_CTL_FBS;
else
- reg &= ~SUNXI_TFR_CTL_LMTF;
-
+ reg &= ~SUNXI_TFR_CTL_FBS;
/*
* If it's a TX only transfer, we don't want to fill the RX
@@ -225,6 +211,9 @@
else
reg |= SUNXI_TFR_CTL_DHB;
+ /* We want to control the chip select manually */
+ reg |= SUNXI_TFR_CTL_CS_MANUAL;
+
sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG, reg);
/* Ensure that we have a parent clock fast enough */
@@ -239,7 +228,7 @@
*
* We have two choices there. Either we can use the clock
* divide rate 1, which is calculated thanks to this formula:
- * SPI_CLK = MOD_CLK / (2 ^ (cdr + 1))
+ * SPI_CLK = MOD_CLK / (2 ^ cdr)
* Or we can use CDR2, which is calculated with the formula:
* SPI_CLK = MOD_CLK / (2 * (cdr + 1))
* Wether we use the former or the latter is set through the
@@ -268,6 +257,8 @@
/* Setup the counters */
sunxi_spi_write(sspi, SUNXI_BURST_CNT_REG, SUNXI_BURST_CNT(tfr->len));
sunxi_spi_write(sspi, SUNXI_XMIT_CNT_REG, SUNXI_XMIT_CNT(tx_len));
+ sunxi_spi_write(sspi, SUNXI_BURST_CTL_CNT_REG,
+ SUNXI_BURST_CTL_CNT_STC(tx_len));
/* Fill the TX FIFO */
sunxi_spi_fill_fifo(sspi, SUNXI_FIFO_DEPTH);
@@ -327,11 +318,19 @@
goto err;
}
- sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
- SUNXI_TFR_CTL_ENABLE | SUNXI_TFR_CTL_MASTER | SUNXI_TFR_CTL_TP);
+ ret = reset_control_deassert(sspi->rstc);
+ if (ret) {
+ dev_err(dev, "Couldn't deassert the device from reset\n");
+ goto err2;
+ }
+
+ sunxi_spi_write(sspi, SUNXI_GBL_CTL_REG,
+ SUNXI_GBL_CTL_BUS_ENABLE | SUNXI_GBL_CTL_MASTER |
SUNXI_GBL_CTL_TP);
return 0;
+err2:
+ clk_disable_unprepare(sspi->mclk);
err:
clk_disable_unprepare(sspi->hclk);
out:
@@ -343,6 +342,7 @@
struct spi_master *master = dev_get_drvdata(dev);
struct sunxi_spi *sspi = spi_master_get_devdata(master);
+ reset_control_assert(sspi->rstc);
clk_disable_unprepare(sspi->mclk);
clk_disable_unprepare(sspi->hclk);
@@ -411,6 +411,13 @@
init_completion(&sspi->done);
+ sspi->rstc = devm_reset_control_get(&pdev->dev, NULL);
+ if (IS_ERR(sspi->rstc)) {
+ dev_err(&pdev->dev, "Couldn't get reset controller\n");
+ ret = PTR_ERR(sspi->rstc);
+ goto err_free_master;
+ }
+
/*
* This wake-up/shutdown pattern is to be able to have the
* device woken up, even if runtime_pm is disabled
@@ -449,7 +456,7 @@
}
static const struct of_device_id sunxi_spi_match[] = {
- { .compatible = "allwinner,sun4i-a10-spi", },
+ { .compatible = "allwinner,sun6i-a31-spi", },
{}
};
MODULE_DEVICE_TABLE(of, sunxi_spi_match);
@@ -472,5 +479,5 @@
MODULE_AUTHOR("Pan Nan <pannan@allwinnertech.com>");
MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>");
-MODULE_DESCRIPTION("Allwinner A1X/A20 SPI controller driver");
+MODULE_DESCRIPTION("Allwinner A31 SPI controller driver");
MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 95+ messages in thread* [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout
@ 2016-05-31 11:52 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-31 11:52 UTC (permalink / raw)
To: linux-arm-kernel
Hello
On 30 May 2016 at 13:23, Mark Brown <broonie@kernel.org> wrote:
> 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...
There are some slight differences and the constants really are
different for each driver.
You would need a register remap adaptation layer and a few qirks for
each revision of the driver.
It's certainly possible to merge them but I am not sure if that gives
easier to maintain code.
On the other hand, comparing the drivers there is different comment
anout clock calculation arithmetic and the code is the same :S
Thanks
Michal
--- spi-sun4i.c 2016-05-31 13:19:27.076510421 +0200
+++ spi-sun6i.c 2016-05-31 13:26:58.123580382 +0200
@@ -19,65 +19,72 @@
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
+#include <linux/reset.h>
#include <linux/spi/spi.h>
-#define SUNXI_FIFO_DEPTH 64
+#define SUNXI_FIFO_DEPTH 128
-#define SUNXI_RXDATA_REG 0x00
+#define SUNXI_TXDATA_REG 0x200
-#define SUNXI_TXDATA_REG 0x04
+#define SUNXI_RXDATA_REG 0x300
#define SUNXI_TFR_CTL_REG 0x08
-#define SUNXI_TFR_CTL_ENABLE BIT(0)
-#define SUNXI_TFR_CTL_MASTER BIT(1)
-#define SUNXI_TFR_CTL_CPHA BIT(2)
-#define SUNXI_TFR_CTL_CPOL BIT(3)
-#define SUNXI_TFR_CTL_CS_ACTIVE_LOW BIT(4)
-#define SUNXI_TFR_CTL_LMTF BIT(6)
-#define SUNXI_TFR_CTL_TF_RST BIT(8)
-#define SUNXI_TFR_CTL_RF_RST BIT(9)
-#define SUNXI_TFR_CTL_XCH BIT(10)
-#define SUNXI_TFR_CTL_CS_MASK 0x3000
-#define SUNXI_TFR_CTL_CS(cs) (((cs) << 12) & SUNXI_TFR_CTL_CS_MASK)
-#define SUNXI_TFR_CTL_DHB BIT(15)
-#define SUNXI_TFR_CTL_CS_MANUAL BIT(16)
-#define SUNXI_TFR_CTL_CS_LEVEL BIT(17)
-#define SUNXI_TFR_CTL_TP BIT(18)
+#define SUNXI_TFR_CTL_CPHA BIT(0)
+#define SUNXI_TFR_CTL_CPOL BIT(1)
+#define SUNXI_TFR_CTL_SPOL BIT(2)
+#define SUNXI_TFR_CTL_CS_MASK 0x30
+#define SUNXI_TFR_CTL_CS(cs) (((cs) << 4) & SUNXI_TFR_CTL_CS_MASK)
+#define SUNXI_TFR_CTL_CS_MANUAL BIT(6)
+#define SUNXI_TFR_CTL_CS_LEVEL BIT(7)
+#define SUNXI_TFR_CTL_DHB BIT(8)
+#define SUNXI_TFR_CTL_FBS BIT(12)
+#define SUNXI_TFR_CTL_XCH BIT(31)
-#define SUNXI_INT_CTL_REG 0x0c
-#define SUNXI_INT_CTL_TC BIT(16)
+#define SUNXI_INT_CTL_REG 0x10
+#define SUNXI_INT_CTL_RF_OVF BIT(8)
+#define SUNXI_INT_CTL_TC BIT(12)
-#define SUNXI_INT_STA_REG 0x10
+#define SUNXI_INT_STA_REG 0x14
-#define SUNXI_DMA_CTL_REG 0x14
+#define SUNXI_FIFO_CTL_REG 0x18
+#define SUNXI_FIFO_CTL_RF_RST BIT(15)
+#define SUNXI_FIFO_CTL_TF_RST BIT(31)
-#define SUNXI_CLK_CTL_REG 0x1c
+#define SUNXI_CLK_CTL_REG 0x24
#define SUNXI_CLK_CTL_CDR2_MASK 0xff
#define SUNXI_CLK_CTL_CDR2(div) (((div) &
SUNXI_CLK_CTL_CDR2_MASK) << 0)
#define SUNXI_CLK_CTL_CDR1_MASK 0xf
#define SUNXI_CLK_CTL_CDR1(div) (((div) &
SUNXI_CLK_CTL_CDR1_MASK) << 8)
#define SUNXI_CLK_CTL_DRS BIT(12)
-#define SUNXI_BURST_CNT_REG 0x20
+#define SUNXI_BURST_CNT_REG 0x30
#define SUNXI_BURST_CNT(cnt) ((cnt) & 0xffffff)
-#define SUNXI_XMIT_CNT_REG 0x24
+#define SUNXI_XMIT_CNT_REG 0x34
#define SUNXI_XMIT_CNT(cnt) ((cnt) & 0xffffff)
-#define SUNXI_FIFO_STA_REG 0x28
+#define SUNXI_FIFO_STA_REG 0x1c
#define SUNXI_FIFO_STA_RF_CNT_MASK 0x7f
#define SUNXI_FIFO_STA_RF_CNT_BITS 0
#define SUNXI_FIFO_STA_TF_CNT_MASK 0x7f
#define SUNXI_FIFO_STA_TF_CNT_BITS 16
-#define SUNXI_WAIT_REG 0x18
+#define SUNXI_BURST_CTL_CNT_REG 0x38
+#define SUNXI_BURST_CTL_CNT_STC(cnt) ((cnt) & 0xffffff)
+
+#define SUNXI_GBL_CTL_REG 0x04
+#define SUNXI_GBL_CTL_BUS_ENABLE BIT(0)
+#define SUNXI_GBL_CTL_MASTER BIT(1)
+#define SUNXI_GBL_CTL_TP BIT(7)
+#define SUNXI_GBL_CTL_RST BIT(31)
struct sunxi_spi {
struct spi_master *master;
void __iomem *base_addr;
struct clk *hclk;
struct clk *mclk;
+ struct reset_control *rstc;
struct completion done;
@@ -136,37 +143,18 @@
u32 reg;
reg = sunxi_spi_read(sspi, SUNXI_TFR_CTL_REG);
-
reg &= ~SUNXI_TFR_CTL_CS_MASK;
reg |= SUNXI_TFR_CTL_CS(spi->chip_select);
- /* We want to control the chip select manually */
- reg |= SUNXI_TFR_CTL_CS_MANUAL;
-
if (enable)
reg |= SUNXI_TFR_CTL_CS_LEVEL;
else
reg &= ~SUNXI_TFR_CTL_CS_LEVEL;
- /*
- * Even though this looks irrelevant since we are supposed to
- * be controlling the chip select manually, this bit also
- * controls the levels of the chip select for inactive
- * devices.
- *
- * If we don't set it, the chip select level will go low by
- * default when the device is idle, which is not really
- * expected in the common case where the chip select is active
- * low.
- */
- if (spi->mode & SPI_CS_HIGH)
- reg &= ~SUNXI_TFR_CTL_CS_ACTIVE_LOW;
- else
- reg |= SUNXI_TFR_CTL_CS_ACTIVE_LOW;
-
sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG, reg);
}
+
static int sunxi_spi_transfer_one(struct spi_master *master,
struct spi_device *spi,
struct spi_transfer *tfr)
@@ -189,17 +177,16 @@
/* Clear pending interrupts */
sunxi_spi_write(sspi, SUNXI_INT_STA_REG, ~0);
-
- reg = sunxi_spi_read(sspi, SUNXI_TFR_CTL_REG);
-
/* Reset FIFOs */
- sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
- reg | SUNXI_TFR_CTL_RF_RST | SUNXI_TFR_CTL_TF_RST);
+ sunxi_spi_write(sspi, SUNXI_FIFO_CTL_REG,
+ SUNXI_FIFO_CTL_RF_RST | SUNXI_FIFO_CTL_TF_RST);
/*
* Setup the transfer control register: Chip Select,
* polarities, etc.
*/
+ reg = sunxi_spi_read(sspi, SUNXI_TFR_CTL_REG);
+
if (spi->mode & SPI_CPOL)
reg |= SUNXI_TFR_CTL_CPOL;
else
@@ -211,10 +198,9 @@
reg &= ~SUNXI_TFR_CTL_CPHA;
if (spi->mode & SPI_LSB_FIRST)
- reg |= SUNXI_TFR_CTL_LMTF;
+ reg |= SUNXI_TFR_CTL_FBS;
else
- reg &= ~SUNXI_TFR_CTL_LMTF;
-
+ reg &= ~SUNXI_TFR_CTL_FBS;
/*
* If it's a TX only transfer, we don't want to fill the RX
@@ -225,6 +211,9 @@
else
reg |= SUNXI_TFR_CTL_DHB;
+ /* We want to control the chip select manually */
+ reg |= SUNXI_TFR_CTL_CS_MANUAL;
+
sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG, reg);
/* Ensure that we have a parent clock fast enough */
@@ -239,7 +228,7 @@
*
* We have two choices there. Either we can use the clock
* divide rate 1, which is calculated thanks to this formula:
- * SPI_CLK = MOD_CLK / (2 ^ (cdr + 1))
+ * SPI_CLK = MOD_CLK / (2 ^ cdr)
* Or we can use CDR2, which is calculated with the formula:
* SPI_CLK = MOD_CLK / (2 * (cdr + 1))
* Wether we use the former or the latter is set through the
@@ -268,6 +257,8 @@
/* Setup the counters */
sunxi_spi_write(sspi, SUNXI_BURST_CNT_REG, SUNXI_BURST_CNT(tfr->len));
sunxi_spi_write(sspi, SUNXI_XMIT_CNT_REG, SUNXI_XMIT_CNT(tx_len));
+ sunxi_spi_write(sspi, SUNXI_BURST_CTL_CNT_REG,
+ SUNXI_BURST_CTL_CNT_STC(tx_len));
/* Fill the TX FIFO */
sunxi_spi_fill_fifo(sspi, SUNXI_FIFO_DEPTH);
@@ -327,11 +318,19 @@
goto err;
}
- sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG,
- SUNXI_TFR_CTL_ENABLE | SUNXI_TFR_CTL_MASTER | SUNXI_TFR_CTL_TP);
+ ret = reset_control_deassert(sspi->rstc);
+ if (ret) {
+ dev_err(dev, "Couldn't deassert the device from reset\n");
+ goto err2;
+ }
+
+ sunxi_spi_write(sspi, SUNXI_GBL_CTL_REG,
+ SUNXI_GBL_CTL_BUS_ENABLE | SUNXI_GBL_CTL_MASTER |
SUNXI_GBL_CTL_TP);
return 0;
+err2:
+ clk_disable_unprepare(sspi->mclk);
err:
clk_disable_unprepare(sspi->hclk);
out:
@@ -343,6 +342,7 @@
struct spi_master *master = dev_get_drvdata(dev);
struct sunxi_spi *sspi = spi_master_get_devdata(master);
+ reset_control_assert(sspi->rstc);
clk_disable_unprepare(sspi->mclk);
clk_disable_unprepare(sspi->hclk);
@@ -411,6 +411,13 @@
init_completion(&sspi->done);
+ sspi->rstc = devm_reset_control_get(&pdev->dev, NULL);
+ if (IS_ERR(sspi->rstc)) {
+ dev_err(&pdev->dev, "Couldn't get reset controller\n");
+ ret = PTR_ERR(sspi->rstc);
+ goto err_free_master;
+ }
+
/*
* This wake-up/shutdown pattern is to be able to have the
* device woken up, even if runtime_pm is disabled
@@ -449,7 +456,7 @@
}
static const struct of_device_id sunxi_spi_match[] = {
- { .compatible = "allwinner,sun4i-a10-spi", },
+ { .compatible = "allwinner,sun6i-a31-spi", },
{}
};
MODULE_DEVICE_TABLE(of, sunxi_spi_match);
@@ -472,5 +479,5 @@
MODULE_AUTHOR("Pan Nan <pannan@allwinnertech.com>");
MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>");
-MODULE_DESCRIPTION("Allwinner A1X/A20 SPI controller driver");
+MODULE_DESCRIPTION("Allwinner A31 SPI controller driver");
MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout
2016-05-30 11:23 ` Mark Brown
(?)
@ 2016-06-01 18:20 ` Maxime Ripard
-1 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-06-01 18:20 UTC (permalink / raw)
To: Mark Brown
Cc: Julian Calaby, Michal Suchanek, linux-sunxi, Chen-Yu Tsai,
linux-spi, Mailing List, Arm,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
[-- Attachment #1: Type: text/plain, Size: 1668 bytes --]
Hi Mark,
On Mon, May 30, 2016 at 12:23:50PM +0100, Mark Brown wrote:
> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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...
I think we already had this discussion a few times :)
The thing is that the SPI framework now deals pretty well with the SPI
controllers, and you basically only have the probe function and how to
start a transfer. Which is nice.
However, the sun4i and sun6i SPI controllers have very significant
register layout differences: the registers offset are different, some
registers in the sun4i have been split in several in the sun6i. So
while I concur that they look alike, merging the two in a single
driver would complicate a lot the code, while we would not be able to
share most of the code, so I am really not sure it's worth it.
Where the issue really lies is that they've been both written at the
same time, and share the same flaws, especially in their probe
method. But that's not really related to the controller itself, but
more because the code has been close to copy/pasted.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout
@ 2016-06-01 18:20 ` Maxime Ripard
0 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-06-01 18:20 UTC (permalink / raw)
To: Mark Brown
Cc: Julian Calaby, Michal Suchanek, linux-sunxi, Chen-Yu Tsai,
linux-spi, Mailing List, Arm, linux-kernel@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 1638 bytes --]
Hi Mark,
On Mon, May 30, 2016 at 12:23:50PM +0100, Mark Brown wrote:
> 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...
I think we already had this discussion a few times :)
The thing is that the SPI framework now deals pretty well with the SPI
controllers, and you basically only have the probe function and how to
start a transfer. Which is nice.
However, the sun4i and sun6i SPI controllers have very significant
register layout differences: the registers offset are different, some
registers in the sun4i have been split in several in the sun6i. So
while I concur that they look alike, merging the two in a single
driver would complicate a lot the code, while we would not be able to
share most of the code, so I am really not sure it's worth it.
Where the issue really lies is that they've been both written at the
same time, and share the same flaws, especially in their probe
method. But that's not really related to the controller itself, but
more because the code has been close to copy/pasted.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 95+ messages in thread
* [linux-sunxi] [PATCH 1/5] spi: sunxi: fix transfer timeout
@ 2016-06-01 18:20 ` Maxime Ripard
0 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-06-01 18:20 UTC (permalink / raw)
To: linux-arm-kernel
Hi Mark,
On Mon, May 30, 2016 at 12:23:50PM +0100, Mark Brown wrote:
> 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...
I think we already had this discussion a few times :)
The thing is that the SPI framework now deals pretty well with the SPI
controllers, and you basically only have the probe function and how to
start a transfer. Which is nice.
However, the sun4i and sun6i SPI controllers have very significant
register layout differences: the registers offset are different, some
registers in the sun4i have been split in several in the sun6i. So
while I concur that they look alike, merging the two in a single
driver would complicate a lot the code, while we would not be able to
share most of the code, so I am really not sure it's worth it.
Where the issue really lies is that they've been both written at the
same time, and share the same flaws, especially in their probe
method. But that's not really related to the controller itself, but
more because the code has been close to copy/pasted.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160601/1248687a/attachment.sig>
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH 1/5] spi: sunxi: fix transfer timeout
2016-05-26 19:25 ` Michal Suchanek
(?)
@ 2016-05-30 9:44 ` Maxime Ripard
-1 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-05-30 9:44 UTC (permalink / raw)
To: Michal Suchanek
Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Mark Brown, Chen-Yu Tsai,
linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
[-- Attachment #1: Type: text/plain, Size: 689 bytes --]
On Thu, May 26, 2016 at 07:25:23PM -0000, Michal Suchanek 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>
> v2:
> - fix build error
> - use unsigned instead of int in max_t
And this generates warnings in checkpatch. Please make sure that you
run it before submitting any patches.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH 1/5] spi: sunxi: fix transfer timeout
@ 2016-05-30 9:44 ` Maxime Ripard
0 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-05-30 9:44 UTC (permalink / raw)
To: Michal Suchanek
Cc: linux-sunxi, Mark Brown, Chen-Yu Tsai, linux-spi,
linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 683 bytes --]
On Thu, May 26, 2016 at 07:25:23PM -0000, Michal Suchanek 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
And this generates warnings in checkpatch. Please make sure that you
run it before submitting any patches.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH 1/5] spi: sunxi: fix transfer timeout
@ 2016-05-30 9:44 ` Maxime Ripard
0 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-05-30 9:44 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, May 26, 2016 at 07:25:23PM -0000, Michal Suchanek 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
And this generates warnings in checkpatch. Please make sure that you
run it before submitting any patches.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160530/6e2521ef/attachment.sig>
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH 2/5] spi: sun4i: fix FIFO limit
2016-05-24 22:56 ` Michal Suchanek
@ 2016-05-26 19:25 ` Michal Suchanek
-1 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-26 19:25 UTC (permalink / raw)
To: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Mark Brown, Maxime Ripard,
Chen-Yu Tsai, linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
When testing SPI without DMA I noticed that filling the FIFO on the
spi controller causes timeout.
Always leave room for one byte in the FIFO.
Signed-off-by: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
v2:
use EMSGSIZE instead of EINVAL
---
drivers/spi/spi-sun4i.c | 8 ++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index fe63bbd..04f1b77 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -180,7 +180,9 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
/* We don't support transfer larger than the FIFO */
if (tfr->len > SUN4I_FIFO_DEPTH)
- return -EINVAL;
+ return -EMSGSIZE;
+ if (tfr->tx_buf && tfr->len >= SUN4I_FIFO_DEPTH)
+ return -EMSGSIZE;
reinit_completion(&sspi->done);
sspi->tx_buf = tfr->tx_buf;
@@ -271,7 +273,9 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
sun4i_spi_write(sspi, SUN4I_XMIT_CNT_REG, SUN4I_XMIT_CNT(tx_len));
/* Fill the TX FIFO */
- sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
+ /* Filling the FIFO fully causes timeout for some reason
+ * at least on spi2 on A10s */
+ sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1);
/* Enable the interrupts */
sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, SUN4I_INT_CTL_TC);
^ permalink raw reply related [flat|nested] 95+ messages in thread
* [PATCH 2/5] spi: sun4i: fix FIFO limit
@ 2016-05-26 19:25 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-26 19:25 UTC (permalink / raw)
To: linux-arm-kernel
When testing SPI without DMA I noticed that filling the FIFO on the
spi controller causes timeout.
Always leave room for one byte in the FIFO.
Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
v2:
use EMSGSIZE instead of EINVAL
---
drivers/spi/spi-sun4i.c | 8 ++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index fe63bbd..04f1b77 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -180,7 +180,9 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
/* We don't support transfer larger than the FIFO */
if (tfr->len > SUN4I_FIFO_DEPTH)
- return -EINVAL;
+ return -EMSGSIZE;
+ if (tfr->tx_buf && tfr->len >= SUN4I_FIFO_DEPTH)
+ return -EMSGSIZE;
reinit_completion(&sspi->done);
sspi->tx_buf = tfr->tx_buf;
@@ -271,7 +273,9 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
sun4i_spi_write(sspi, SUN4I_XMIT_CNT_REG, SUN4I_XMIT_CNT(tx_len));
/* Fill the TX FIFO */
- sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
+ /* Filling the FIFO fully causes timeout for some reason
+ * at least on spi2 on A10s */
+ sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1);
/* Enable the interrupts */
sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, SUN4I_INT_CTL_TC);
^ permalink raw reply related [flat|nested] 95+ messages in thread
[parent not found: <e315008b5e9dc3f1490507508fd2f6e94767dfbb.1464130597.git.hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH 2/5] spi: sun4i: fix FIFO limit
2016-05-26 19:25 ` Michal Suchanek
(?)
@ 2016-05-30 8:37 ` Maxime Ripard
-1 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-05-30 8:37 UTC (permalink / raw)
To: Michal Suchanek
Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Mark Brown, Chen-Yu Tsai,
linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
[-- Attachment #1: Type: text/plain, Size: 1672 bytes --]
Hi,
On Thu, May 26, 2016 at 07:25:24PM -0000, Michal Suchanek wrote:
> When testing SPI without DMA I noticed that filling the FIFO on the
> spi controller causes timeout.
>
> Always leave room for one byte in the FIFO.
>
> Signed-off-by: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Sending it to stable would be great (with a fixes tag too).
>
> ---
> v2:
> use EMSGSIZE instead of EINVAL
> ---
> drivers/spi/spi-sun4i.c | 8 ++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index fe63bbd..04f1b77 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
> @@ -180,7 +180,9 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>
> /* We don't support transfer larger than the FIFO */
> if (tfr->len > SUN4I_FIFO_DEPTH)
> - return -EINVAL;
> + return -EMSGSIZE;
One new line here.
> + if (tfr->tx_buf && tfr->len >= SUN4I_FIFO_DEPTH)
> + return -EMSGSIZE;
>
> reinit_completion(&sspi->done);
> sspi->tx_buf = tfr->tx_buf;
> @@ -271,7 +273,9 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
> sun4i_spi_write(sspi, SUN4I_XMIT_CNT_REG, SUN4I_XMIT_CNT(tx_len));
>
> /* Fill the TX FIFO */
> - sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
> + /* Filling the FIFO fully causes timeout for some reason
> + * at least on spi2 on A10s */
This is not the proper comment style and generate a warning with
checkpatch, please fix that (and merge the comment with the previous
comment).
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH 2/5] spi: sun4i: fix FIFO limit
@ 2016-05-30 8:37 ` Maxime Ripard
0 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-05-30 8:37 UTC (permalink / raw)
To: Michal Suchanek
Cc: linux-sunxi, Mark Brown, Chen-Yu Tsai, linux-spi,
linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1700 bytes --]
Hi,
On Thu, May 26, 2016 at 07:25:24PM -0000, Michal Suchanek wrote:
> When testing SPI without DMA I noticed that filling the FIFO on the
> spi controller causes timeout.
>
> Always leave room for one byte in the FIFO.
>
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
Sending it to stable would be great (with a fixes tag too).
>
> ---
> v2:
> use EMSGSIZE instead of EINVAL
> ---
> drivers/spi/spi-sun4i.c | 8 ++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index fe63bbd..04f1b77 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
> @@ -180,7 +180,9 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>
> /* We don't support transfer larger than the FIFO */
> if (tfr->len > SUN4I_FIFO_DEPTH)
> - return -EINVAL;
> + return -EMSGSIZE;
One new line here.
> + if (tfr->tx_buf && tfr->len >= SUN4I_FIFO_DEPTH)
> + return -EMSGSIZE;
>
> reinit_completion(&sspi->done);
> sspi->tx_buf = tfr->tx_buf;
> @@ -271,7 +273,9 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
> sun4i_spi_write(sspi, SUN4I_XMIT_CNT_REG, SUN4I_XMIT_CNT(tx_len));
>
> /* Fill the TX FIFO */
> - sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
> + /* Filling the FIFO fully causes timeout for some reason
> + * at least on spi2 on A10s */
This is not the proper comment style and generate a warning with
checkpatch, please fix that (and merge the comment with the previous
comment).
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH 2/5] spi: sun4i: fix FIFO limit
@ 2016-05-30 8:37 ` Maxime Ripard
0 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-05-30 8:37 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Thu, May 26, 2016 at 07:25:24PM -0000, Michal Suchanek wrote:
> When testing SPI without DMA I noticed that filling the FIFO on the
> spi controller causes timeout.
>
> Always leave room for one byte in the FIFO.
>
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
Sending it to stable would be great (with a fixes tag too).
>
> ---
> v2:
> use EMSGSIZE instead of EINVAL
> ---
> drivers/spi/spi-sun4i.c | 8 ++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index fe63bbd..04f1b77 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
> @@ -180,7 +180,9 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>
> /* We don't support transfer larger than the FIFO */
> if (tfr->len > SUN4I_FIFO_DEPTH)
> - return -EINVAL;
> + return -EMSGSIZE;
One new line here.
> + if (tfr->tx_buf && tfr->len >= SUN4I_FIFO_DEPTH)
> + return -EMSGSIZE;
>
> reinit_completion(&sspi->done);
> sspi->tx_buf = tfr->tx_buf;
> @@ -271,7 +273,9 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
> sun4i_spi_write(sspi, SUN4I_XMIT_CNT_REG, SUN4I_XMIT_CNT(tx_len));
>
> /* Fill the TX FIFO */
> - sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
> + /* Filling the FIFO fully causes timeout for some reason
> + * at least on spi2 on A10s */
This is not the proper comment style and generate a warning with
checkpatch, please fix that (and merge the comment with the previous
comment).
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160530/70e12939/attachment.sig>
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH 3/5] spi: sunxi: expose maximum transfer size limit
2016-05-24 22:56 ` Michal Suchanek
@ 2016-05-26 19:25 ` Michal Suchanek
-1 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-26 19:25 UTC (permalink / raw)
To: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Mark Brown, Maxime Ripard,
Chen-Yu Tsai, linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
The sun4i spi hardware can trasfer at most 63 bytes of data without DMA
support so report the limitation. Same on sun6i.
Signed-off-by: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/spi/spi-sun4i.c | 6 ++++++
drivers/spi/spi-sun6i.c | 6 ++++++
2 files changed, 12 insertions(+)
diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index 04f1b77..bf52b09 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -167,6 +167,11 @@ static void sun4i_spi_set_cs(struct spi_device *spi, bool enable)
sun4i_spi_write(sspi, SUN4I_CTL_REG, reg);
}
+static size_t sun4i_spi_max_transfer_size(struct spi_device *spi)
+{
+ return SUN4I_FIFO_DEPTH - 1;
+}
+
static int sun4i_spi_transfer_one(struct spi_master *master,
struct spi_device *spi,
struct spi_transfer *tfr)
@@ -407,6 +412,7 @@ static int sun4i_spi_probe(struct platform_device *pdev)
master->bits_per_word_mask = SPI_BPW_MASK(8);
master->dev.of_node = pdev->dev.of_node;
master->auto_runtime_pm = true;
+ master->max_transfer_size = sun4i_spi_max_transfer_size;
sspi->hclk = devm_clk_get(&pdev->dev, "ahb");
if (IS_ERR(sspi->hclk)) {
diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 8954a62..f491a41 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -154,6 +154,11 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
}
+static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
+{
+ return SUN6I_FIFO_DEPTH - 1;
+}
+
static int sun6i_spi_transfer_one(struct spi_master *master,
struct spi_device *spi,
struct spi_transfer *tfr)
@@ -418,6 +423,7 @@ static int sun6i_spi_probe(struct platform_device *pdev)
master->bits_per_word_mask = SPI_BPW_MASK(8);
master->dev.of_node = pdev->dev.of_node;
master->auto_runtime_pm = true;
+ master->max_transfer_size = sun6i_spi_max_transfer_size;
sspi->hclk = devm_clk_get(&pdev->dev, "ahb");
if (IS_ERR(sspi->hclk)) {
--
2.8.1
^ permalink raw reply related [flat|nested] 95+ messages in thread* [PATCH 3/5] spi: sunxi: expose maximum transfer size limit
@ 2016-05-26 19:25 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-26 19:25 UTC (permalink / raw)
To: linux-arm-kernel
The sun4i spi hardware can trasfer at most 63 bytes of data without DMA
support so report the limitation. Same on sun6i.
Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
drivers/spi/spi-sun4i.c | 6 ++++++
drivers/spi/spi-sun6i.c | 6 ++++++
2 files changed, 12 insertions(+)
diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index 04f1b77..bf52b09 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -167,6 +167,11 @@ static void sun4i_spi_set_cs(struct spi_device *spi, bool enable)
sun4i_spi_write(sspi, SUN4I_CTL_REG, reg);
}
+static size_t sun4i_spi_max_transfer_size(struct spi_device *spi)
+{
+ return SUN4I_FIFO_DEPTH - 1;
+}
+
static int sun4i_spi_transfer_one(struct spi_master *master,
struct spi_device *spi,
struct spi_transfer *tfr)
@@ -407,6 +412,7 @@ static int sun4i_spi_probe(struct platform_device *pdev)
master->bits_per_word_mask = SPI_BPW_MASK(8);
master->dev.of_node = pdev->dev.of_node;
master->auto_runtime_pm = true;
+ master->max_transfer_size = sun4i_spi_max_transfer_size;
sspi->hclk = devm_clk_get(&pdev->dev, "ahb");
if (IS_ERR(sspi->hclk)) {
diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 8954a62..f491a41 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -154,6 +154,11 @@ static void sun6i_spi_set_cs(struct spi_device *spi, bool enable)
}
+static size_t sun6i_spi_max_transfer_size(struct spi_device *spi)
+{
+ return SUN6I_FIFO_DEPTH - 1;
+}
+
static int sun6i_spi_transfer_one(struct spi_master *master,
struct spi_device *spi,
struct spi_transfer *tfr)
@@ -418,6 +423,7 @@ static int sun6i_spi_probe(struct platform_device *pdev)
master->bits_per_word_mask = SPI_BPW_MASK(8);
master->dev.of_node = pdev->dev.of_node;
master->auto_runtime_pm = true;
+ master->max_transfer_size = sun6i_spi_max_transfer_size;
sspi->hclk = devm_clk_get(&pdev->dev, "ahb");
if (IS_ERR(sspi->hclk)) {
--
2.8.1
^ permalink raw reply related [flat|nested] 95+ messages in thread[parent not found: <5fffb7eca6f4b70853d92be2403595d6d06bede7.1464130597.git.hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH 3/5] spi: sunxi: expose maximum transfer size limit
2016-05-26 19:25 ` Michal Suchanek
(?)
@ 2016-05-30 8:37 ` Maxime Ripard
-1 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-05-30 8:37 UTC (permalink / raw)
To: Michal Suchanek
Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Mark Brown, Chen-Yu Tsai,
linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
[-- Attachment #1: Type: text/plain, Size: 358 bytes --]
On Thu, May 26, 2016 at 07:25:24PM -0000, Michal Suchanek wrote:
> The sun4i spi hardware can trasfer at most 63 bytes of data without DMA
> support so report the limitation. Same on sun6i.
Is it? Is there timeouts on the A31 and later SoCs?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH 3/5] spi: sunxi: expose maximum transfer size limit
@ 2016-05-30 8:37 ` Maxime Ripard
0 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-05-30 8:37 UTC (permalink / raw)
To: Michal Suchanek
Cc: linux-sunxi, Mark Brown, Chen-Yu Tsai, linux-spi,
linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 371 bytes --]
On Thu, May 26, 2016 at 07:25:24PM -0000, Michal Suchanek wrote:
> The sun4i spi hardware can trasfer at most 63 bytes of data without DMA
> support so report the limitation. Same on sun6i.
Is it? Is there timeouts on the A31 and later SoCs?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH 3/5] spi: sunxi: expose maximum transfer size limit
@ 2016-05-30 8:37 ` Maxime Ripard
0 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-05-30 8:37 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, May 26, 2016 at 07:25:24PM -0000, Michal Suchanek wrote:
> The sun4i spi hardware can trasfer at most 63 bytes of data without DMA
> support so report the limitation. Same on sun6i.
Is it? Is there timeouts on the A31 and later SoCs?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160530/4739a050/attachment.sig>
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH 3/5] spi: sunxi: expose maximum transfer size limit
2016-05-30 8:37 ` Maxime Ripard
(?)
@ 2016-05-30 8:57 ` Michal Suchanek
-1 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-30 8:57 UTC (permalink / raw)
To: Maxime Ripard
Cc: linux-sunxi, Mark Brown, Chen-Yu Tsai, linux-spi,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Linux Kernel Mailing List
Hello,
On 30 May 2016 at 10:37, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Thu, May 26, 2016 at 07:25:24PM -0000, Michal Suchanek wrote:
>> The sun4i spi hardware can trasfer at most 63 bytes of data without DMA
>> support so report the limitation. Same on sun6i.
>
> Is it? Is there timeouts on the A31 and later SoCs?
>
I have no sun6i hardware to test with.
This is an advisory limit you can query and it better be smaller
rather than unreliable. The hard limit in the driver is still
SUN6I_FIFO_DEPTH (which is 128 bytes on sun6i).
You can test his without any actual SPI device. Unused pins you can
multiplex as SPI suffice.
Thanks
Michal
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH 3/5] spi: sunxi: expose maximum transfer size limit
@ 2016-05-30 8:57 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-30 8:57 UTC (permalink / raw)
To: Maxime Ripard
Cc: linux-sunxi, Mark Brown, Chen-Yu Tsai, linux-spi,
linux-arm-kernel@lists.infradead.org, Linux Kernel Mailing List
Hello,
On 30 May 2016 at 10:37, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> On Thu, May 26, 2016 at 07:25:24PM -0000, Michal Suchanek wrote:
>> The sun4i spi hardware can trasfer at most 63 bytes of data without DMA
>> support so report the limitation. Same on sun6i.
>
> Is it? Is there timeouts on the A31 and later SoCs?
>
I have no sun6i hardware to test with.
This is an advisory limit you can query and it better be smaller
rather than unreliable. The hard limit in the driver is still
SUN6I_FIFO_DEPTH (which is 128 bytes on sun6i).
You can test his without any actual SPI device. Unused pins you can
multiplex as SPI suffice.
Thanks
Michal
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH 3/5] spi: sunxi: expose maximum transfer size limit
@ 2016-05-30 8:57 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-30 8:57 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
On 30 May 2016 at 10:37, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> On Thu, May 26, 2016 at 07:25:24PM -0000, Michal Suchanek wrote:
>> The sun4i spi hardware can trasfer at most 63 bytes of data without DMA
>> support so report the limitation. Same on sun6i.
>
> Is it? Is there timeouts on the A31 and later SoCs?
>
I have no sun6i hardware to test with.
This is an advisory limit you can query and it better be smaller
rather than unreliable. The hard limit in the driver is still
SUN6I_FIFO_DEPTH (which is 128 bytes on sun6i).
You can test his without any actual SPI device. Unused pins you can
multiplex as SPI suffice.
Thanks
Michal
^ permalink raw reply [flat|nested] 95+ messages in thread
[parent not found: <CAOMqctQdbipvvTnF253S4jGDos1crYXk9nuzDcRvy4geeS=GrQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 3/5] spi: sunxi: expose maximum transfer size limit
2016-05-30 8:57 ` Michal Suchanek
(?)
@ 2016-06-01 18:14 ` Maxime Ripard
-1 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-06-01 18:14 UTC (permalink / raw)
To: Michal Suchanek
Cc: linux-sunxi, Mark Brown, Chen-Yu Tsai, linux-spi,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Linux Kernel Mailing List
[-- Attachment #1: Type: text/plain, Size: 1023 bytes --]
On Mon, May 30, 2016 at 10:57:10AM +0200, Michal Suchanek wrote:
> Hello,
>
> On 30 May 2016 at 10:37, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > On Thu, May 26, 2016 at 07:25:24PM -0000, Michal Suchanek wrote:
> >> The sun4i spi hardware can trasfer at most 63 bytes of data without DMA
> >> support so report the limitation. Same on sun6i.
> >
> > Is it? Is there timeouts on the A31 and later SoCs?
> >
>
> I have no sun6i hardware to test with.
>
> This is an advisory limit you can query and it better be smaller
> rather than unreliable. The hard limit in the driver is still
> SUN6I_FIFO_DEPTH (which is 128 bytes on sun6i).
>
> You can test his without any actual SPI device. Unused pins you can
> multiplex as SPI suffice.
Ok, for the time being:
Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH 3/5] spi: sunxi: expose maximum transfer size limit
@ 2016-06-01 18:14 ` Maxime Ripard
0 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-06-01 18:14 UTC (permalink / raw)
To: Michal Suchanek
Cc: linux-sunxi, Mark Brown, Chen-Yu Tsai, linux-spi,
linux-arm-kernel@lists.infradead.org, Linux Kernel Mailing List
[-- Attachment #1: Type: text/plain, Size: 993 bytes --]
On Mon, May 30, 2016 at 10:57:10AM +0200, Michal Suchanek wrote:
> Hello,
>
> On 30 May 2016 at 10:37, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> > On Thu, May 26, 2016 at 07:25:24PM -0000, Michal Suchanek wrote:
> >> The sun4i spi hardware can trasfer at most 63 bytes of data without DMA
> >> support so report the limitation. Same on sun6i.
> >
> > Is it? Is there timeouts on the A31 and later SoCs?
> >
>
> I have no sun6i hardware to test with.
>
> This is an advisory limit you can query and it better be smaller
> rather than unreliable. The hard limit in the driver is still
> SUN6I_FIFO_DEPTH (which is 128 bytes on sun6i).
>
> You can test his without any actual SPI device. Unused pins you can
> multiplex as SPI suffice.
Ok, for the time being:
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH 3/5] spi: sunxi: expose maximum transfer size limit
@ 2016-06-01 18:14 ` Maxime Ripard
0 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-06-01 18:14 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, May 30, 2016 at 10:57:10AM +0200, Michal Suchanek wrote:
> Hello,
>
> On 30 May 2016 at 10:37, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> > On Thu, May 26, 2016 at 07:25:24PM -0000, Michal Suchanek wrote:
> >> The sun4i spi hardware can trasfer at most 63 bytes of data without DMA
> >> support so report the limitation. Same on sun6i.
> >
> > Is it? Is there timeouts on the A31 and later SoCs?
> >
>
> I have no sun6i hardware to test with.
>
> This is an advisory limit you can query and it better be smaller
> rather than unreliable. The hard limit in the driver is still
> SUN6I_FIFO_DEPTH (which is 128 bytes on sun6i).
>
> You can test his without any actual SPI device. Unused pins you can
> multiplex as SPI suffice.
Ok, for the time being:
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160601/13d59b09/attachment.sig>
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH 5/5] RFC spi: sun4i: add DMA support
2016-05-24 22:56 ` Michal Suchanek
@ 2016-05-26 19:25 ` Michal Suchanek
-1 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-26 19:25 UTC (permalink / raw)
To: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Mark Brown, Maxime Ripard,
Chen-Yu Tsai, linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
From: Emilio López <emilio-0Z03zUJReD5OxF6Tv1QG9Q@public.gmane.org>
This patch adds support for 64 byte or bigger transfers on the
sun4i SPI controller. Said transfers will be performed via DMA.
Signed-off-by: Emilio López <emilio-0Z03zUJReD5OxF6Tv1QG9Q@public.gmane.org>
Signed-off-by: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
v2:
- fallback to previous behaviour when DMA initialization fails
+ this has the problem that when the driver happens to load before the dma
driver it will not use dma - can be addressed with a module parameter
+ the issue with dma_async_tx_descriptor *desc_tx = NULL, *desc_rx = NULL; ...
dma_wait_for_async_tx(desc_rx); remains unaddressed
---
drivers/spi/spi-sun4i.c | 171 ++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 158 insertions(+), 13 deletions(-)
diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index e1a75dd6..ed2269c 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -14,6 +14,8 @@
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/device.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/module.h>
@@ -34,6 +36,7 @@
#define SUN4I_CTL_CPHA BIT(2)
#define SUN4I_CTL_CPOL BIT(3)
#define SUN4I_CTL_CS_ACTIVE_LOW BIT(4)
+#define SUN4I_CTL_DMAMC_DEDICATED BIT(5)
#define SUN4I_CTL_LMTF BIT(6)
#define SUN4I_CTL_TF_RST BIT(8)
#define SUN4I_CTL_RF_RST BIT(9)
@@ -51,6 +54,8 @@
#define SUN4I_INT_STA_REG 0x10
#define SUN4I_DMA_CTL_REG 0x14
+#define SUN4I_DMA_CTL_RF_READY BIT(0)
+#define SUN4I_DMA_CTL_TF_NOT_FULL BIT(10)
#define SUN4I_WAIT_REG 0x18
@@ -130,6 +135,13 @@ static inline void sun4i_spi_fill_fifo(struct sun4i_spi *sspi, int len)
}
}
+static bool sun4i_spi_can_dma(struct spi_master *master,
+ struct spi_device *spi,
+ struct spi_transfer *tfr)
+{
+ return tfr->len >= SUN4I_FIFO_DEPTH;
+}
+
static void sun4i_spi_set_cs(struct spi_device *spi, bool enable)
{
struct sun4i_spi *sspi = spi_master_get_devdata(spi->master);
@@ -177,17 +189,20 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
struct spi_transfer *tfr)
{
struct sun4i_spi *sspi = spi_master_get_devdata(master);
+ struct dma_async_tx_descriptor *desc_tx = NULL, *desc_rx = NULL;
unsigned int mclk_rate, div, timeout;
unsigned int start, end, tx_time;
unsigned int tx_len = 0;
+ u32 reg, trigger = 0;
int ret = 0;
- u32 reg;
- /* We don't support transfer larger than the FIFO */
- if (tfr->len > SUN4I_FIFO_DEPTH)
- return -EMSGSIZE;
- if (tfr->tx_buf && tfr->len >= SUN4I_FIFO_DEPTH)
- return -EMSGSIZE;
+ if (!master->can_dma) {
+ /* We don't support transfer larger than the FIFO */
+ if (tfr->len > SUN4I_FIFO_DEPTH)
+ return -EMSGSIZE;
+ if (tfr->tx_buf && tfr->len >= SUN4I_FIFO_DEPTH)
+ return -EMSGSIZE;
+ }
reinit_completion(&sspi->done);
sspi->tx_buf = tfr->tx_buf;
@@ -277,14 +292,67 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
sun4i_spi_write(sspi, SUN4I_BURST_CNT_REG, SUN4I_BURST_CNT(tfr->len));
sun4i_spi_write(sspi, SUN4I_XMIT_CNT_REG, SUN4I_XMIT_CNT(tx_len));
- /* Fill the TX FIFO */
- /* Filling the FIFO fully causes timeout for some reason
- * at least on spi2 on A10s */
- sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1);
-
/* Enable the interrupts */
sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, SUN4I_INT_CTL_TC);
+ if (sun4i_spi_can_dma(master, spi, tfr)) {
+ dev_dbg(&sspi->master->dev, "Using DMA mode for transfer\n");
+
+ if (sspi->tx_buf) {
+ desc_tx = dmaengine_prep_slave_sg(master->dma_tx,
+ tfr->tx_sg.sgl, tfr->tx_sg.nents,
+ DMA_TO_DEVICE,
+ DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+ if (!desc_tx) {
+ dev_err(&sspi->master->dev,
+ "Couldn't prepare dma slave\n");
+ return -EIO;
+ }
+
+ trigger |= SUN4I_DMA_CTL_TF_NOT_FULL;
+
+ dmaengine_submit(desc_tx);
+ dma_async_issue_pending(master->dma_tx);
+
+ }
+
+ if (sspi->rx_buf) {
+ desc_rx = dmaengine_prep_slave_sg(master->dma_rx,
+ tfr->rx_sg.sgl, tfr->rx_sg.nents,
+ DMA_FROM_DEVICE,
+ DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+ if (!desc_rx) {
+ dev_err(&sspi->master->dev,
+ "Couldn't prepare dma slave\n");
+ return -EIO;
+ }
+
+ trigger |= SUN4I_DMA_CTL_RF_READY;
+
+ dmaengine_submit(desc_rx);
+ dma_async_issue_pending(master->dma_rx);
+ }
+
+ /* Enable Dedicated DMA requests */
+ reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
+ reg |= SUN4I_CTL_DMAMC_DEDICATED;
+ sun4i_spi_write(sspi, SUN4I_CTL_REG, reg);
+ sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, trigger);
+ } else {
+ dev_dbg(&sspi->master->dev, "Using PIO mode for transfer\n");
+
+ /* Disable DMA requests */
+ reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
+ sun4i_spi_write(sspi, SUN4I_CTL_REG,
+ reg & ~SUN4I_CTL_DMAMC_DEDICATED);
+ sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, 0);
+
+ /* Fill the TX FIFO */
+ /* Filling the FIFO fully causes timeout for some reason
+ * at least on spi2 on A10s */
+ sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1);
+ }
+
/* Start the transfer */
reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
@@ -304,7 +372,16 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
goto out;
}
- sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
+ if (sun4i_spi_can_dma(master, spi, tfr) && desc_rx) {
+ /* The receive transfer should be the last one to finish */
+ /* This seems to work OK.
+ * TODO: look for some drivers which do something
+ * more sensible here. */
+ dma_wait_for_async_tx(desc_rx);
+ } else {
+ /* This should be noop with tx only dma */
+ sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
+ }
out:
sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
@@ -369,6 +446,7 @@ static int sun4i_spi_runtime_suspend(struct device *dev)
static int sun4i_spi_probe(struct platform_device *pdev)
{
+ struct dma_slave_config dma_sconfig;
struct spi_master *master;
struct sun4i_spi *sspi;
struct resource *res;
@@ -404,6 +482,7 @@ static int sun4i_spi_probe(struct platform_device *pdev)
goto err_free_master;
}
+ init_completion(&sspi->done);
sspi->master = master;
master->max_speed_hz = 100*1000*1000;
master->min_speed_hz = 3*1000;
@@ -430,8 +509,55 @@ static int sun4i_spi_probe(struct platform_device *pdev)
goto err_free_master;
}
- init_completion(&sspi->done);
+ master->dma_tx = dma_request_slave_channel_reason(&pdev->dev, "tx");
+ if (IS_ERR(master->dma_tx)) {
+ dev_err(&pdev->dev, "Unable to acquire DMA channel TX\n");
+ ret = PTR_ERR(master->dma_tx);
+ master->dma_tx = NULL;
+ goto wakeup;
+ }
+
+ dma_sconfig.direction = DMA_MEM_TO_DEV;
+ dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconfig.dst_addr = res->start + SUN4I_TXDATA_REG;
+ dma_sconfig.src_maxburst = 1;
+ dma_sconfig.dst_maxburst = 1;
+ ret = dmaengine_slave_config(master->dma_tx, &dma_sconfig);
+ if (ret) {
+ dev_err(&pdev->dev, "Unable to configure TX DMA slave\n");
+ goto err_tx_dma_release;
+ }
+
+ master->dma_rx = dma_request_slave_channel_reason(&pdev->dev, "rx");
+ if (IS_ERR(master->dma_rx)) {
+ dev_err(&pdev->dev, "Unable to acquire DMA channel RX\n");
+ ret = PTR_ERR(master->dma_rx);
+ goto err_tx_dma_release;
+ }
+
+ dma_sconfig.direction = DMA_DEV_TO_MEM;
+ dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconfig.src_addr = res->start + SUN4I_RXDATA_REG;
+ dma_sconfig.src_maxburst = 1;
+ dma_sconfig.dst_maxburst = 1;
+
+ ret = dmaengine_slave_config(master->dma_rx, &dma_sconfig);
+ if (ret) {
+ dev_err(&pdev->dev, "Unable to configure RX DMA slave\n");
+ goto err_rx_dma_release;
+ }
+
+ /* This is a bit dodgy. If you set can_dma then map_msg in spi.c
+ * apparently dereferences your dma channels if non-NULL even if your
+ * can_dma never returns true (and crashes if the channel is an error
+ * pointer). So just don't set can_dma unless both channels are valid.
+ */
+ master->can_dma = sun4i_spi_can_dma;
+ master->max_transfer_size = NULL; /* no known limit */
+wakeup:
/*
* This wake-up/shutdown pattern is to be able to have the
* device woken up, even if runtime_pm is disabled
@@ -454,18 +580,37 @@ static int sun4i_spi_probe(struct platform_device *pdev)
return 0;
+err_rx_dma_release:
+ dma_release_channel(master->dma_rx);
+err_tx_dma_release:
+ dma_release_channel(master->dma_tx);
+ master->dma_tx = NULL;
+ master->dma_rx = NULL;
+ goto wakeup;
+
err_pm_disable:
pm_runtime_disable(&pdev->dev);
sun4i_spi_runtime_suspend(&pdev->dev);
err_free_master:
+ if (master->can_dma) {
+ dma_release_channel(master->dma_rx);
+ dma_release_channel(master->dma_tx);
+ }
spi_master_put(master);
return ret;
}
static int sun4i_spi_remove(struct platform_device *pdev)
{
+ struct spi_master *master = platform_get_drvdata(pdev);
+
pm_runtime_disable(&pdev->dev);
+ if (master->can_dma) {
+ dma_release_channel(master->dma_rx);
+ dma_release_channel(master->dma_tx);
+ }
+
return 0;
}
--
2.8.1
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply related [flat|nested] 95+ messages in thread* [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-05-26 19:25 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-26 19:25 UTC (permalink / raw)
To: linux-arm-kernel
From: Emilio L?pez <emilio@elopez.com.ar>
This patch adds support for 64 byte or bigger transfers on the
sun4i SPI controller. Said transfers will be performed via DMA.
Signed-off-by: Emilio L?pez <emilio@elopez.com.ar>
Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
v2:
- fallback to previous behaviour when DMA initialization fails
+ this has the problem that when the driver happens to load before the dma
driver it will not use dma - can be addressed with a module parameter
+ the issue with dma_async_tx_descriptor *desc_tx = NULL, *desc_rx = NULL; ...
dma_wait_for_async_tx(desc_rx); remains unaddressed
---
drivers/spi/spi-sun4i.c | 171 ++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 158 insertions(+), 13 deletions(-)
diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index e1a75dd6..ed2269c 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -14,6 +14,8 @@
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/device.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/module.h>
@@ -34,6 +36,7 @@
#define SUN4I_CTL_CPHA BIT(2)
#define SUN4I_CTL_CPOL BIT(3)
#define SUN4I_CTL_CS_ACTIVE_LOW BIT(4)
+#define SUN4I_CTL_DMAMC_DEDICATED BIT(5)
#define SUN4I_CTL_LMTF BIT(6)
#define SUN4I_CTL_TF_RST BIT(8)
#define SUN4I_CTL_RF_RST BIT(9)
@@ -51,6 +54,8 @@
#define SUN4I_INT_STA_REG 0x10
#define SUN4I_DMA_CTL_REG 0x14
+#define SUN4I_DMA_CTL_RF_READY BIT(0)
+#define SUN4I_DMA_CTL_TF_NOT_FULL BIT(10)
#define SUN4I_WAIT_REG 0x18
@@ -130,6 +135,13 @@ static inline void sun4i_spi_fill_fifo(struct sun4i_spi *sspi, int len)
}
}
+static bool sun4i_spi_can_dma(struct spi_master *master,
+ struct spi_device *spi,
+ struct spi_transfer *tfr)
+{
+ return tfr->len >= SUN4I_FIFO_DEPTH;
+}
+
static void sun4i_spi_set_cs(struct spi_device *spi, bool enable)
{
struct sun4i_spi *sspi = spi_master_get_devdata(spi->master);
@@ -177,17 +189,20 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
struct spi_transfer *tfr)
{
struct sun4i_spi *sspi = spi_master_get_devdata(master);
+ struct dma_async_tx_descriptor *desc_tx = NULL, *desc_rx = NULL;
unsigned int mclk_rate, div, timeout;
unsigned int start, end, tx_time;
unsigned int tx_len = 0;
+ u32 reg, trigger = 0;
int ret = 0;
- u32 reg;
- /* We don't support transfer larger than the FIFO */
- if (tfr->len > SUN4I_FIFO_DEPTH)
- return -EMSGSIZE;
- if (tfr->tx_buf && tfr->len >= SUN4I_FIFO_DEPTH)
- return -EMSGSIZE;
+ if (!master->can_dma) {
+ /* We don't support transfer larger than the FIFO */
+ if (tfr->len > SUN4I_FIFO_DEPTH)
+ return -EMSGSIZE;
+ if (tfr->tx_buf && tfr->len >= SUN4I_FIFO_DEPTH)
+ return -EMSGSIZE;
+ }
reinit_completion(&sspi->done);
sspi->tx_buf = tfr->tx_buf;
@@ -277,14 +292,67 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
sun4i_spi_write(sspi, SUN4I_BURST_CNT_REG, SUN4I_BURST_CNT(tfr->len));
sun4i_spi_write(sspi, SUN4I_XMIT_CNT_REG, SUN4I_XMIT_CNT(tx_len));
- /* Fill the TX FIFO */
- /* Filling the FIFO fully causes timeout for some reason
- * at least on spi2 on A10s */
- sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1);
-
/* Enable the interrupts */
sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, SUN4I_INT_CTL_TC);
+ if (sun4i_spi_can_dma(master, spi, tfr)) {
+ dev_dbg(&sspi->master->dev, "Using DMA mode for transfer\n");
+
+ if (sspi->tx_buf) {
+ desc_tx = dmaengine_prep_slave_sg(master->dma_tx,
+ tfr->tx_sg.sgl, tfr->tx_sg.nents,
+ DMA_TO_DEVICE,
+ DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+ if (!desc_tx) {
+ dev_err(&sspi->master->dev,
+ "Couldn't prepare dma slave\n");
+ return -EIO;
+ }
+
+ trigger |= SUN4I_DMA_CTL_TF_NOT_FULL;
+
+ dmaengine_submit(desc_tx);
+ dma_async_issue_pending(master->dma_tx);
+
+ }
+
+ if (sspi->rx_buf) {
+ desc_rx = dmaengine_prep_slave_sg(master->dma_rx,
+ tfr->rx_sg.sgl, tfr->rx_sg.nents,
+ DMA_FROM_DEVICE,
+ DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+ if (!desc_rx) {
+ dev_err(&sspi->master->dev,
+ "Couldn't prepare dma slave\n");
+ return -EIO;
+ }
+
+ trigger |= SUN4I_DMA_CTL_RF_READY;
+
+ dmaengine_submit(desc_rx);
+ dma_async_issue_pending(master->dma_rx);
+ }
+
+ /* Enable Dedicated DMA requests */
+ reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
+ reg |= SUN4I_CTL_DMAMC_DEDICATED;
+ sun4i_spi_write(sspi, SUN4I_CTL_REG, reg);
+ sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, trigger);
+ } else {
+ dev_dbg(&sspi->master->dev, "Using PIO mode for transfer\n");
+
+ /* Disable DMA requests */
+ reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
+ sun4i_spi_write(sspi, SUN4I_CTL_REG,
+ reg & ~SUN4I_CTL_DMAMC_DEDICATED);
+ sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, 0);
+
+ /* Fill the TX FIFO */
+ /* Filling the FIFO fully causes timeout for some reason
+ * at least on spi2 on A10s */
+ sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1);
+ }
+
/* Start the transfer */
reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
@@ -304,7 +372,16 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
goto out;
}
- sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
+ if (sun4i_spi_can_dma(master, spi, tfr) && desc_rx) {
+ /* The receive transfer should be the last one to finish */
+ /* This seems to work OK.
+ * TODO: look for some drivers which do something
+ * more sensible here. */
+ dma_wait_for_async_tx(desc_rx);
+ } else {
+ /* This should be noop with tx only dma */
+ sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
+ }
out:
sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
@@ -369,6 +446,7 @@ static int sun4i_spi_runtime_suspend(struct device *dev)
static int sun4i_spi_probe(struct platform_device *pdev)
{
+ struct dma_slave_config dma_sconfig;
struct spi_master *master;
struct sun4i_spi *sspi;
struct resource *res;
@@ -404,6 +482,7 @@ static int sun4i_spi_probe(struct platform_device *pdev)
goto err_free_master;
}
+ init_completion(&sspi->done);
sspi->master = master;
master->max_speed_hz = 100*1000*1000;
master->min_speed_hz = 3*1000;
@@ -430,8 +509,55 @@ static int sun4i_spi_probe(struct platform_device *pdev)
goto err_free_master;
}
- init_completion(&sspi->done);
+ master->dma_tx = dma_request_slave_channel_reason(&pdev->dev, "tx");
+ if (IS_ERR(master->dma_tx)) {
+ dev_err(&pdev->dev, "Unable to acquire DMA channel TX\n");
+ ret = PTR_ERR(master->dma_tx);
+ master->dma_tx = NULL;
+ goto wakeup;
+ }
+
+ dma_sconfig.direction = DMA_MEM_TO_DEV;
+ dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconfig.dst_addr = res->start + SUN4I_TXDATA_REG;
+ dma_sconfig.src_maxburst = 1;
+ dma_sconfig.dst_maxburst = 1;
+ ret = dmaengine_slave_config(master->dma_tx, &dma_sconfig);
+ if (ret) {
+ dev_err(&pdev->dev, "Unable to configure TX DMA slave\n");
+ goto err_tx_dma_release;
+ }
+
+ master->dma_rx = dma_request_slave_channel_reason(&pdev->dev, "rx");
+ if (IS_ERR(master->dma_rx)) {
+ dev_err(&pdev->dev, "Unable to acquire DMA channel RX\n");
+ ret = PTR_ERR(master->dma_rx);
+ goto err_tx_dma_release;
+ }
+
+ dma_sconfig.direction = DMA_DEV_TO_MEM;
+ dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconfig.src_addr = res->start + SUN4I_RXDATA_REG;
+ dma_sconfig.src_maxburst = 1;
+ dma_sconfig.dst_maxburst = 1;
+
+ ret = dmaengine_slave_config(master->dma_rx, &dma_sconfig);
+ if (ret) {
+ dev_err(&pdev->dev, "Unable to configure RX DMA slave\n");
+ goto err_rx_dma_release;
+ }
+
+ /* This is a bit dodgy. If you set can_dma then map_msg in spi.c
+ * apparently dereferences your dma channels if non-NULL even if your
+ * can_dma never returns true (and crashes if the channel is an error
+ * pointer). So just don't set can_dma unless both channels are valid.
+ */
+ master->can_dma = sun4i_spi_can_dma;
+ master->max_transfer_size = NULL; /* no known limit */
+wakeup:
/*
* This wake-up/shutdown pattern is to be able to have the
* device woken up, even if runtime_pm is disabled
@@ -454,18 +580,37 @@ static int sun4i_spi_probe(struct platform_device *pdev)
return 0;
+err_rx_dma_release:
+ dma_release_channel(master->dma_rx);
+err_tx_dma_release:
+ dma_release_channel(master->dma_tx);
+ master->dma_tx = NULL;
+ master->dma_rx = NULL;
+ goto wakeup;
+
err_pm_disable:
pm_runtime_disable(&pdev->dev);
sun4i_spi_runtime_suspend(&pdev->dev);
err_free_master:
+ if (master->can_dma) {
+ dma_release_channel(master->dma_rx);
+ dma_release_channel(master->dma_tx);
+ }
spi_master_put(master);
return ret;
}
static int sun4i_spi_remove(struct platform_device *pdev)
{
+ struct spi_master *master = platform_get_drvdata(pdev);
+
pm_runtime_disable(&pdev->dev);
+ if (master->can_dma) {
+ dma_release_channel(master->dma_rx);
+ dma_release_channel(master->dma_tx);
+ }
+
return 0;
}
--
2.8.1
^ permalink raw reply related [flat|nested] 95+ messages in thread[parent not found: <ba0d6eb37cc4b0d2c46acbf9fcd7d644b3545ce8.1464130597.git.hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
2016-05-26 19:25 ` Michal Suchanek
(?)
@ 2016-05-30 11:26 ` Mark Brown
-1 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-05-30 11:26 UTC (permalink / raw)
To: Michal Suchanek
Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Maxime Ripard, Chen-Yu Tsai,
linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
[-- Attachment #1: Type: text/plain, Size: 409 bytes --]
On Thu, May 26, 2016 at 07:25:25PM -0000, Michal Suchanek wrote:
> - fallback to previous behaviour when DMA initialization fails
>
> + this has the problem that when the driver happens to load before the dma
> driver it will not use dma - can be addressed with a module parameter
No, you should pay attention to the error you are getting and let probe
deferral happen if that's the error you get.
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-05-30 11:26 ` Mark Brown
0 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-05-30 11:26 UTC (permalink / raw)
To: Michal Suchanek
Cc: linux-sunxi, Maxime Ripard, Chen-Yu Tsai, linux-spi,
linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 419 bytes --]
On Thu, May 26, 2016 at 07:25:25PM -0000, Michal Suchanek wrote:
> - fallback to previous behaviour when DMA initialization fails
>
> + this has the problem that when the driver happens to load before the dma
> driver it will not use dma - can be addressed with a module parameter
No, you should pay attention to the error you are getting and let probe
deferral happen if that's the error you get.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-05-30 11:26 ` Mark Brown
0 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-05-30 11:26 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, May 26, 2016 at 07:25:25PM -0000, Michal Suchanek wrote:
> - fallback to previous behaviour when DMA initialization fails
>
> + this has the problem that when the driver happens to load before the dma
> driver it will not use dma - can be addressed with a module parameter
No, you should pay attention to the error you are getting and let probe
deferral happen if that's the error you get.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160530/076d2e84/attachment.sig>
^ permalink raw reply [flat|nested] 95+ messages in thread
[parent not found: <20160530112610.GJ29837-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>]
* Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
2016-05-30 11:26 ` Mark Brown
(?)
@ 2016-05-30 12:11 ` Geert Uytterhoeven
-1 siblings, 0 replies; 95+ messages in thread
From: Geert Uytterhoeven @ 2016-05-30 12:11 UTC (permalink / raw)
To: Mark Brown
Cc: Michal Suchanek, linux-sunxi, Maxime Ripard, Chen-Yu Tsai,
linux-spi,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Hi Mark,
On Mon, May 30, 2016 at 1:26 PM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Thu, May 26, 2016 at 07:25:25PM -0000, Michal Suchanek wrote:
>> - fallback to previous behaviour when DMA initialization fails
>>
>> + this has the problem that when the driver happens to load before the dma
>> driver it will not use dma - can be addressed with a module parameter
>
> No, you should pay attention to the error you are getting and let probe
> deferral happen if that's the error you get.
Unfortunately DMA is an optional feature.
There's no way to distinguish between -EPROBE_DEFER due to the SPI master
driver being probed before the DMA engine driver, and -EPROBE_DEFER due to
support for the DMA engine not having been compiled in.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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
^ permalink raw reply [flat|nested] 95+ messages in thread* Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-05-30 12:11 ` Geert Uytterhoeven
0 siblings, 0 replies; 95+ messages in thread
From: Geert Uytterhoeven @ 2016-05-30 12:11 UTC (permalink / raw)
To: Mark Brown
Cc: Michal Suchanek, linux-sunxi, Maxime Ripard, Chen-Yu Tsai,
linux-spi, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Hi Mark,
On Mon, May 30, 2016 at 1:26 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, May 26, 2016 at 07:25:25PM -0000, Michal Suchanek wrote:
>> - fallback to previous behaviour when DMA initialization fails
>>
>> + this has the problem that when the driver happens to load before the dma
>> driver it will not use dma - can be addressed with a module parameter
>
> No, you should pay attention to the error you are getting and let probe
> deferral happen if that's the error you get.
Unfortunately DMA is an optional feature.
There's no way to distinguish between -EPROBE_DEFER due to the SPI master
driver being probed before the DMA engine driver, and -EPROBE_DEFER due to
support for the DMA engine not having been compiled in.
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
^ permalink raw reply [flat|nested] 95+ messages in thread* [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-05-30 12:11 ` Geert Uytterhoeven
0 siblings, 0 replies; 95+ messages in thread
From: Geert Uytterhoeven @ 2016-05-30 12:11 UTC (permalink / raw)
To: linux-arm-kernel
Hi Mark,
On Mon, May 30, 2016 at 1:26 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, May 26, 2016 at 07:25:25PM -0000, Michal Suchanek wrote:
>> - fallback to previous behaviour when DMA initialization fails
>>
>> + this has the problem that when the driver happens to load before the dma
>> driver it will not use dma - can be addressed with a module parameter
>
> No, you should pay attention to the error you are getting and let probe
> deferral happen if that's the error you get.
Unfortunately DMA is an optional feature.
There's no way to distinguish between -EPROBE_DEFER due to the SPI master
driver being probed before the DMA engine driver, and -EPROBE_DEFER due to
support for the DMA engine not having been compiled in.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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
^ permalink raw reply [flat|nested] 95+ messages in thread[parent not found: <CAMuHMdVThvtqgFnyqoqypwwf1d81uu9NBo6-heTZBcvBxRevNw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
2016-05-30 12:11 ` Geert Uytterhoeven
(?)
@ 2016-05-30 15:03 ` Mark Brown
-1 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-05-30 15:03 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Michal Suchanek, linux-sunxi, Maxime Ripard, Chen-Yu Tsai,
linux-spi,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
[-- Attachment #1: Type: text/plain, Size: 1393 bytes --]
On Mon, May 30, 2016 at 02:11:51PM +0200, Geert Uytterhoeven wrote:
> On Mon, May 30, 2016 at 1:26 PM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Thu, May 26, 2016 at 07:25:25PM -0000, Michal Suchanek wrote:
> >> - fallback to previous behaviour when DMA initialization fails
> >>
> >> + this has the problem that when the driver happens to load before the dma
> >> driver it will not use dma - can be addressed with a module parameter
> > No, you should pay attention to the error you are getting and let probe
> > deferral happen if that's the error you get.
> Unfortunately DMA is an optional feature.
> There's no way to distinguish between -EPROBE_DEFER due to the SPI master
> driver being probed before the DMA engine driver, and -EPROBE_DEFER due to
> support for the DMA engine not having been compiled in.
I really don't think it's worth caring too much about cases where the
DMA driver hasn't been compiled in, it's not like SPI is the only thing
that's going to be using it. I really think it's better to defer the
problem - not getting DMA (or worse, only getting DMA on some boots) is
not great and if people are optimising on that level my feeling is that
they're probably going to be OK with customizing DT to match. Ideally
we would have something a bit nicer than deferred probe but right now
this seems the more helpful option.
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-05-30 15:03 ` Mark Brown
0 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-05-30 15:03 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Michal Suchanek, linux-sunxi, Maxime Ripard, Chen-Yu Tsai,
linux-spi, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 1365 bytes --]
On Mon, May 30, 2016 at 02:11:51PM +0200, Geert Uytterhoeven wrote:
> On Mon, May 30, 2016 at 1:26 PM, Mark Brown <broonie@kernel.org> wrote:
> > On Thu, May 26, 2016 at 07:25:25PM -0000, Michal Suchanek wrote:
> >> - fallback to previous behaviour when DMA initialization fails
> >>
> >> + this has the problem that when the driver happens to load before the dma
> >> driver it will not use dma - can be addressed with a module parameter
> > No, you should pay attention to the error you are getting and let probe
> > deferral happen if that's the error you get.
> Unfortunately DMA is an optional feature.
> There's no way to distinguish between -EPROBE_DEFER due to the SPI master
> driver being probed before the DMA engine driver, and -EPROBE_DEFER due to
> support for the DMA engine not having been compiled in.
I really don't think it's worth caring too much about cases where the
DMA driver hasn't been compiled in, it's not like SPI is the only thing
that's going to be using it. I really think it's better to defer the
problem - not getting DMA (or worse, only getting DMA on some boots) is
not great and if people are optimising on that level my feeling is that
they're probably going to be OK with customizing DT to match. Ideally
we would have something a bit nicer than deferred probe but right now
this seems the more helpful option.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-05-30 15:03 ` Mark Brown
0 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-05-30 15:03 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, May 30, 2016 at 02:11:51PM +0200, Geert Uytterhoeven wrote:
> On Mon, May 30, 2016 at 1:26 PM, Mark Brown <broonie@kernel.org> wrote:
> > On Thu, May 26, 2016 at 07:25:25PM -0000, Michal Suchanek wrote:
> >> - fallback to previous behaviour when DMA initialization fails
> >>
> >> + this has the problem that when the driver happens to load before the dma
> >> driver it will not use dma - can be addressed with a module parameter
> > No, you should pay attention to the error you are getting and let probe
> > deferral happen if that's the error you get.
> Unfortunately DMA is an optional feature.
> There's no way to distinguish between -EPROBE_DEFER due to the SPI master
> driver being probed before the DMA engine driver, and -EPROBE_DEFER due to
> support for the DMA engine not having been compiled in.
I really don't think it's worth caring too much about cases where the
DMA driver hasn't been compiled in, it's not like SPI is the only thing
that's going to be using it. I really think it's better to defer the
problem - not getting DMA (or worse, only getting DMA on some boots) is
not great and if people are optimising on that level my feeling is that
they're probably going to be OK with customizing DT to match. Ideally
we would have something a bit nicer than deferred probe but right now
this seems the more helpful option.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160530/7e494196/attachment.sig>
^ permalink raw reply [flat|nested] 95+ messages in thread
[parent not found: <20160530150343.GL29837-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>]
* Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
2016-05-30 15:03 ` Mark Brown
(?)
@ 2016-05-30 15:28 ` Michal Suchanek
-1 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-30 15:28 UTC (permalink / raw)
To: Mark Brown
Cc: Geert Uytterhoeven, linux-sunxi, Maxime Ripard, Chen-Yu Tsai,
linux-spi,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Hello,
On 30 May 2016 at 17:03, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, May 30, 2016 at 02:11:51PM +0200, Geert Uytterhoeven wrote:
>> On Mon, May 30, 2016 at 1:26 PM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> > On Thu, May 26, 2016 at 07:25:25PM -0000, Michal Suchanek wrote:
>> >> - fallback to previous behaviour when DMA initialization fails
>> >>
>> >> + this has the problem that when the driver happens to load before the dma
>> >> driver it will not use dma - can be addressed with a module parameter
>
>> > No, you should pay attention to the error you are getting and let probe
>> > deferral happen if that's the error you get.
>
>> Unfortunately DMA is an optional feature.
>
>> There's no way to distinguish between -EPROBE_DEFER due to the SPI master
>> driver being probed before the DMA engine driver, and -EPROBE_DEFER due to
>> support for the DMA engine not having been compiled in.
>
> I really don't think it's worth caring too much about cases where the
> DMA driver hasn't been compiled in, it's not like SPI is the only thing
> that's going to be using it. I really think it's better to defer the
> problem - not getting DMA (or worse, only getting DMA on some boots) is
> not great and if people are optimising on that level my feeling is that
> they're probably going to be OK with customizing DT to match. Ideally
> we would have something a bit nicer than deferred probe but right now
> this seems the more helpful option.
It's what the driver did to start with and it was requested to fall
back to non-DMA in the case DMA is not available.
We get to the much discussed problem that it's never possible to tell
if a driver is available or not.
It's possible to add a parameter like require_dma which could be used
to load the driver without dma if unset. If it was set by default then
driver ordering is not important so long as dma driver is loaded
eventually. Also an informative print that such parameter exists when
probing the driver is deferred would be helpful. It would probably
create quite a bit of log spam, however. The driver can be deferred
several times during boot.
Thanks
Michal
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-05-30 15:28 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-30 15:28 UTC (permalink / raw)
To: Mark Brown
Cc: Geert Uytterhoeven, linux-sunxi, Maxime Ripard, Chen-Yu Tsai,
linux-spi, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Hello,
On 30 May 2016 at 17:03, Mark Brown <broonie@kernel.org> wrote:
> On Mon, May 30, 2016 at 02:11:51PM +0200, Geert Uytterhoeven wrote:
>> On Mon, May 30, 2016 at 1:26 PM, Mark Brown <broonie@kernel.org> wrote:
>> > On Thu, May 26, 2016 at 07:25:25PM -0000, Michal Suchanek wrote:
>> >> - fallback to previous behaviour when DMA initialization fails
>> >>
>> >> + this has the problem that when the driver happens to load before the dma
>> >> driver it will not use dma - can be addressed with a module parameter
>
>> > No, you should pay attention to the error you are getting and let probe
>> > deferral happen if that's the error you get.
>
>> Unfortunately DMA is an optional feature.
>
>> There's no way to distinguish between -EPROBE_DEFER due to the SPI master
>> driver being probed before the DMA engine driver, and -EPROBE_DEFER due to
>> support for the DMA engine not having been compiled in.
>
> I really don't think it's worth caring too much about cases where the
> DMA driver hasn't been compiled in, it's not like SPI is the only thing
> that's going to be using it. I really think it's better to defer the
> problem - not getting DMA (or worse, only getting DMA on some boots) is
> not great and if people are optimising on that level my feeling is that
> they're probably going to be OK with customizing DT to match. Ideally
> we would have something a bit nicer than deferred probe but right now
> this seems the more helpful option.
It's what the driver did to start with and it was requested to fall
back to non-DMA in the case DMA is not available.
We get to the much discussed problem that it's never possible to tell
if a driver is available or not.
It's possible to add a parameter like require_dma which could be used
to load the driver without dma if unset. If it was set by default then
driver ordering is not important so long as dma driver is loaded
eventually. Also an informative print that such parameter exists when
probing the driver is deferred would be helpful. It would probably
create quite a bit of log spam, however. The driver can be deferred
several times during boot.
Thanks
Michal
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-05-30 15:28 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-30 15:28 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
On 30 May 2016 at 17:03, Mark Brown <broonie@kernel.org> wrote:
> On Mon, May 30, 2016 at 02:11:51PM +0200, Geert Uytterhoeven wrote:
>> On Mon, May 30, 2016 at 1:26 PM, Mark Brown <broonie@kernel.org> wrote:
>> > On Thu, May 26, 2016 at 07:25:25PM -0000, Michal Suchanek wrote:
>> >> - fallback to previous behaviour when DMA initialization fails
>> >>
>> >> + this has the problem that when the driver happens to load before the dma
>> >> driver it will not use dma - can be addressed with a module parameter
>
>> > No, you should pay attention to the error you are getting and let probe
>> > deferral happen if that's the error you get.
>
>> Unfortunately DMA is an optional feature.
>
>> There's no way to distinguish between -EPROBE_DEFER due to the SPI master
>> driver being probed before the DMA engine driver, and -EPROBE_DEFER due to
>> support for the DMA engine not having been compiled in.
>
> I really don't think it's worth caring too much about cases where the
> DMA driver hasn't been compiled in, it's not like SPI is the only thing
> that's going to be using it. I really think it's better to defer the
> problem - not getting DMA (or worse, only getting DMA on some boots) is
> not great and if people are optimising on that level my feeling is that
> they're probably going to be OK with customizing DT to match. Ideally
> we would have something a bit nicer than deferred probe but right now
> this seems the more helpful option.
It's what the driver did to start with and it was requested to fall
back to non-DMA in the case DMA is not available.
We get to the much discussed problem that it's never possible to tell
if a driver is available or not.
It's possible to add a parameter like require_dma which could be used
to load the driver without dma if unset. If it was set by default then
driver ordering is not important so long as dma driver is loaded
eventually. Also an informative print that such parameter exists when
probing the driver is deferred would be helpful. It would probably
create quite a bit of log spam, however. The driver can be deferred
several times during boot.
Thanks
Michal
^ permalink raw reply [flat|nested] 95+ messages in thread
[parent not found: <CAOMqctQb-YYprA7H5qCcYo2hxXiUhjW2z3JbZ3-FhbfcjFzBpw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
2016-05-30 15:28 ` Michal Suchanek
(?)
@ 2016-05-30 15:50 ` Mark Brown
-1 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-05-30 15:50 UTC (permalink / raw)
To: Michal Suchanek
Cc: Geert Uytterhoeven, linux-sunxi, Maxime Ripard, Chen-Yu Tsai,
linux-spi,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]
On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
> On 30 May 2016 at 17:03, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > I really don't think it's worth caring too much about cases where the
> > DMA driver hasn't been compiled in, it's not like SPI is the only thing
> It's what the driver did to start with and it was requested to fall
> back to non-DMA in the case DMA is not available.
Why? I really can't see any sensible use case for this that doesn't
have a better solution available.
> It's possible to add a parameter like require_dma which could be used
> to load the driver without dma if unset. If it was set by default then
> driver ordering is not important so long as dma driver is loaded
> eventually. Also an informative print that such parameter exists when
> probing the driver is deferred would be helpful. It would probably
> create quite a bit of log spam, however. The driver can be deferred
> several times during boot.
That seems fairly hacky, if we were going to do anything like that it
should be the other way around so that we default to trying to use
resources and even then it seems like something that should be handled
at a framework level rather than having random options in individual
drivers to ignore things. Having things behave inconsistently between
different drivers is going to lead to a worse user experience and if
this is a good idea for one driver it seems like it'd be a good idea for
all of them.
But really
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-05-30 15:50 ` Mark Brown
0 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-05-30 15:50 UTC (permalink / raw)
To: Michal Suchanek
Cc: Geert Uytterhoeven, linux-sunxi, Maxime Ripard, Chen-Yu Tsai,
linux-spi, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 1509 bytes --]
On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
> On 30 May 2016 at 17:03, Mark Brown <broonie@kernel.org> wrote:
> > I really don't think it's worth caring too much about cases where the
> > DMA driver hasn't been compiled in, it's not like SPI is the only thing
> It's what the driver did to start with and it was requested to fall
> back to non-DMA in the case DMA is not available.
Why? I really can't see any sensible use case for this that doesn't
have a better solution available.
> It's possible to add a parameter like require_dma which could be used
> to load the driver without dma if unset. If it was set by default then
> driver ordering is not important so long as dma driver is loaded
> eventually. Also an informative print that such parameter exists when
> probing the driver is deferred would be helpful. It would probably
> create quite a bit of log spam, however. The driver can be deferred
> several times during boot.
That seems fairly hacky, if we were going to do anything like that it
should be the other way around so that we default to trying to use
resources and even then it seems like something that should be handled
at a framework level rather than having random options in individual
drivers to ignore things. Having things behave inconsistently between
different drivers is going to lead to a worse user experience and if
this is a good idea for one driver it seems like it'd be a good idea for
all of them.
But really
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-05-30 15:50 ` Mark Brown
0 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-05-30 15:50 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
> On 30 May 2016 at 17:03, Mark Brown <broonie@kernel.org> wrote:
> > I really don't think it's worth caring too much about cases where the
> > DMA driver hasn't been compiled in, it's not like SPI is the only thing
> It's what the driver did to start with and it was requested to fall
> back to non-DMA in the case DMA is not available.
Why? I really can't see any sensible use case for this that doesn't
have a better solution available.
> It's possible to add a parameter like require_dma which could be used
> to load the driver without dma if unset. If it was set by default then
> driver ordering is not important so long as dma driver is loaded
> eventually. Also an informative print that such parameter exists when
> probing the driver is deferred would be helpful. It would probably
> create quite a bit of log spam, however. The driver can be deferred
> several times during boot.
That seems fairly hacky, if we were going to do anything like that it
should be the other way around so that we default to trying to use
resources and even then it seems like something that should be handled
at a framework level rather than having random options in individual
drivers to ignore things. Having things behave inconsistently between
different drivers is going to lead to a worse user experience and if
this is a good idea for one driver it seems like it'd be a good idea for
all of them.
But really
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160530/4ad69a21/attachment.sig>
^ permalink raw reply [flat|nested] 95+ messages in thread
[parent not found: <20160530155016.GO29837-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>]
* Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
2016-05-30 15:50 ` Mark Brown
(?)
@ 2016-05-31 10:44 ` Michal Suchanek
-1 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-31 10:44 UTC (permalink / raw)
To: Mark Brown
Cc: Geert Uytterhoeven, linux-sunxi, Maxime Ripard, Chen-Yu Tsai,
linux-spi,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On 30 May 2016 at 17:50, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
>> On 30 May 2016 at 17:03, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
>> > I really don't think it's worth caring too much about cases where the
>> > DMA driver hasn't been compiled in, it's not like SPI is the only thing
>
>> It's what the driver did to start with and it was requested to fall
>> back to non-DMA in the case DMA is not available.
>
> Why? I really can't see any sensible use case for this that doesn't
> have a better solution available.
Of course, the solution is to compile in the DMA driver.
It's been argued that some drivers which use only short transfers will
just work.
>
>> It's possible to add a parameter like require_dma which could be used
>> to load the driver without dma if unset. If it was set by default then
>> driver ordering is not important so long as dma driver is loaded
>> eventually. Also an informative print that such parameter exists when
>> probing the driver is deferred would be helpful. It would probably
>> create quite a bit of log spam, however. The driver can be deferred
>> several times during boot.
>
> That seems fairly hacky, if we were going to do anything like that it
> should be the other way around so that we default to trying to use
> resources and even then it seems like something that should be handled
> at a framework level rather than having random options in individual
> drivers to ignore things. Having things behave inconsistently between
> different drivers is going to lead to a worse user experience and if
> this is a good idea for one driver it seems like it'd be a good idea for
> all of them.
Hacky but doable if desirable. It's awesome for testing SPI transfer
fragmentation with different drivers ;-)
The previous discussion of this driver is here
http://thread.gmane.org/gmane.linux.kernel/2162327
Thanks
Michal
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-05-31 10:44 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-31 10:44 UTC (permalink / raw)
To: Mark Brown
Cc: Geert Uytterhoeven, linux-sunxi, Maxime Ripard, Chen-Yu Tsai,
linux-spi, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
On 30 May 2016 at 17:50, Mark Brown <broonie@kernel.org> wrote:
> On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
>> On 30 May 2016 at 17:03, Mark Brown <broonie@kernel.org> wrote:
>
>> > I really don't think it's worth caring too much about cases where the
>> > DMA driver hasn't been compiled in, it's not like SPI is the only thing
>
>> It's what the driver did to start with and it was requested to fall
>> back to non-DMA in the case DMA is not available.
>
> Why? I really can't see any sensible use case for this that doesn't
> have a better solution available.
Of course, the solution is to compile in the DMA driver.
It's been argued that some drivers which use only short transfers will
just work.
>
>> It's possible to add a parameter like require_dma which could be used
>> to load the driver without dma if unset. If it was set by default then
>> driver ordering is not important so long as dma driver is loaded
>> eventually. Also an informative print that such parameter exists when
>> probing the driver is deferred would be helpful. It would probably
>> create quite a bit of log spam, however. The driver can be deferred
>> several times during boot.
>
> That seems fairly hacky, if we were going to do anything like that it
> should be the other way around so that we default to trying to use
> resources and even then it seems like something that should be handled
> at a framework level rather than having random options in individual
> drivers to ignore things. Having things behave inconsistently between
> different drivers is going to lead to a worse user experience and if
> this is a good idea for one driver it seems like it'd be a good idea for
> all of them.
Hacky but doable if desirable. It's awesome for testing SPI transfer
fragmentation with different drivers ;-)
The previous discussion of this driver is here
http://thread.gmane.org/gmane.linux.kernel/2162327
Thanks
Michal
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-05-31 10:44 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-31 10:44 UTC (permalink / raw)
To: linux-arm-kernel
On 30 May 2016 at 17:50, Mark Brown <broonie@kernel.org> wrote:
> On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
>> On 30 May 2016 at 17:03, Mark Brown <broonie@kernel.org> wrote:
>
>> > I really don't think it's worth caring too much about cases where the
>> > DMA driver hasn't been compiled in, it's not like SPI is the only thing
>
>> It's what the driver did to start with and it was requested to fall
>> back to non-DMA in the case DMA is not available.
>
> Why? I really can't see any sensible use case for this that doesn't
> have a better solution available.
Of course, the solution is to compile in the DMA driver.
It's been argued that some drivers which use only short transfers will
just work.
>
>> It's possible to add a parameter like require_dma which could be used
>> to load the driver without dma if unset. If it was set by default then
>> driver ordering is not important so long as dma driver is loaded
>> eventually. Also an informative print that such parameter exists when
>> probing the driver is deferred would be helpful. It would probably
>> create quite a bit of log spam, however. The driver can be deferred
>> several times during boot.
>
> That seems fairly hacky, if we were going to do anything like that it
> should be the other way around so that we default to trying to use
> resources and even then it seems like something that should be handled
> at a framework level rather than having random options in individual
> drivers to ignore things. Having things behave inconsistently between
> different drivers is going to lead to a worse user experience and if
> this is a good idea for one driver it seems like it'd be a good idea for
> all of them.
Hacky but doable if desirable. It's awesome for testing SPI transfer
fragmentation with different drivers ;-)
The previous discussion of this driver is here
http://thread.gmane.org/gmane.linux.kernel/2162327
Thanks
Michal
^ permalink raw reply [flat|nested] 95+ messages in thread
[parent not found: <CAOMqctQijjO2amwSmSMGp96KBRGrLb0fvXRwnybOEqvV3-HZ0g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
2016-05-31 10:44 ` Michal Suchanek
(?)
@ 2016-05-31 13:27 ` Mark Brown
-1 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-05-31 13:27 UTC (permalink / raw)
To: Michal Suchanek
Cc: Geert Uytterhoeven, linux-sunxi, Maxime Ripard, Chen-Yu Tsai,
linux-spi,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
[-- Attachment #1: Type: text/plain, Size: 765 bytes --]
On Tue, May 31, 2016 at 12:44:54PM +0200, Michal Suchanek wrote:
> On 30 May 2016 at 17:50, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
> >> It's what the driver did to start with and it was requested to fall
> >> back to non-DMA in the case DMA is not available.
> > Why? I really can't see any sensible use case for this that doesn't
> > have a better solution available.
> Of course, the solution is to compile in the DMA driver.
> It's been argued that some drivers which use only short transfers will
> just work.
With nothing else in the system that needs DMA? It's making the
performance of the system less reliable for the benefit of a very narrow
use case.
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-05-31 13:27 ` Mark Brown
0 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-05-31 13:27 UTC (permalink / raw)
To: Michal Suchanek
Cc: Geert Uytterhoeven, linux-sunxi, Maxime Ripard, Chen-Yu Tsai,
linux-spi, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 737 bytes --]
On Tue, May 31, 2016 at 12:44:54PM +0200, Michal Suchanek wrote:
> On 30 May 2016 at 17:50, Mark Brown <broonie@kernel.org> wrote:
> > On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
> >> It's what the driver did to start with and it was requested to fall
> >> back to non-DMA in the case DMA is not available.
> > Why? I really can't see any sensible use case for this that doesn't
> > have a better solution available.
> Of course, the solution is to compile in the DMA driver.
> It's been argued that some drivers which use only short transfers will
> just work.
With nothing else in the system that needs DMA? It's making the
performance of the system less reliable for the benefit of a very narrow
use case.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-05-31 13:27 ` Mark Brown
0 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-05-31 13:27 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, May 31, 2016 at 12:44:54PM +0200, Michal Suchanek wrote:
> On 30 May 2016 at 17:50, Mark Brown <broonie@kernel.org> wrote:
> > On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
> >> It's what the driver did to start with and it was requested to fall
> >> back to non-DMA in the case DMA is not available.
> > Why? I really can't see any sensible use case for this that doesn't
> > have a better solution available.
> Of course, the solution is to compile in the DMA driver.
> It's been argued that some drivers which use only short transfers will
> just work.
With nothing else in the system that needs DMA? It's making the
performance of the system less reliable for the benefit of a very narrow
use case.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160531/f521afd4/attachment.sig>
^ permalink raw reply [flat|nested] 95+ messages in thread
[parent not found: <20160531132706.GC29837-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>]
* Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
2016-05-31 13:27 ` Mark Brown
(?)
@ 2016-05-31 14:19 ` Michal Suchanek
-1 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-31 14:19 UTC (permalink / raw)
To: Mark Brown
Cc: Geert Uytterhoeven, linux-sunxi, Maxime Ripard, Chen-Yu Tsai,
linux-spi,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On 31 May 2016 at 15:27, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, May 31, 2016 at 12:44:54PM +0200, Michal Suchanek wrote:
>> On 30 May 2016 at 17:50, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> > On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
>
>> >> It's what the driver did to start with and it was requested to fall
>> >> back to non-DMA in the case DMA is not available.
>
>> > Why? I really can't see any sensible use case for this that doesn't
>> > have a better solution available.
>
>> Of course, the solution is to compile in the DMA driver.
>
>> It's been argued that some drivers which use only short transfers will
>> just work.
>
> With nothing else in the system that needs DMA? It's making the
> performance of the system less reliable for the benefit of a very narrow
> use case.
Some of the platform devices have dedicated DMA *controller* built
into the device IP so the DMA engine really is optional on many sunxi
devices. Besides SPI you definitely need the DMA engine for audio. You
probably don't need it for storage and graphics. I don't have any idea
if it's used for USB and Ethernet.
Thanks
Michal
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-05-31 14:19 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-31 14:19 UTC (permalink / raw)
To: Mark Brown
Cc: Geert Uytterhoeven, linux-sunxi, Maxime Ripard, Chen-Yu Tsai,
linux-spi, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
On 31 May 2016 at 15:27, Mark Brown <broonie@kernel.org> wrote:
> On Tue, May 31, 2016 at 12:44:54PM +0200, Michal Suchanek wrote:
>> On 30 May 2016 at 17:50, Mark Brown <broonie@kernel.org> wrote:
>> > On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
>
>> >> It's what the driver did to start with and it was requested to fall
>> >> back to non-DMA in the case DMA is not available.
>
>> > Why? I really can't see any sensible use case for this that doesn't
>> > have a better solution available.
>
>> Of course, the solution is to compile in the DMA driver.
>
>> It's been argued that some drivers which use only short transfers will
>> just work.
>
> With nothing else in the system that needs DMA? It's making the
> performance of the system less reliable for the benefit of a very narrow
> use case.
Some of the platform devices have dedicated DMA *controller* built
into the device IP so the DMA engine really is optional on many sunxi
devices. Besides SPI you definitely need the DMA engine for audio. You
probably don't need it for storage and graphics. I don't have any idea
if it's used for USB and Ethernet.
Thanks
Michal
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-05-31 14:19 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-31 14:19 UTC (permalink / raw)
To: linux-arm-kernel
On 31 May 2016 at 15:27, Mark Brown <broonie@kernel.org> wrote:
> On Tue, May 31, 2016 at 12:44:54PM +0200, Michal Suchanek wrote:
>> On 30 May 2016 at 17:50, Mark Brown <broonie@kernel.org> wrote:
>> > On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
>
>> >> It's what the driver did to start with and it was requested to fall
>> >> back to non-DMA in the case DMA is not available.
>
>> > Why? I really can't see any sensible use case for this that doesn't
>> > have a better solution available.
>
>> Of course, the solution is to compile in the DMA driver.
>
>> It's been argued that some drivers which use only short transfers will
>> just work.
>
> With nothing else in the system that needs DMA? It's making the
> performance of the system less reliable for the benefit of a very narrow
> use case.
Some of the platform devices have dedicated DMA *controller* built
into the device IP so the DMA engine really is optional on many sunxi
devices. Besides SPI you definitely need the DMA engine for audio. You
probably don't need it for storage and graphics. I don't have any idea
if it's used for USB and Ethernet.
Thanks
Michal
^ permalink raw reply [flat|nested] 95+ messages in thread
[parent not found: <CAOMqctSWnbUaOveMUhz+yOJqTOK4EhDd5jUqKoHCohfndNAjWA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
2016-05-31 14:19 ` Michal Suchanek
(?)
@ 2016-06-02 8:18 ` Maxime Ripard
-1 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-06-02 8:18 UTC (permalink / raw)
To: Michal Suchanek
Cc: Mark Brown, Geert Uytterhoeven, linux-sunxi, Chen-Yu Tsai,
linux-spi,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]
On Tue, May 31, 2016 at 04:19:28PM +0200, Michal Suchanek wrote:
> On 31 May 2016 at 15:27, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Tue, May 31, 2016 at 12:44:54PM +0200, Michal Suchanek wrote:
> >> On 30 May 2016 at 17:50, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >> > On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
> >
> >> >> It's what the driver did to start with and it was requested to fall
> >> >> back to non-DMA in the case DMA is not available.
> >
> >> > Why? I really can't see any sensible use case for this that doesn't
> >> > have a better solution available.
> >
> >> Of course, the solution is to compile in the DMA driver.
> >
> >> It's been argued that some drivers which use only short transfers will
> >> just work.
> >
> > With nothing else in the system that needs DMA? It's making the
> > performance of the system less reliable for the benefit of a very narrow
> > use case.
>
> Some of the platform devices have dedicated DMA *controller* built
> into the device IP so the DMA engine really is optional on many sunxi
> devices. Besides SPI you definitely need the DMA engine for audio. You
> probably don't need it for storage and graphics. I don't have any idea
> if it's used for USB and Ethernet.
USB and Ethernet have their own dedicated DMA engines. So currently,
the only driver that requires it is the audio codec.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-06-02 8:18 ` Maxime Ripard
0 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-06-02 8:18 UTC (permalink / raw)
To: Michal Suchanek
Cc: Mark Brown, Geert Uytterhoeven, linux-sunxi, Chen-Yu Tsai,
linux-spi, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 1528 bytes --]
On Tue, May 31, 2016 at 04:19:28PM +0200, Michal Suchanek wrote:
> On 31 May 2016 at 15:27, Mark Brown <broonie@kernel.org> wrote:
> > On Tue, May 31, 2016 at 12:44:54PM +0200, Michal Suchanek wrote:
> >> On 30 May 2016 at 17:50, Mark Brown <broonie@kernel.org> wrote:
> >> > On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
> >
> >> >> It's what the driver did to start with and it was requested to fall
> >> >> back to non-DMA in the case DMA is not available.
> >
> >> > Why? I really can't see any sensible use case for this that doesn't
> >> > have a better solution available.
> >
> >> Of course, the solution is to compile in the DMA driver.
> >
> >> It's been argued that some drivers which use only short transfers will
> >> just work.
> >
> > With nothing else in the system that needs DMA? It's making the
> > performance of the system less reliable for the benefit of a very narrow
> > use case.
>
> Some of the platform devices have dedicated DMA *controller* built
> into the device IP so the DMA engine really is optional on many sunxi
> devices. Besides SPI you definitely need the DMA engine for audio. You
> probably don't need it for storage and graphics. I don't have any idea
> if it's used for USB and Ethernet.
USB and Ethernet have their own dedicated DMA engines. So currently,
the only driver that requires it is the audio codec.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-06-02 8:18 ` Maxime Ripard
0 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-06-02 8:18 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, May 31, 2016 at 04:19:28PM +0200, Michal Suchanek wrote:
> On 31 May 2016 at 15:27, Mark Brown <broonie@kernel.org> wrote:
> > On Tue, May 31, 2016 at 12:44:54PM +0200, Michal Suchanek wrote:
> >> On 30 May 2016 at 17:50, Mark Brown <broonie@kernel.org> wrote:
> >> > On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
> >
> >> >> It's what the driver did to start with and it was requested to fall
> >> >> back to non-DMA in the case DMA is not available.
> >
> >> > Why? I really can't see any sensible use case for this that doesn't
> >> > have a better solution available.
> >
> >> Of course, the solution is to compile in the DMA driver.
> >
> >> It's been argued that some drivers which use only short transfers will
> >> just work.
> >
> > With nothing else in the system that needs DMA? It's making the
> > performance of the system less reliable for the benefit of a very narrow
> > use case.
>
> Some of the platform devices have dedicated DMA *controller* built
> into the device IP so the DMA engine really is optional on many sunxi
> devices. Besides SPI you definitely need the DMA engine for audio. You
> probably don't need it for storage and graphics. I don't have any idea
> if it's used for USB and Ethernet.
USB and Ethernet have their own dedicated DMA engines. So currently,
the only driver that requires it is the audio codec.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160602/643ba51f/attachment.sig>
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
2016-05-30 15:50 ` Mark Brown
(?)
@ 2016-06-01 18:00 ` Maxime Ripard
-1 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-06-01 18:00 UTC (permalink / raw)
To: Mark Brown
Cc: Michal Suchanek, Geert Uytterhoeven, linux-sunxi, Chen-Yu Tsai,
linux-spi,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
[-- Attachment #1: Type: text/plain, Size: 1093 bytes --]
Hi,
On Mon, May 30, 2016 at 04:50:16PM +0100, Mark Brown wrote:
> On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
> > On 30 May 2016 at 17:03, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> > > I really don't think it's worth caring too much about cases where the
> > > DMA driver hasn't been compiled in, it's not like SPI is the only thing
>
> > It's what the driver did to start with and it was requested to fall
> > back to non-DMA in the case DMA is not available.
>
> Why? I really can't see any sensible use case for this that doesn't
> have a better solution available.
SPI works just fine without DMA, which might just be considered an
(optional) optimisation.
We've been using it without DMA for years now, and it was working just
fine, and it will work even better with the other patches in this
serie. There's no reason to add a hard dependency on something that we
don't really need.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-06-01 18:00 ` Maxime Ripard
0 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-06-01 18:00 UTC (permalink / raw)
To: Mark Brown
Cc: Michal Suchanek, Geert Uytterhoeven, linux-sunxi, Chen-Yu Tsai,
linux-spi, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 1064 bytes --]
Hi,
On Mon, May 30, 2016 at 04:50:16PM +0100, Mark Brown wrote:
> On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
> > On 30 May 2016 at 17:03, Mark Brown <broonie@kernel.org> wrote:
>
> > > I really don't think it's worth caring too much about cases where the
> > > DMA driver hasn't been compiled in, it's not like SPI is the only thing
>
> > It's what the driver did to start with and it was requested to fall
> > back to non-DMA in the case DMA is not available.
>
> Why? I really can't see any sensible use case for this that doesn't
> have a better solution available.
SPI works just fine without DMA, which might just be considered an
(optional) optimisation.
We've been using it without DMA for years now, and it was working just
fine, and it will work even better with the other patches in this
serie. There's no reason to add a hard dependency on something that we
don't really need.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-06-01 18:00 ` Maxime Ripard
0 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-06-01 18:00 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Mon, May 30, 2016 at 04:50:16PM +0100, Mark Brown wrote:
> On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
> > On 30 May 2016 at 17:03, Mark Brown <broonie@kernel.org> wrote:
>
> > > I really don't think it's worth caring too much about cases where the
> > > DMA driver hasn't been compiled in, it's not like SPI is the only thing
>
> > It's what the driver did to start with and it was requested to fall
> > back to non-DMA in the case DMA is not available.
>
> Why? I really can't see any sensible use case for this that doesn't
> have a better solution available.
SPI works just fine without DMA, which might just be considered an
(optional) optimisation.
We've been using it without DMA for years now, and it was working just
fine, and it will work even better with the other patches in this
serie. There's no reason to add a hard dependency on something that we
don't really need.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160601/10a3f78e/attachment.sig>
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
2016-06-01 18:00 ` Maxime Ripard
(?)
@ 2016-06-02 4:42 ` Priit Laes
-1 siblings, 0 replies; 95+ messages in thread
From: Priit Laes @ 2016-06-02 4:42 UTC (permalink / raw)
To: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, Mark Brown
Cc: Michal Suchanek, Geert Uytterhoeven, linux-sunxi, Chen-Yu Tsai,
linux-spi,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:
> Hi,
>
> On Mon, May 30, 2016 at 04:50:16PM +0100, Mark Brown wrote:
> >
> > On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
> > >
> > > On 30 May 2016 at 17:03, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > >
> > > >
> > > > I really don't think it's worth caring too much about cases
> > > > where the
> > > > DMA driver hasn't been compiled in, it's not like SPI is the
> > > > only thing
> > >
> > > It's what the driver did to start with and it was requested to
> > > fall
> > > back to non-DMA in the case DMA is not available.
> > Why? I really can't see any sensible use case for this that
> > doesn't
> > have a better solution available.
> SPI works just fine without DMA, which might just be considered an
> (optional) optimisation.
>
> We've been using it without DMA for years now, and it was working
> just
> fine, and it will work even better with the other patches in this
> serie. There's no reason to add a hard dependency on something that
> we
> don't really need.
>
Actually it non-DMA case works fine if you don't need SPI transfers
larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.
This was addressed by this patch, but was never applied:
http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-06-02 4:42 ` Priit Laes
0 siblings, 0 replies; 95+ messages in thread
From: Priit Laes @ 2016-06-02 4:42 UTC (permalink / raw)
To: maxime.ripard, Mark Brown
Cc: Michal Suchanek, Geert Uytterhoeven, linux-sunxi, Chen-Yu Tsai,
linux-spi, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:
> Hi,
>
> On Mon, May 30, 2016 at 04:50:16PM +0100, Mark Brown wrote:
> >
> > On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
> > >
> > > On 30 May 2016 at 17:03, Mark Brown <broonie@kernel.org> wrote:
> > >
> > > >
> > > > I really don't think it's worth caring too much about cases
> > > > where the
> > > > DMA driver hasn't been compiled in, it's not like SPI is the
> > > > only thing
> > >
> > > It's what the driver did to start with and it was requested to
> > > fall
> > > back to non-DMA in the case DMA is not available.
> > Why? I really can't see any sensible use case for this that
> > doesn't
> > have a better solution available.
> SPI works just fine without DMA, which might just be considered an
> (optional) optimisation.
>
> We've been using it without DMA for years now, and it was working
> just
> fine, and it will work even better with the other patches in this
> serie. There's no reason to add a hard dependency on something that
> we
> don't really need.
>
Actually it non-DMA case works fine if you don't need SPI transfers
larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.
This was addressed by this patch, but was never applied:
http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950
^ permalink raw reply [flat|nested] 95+ messages in thread
* [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-06-02 4:42 ` Priit Laes
0 siblings, 0 replies; 95+ messages in thread
From: Priit Laes @ 2016-06-02 4:42 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:
> Hi,
>
> On Mon, May 30, 2016 at 04:50:16PM +0100, Mark Brown wrote:
> >
> > On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
> > >
> > > On 30 May 2016 at 17:03, Mark Brown <broonie@kernel.org> wrote:
> > >
> > > >
> > > > I really don't think it's worth caring too much about cases
> > > > where the
> > > > DMA driver hasn't been compiled in, it's not like SPI is the
> > > > only thing
> > >
> > > It's what the driver did to start with and it was requested to
> > > fall
> > > back to non-DMA in the case DMA is not available.
> > Why???I really can't see any sensible use case for this that
> > doesn't
> > have a better solution available.
> SPI works just fine without DMA, which might just be considered an
> (optional) optimisation.
>
> We've been using it without DMA for years now, and it was working
> just
> fine, and it will work even better with the other patches in this
> serie. There's no reason to add a hard dependency on something that
> we
> don't really need.
>
Actually it non-DMA case works fine if you don't need SPI transfers
larger than?SUN4I_FIFO_DEPTH - 1, which is 63 bytes.
This was addressed by this patch, but was never applied:
http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950
^ permalink raw reply [flat|nested] 95+ messages in thread
[parent not found: <1464842546.23881.3.camel-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org>]
* Re: Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
2016-06-02 4:42 ` Priit Laes
(?)
@ 2016-06-02 9:18 ` Mark Brown
-1 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-06-02 9:18 UTC (permalink / raw)
To: Priit Laes
Cc: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, Michal Suchanek,
Geert Uytterhoeven, linux-sunxi, Chen-Yu Tsai, linux-spi,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
[-- Attachment #1: Type: text/plain, Size: 410 bytes --]
On Thu, Jun 02, 2016 at 07:42:26AM +0300, Priit Laes wrote:
> This was addressed by this patch, but was never applied:
> http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950
I've never seen that patch before, if it was CCed to me there's a good
chance it'd have been deleted unread because you put ARM: at the start
of the subject line which says it's a patch for arch/arm. You'll need
to resubmit.
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-06-02 9:18 ` Mark Brown
0 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-06-02 9:18 UTC (permalink / raw)
To: Priit Laes
Cc: maxime.ripard, Michal Suchanek, Geert Uytterhoeven, linux-sunxi,
Chen-Yu Tsai, linux-spi, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 411 bytes --]
On Thu, Jun 02, 2016 at 07:42:26AM +0300, Priit Laes wrote:
> This was addressed by this patch, but was never applied:
> http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950
I've never seen that patch before, if it was CCed to me there's a good
chance it'd have been deleted unread because you put ARM: at the start
of the subject line which says it's a patch for arch/arm. You'll need
to resubmit.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 95+ messages in thread
* [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-06-02 9:18 ` Mark Brown
0 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-06-02 9:18 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jun 02, 2016 at 07:42:26AM +0300, Priit Laes wrote:
> This was addressed by this patch, but was never applied:
> http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950
I've never seen that patch before, if it was CCed to me there's a good
chance it'd have been deleted unread because you put ARM: at the start
of the subject line which says it's a patch for arch/arm. You'll need
to resubmit.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160602/467fd971/attachment.sig>
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
2016-06-02 4:42 ` Priit Laes
(?)
@ 2016-06-02 12:14 ` Michal Suchanek
-1 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-06-02 12:14 UTC (permalink / raw)
To: Priit Laes
Cc: Maxime Ripard, Mark Brown, Geert Uytterhoeven, linux-sunxi,
Chen-Yu Tsai, linux-spi,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On 2 June 2016 at 06:42, Priit Laes <plaes-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org> wrote:
> On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:
>> Hi,
>>
>> On Mon, May 30, 2016 at 04:50:16PM +0100, Mark Brown wrote:
>> >
>> > On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
>> > >
>> > > On 30 May 2016 at 17:03, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> > >
>> > > >
>> > > > I really don't think it's worth caring too much about cases
>> > > > where the
>> > > > DMA driver hasn't been compiled in, it's not like SPI is the
>> > > > only thing
>> > >
>> > > It's what the driver did to start with and it was requested to
>> > > fall
>> > > back to non-DMA in the case DMA is not available.
>> > Why? I really can't see any sensible use case for this that
>> > doesn't
>> > have a better solution available.
>> SPI works just fine without DMA, which might just be considered an
>> (optional) optimisation.
>>
>> We've been using it without DMA for years now, and it was working
>> just
>> fine, and it will work even better with the other patches in this
>> serie. There's no reason to add a hard dependency on something that
>> we
>> don't really need.
>>
>
> Actually it non-DMA case works fine if you don't need SPI transfers
> larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.
>
> This was addressed by this patch, but was never applied:
> http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950
And the code added in that patch will never run unless you
1) use long spi transfers
2) compile in/load SPI without DMA support
There is no reason for doing 2) since we have do DMA support for sunxi.
So that's another code path that needs maintenance and testing and
likely will not get it.
Thanks
Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-06-02 12:14 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-06-02 12:14 UTC (permalink / raw)
To: Priit Laes
Cc: Maxime Ripard, Mark Brown, Geert Uytterhoeven, linux-sunxi,
Chen-Yu Tsai, linux-spi, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
On 2 June 2016 at 06:42, Priit Laes <plaes@plaes.org> wrote:
> On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:
>> Hi,
>>
>> On Mon, May 30, 2016 at 04:50:16PM +0100, Mark Brown wrote:
>> >
>> > On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
>> > >
>> > > On 30 May 2016 at 17:03, Mark Brown <broonie@kernel.org> wrote:
>> > >
>> > > >
>> > > > I really don't think it's worth caring too much about cases
>> > > > where the
>> > > > DMA driver hasn't been compiled in, it's not like SPI is the
>> > > > only thing
>> > >
>> > > It's what the driver did to start with and it was requested to
>> > > fall
>> > > back to non-DMA in the case DMA is not available.
>> > Why? I really can't see any sensible use case for this that
>> > doesn't
>> > have a better solution available.
>> SPI works just fine without DMA, which might just be considered an
>> (optional) optimisation.
>>
>> We've been using it without DMA for years now, and it was working
>> just
>> fine, and it will work even better with the other patches in this
>> serie. There's no reason to add a hard dependency on something that
>> we
>> don't really need.
>>
>
> Actually it non-DMA case works fine if you don't need SPI transfers
> larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.
>
> This was addressed by this patch, but was never applied:
> http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950
And the code added in that patch will never run unless you
1) use long spi transfers
2) compile in/load SPI without DMA support
There is no reason for doing 2) since we have do DMA support for sunxi.
So that's another code path that needs maintenance and testing and
likely will not get it.
Thanks
Michal
^ permalink raw reply [flat|nested] 95+ messages in thread
* [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-06-02 12:14 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-06-02 12:14 UTC (permalink / raw)
To: linux-arm-kernel
On 2 June 2016 at 06:42, Priit Laes <plaes@plaes.org> wrote:
> On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:
>> Hi,
>>
>> On Mon, May 30, 2016 at 04:50:16PM +0100, Mark Brown wrote:
>> >
>> > On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
>> > >
>> > > On 30 May 2016 at 17:03, Mark Brown <broonie@kernel.org> wrote:
>> > >
>> > > >
>> > > > I really don't think it's worth caring too much about cases
>> > > > where the
>> > > > DMA driver hasn't been compiled in, it's not like SPI is the
>> > > > only thing
>> > >
>> > > It's what the driver did to start with and it was requested to
>> > > fall
>> > > back to non-DMA in the case DMA is not available.
>> > Why? I really can't see any sensible use case for this that
>> > doesn't
>> > have a better solution available.
>> SPI works just fine without DMA, which might just be considered an
>> (optional) optimisation.
>>
>> We've been using it without DMA for years now, and it was working
>> just
>> fine, and it will work even better with the other patches in this
>> serie. There's no reason to add a hard dependency on something that
>> we
>> don't really need.
>>
>
> Actually it non-DMA case works fine if you don't need SPI transfers
> larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.
>
> This was addressed by this patch, but was never applied:
> http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950
And the code added in that patch will never run unless you
1) use long spi transfers
2) compile in/load SPI without DMA support
There is no reason for doing 2) since we have do DMA support for sunxi.
So that's another code path that needs maintenance and testing and
likely will not get it.
Thanks
Michal
^ permalink raw reply [flat|nested] 95+ messages in thread
[parent not found: <CAOMqctRMi8Zuwqvakka8LkgvecWVAfzUcER9+ui9WBYS78fWtQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
2016-06-02 12:14 ` Michal Suchanek
(?)
@ 2016-06-02 14:26 ` Mark Brown
-1 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-06-02 14:26 UTC (permalink / raw)
To: Michal Suchanek
Cc: Priit Laes, Maxime Ripard, Geert Uytterhoeven, linux-sunxi,
Chen-Yu Tsai, linux-spi,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
[-- Attachment #1: Type: text/plain, Size: 1387 bytes --]
On Thu, Jun 02, 2016 at 02:14:26PM +0200, Michal Suchanek wrote:
> On 2 June 2016 at 06:42, Priit Laes <plaes-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org> wrote:
> > On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:
> > Actually it non-DMA case works fine if you don't need SPI transfers
> > larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.
> > This was addressed by this patch, but was never applied:
> > http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950
> And the code added in that patch will never run unless you
> 1) use long spi transfers
> 2) compile in/load SPI without DMA support
> There is no reason for doing 2) since we have do DMA support for sunxi.
Well, presumably such code exists and is being worked on?
> So that's another code path that needs maintenance and testing and
> likely will not get it.
Oh, come on. You might not want to use it yourself but the chances are
that someone will want to use it just like the situation with all the
other SPI drivers. It's a perfectly reasonable and sensible feature to
support upstream.
I really do not understand why there is such a strong desire to have
these devices be a special snowflake here, the worst that's likely to
happen here is that you're going to end up having to either remove the
DMA controller from the DT or load the driver for it neither of which
seem like the end of the world.
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-06-02 14:26 ` Mark Brown
0 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-06-02 14:26 UTC (permalink / raw)
To: Michal Suchanek
Cc: Priit Laes, Maxime Ripard, Geert Uytterhoeven, linux-sunxi,
Chen-Yu Tsai, linux-spi, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 1358 bytes --]
On Thu, Jun 02, 2016 at 02:14:26PM +0200, Michal Suchanek wrote:
> On 2 June 2016 at 06:42, Priit Laes <plaes@plaes.org> wrote:
> > On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:
> > Actually it non-DMA case works fine if you don't need SPI transfers
> > larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.
> > This was addressed by this patch, but was never applied:
> > http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950
> And the code added in that patch will never run unless you
> 1) use long spi transfers
> 2) compile in/load SPI without DMA support
> There is no reason for doing 2) since we have do DMA support for sunxi.
Well, presumably such code exists and is being worked on?
> So that's another code path that needs maintenance and testing and
> likely will not get it.
Oh, come on. You might not want to use it yourself but the chances are
that someone will want to use it just like the situation with all the
other SPI drivers. It's a perfectly reasonable and sensible feature to
support upstream.
I really do not understand why there is such a strong desire to have
these devices be a special snowflake here, the worst that's likely to
happen here is that you're going to end up having to either remove the
DMA controller from the DT or load the driver for it neither of which
seem like the end of the world.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 95+ messages in thread
* [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-06-02 14:26 ` Mark Brown
0 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-06-02 14:26 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jun 02, 2016 at 02:14:26PM +0200, Michal Suchanek wrote:
> On 2 June 2016 at 06:42, Priit Laes <plaes@plaes.org> wrote:
> > On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:
> > Actually it non-DMA case works fine if you don't need SPI transfers
> > larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.
> > This was addressed by this patch, but was never applied:
> > http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950
> And the code added in that patch will never run unless you
> 1) use long spi transfers
> 2) compile in/load SPI without DMA support
> There is no reason for doing 2) since we have do DMA support for sunxi.
Well, presumably such code exists and is being worked on?
> So that's another code path that needs maintenance and testing and
> likely will not get it.
Oh, come on. You might not want to use it yourself but the chances are
that someone will want to use it just like the situation with all the
other SPI drivers. It's a perfectly reasonable and sensible feature to
support upstream.
I really do not understand why there is such a strong desire to have
these devices be a special snowflake here, the worst that's likely to
happen here is that you're going to end up having to either remove the
DMA controller from the DT or load the driver for it neither of which
seem like the end of the world.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160602/276d2e77/attachment-0001.sig>
^ permalink raw reply [flat|nested] 95+ messages in thread
[parent not found: <20160602142649.GJ2282-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>]
* Re: Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
2016-06-02 14:26 ` Mark Brown
(?)
@ 2016-06-05 11:27 ` Michal Suchanek
-1 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-06-05 11:27 UTC (permalink / raw)
To: Mark Brown
Cc: Priit Laes, Maxime Ripard, Geert Uytterhoeven, linux-sunxi,
Chen-Yu Tsai, linux-spi,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On 2 June 2016 at 16:26, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Jun 02, 2016 at 02:14:26PM +0200, Michal Suchanek wrote:
>> On 2 June 2016 at 06:42, Priit Laes <plaes-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org> wrote:
>> > On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:
>
>> > Actually it non-DMA case works fine if you don't need SPI transfers
>> > larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.
>
>> > This was addressed by this patch, but was never applied:
>> > http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950
>
>> And the code added in that patch will never run unless you
>
>> 1) use long spi transfers
>> 2) compile in/load SPI without DMA support
>
>> There is no reason for doing 2) since we have do DMA support for sunxi.
>
> Well, presumably such code exists and is being worked on?
Which code are you referring to?
This is a reply to patch which adds DMA support to the SPI driver so
that it can work for arbitrarily long transfers.
So there is code for fully working driver.
>
>> So that's another code path that needs maintenance and testing and
>> likely will not get it.
>
> Oh, come on. You might not want to use it yourself but the chances are
> that someone will want to use it just like the situation with all the
> other SPI drivers. It's a perfectly reasonable and sensible feature to
> support upstream.
Is it?
Once we have *one* driver that works for arbitrarily long transfers
and it works out of the box with the board defconfig 99% of people who
will use the SPI driver for anything will use this driver. Any other
variant will go untested.
And for the driver to also work without DMA you have to *tell* it to
probe without DMA because it cannot know you are not going to load a
DMA driver later.
>
> I really do not understand why there is such a strong desire to have
> these devices be a special snowflake here, the worst that's likely to
> happen here is that you're going to end up having to either remove the
> DMA controller from the DT or load the driver for it neither of which
> seem like the end of the world.
Why would you do that?
Thanks
Michal
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-06-05 11:27 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-06-05 11:27 UTC (permalink / raw)
To: Mark Brown
Cc: Priit Laes, Maxime Ripard, Geert Uytterhoeven, linux-sunxi,
Chen-Yu Tsai, linux-spi, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
On 2 June 2016 at 16:26, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Jun 02, 2016 at 02:14:26PM +0200, Michal Suchanek wrote:
>> On 2 June 2016 at 06:42, Priit Laes <plaes@plaes.org> wrote:
>> > On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:
>
>> > Actually it non-DMA case works fine if you don't need SPI transfers
>> > larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.
>
>> > This was addressed by this patch, but was never applied:
>> > http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950
>
>> And the code added in that patch will never run unless you
>
>> 1) use long spi transfers
>> 2) compile in/load SPI without DMA support
>
>> There is no reason for doing 2) since we have do DMA support for sunxi.
>
> Well, presumably such code exists and is being worked on?
Which code are you referring to?
This is a reply to patch which adds DMA support to the SPI driver so
that it can work for arbitrarily long transfers.
So there is code for fully working driver.
>
>> So that's another code path that needs maintenance and testing and
>> likely will not get it.
>
> Oh, come on. You might not want to use it yourself but the chances are
> that someone will want to use it just like the situation with all the
> other SPI drivers. It's a perfectly reasonable and sensible feature to
> support upstream.
Is it?
Once we have *one* driver that works for arbitrarily long transfers
and it works out of the box with the board defconfig 99% of people who
will use the SPI driver for anything will use this driver. Any other
variant will go untested.
And for the driver to also work without DMA you have to *tell* it to
probe without DMA because it cannot know you are not going to load a
DMA driver later.
>
> I really do not understand why there is such a strong desire to have
> these devices be a special snowflake here, the worst that's likely to
> happen here is that you're going to end up having to either remove the
> DMA controller from the DT or load the driver for it neither of which
> seem like the end of the world.
Why would you do that?
Thanks
Michal
^ permalink raw reply [flat|nested] 95+ messages in thread
* [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-06-05 11:27 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-06-05 11:27 UTC (permalink / raw)
To: linux-arm-kernel
On 2 June 2016 at 16:26, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Jun 02, 2016 at 02:14:26PM +0200, Michal Suchanek wrote:
>> On 2 June 2016 at 06:42, Priit Laes <plaes@plaes.org> wrote:
>> > On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:
>
>> > Actually it non-DMA case works fine if you don't need SPI transfers
>> > larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.
>
>> > This was addressed by this patch, but was never applied:
>> > http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950
>
>> And the code added in that patch will never run unless you
>
>> 1) use long spi transfers
>> 2) compile in/load SPI without DMA support
>
>> There is no reason for doing 2) since we have do DMA support for sunxi.
>
> Well, presumably such code exists and is being worked on?
Which code are you referring to?
This is a reply to patch which adds DMA support to the SPI driver so
that it can work for arbitrarily long transfers.
So there is code for fully working driver.
>
>> So that's another code path that needs maintenance and testing and
>> likely will not get it.
>
> Oh, come on. You might not want to use it yourself but the chances are
> that someone will want to use it just like the situation with all the
> other SPI drivers. It's a perfectly reasonable and sensible feature to
> support upstream.
Is it?
Once we have *one* driver that works for arbitrarily long transfers
and it works out of the box with the board defconfig 99% of people who
will use the SPI driver for anything will use this driver. Any other
variant will go untested.
And for the driver to also work without DMA you have to *tell* it to
probe without DMA because it cannot know you are not going to load a
DMA driver later.
>
> I really do not understand why there is such a strong desire to have
> these devices be a special snowflake here, the worst that's likely to
> happen here is that you're going to end up having to either remove the
> DMA controller from the DT or load the driver for it neither of which
> seem like the end of the world.
Why would you do that?
Thanks
Michal
^ permalink raw reply [flat|nested] 95+ messages in thread
[parent not found: <CAOMqctQwFif=yFF1snfZJc7+sGS7wcC-6oNTCfjW_MSUWHw+xQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
2016-06-05 11:27 ` Michal Suchanek
(?)
@ 2016-06-06 11:36 ` Mark Brown
-1 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-06-06 11:36 UTC (permalink / raw)
To: Michal Suchanek
Cc: Priit Laes, Maxime Ripard, Geert Uytterhoeven, linux-sunxi,
Chen-Yu Tsai, linux-spi,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
[-- Attachment #1: Type: text/plain, Size: 2317 bytes --]
On Sun, Jun 05, 2016 at 01:27:11PM +0200, Michal Suchanek wrote:
> On 2 June 2016 at 16:26, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Thu, Jun 02, 2016 at 02:14:26PM +0200, Michal Suchanek wrote:
> >> And the code added in that patch will never run unless you
> >> 1) use long spi transfers
> >> 2) compile in/load SPI without DMA support
> >> There is no reason for doing 2) since we have do DMA support for sunxi.
> > Well, presumably such code exists and is being worked on?
> Which code are you referring to?
> This is a reply to patch which adds DMA support to the SPI driver so
> that it can work for arbitrarily long transfers.
It sounded like there might be a missing DMA driver? Though I think I
was misreading the above.
> > Oh, come on. You might not want to use it yourself but the chances are
> > that someone will want to use it just like the situation with all the
> > other SPI drivers. It's a perfectly reasonable and sensible feature to
> > support upstream.
> Is it?
> Once we have *one* driver that works for arbitrarily long transfers
> and it works out of the box with the board defconfig 99% of people who
> will use the SPI driver for anything will use this driver. Any other
> variant will go untested.
We don't want multiple drivers for this and general maintainability
reasons, I'm not sure where the idea that we might want multiple drivers
came from?
> And for the driver to also work without DMA you have to *tell* it to
> probe without DMA because it cannot know you are not going to load a
> DMA driver later.
Yes. This puts the cost on the special snowflake systems that
absolutely cannot have any DMA support in their systems for some reason
while keeping the driver featureful by default, and keeps that cost
fairly small.
> > I really do not understand why there is such a strong desire to have
> > these devices be a special snowflake here, the worst that's likely to
> > happen here is that you're going to end up having to either remove the
> > DMA controller from the DT or load the driver for it neither of which
> > seem like the end of the world.
> Why would you do that?
Like I say I really don't understand why this is. I'm just saying there
are ways to do this fairly simply if people really want it on their
systems.
^ permalink raw reply [flat|nested] 95+ messages in thread
* Re: [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-06-06 11:36 ` Mark Brown
0 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-06-06 11:36 UTC (permalink / raw)
To: Michal Suchanek
Cc: Priit Laes, Maxime Ripard, Geert Uytterhoeven, linux-sunxi,
Chen-Yu Tsai, linux-spi, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 2289 bytes --]
On Sun, Jun 05, 2016 at 01:27:11PM +0200, Michal Suchanek wrote:
> On 2 June 2016 at 16:26, Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Jun 02, 2016 at 02:14:26PM +0200, Michal Suchanek wrote:
> >> And the code added in that patch will never run unless you
> >> 1) use long spi transfers
> >> 2) compile in/load SPI without DMA support
> >> There is no reason for doing 2) since we have do DMA support for sunxi.
> > Well, presumably such code exists and is being worked on?
> Which code are you referring to?
> This is a reply to patch which adds DMA support to the SPI driver so
> that it can work for arbitrarily long transfers.
It sounded like there might be a missing DMA driver? Though I think I
was misreading the above.
> > Oh, come on. You might not want to use it yourself but the chances are
> > that someone will want to use it just like the situation with all the
> > other SPI drivers. It's a perfectly reasonable and sensible feature to
> > support upstream.
> Is it?
> Once we have *one* driver that works for arbitrarily long transfers
> and it works out of the box with the board defconfig 99% of people who
> will use the SPI driver for anything will use this driver. Any other
> variant will go untested.
We don't want multiple drivers for this and general maintainability
reasons, I'm not sure where the idea that we might want multiple drivers
came from?
> And for the driver to also work without DMA you have to *tell* it to
> probe without DMA because it cannot know you are not going to load a
> DMA driver later.
Yes. This puts the cost on the special snowflake systems that
absolutely cannot have any DMA support in their systems for some reason
while keeping the driver featureful by default, and keeps that cost
fairly small.
> > I really do not understand why there is such a strong desire to have
> > these devices be a special snowflake here, the worst that's likely to
> > happen here is that you're going to end up having to either remove the
> > DMA controller from the DT or load the driver for it neither of which
> > seem like the end of the world.
> Why would you do that?
Like I say I really don't understand why this is. I'm just saying there
are ways to do this fairly simply if people really want it on their
systems.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 95+ messages in thread
* [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support
@ 2016-06-06 11:36 ` Mark Brown
0 siblings, 0 replies; 95+ messages in thread
From: Mark Brown @ 2016-06-06 11:36 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Jun 05, 2016 at 01:27:11PM +0200, Michal Suchanek wrote:
> On 2 June 2016 at 16:26, Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Jun 02, 2016 at 02:14:26PM +0200, Michal Suchanek wrote:
> >> And the code added in that patch will never run unless you
> >> 1) use long spi transfers
> >> 2) compile in/load SPI without DMA support
> >> There is no reason for doing 2) since we have do DMA support for sunxi.
> > Well, presumably such code exists and is being worked on?
> Which code are you referring to?
> This is a reply to patch which adds DMA support to the SPI driver so
> that it can work for arbitrarily long transfers.
It sounded like there might be a missing DMA driver? Though I think I
was misreading the above.
> > Oh, come on. You might not want to use it yourself but the chances are
> > that someone will want to use it just like the situation with all the
> > other SPI drivers. It's a perfectly reasonable and sensible feature to
> > support upstream.
> Is it?
> Once we have *one* driver that works for arbitrarily long transfers
> and it works out of the box with the board defconfig 99% of people who
> will use the SPI driver for anything will use this driver. Any other
> variant will go untested.
We don't want multiple drivers for this and general maintainability
reasons, I'm not sure where the idea that we might want multiple drivers
came from?
> And for the driver to also work without DMA you have to *tell* it to
> probe without DMA because it cannot know you are not going to load a
> DMA driver later.
Yes. This puts the cost on the special snowflake systems that
absolutely cannot have any DMA support in their systems for some reason
while keeping the driver featureful by default, and keeps that cost
fairly small.
> > I really do not understand why there is such a strong desire to have
> > these devices be a special snowflake here, the worst that's likely to
> > happen here is that you're going to end up having to either remove the
> > DMA controller from the DT or load the driver for it neither of which
> > seem like the end of the world.
> Why would you do that?
Like I say I really don't understand why this is. I'm just saying there
are ways to do this fairly simply if people really want it on their
systems.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160606/c5b03fc2/attachment.sig>
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH 4/5] spi: sunxi: set maximum and minimum speed of SPI master
2016-05-24 22:56 ` Michal Suchanek
@ 2016-05-26 19:25 ` Michal Suchanek
-1 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-26 19:25 UTC (permalink / raw)
To: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Mark Brown, Maxime Ripard,
Chen-Yu Tsai, linux-spi-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
The maximum speed of SPI master is used when maximum speed of SPI slave
is not specified. Also the __spi_validate function should check that
transfer speeds do not exceed the master limits.
The user manual for A10 and A31 specifies maximum
speed of the SPI clock as 100MHz and minimum as 3kHz.
Setting the SPI clock to out-of-spec values can lock up the SoC.
Signed-off-by: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/spi/spi-sun4i.c | 2 ++
drivers/spi/spi-sun6i.c | 2 ++
2 files changed, 4 insertions(+)
diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index bf52b09..e1a75dd6 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -405,6 +405,8 @@ static int sun4i_spi_probe(struct platform_device *pdev)
}
sspi->master = master;
+ master->max_speed_hz = 100*1000*1000;
+ master->min_speed_hz = 3*1000;
master->set_cs = sun4i_spi_set_cs;
master->transfer_one = sun4i_spi_transfer_one;
master->num_chipselect = 4;
diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 1952956..0c378ff 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -400,6 +400,8 @@ static int sun6i_spi_probe(struct platform_device *pdev)
}
sspi->master = master;
+ master->max_speed_hz = 100*1000*1000;
+ master->min_speed_hz = 3*1000;
master->set_cs = sun6i_spi_set_cs;
master->transfer_one = sun6i_spi_transfer_one;
master->num_chipselect = 4;
--
2.8.1
^ permalink raw reply related [flat|nested] 95+ messages in thread
* [PATCH 4/5] spi: sunxi: set maximum and minimum speed of SPI master
@ 2016-05-26 19:25 ` Michal Suchanek
0 siblings, 0 replies; 95+ messages in thread
From: Michal Suchanek @ 2016-05-26 19:25 UTC (permalink / raw)
To: linux-arm-kernel
The maximum speed of SPI master is used when maximum speed of SPI slave
is not specified. Also the __spi_validate function should check that
transfer speeds do not exceed the master limits.
The user manual for A10 and A31 specifies maximum
speed of the SPI clock as 100MHz and minimum as 3kHz.
Setting the SPI clock to out-of-spec values can lock up the SoC.
Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
drivers/spi/spi-sun4i.c | 2 ++
drivers/spi/spi-sun6i.c | 2 ++
2 files changed, 4 insertions(+)
diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index bf52b09..e1a75dd6 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -405,6 +405,8 @@ static int sun4i_spi_probe(struct platform_device *pdev)
}
sspi->master = master;
+ master->max_speed_hz = 100*1000*1000;
+ master->min_speed_hz = 3*1000;
master->set_cs = sun4i_spi_set_cs;
master->transfer_one = sun4i_spi_transfer_one;
master->num_chipselect = 4;
diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 1952956..0c378ff 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -400,6 +400,8 @@ static int sun6i_spi_probe(struct platform_device *pdev)
}
sspi->master = master;
+ master->max_speed_hz = 100*1000*1000;
+ master->min_speed_hz = 3*1000;
master->set_cs = sun6i_spi_set_cs;
master->transfer_one = sun6i_spi_transfer_one;
master->num_chipselect = 4;
--
2.8.1
^ permalink raw reply related [flat|nested] 95+ messages in thread
* Re: [PATCH 4/5] spi: sunxi: set maximum and minimum speed of SPI master
2016-05-26 19:25 ` Michal Suchanek
@ 2016-05-30 9:17 ` Maxime Ripard
-1 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-05-30 9:17 UTC (permalink / raw)
To: Michal Suchanek
Cc: linux-sunxi, Mark Brown, Chen-Yu Tsai, linux-spi,
linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1289 bytes --]
Hi,
On Thu, May 26, 2016 at 07:25:25PM -0000, Michal Suchanek wrote:
> The maximum speed of SPI master is used when maximum speed of SPI slave
> is not specified. Also the __spi_validate function should check that
> transfer speeds do not exceed the master limits.
>
> The user manual for A10 and A31 specifies maximum
> speed of the SPI clock as 100MHz and minimum as 3kHz.
>
> Setting the SPI clock to out-of-spec values can lock up the SoC.
>
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
> drivers/spi/spi-sun4i.c | 2 ++
> drivers/spi/spi-sun6i.c | 2 ++
> 2 files changed, 4 insertions(+)
>
> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index bf52b09..e1a75dd6 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
> @@ -405,6 +405,8 @@ static int sun4i_spi_probe(struct platform_device *pdev)
> }
>
> sspi->master = master;
> + master->max_speed_hz = 100*1000*1000;
You need spaces around the * operator.
> + master->min_speed_hz = 3*1000;
And I'm not exactly sure why you have that weird indentation.
The same applies for the sun6i driver
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 95+ messages in thread
* [PATCH 4/5] spi: sunxi: set maximum and minimum speed of SPI master
@ 2016-05-30 9:17 ` Maxime Ripard
0 siblings, 0 replies; 95+ messages in thread
From: Maxime Ripard @ 2016-05-30 9:17 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Thu, May 26, 2016 at 07:25:25PM -0000, Michal Suchanek wrote:
> The maximum speed of SPI master is used when maximum speed of SPI slave
> is not specified. Also the __spi_validate function should check that
> transfer speeds do not exceed the master limits.
>
> The user manual for A10 and A31 specifies maximum
> speed of the SPI clock as 100MHz and minimum as 3kHz.
>
> Setting the SPI clock to out-of-spec values can lock up the SoC.
>
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
> drivers/spi/spi-sun4i.c | 2 ++
> drivers/spi/spi-sun6i.c | 2 ++
> 2 files changed, 4 insertions(+)
>
> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index bf52b09..e1a75dd6 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
> @@ -405,6 +405,8 @@ static int sun4i_spi_probe(struct platform_device *pdev)
> }
>
> sspi->master = master;
> + master->max_speed_hz = 100*1000*1000;
You need spaces around the * operator.
> + master->min_speed_hz = 3*1000;
And I'm not exactly sure why you have that weird indentation.
The same applies for the sun6i driver
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160530/2d0eafc9/attachment.sig>
^ permalink raw reply [flat|nested] 95+ messages in thread