[PATCH 1/2] spi: pxa2xx: fix SCR (divisor) calculation

[PATCH 1/2] spi: pxa2xx: fix SCR (divisor) calculation

[Flavio Suligoi]

Calculate the divisor for the SCR (Serial Clock Rate), avoiding
that the SSP transmission rate can be greater than the device rate.

When the division between the SSP clock and the device rate generates
a reminder, we have to increment by one the divisor.
In this way the resulting SSP clock will never be greater than the
device SPI max frequency.

For example, with:

 - ssp_clk  = 50 MHz
 - dev freq = 15 MHz

without this patch the SSP clock will be greater than 15 MHz:

 - 25 MHz for PXA25x_SSP and CE4100_SSP
 - 16,56 MHz for the others

Instead, with this patch, we have in both case an SSP clock of 12.5MHz,
so the max rate of the SPI device clock is respected.

Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
---
 drivers/spi/spi-pxa2xx.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index f7068cc..c9560a1 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -884,10 +884,15 @@ static unsigned int ssp_get_clk_div(struct driver_data *drv_data, int rate)
 
 	rate = min_t(int, ssp_clk, rate);
 
+	/*
+	 * Calculate the divisor for the SCR (Serial Clock Rate), avoiding
+	 * that the SSP transmission rate can be greater than the device rate
+	 */
 	if (ssp->type == PXA25x_SSP || ssp->type == CE4100_SSP)
-		return (ssp_clk / (2 * rate) - 1) & 0xff;
+		return (ssp_clk / (2 * rate) - 1 +
+			(ssp_clk % (2 * rate) ? 1 : 0)) & 0xff;
 	else
-		return (ssp_clk / rate - 1) & 0xfff;
+		return (ssp_clk / rate - 1 + (ssp_clk % rate ? 1 : 0)) & 0xfff;
 }
 
 static unsigned int pxa2xx_ssp_get_clk_div(struct driver_data *drv_data,
-- 
2.7.4

[PATCH 2/2] spi: pxa2xx: use a module softdep for dw_dmac

[Flavio Suligoi]

With dw_dmac, sometimes the request of a DMA channel fails because
the DMA driver is not ready, so an explicit dependency request
is necessary.

Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
---
 drivers/spi/spi-pxa2xx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index c9560a1..e8ac26b 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1962,3 +1962,5 @@ static void __exit pxa2xx_spi_exit(void)
 	platform_driver_unregister(&driver);
 }
 module_exit(pxa2xx_spi_exit);
+
+MODULE_SOFTDEP("pre: dw_dmac");
-- 
2.7.4

Re: [PATCH 2/2] spi: pxa2xx: use a module softdep for dw_dmac

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 378 bytes --]

On Wed, Apr 10, 2019 at 02:51:36PM +0200, Flavio Suligoi wrote:
> With dw_dmac, sometimes the request of a DMA channel fails because
> the DMA driver is not ready, so an explicit dependency request
> is necessary.

While this isn't going to hurt anything and might actually help so it's
fine doesn't this also suggest that there's an issue with deferred probe
going on as well?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH 2/2] spi: pxa2xx: use a module softdep for dw_dmac

[Flavio Suligoi]

Hi Mark,

> On Wed, Apr 10, 2019 at 02:51:36PM +0200, Flavio Suligoi wrote:
> > With dw_dmac, sometimes the request of a DMA channel fails because
> > the DMA driver is not ready, so an explicit dependency request
> > is necessary.
> 
> While this isn't going to hurt anything and might actually help so it's
> fine doesn't this also suggest that there's an issue with deferred probe
> going on as well?

I think that the problem could be related to how the DMA channel is requested.
At the moment the function used are:

pxa2xx_spi_dma_setup --> dma_request_slave_channel_compat -->
--> __dma_request_slave_channel_compat --> dma_request_slave_channel -->
--> dma_request_chan

Actually the final function "dma_request_chan" return
the channel number or "-EPROBE_DEFER" if it's not ready.
But this information ("-EPROBE_DEFER") is lost in the penultimate function 
"dma_request_slave_channel", which return only the chann, if all is ok, or
NULL, in case of errors.
So the deferral mechanism is not used.

Flavio

Re: [PATCH 2/2] spi: pxa2xx: use a module softdep for dw_dmac

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1039 bytes --]

On Wed, Apr 10, 2019 at 02:05:38PM +0000, Flavio Suligoi wrote:
> Hi Mark,

> > While this isn't going to hurt anything and might actually help so it's
> > fine doesn't this also suggest that there's an issue with deferred probe
> > going on as well?

> I think that the problem could be related to how the DMA channel is requested.
> At the moment the function used are:

> pxa2xx_spi_dma_setup --> dma_request_slave_channel_compat -->
> --> __dma_request_slave_channel_compat --> dma_request_slave_channel -->
> --> dma_request_chan

