From: "he, bo" <[email protected]>
We find spi can't work on board. More debug shows it's related
to the following patch that changed the chip selection assert and
deassert timing.
commit d5898e19c0d74cd41b9f5c8c8ea87e559c3fe0c1
spi: pxa2xx: Use core message processing loop
Convert the pump_transfers() transfer tasklet to transfer_one()
hook the SPI core calls to process single transfer instead of
handling message processing and chip select handling in the driver.
This not only simplifies the driver but also brings transfer
statistics from the core.
We modify the chip selection timeing action back to before the patch.
The spi on the board works.
Signed-off-by: xiao jin <[email protected]>
Signed-off-by: he, bo <[email protected]>
---
drivers/spi/spi-pxa2xx.c | 41 ++++++++++++++--------------------------
1 file changed, 14 insertions(+), 27 deletions(-)
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index ec4b65100..0affa09da 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -340,11 +340,9 @@ static void lpss_ssp_setup(struct driver_data *drv_data)
}
}
-static void lpss_ssp_select_cs(struct spi_device *spi,
+static void lpss_ssp_select_cs(struct driver_data *drv_data,
const struct lpss_config *config)
{
- struct driver_data *drv_data =
- spi_controller_get_devdata(spi->controller);
u32 value, cs;
if (!config->cs_sel_mask)
@@ -352,7 +350,7 @@ static void lpss_ssp_select_cs(struct spi_device *spi,
value = __lpss_ssp_read_priv(drv_data, config->reg_cs_ctrl);
- cs = spi->chip_select;
+ cs = drv_data->master->cur_msg->spi->chip_select;
cs <<= config->cs_sel_shift;
if (cs != (value & config->cs_sel_mask)) {
/*
@@ -371,17 +369,15 @@ static void lpss_ssp_select_cs(struct spi_device *spi,
}
}
-static void lpss_ssp_cs_control(struct spi_device *spi, bool enable)
+static void lpss_ssp_cs_control(struct driver_data *drv_data, bool enable)
{
- struct driver_data *drv_data =
- spi_controller_get_devdata(spi->controller);
const struct lpss_config *config;
u32 value;
config = lpss_get_config(drv_data);
if (enable)
- lpss_ssp_select_cs(spi, config);
+ lpss_ssp_select_cs(drv_data, config);
value = __lpss_ssp_read_priv(drv_data, config->reg_cs_ctrl);
if (enable)
@@ -391,11 +387,10 @@ static void lpss_ssp_cs_control(struct spi_device *spi, bool enable)
__lpss_ssp_write_priv(drv_data, config->reg_cs_ctrl, value);
}
-static void cs_assert(struct spi_device *spi)
+static void cs_assert(struct driver_data *drv_data)
{
- struct chip_data *chip = spi_get_ctldata(spi);
- struct driver_data *drv_data =
- spi_controller_get_devdata(spi->controller);
+ struct chip_data *chip =
+ spi_get_ctldata(drv_data->master->cur_msg->spi);
if (drv_data->ssp_type == CE4100_SSP) {
pxa2xx_spi_write(drv_data, SSSR, chip->frm);
@@ -413,14 +408,13 @@ static void cs_assert(struct spi_device *spi)
}
if (is_lpss_ssp(drv_data))
- lpss_ssp_cs_control(spi, true);
+ lpss_ssp_cs_control(drv_data, true);
}
-static void cs_deassert(struct spi_device *spi)
+static void cs_deassert(struct driver_data *drv_data)
{
- struct chip_data *chip = spi_get_ctldata(spi);
- struct driver_data *drv_data =
- spi_controller_get_devdata(spi->controller);
+ struct chip_data *chip =
+ spi_get_ctldata(drv_data->master->cur_msg->spi);
unsigned long timeout;
if (drv_data->ssp_type == CE4100_SSP)
@@ -443,15 +437,7 @@ static void cs_deassert(struct spi_device *spi)
}
if (is_lpss_ssp(drv_data))
- lpss_ssp_cs_control(spi, false);
-}
-
-static void pxa2xx_spi_set_cs(struct spi_device *spi, bool level)
-{
- if (level)
- cs_deassert(spi);
- else
- cs_assert(spi);
+ lpss_ssp_cs_control(drv_data, false);
}
int pxa2xx_spi_flush(struct driver_data *drv_data)
@@ -610,6 +596,7 @@ static void int_transfer_complete(struct driver_data *drv_data)
if (!pxa25x_ssp_comp(drv_data))
pxa2xx_spi_write(drv_data, SSTO, 0);
+ cs_deassert(drv_data);
spi_finalize_current_transfer(drv_data->master);
}
@@ -1070,6 +1057,7 @@ static int pxa2xx_spi_transfer_one(struct spi_controller *master,
pxa2xx_spi_write(drv_data, SSTO, chip->timeout);
}
+ cs_assert(drv_data);
/*
* Release the data by enabling service requests and interrupts,
* without changing any mode bits
@@ -1563,7 +1551,6 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
master->dma_alignment = DMA_ALIGNMENT;
master->cleanup = cleanup;
master->setup = setup;
- master->set_cs = pxa2xx_spi_set_cs;
master->transfer_one = pxa2xx_spi_transfer_one;
master->handle_err = pxa2xx_spi_handle_err;
master->unprepare_transfer_hardware = pxa2xx_spi_unprepare_transfer;
--
2.20.1
Hi
On 3/6/19 5:05 AM, xiao jin wrote:
> From: "he, bo" <[email protected]>
>
> We find spi can't work on board. More debug shows it's related
> to the following patch that changed the chip selection assert and
> deassert timing.
>
^^ timing caught my attention. More below.
> @@ -610,6 +596,7 @@ static void int_transfer_complete(struct driver_data *drv_data)
> if (!pxa25x_ssp_comp(drv_data))
> pxa2xx_spi_write(drv_data, SSTO, 0);
>
> + cs_deassert(drv_data);
> spi_finalize_current_transfer(drv_data->master);
This
> @@ -1070,6 +1057,7 @@ static int pxa2xx_spi_transfer_one(struct spi_controller *master,
> pxa2xx_spi_write(drv_data, SSTO, chip->timeout);
> }
>
> + cs_assert(drv_data);
and this is not correct with core message loop. It will cause the chip
select is toggled with each transfer in PIO mode. If there is no
cs_change flag set then there shouldn't be CS toggling between the
transfers if SPI message consists of multiple transfers.
More over this patch also will regress with DMA mode since there won't
be CS deassert at all.
Timing reminded me I've seen two cases where there was a timing related
glitch in CS output:
d0283eb2dbc1 ("spi: pxa2xx: Add output control for multiple Intel LPSS
chip selects")
7a8d44bc89e5 ("spi: pxa2xx: Fix too early chipselect deassert")
Do you have a possibility to measure with an oscilloscope what goes
wrong with the CS after d5898e19c0d7 ("spi: pxa2xx: Use core message
processing loop")?
Can you share your setup if I can reproduce it here? E.g. SPI clock
frequency, single or multiple CS, frequency of occurrence, etc
--
Jarkko
Hi Jarkko,
On 3/6/2019 3:52 PM, Jarkko Nikula wrote:
> Hi
>
> On 3/6/19 5:05 AM, xiao jin wrote:
>> From: "he, bo" <[email protected]>
>>
>> We find spi can't work on board. More debug shows it's related
>> to the following patch that changed the chip selection assert and
>> deassert timing.
>>
> ^^ timing caught my attention. More below.
>
>> @@ -610,6 +596,7 @@ static void int_transfer_complete(struct
>> driver_data *drv_data)
>> if (!pxa25x_ssp_comp(drv_data))
>> pxa2xx_spi_write(drv_data, SSTO, 0);
>> + cs_deassert(drv_data);
>> spi_finalize_current_transfer(drv_data->master);
>
> This
>
>> @@ -1070,6 +1057,7 @@ static int pxa2xx_spi_transfer_one(struct
>> spi_controller *master,
>> pxa2xx_spi_write(drv_data, SSTO, chip->timeout);
>> }
>> + cs_assert(drv_data);
>
> and this is not correct with core message loop. It will cause the chip
> select is toggled with each transfer in PIO mode. If there is no
> cs_change flag set then there shouldn't be CS toggling between the
> transfers if SPI message consists of multiple transfers.
>
> More over this patch also will regress with DMA mode since there won't
> be CS deassert at all.
>
> Timing reminded me I've seen two cases where there was a timing
> related glitch in CS output:
>
> d0283eb2dbc1 ("spi: pxa2xx: Add output control for multiple Intel LPSS
> chip selects")
> 7a8d44bc89e5 ("spi: pxa2xx: Fix too early chipselect deassert")
>
> Do you have a possibility to measure with an oscilloscope what goes
> wrong with the CS after d5898e19c0d7 ("spi: pxa2xx: Use core message
> processing loop")?
>
> Can you share your setup if I can reproduce it here? E.g. SPI clock
> frequency, single or multiple CS, frequency of occurrence, etc
>
In our setup the pxa_ssp_type is LPSS_BXT_SSP. With more debug we find
that the problem is caused in the case of "restart the SSP" when
pxa2xx_spi_transfer_one.
We will send another RFC patch.
Thanks.