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 <[email protected]>
---
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
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 <[email protected]>
---
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
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?
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
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.
> > 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
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.
> > > 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
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 <[email protected]>
> ---
> 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
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
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
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