> Actually the final function "dma_request_chan" return
> the channel number or "-EPROBE_DEFER" if it's not ready.
> But this information ("-EPROBE_DEFER") is lost in the penultimate function 
> "dma_request_slave_channel", which return only the chann, if all is ok, or
> NULL, in case of errors.
> So the deferral mechanism is not used.

Right, yes - that analysis seems correct.  The interfaces seem a bit
weird here but fixing them looks like the most complete and robust fix.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH 2/2] spi: pxa2xx: use a module softdep for dw_dmac

[Flavio Suligoi]

> > I think that the problem could be related to how the DMA channel is
> requested.
> > At the moment the function used are:
> 
> > pxa2xx_spi_dma_setup --> dma_request_slave_channel_compat -->
> > --> __dma_request_slave_channel_compat --> dma_request_slave_channel -->
> > --> dma_request_chan
> 
> > Actually the final function "dma_request_chan" return
> > the channel number or "-EPROBE_DEFER" if it's not ready.
> > But this information ("-EPROBE_DEFER") is lost in the penultimate
> function
> > "dma_request_slave_channel", which return only the chann, if all is ok,
> or
> > NULL, in case of errors.
> > So the deferral mechanism is not used.
> 
> Right, yes - that analysis seems correct.  The interfaces seem a bit
> weird here but fixing them looks like the most complete and robust fix.

Ok Mark, I'll fix this problem as soon as I can, using EPROBE_DEFER.
For now, in my application, I use the patch that I already sent,
with the "softdep" workaround:

MODULE_SOFTDEP("pre: dw_dmac");

I tested it a lot, with more than 2000 cold reboot (automatic
switch on/off using a controlled power supply) and it always worked good.

Thanks for your help!

Flavio

Re: [PATCH 2/2] spi: pxa2xx: use a module softdep for dw_dmac

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

On Thu, Apr 11, 2019 at 07:14:06AM +0000, Flavio Suligoi wrote:

> > Right, yes - that analysis seems correct.  The interfaces seem a bit
> > weird here but fixing them looks like the most complete and robust fix.

> Ok Mark, I'll fix this problem as soon as I can, using EPROBE_DEFER.
> For now, in my application, I use the patch that I already sent,
> with the "softdep" workaround:

> MODULE_SOFTDEP("pre: dw_dmac");

> I tested it a lot, with more than 2000 cold reboot (automatic
> switch on/off using a controlled power supply) and it always worked good.

Right, and to be clear that patch is good and useful independently of
the deferred probe fix so assuming nothing else comes up in review I'll
apply it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH 2/2] spi: pxa2xx: use a module softdep for dw_dmac

[Flavio Suligoi]

> > > Right, yes - that analysis seems correct.  The interfaces seem a bit
> > > weird here but fixing them looks like the most complete and robust
> fix.
> 
> > Ok Mark, I'll fix this problem as soon as I can, using EPROBE_DEFER.
> > For now, in my application, I use the patch that I already sent,
> > with the "softdep" workaround:
> 
> > MODULE_SOFTDEP("pre: dw_dmac");
> 
> > I tested it a lot, with more than 2000 cold reboot (automatic
> > switch on/off using a controlled power supply) and it always worked
> good.
> 
> Right, and to be clear that patch is good and useful independently of
> the deferred probe fix so assuming nothing else comes up in review I'll
> apply it.

Thanks Mark,

Flavio

Applied "spi: pxa2xx: use a module softdep for dw_dmac" to the spi tree

[Mark Brown]

The patch

   spi: pxa2xx: use a module softdep for dw_dmac

has been applied to the spi tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-5.2

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 2c54c4a640ed4dc9db03693641a1a651535f05f1 Mon Sep 17 00:00:00 2001
From: Flavio Suligoi <f.suligoi@asem.it>
Date: Wed, 10 Apr 2019 14:51:36 +0200
Subject: [PATCH] spi: pxa2xx: use a module softdep for dw_dmac

With dw_dmac, sometimes the request of a DMA channel fails because
the DMA driver is not ready, so an explicit dependency request
is necessary.

Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-pxa2xx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index f7068ccfe7d2..3e6811ef37e8 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1957,3 +1957,5 @@ static void __exit pxa2xx_spi_exit(void)
 	platform_driver_unregister(&driver);
 }
 module_exit(pxa2xx_spi_exit);
+
+MODULE_SOFTDEP("pre: dw_dmac");
-- 
2.20.1

Applied "spi: pxa2xx: use a module softdep for dw_dmac" to the spi tree

[Mark Brown]

The patch

   spi: pxa2xx: use a module softdep for dw_dmac

has been applied to the spi tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 51ebf6acb00fa6f965e600f848bee5bddddd2054 Mon Sep 17 00:00:00 2001
From: Flavio Suligoi <f.suligoi@asem.it>
Date: Wed, 10 Apr 2019 14:51:36 +0200
Subject: [PATCH] spi: pxa2xx: use a module softdep for dw_dmac

With dw_dmac, sometimes the request of a DMA channel fails because
the DMA driver is not ready, so an explicit dependency request
is necessary.

Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-pxa2xx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index f7068ccfe7d2..3e6811ef37e8 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1957,3 +1957,5 @@ static void __exit pxa2xx_spi_exit(void)
 	platform_driver_unregister(&driver);
 }
 module_exit(pxa2xx_spi_exit);
+
+MODULE_SOFTDEP("pre: dw_dmac");
-- 
2.20.1

Re: [PATCH 1/2] spi: pxa2xx: fix SCR (divisor) calculation

[Jarkko Nikula]

On 4/10/19 3:51 PM, Flavio Suligoi wrote:
> Calculate the divisor for the SCR (Serial Clock Rate), avoiding
> that the SSP transmission rate can be greater than the device rate.
> 
> When the division between the SSP clock and the device rate generates
> a reminder, we have to increment by one the divisor.
> In this way the resulting SSP clock will never be greater than the
> device SPI max frequency.
> 
> For example, with:
> 
>   - ssp_clk  = 50 MHz
>   - dev freq = 15 MHz
> 
> without this patch the SSP clock will be greater than 15 MHz:
> 
>   - 25 MHz for PXA25x_SSP and CE4100_SSP
>   - 16,56 MHz for the others
> 
> Instead, with this patch, we have in both case an SSP clock of 12.5MHz,
> so the max rate of the SPI device clock is respected.
> 
> Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
> ---
>   drivers/spi/spi-pxa2xx.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
> index f7068cc..c9560a1 100644
> --- a/drivers/spi/spi-pxa2xx.c
> +++ b/drivers/spi/spi-pxa2xx.c
> @@ -884,10 +884,15 @@ static unsigned int ssp_get_clk_div(struct driver_data *drv_data, int rate)
>   
>   	rate = min_t(int, ssp_clk, rate);
>   
> +	/*
> +	 * Calculate the divisor for the SCR (Serial Clock Rate), avoiding
> +	 * that the SSP transmission rate can be greater than the device rate
> +	 */
>   	if (ssp->type == PXA25x_SSP || ssp->type == CE4100_SSP)
> -		return (ssp_clk / (2 * rate) - 1) & 0xff;
> +		return (ssp_clk / (2 * rate) - 1 +
> +			(ssp_clk % (2 * rate) ? 1 : 0)) & 0xff;
>   	else
> -		return (ssp_clk / rate - 1) & 0xfff;
> +		return (ssp_clk / rate - 1 + (ssp_clk % rate ? 1 : 0)) & 0xfff;
>   }
>   
I think DIV_ROUND_UP() - 1 would also fix this?

I realized we have also another issue here with the low rates. 
Calculated divider will underflow due masking with 0xff or 0xfff when 
the rate is low enough.

Would you want to fix that by setting the ctlr->min_speed_hz so that spi 
core can validate the rate? I'm asking since it goes well together with 
your fix. Maybe one patch setting the min_speed_hz and getting masking 
off from here and then another fixing the rate calculation.

-- 
Jarkko

RE: [PATCH 1/2] spi: pxa2xx: fix SCR (divisor) calculation

[Flavio Suligoi]

Hi Jarkko,

> > diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
> > index f7068cc..c9560a1 100644
> > --- a/drivers/spi/spi-pxa2xx.c
> > +++ b/drivers/spi/spi-pxa2xx.c
> > @@ -884,10 +884,15 @@ static unsigned int ssp_get_clk_div(struct
> driver_data *drv_data, int rate)
> >
> >   	rate = min_t(int, ssp_clk, rate);
> >
> > +	/*
> > +	 * Calculate the divisor for the SCR (Serial Clock Rate), avoiding
> > +	 * that the SSP transmission rate can be greater than the device
> rate
> > +	 */
> >   	if (ssp->type == PXA25x_SSP || ssp->type == CE4100_SSP)
> > -		return (ssp_clk / (2 * rate) - 1) & 0xff;
> > +		return (ssp_clk / (2 * rate) - 1 +
> > +			(ssp_clk % (2 * rate) ? 1 : 0)) & 0xff;
> >   	else
> > -		return (ssp_clk / rate - 1) & 0xfff;
> > +		return (ssp_clk / rate - 1 + (ssp_clk % rate ? 1 : 0)) &
> 0xfff;
> >   }
> >
> I think DIV_ROUND_UP() - 1 would also fix this?
> 
> I realized we have also another issue here with the low rates.
> Calculated divider will underflow due masking with 0xff or 0xfff when
> the rate is low enough.
> 
> Would you want to fix that by setting the ctlr->min_speed_hz so that spi
> core can validate the rate? I'm asking since it goes well together with
> your fix. Maybe one patch setting the min_speed_hz and getting masking
> off from here and then another fixing the rate calculation.

Ok for the two separate patches, I prepare them as soon as I can.

Thanks,

Flavio