[PATCH] [RFC] spi: pxa2xx: Do cs if restart the SSP during pxa2xx_spi_transfer_one()

[PATCH] [RFC] spi: pxa2xx: Do cs if restart the SSP during pxa2xx_spi_transfer_one()

[xiao jin]

The spi-pxa2xx can't read and write data correctly on our board.
The pxa_ssp_type is LPSS_BXT_SSP in our case.

With more debug we find that it's related to restart the SPP
during pxa2xx_spi_transfer_one().

In the normal case the spi_transfer_one_message() calls spi-pxa2xx
cs_assert before transferring one message. After completing the
transfer it calls spi-pxa2xx cs_deassert. The spi-pxa2xx works
well.

But in some other case pxa2xx_spi_unprepare_transfer() is called
that clears SSCR0_SSE bit before the next transfer. In the next
transfer the spi-pxa2xx driver will restart the SSP as the SSE
bit is cleared. The cs_assert before the SSP restart can't ensure
spi-pxa2xx work well.

The patch is to do cs again if spi-pxa2xx restar the SSP during
pxa2xx_spi_transfer_one()

Signed-off-by: xiao jin <jin.xiao@intel.com>
Signed-off-by: he, bo <bo.he@intel.com>
---
 drivers/spi/spi-pxa2xx.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 14f4ea59caff..1a2ea46858d9 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -928,6 +928,7 @@ static int pxa2xx_spi_transfer_one(struct spi_controller *master,
 	u32 cr1;
 	int err;
 	int dma_mapped;
+	bool need_cs_change = false;
 
 	/* Check if we can DMA this transfer */
 	if (transfer->len > MAX_DMA_LEN && chip->enable_dma) {
@@ -1056,6 +1057,11 @@ static int pxa2xx_spi_transfer_one(struct spi_controller *master,
 	if ((pxa2xx_spi_read(drv_data, SSCR0) != cr0)
 	    || (pxa2xx_spi_read(drv_data, SSCR1) & change_mask)
 	    != (cr1 & change_mask)) {
+		/* It needs to deassert the chip selection
+		 * firstly before restart the SPP */
+		need_cs_change = true;
+		cs_deassert(spi);
+
 		/* stop the SSP, and update the other bits */
 		pxa2xx_spi_write(drv_data, SSCR0, cr0 & ~SSCR0_SSE);
 		if (!pxa25x_ssp_comp(drv_data))
@@ -1070,6 +1076,8 @@ static int pxa2xx_spi_transfer_one(struct spi_controller *master,
 			pxa2xx_spi_write(drv_data, SSTO, chip->timeout);
 	}
 
+	if (need_cs_change)
+		cs_assert(spi);
 	/*
 	 * Release the data by enabling service requests and interrupts,
 	 * without changing any mode bits
-- 
2.21.0

Re: [PATCH] [RFC] spi: pxa2xx: Do cs if restart the SSP during pxa2xx_spi_transfer_one()

[Jarkko Nikula]

Hi

Is this also related to the regression with d5898e19c0d7 ("spi: pxa2xx: 
Use core message processing loop") you have found or another issue?

Comments below.

On 3/7/19 9:24 AM, xiao jin wrote:
> The spi-pxa2xx can't read and write data correctly on our board.
> The pxa_ssp_type is LPSS_BXT_SSP in our case.
> 
> With more debug we find that it's related to restart the SPP
> during pxa2xx_spi_transfer_one().
> 
> In the normal case the spi_transfer_one_message() calls spi-pxa2xx
> cs_assert before transferring one message. After completing the
> transfer it calls spi-pxa2xx cs_deassert. The spi-pxa2xx works
> well.
> 
> But in some other case pxa2xx_spi_unprepare_transfer() is called
> that clears SSCR0_SSE bit before the next transfer. In the next
> transfer the spi-pxa2xx driver will restart the SSP as the SSE
> bit is cleared. The cs_assert before the SSP restart can't ensure
> spi-pxa2xx work well.
> 
> The patch is to do cs again if spi-pxa2xx restar the SSP during
> pxa2xx_spi_transfer_one()
> 
Hmm.. please correct me if I'm wrong but pxa2xx_spi_unprepare_transfer() 
is called always when there is no more messages pending and the spi core 
should have deasserted the CS already?

More below.

> Signed-off-by: xiao jin <jin.xiao@intel.com>
> Signed-off-by: he, bo <bo.he@intel.com>
> ---
>   drivers/spi/spi-pxa2xx.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
> index 14f4ea59caff..1a2ea46858d9 100644
> --- a/drivers/spi/spi-pxa2xx.c
> +++ b/drivers/spi/spi-pxa2xx.c
> @@ -928,6 +928,7 @@ static int pxa2xx_spi_transfer_one(struct spi_controller *master,
>   	u32 cr1;
>   	int err;
>   	int dma_mapped;
> +	bool need_cs_change = false;
>   
>   	/* Check if we can DMA this transfer */
>   	if (transfer->len > MAX_DMA_LEN && chip->enable_dma) {
> @@ -1056,6 +1057,11 @@ static int pxa2xx_spi_transfer_one(struct spi_controller *master,
>   	if ((pxa2xx_spi_read(drv_data, SSCR0) != cr0)
>   	    || (pxa2xx_spi_read(drv_data, SSCR1) & change_mask)
>   	    != (cr1 & change_mask)) {
> +		/* It needs to deassert the chip selection
> +		 * firstly before restart the SPP */
> +		need_cs_change = true;
> +		cs_deassert(spi);
> +

I think code comes here at the beginning of each transfer so will be hit 
multiple times before pxa2xx_spi_unprepare_transfer() if SPI message 
consists of multiple transfers.

This makes me wondering if the device driver setting up the "struct 
spi_transfer" is maybe missing the cs_change flag set for transfers 
before last one in case HW needs CS toggling between transfers? For 
instance what following drivers are doing with the cs_change flag:

drivers/char/tpm/tpm_tis_spi.c: tpm_tis_spi_transfer()
drivers/input/touchscreen/ad7877.c: ad7877_read(), ad7877_read_adc()

-- 
Jarkko

Re: [PATCH] [RFC] spi: pxa2xx: Do cs if restart the SSP during pxa2xx_spi_transfer_one()

[Jarkko Nikula]

Hi

Is this also related to the regression with d5898e19c0d7 ("spi: pxa2xx: 
Use core message processing loop") you have found or another issue?

Comments below.

On 3/7/19 9:24 AM, xiao jin wrote:
> The spi-pxa2xx can't read and write data correctly on our board.
> The pxa_ssp_type is LPSS_BXT_SSP in our case.
> 
> With more debug we find that it's related to restart the SPP
> during pxa2xx_spi_transfer_one().
> 
> In the normal case the spi_transfer_one_message() calls spi-pxa2xx
> cs_assert before transferring one message. After completing the
> transfer it calls spi-pxa2xx cs_deassert. The spi-pxa2xx works
> well.
> 
> But in some other case pxa2xx_spi_unprepare_transfer() is called
> that clears SSCR0_SSE bit before the next transfer. In the next
> transfer the spi-pxa2xx driver will restart the SSP as the SSE
> bit is cleared. The cs_assert before the SSP restart can't ensure
> spi-pxa2xx work well.
> 
> The patch is to do cs again if spi-pxa2xx restar the SSP during
> pxa2xx_spi_transfer_one()
> 
Hmm.. please correct me if I'm wrong but pxa2xx_spi_unprepare_transfer() 
is called always when there is no more messages pending and the spi core 
should have deasserted the CS already?

More below.

> Signed-off-by: xiao jin <jin.xiao@intel.com>
> Signed-off-by: he, bo <bo.he@intel.com>
> ---
>   drivers/spi/spi-pxa2xx.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
> index 14f4ea59caff..1a2ea46858d9 100644
> --- a/drivers/spi/spi-pxa2xx.c
> +++ b/drivers/spi/spi-pxa2xx.c
> @@ -928,6 +928,7 @@ static int pxa2xx_spi_transfer_one(struct spi_controller *master,
>   	u32 cr1;
>   	int err;
>   	int dma_mapped;
> +	bool need_cs_change = false;
>   
>   	/* Check if we can DMA this transfer */
>   	if (transfer->len > MAX_DMA_LEN && chip->enable_dma) {
> @@ -1056,6 +1057,11 @@ static int pxa2xx_spi_transfer_one(struct spi_controller *master,
>   	if ((pxa2xx_spi_read(drv_data, SSCR0) != cr0)
>   	    || (pxa2xx_spi_read(drv_data, SSCR1) & change_mask)
>   	    != (cr1 & change_mask)) {
> +		/* It needs to deassert the chip selection
> +		 * firstly before restart the SPP */
> +		need_cs_change = true;
> +		cs_deassert(spi);
> +

I think code comes here at the beginning of each transfer so will be hit 
multiple times before pxa2xx_spi_unprepare_transfer() if SPI message 
consists of multiple transfers.

This makes me wondering if the device driver setting up the "struct 
spi_transfer" is maybe missing the cs_change flag set for transfers 
before last one in case HW needs CS toggling between transfers? For 
instance what following drivers are doing with the cs_change flag:

drivers/char/tpm/tpm_tis_spi.c: tpm_tis_spi_transfer()
drivers/input/touchscreen/ad7877.c: ad7877_read(), ad7877_read_adc()

-- 
Jarkko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] spi: pxa2xx: Do cs if restart the SSP during pxa2xx_spi_transfer_one()

[Mark Brown]

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

On Thu, Mar 07, 2019 at 05:26:53PM +0200, Jarkko Nikula wrote:
> On 3/7/19 9:24 AM, xiao jin wrote:

> > The patch is to do cs again if spi-pxa2xx restar the SSP during
> > pxa2xx_spi_transfer_one()

> Hmm.. please correct me if I'm wrong but pxa2xx_spi_unprepare_transfer() is
> called always when there is no more messages pending and the spi core should
> have deasserted the CS already?

Yes.

> > @@ -1056,6 +1057,11 @@ static int pxa2xx_spi_transfer_one(struct spi_controller *master,
> >   	if ((pxa2xx_spi_read(drv_data, SSCR0) != cr0)
> >   	    || (pxa2xx_spi_read(drv_data, SSCR1) & change_mask)
> >   	    != (cr1 & change_mask)) {
> > +		/* It needs to deassert the chip selection
> > +		 * firstly before restart the SPP */
> > +		need_cs_change = true;
> > +		cs_deassert(spi);

> I think code comes here at the beginning of each transfer so will be hit
> multiple times before pxa2xx_spi_unprepare_transfer() if SPI message
> consists of multiple transfers.

> This makes me wondering if the device driver setting up the "struct
> spi_transfer" is maybe missing the cs_change flag set for transfers before
> last one in case HW needs CS toggling between transfers? For instance what
> following drivers are doing with the cs_change flag:

> drivers/char/tpm/tpm_tis_spi.c: tpm_tis_spi_transfer()
> drivers/input/touchscreen/ad7877.c: ad7877_read(), ad7877_read_adc()

Right, this really feels like it's fixing the wrong thing.  

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

Re: [PATCH] [RFC] spi: pxa2xx: Do cs if restart the SSP during pxa2xx_spi_transfer_one()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1479 bytes --]

On Thu, Mar 07, 2019 at 05:26:53PM +0200, Jarkko Nikula wrote:
> On 3/7/19 9:24 AM, xiao jin wrote:

> > The patch is to do cs again if spi-pxa2xx restar the SSP during
> > pxa2xx_spi_transfer_one()

> Hmm.. please correct me if I'm wrong but pxa2xx_spi_unprepare_transfer() is
> called always when there is no more messages pending and the spi core should
> have deasserted the CS already?

Yes.

> > @@ -1056,6 +1057,11 @@ static int pxa2xx_spi_transfer_one(struct spi_controller *master,
> >   	if ((pxa2xx_spi_read(drv_data, SSCR0) != cr0)
> >   	    || (pxa2xx_spi_read(drv_data, SSCR1) & change_mask)
> >   	    != (cr1 & change_mask)) {
> > +		/* It needs to deassert the chip selection
> > +		 * firstly before restart the SPP */
> > +		need_cs_change = true;
> > +		cs_deassert(spi);

> I think code comes here at the beginning of each transfer so will be hit
> multiple times before pxa2xx_spi_unprepare_transfer() if SPI message
> consists of multiple transfers.

> This makes me wondering if the device driver setting up the "struct
> spi_transfer" is maybe missing the cs_change flag set for transfers before
> last one in case HW needs CS toggling between transfers? For instance what
> following drivers are doing with the cs_change flag:

> drivers/char/tpm/tpm_tis_spi.c: tpm_tis_spi_transfer()
> drivers/input/touchscreen/ad7877.c: ad7877_read(), ad7877_read_adc()

Right, this really feels like it's fixing the wrong thing.  

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] spi: pxa2xx: Do cs if restart the SSP during pxa2xx_spi_transfer_one()

[Xiao, Jin]

Hi

On 3/8/2019 12:09 AM, Mark Brown wrote:
> On Thu, Mar 07, 2019 at 05:26:53PM +0200, Jarkko Nikula wrote:
>> On 3/7/19 9:24 AM, xiao jin wrote:
>>> The patch is to do cs again if spi-pxa2xx restar the SSP during
>>> pxa2xx_spi_transfer_one()
>> Hmm.. please correct me if I'm wrong but pxa2xx_spi_unprepare_transfer() is
>> called always when there is no more messages pending and the spi core should
>> have deasserted the CS already?
> Yes.

I try to describe the situation in more detail.

             cpu0                         cpu1

     => spi_transfer_one_message

     => __spi_pump_messages

     => __spi_sync

     => spi_sync

     => spi_sync_transfer.constprop.2

     => spi_write

     => fm1388_spi_burst_write

     => fm1388_fw_loaded

     => request_firmware_work_func

     => process_one_work

                 => pxa2xx_spi_unprepare_transfer

                 => spi_pump_messages

                 => kthread_worker_fn

Yes, the spi core has de-asserted the CS before the 
pxa2xx_spi_unprepare_transfer(). The problem on my side is that the new 
transfer will restart the SSP in pxa2xx_spi_transfer_one(). The spi core 
has asserted the CS again before restart the SSP.  In my test the CS 
assert before the restart SSP can't ensure the spi transfer working 
correctly.

>>> @@ -1056,6 +1057,11 @@ static int pxa2xx_spi_transfer_one(struct spi_controller *master,
>>>    	if ((pxa2xx_spi_read(drv_data, SSCR0) != cr0)
>>>    	    || (pxa2xx_spi_read(drv_data, SSCR1) & change_mask)
>>>    	    != (cr1 & change_mask)) {
>>> +		/* It needs to deassert the chip selection
>>> +		 * firstly before restart the SPP */
>>> +		need_cs_change = true;
>>> +		cs_deassert(spi);
>> I think code comes here at the beginning of each transfer so will be hit
>> multiple times before pxa2xx_spi_unprepare_transfer() if SPI message
>> consists of multiple transfers.
>> This makes me wondering if the device driver setting up the "struct
>> spi_transfer" is maybe missing the cs_change flag set for transfers before
>> last one in case HW needs CS toggling between transfers? For instance what
>> following drivers are doing with the cs_change flag:
>> drivers/char/tpm/tpm_tis_spi.c: tpm_tis_spi_transfer()
>> drivers/input/touchscreen/ad7877.c: ad7877_read(), ad7877_read_adc()
> Right, this really feels like it's fixing the wrong thing.

I check the cs_change flag in the spi_transfer_one_message(). The 
cs_change flag is used in two cases. If the transfer is the last one it 
is used to keep the CS assert. If the transfer is not the last one it's 
used to generate the 10us pulse and then de-assert the CS. In my case 
the transfer is the last one and it can work correctly if I re-assert 
the CS after restart the SSP in the pxa2xx_spi_transfer_one() which is 
called from spi_transfer_one_message(). From my experiments both 
cs_change flag cases in the spi_transfer_one_message() can't help the 
problem. I am not sure if I fully understand your point.

Re: [PATCH] [RFC] spi: pxa2xx: Do cs if restart the SSP during pxa2xx_spi_transfer_one()

[Xiao, Jin]

Hi

On 3/8/2019 12:09 AM, Mark Brown wrote:
> On Thu, Mar 07, 2019 at 05:26:53PM +0200, Jarkko Nikula wrote:
>> On 3/7/19 9:24 AM, xiao jin wrote:
>>> The patch is to do cs again if spi-pxa2xx restar the SSP during
>>> pxa2xx_spi_transfer_one()
>> Hmm.. please correct me if I'm wrong but pxa2xx_spi_unprepare_transfer() is
>> called always when there is no more messages pending and the spi core should
>> have deasserted the CS already?
> Yes.

I try to describe the situation in more detail.

             cpu0                         cpu1

     => spi_transfer_one_message

     => __spi_pump_messages

     => __spi_sync

     => spi_sync

     => spi_sync_transfer.constprop.2

     => spi_write

     => fm1388_spi_burst_write

     => fm1388_fw_loaded

     => request_firmware_work_func

     => process_one_work

                 => pxa2xx_spi_unprepare_transfer

                 => spi_pump_messages

                 => kthread_worker_fn

Yes, the spi core has de-asserted the CS before the 
pxa2xx_spi_unprepare_transfer(). The problem on my side is that the new 
transfer will restart the SSP in pxa2xx_spi_transfer_one(). The spi core 
has asserted the CS again before restart the SSP.  In my test the CS 
assert before the restart SSP can't ensure the spi transfer working 
correctly.

>>> @@ -1056,6 +1057,11 @@ static int pxa2xx_spi_transfer_one(struct spi_controller *master,
>>>    	if ((pxa2xx_spi_read(drv_data, SSCR0) != cr0)
>>>    	    || (pxa2xx_spi_read(drv_data, SSCR1) & change_mask)
>>>    	    != (cr1 & change_mask)) {
>>> +		/* It needs to deassert the chip selection
>>> +		 * firstly before restart the SPP */
>>> +		need_cs_change = true;
>>> +		cs_deassert(spi);
>> I think code comes here at the beginning of each transfer so will be hit
>> multiple times before pxa2xx_spi_unprepare_transfer() if SPI message
>> consists of multiple transfers.
>> This makes me wondering if the device driver setting up the "struct
>> spi_transfer" is maybe missing the cs_change flag set for transfers before
>> last one in case HW needs CS toggling between transfers? For instance what
>> following drivers are doing with the cs_change flag:
>> drivers/char/tpm/tpm_tis_spi.c: tpm_tis_spi_transfer()
>> drivers/input/touchscreen/ad7877.c: ad7877_read(), ad7877_read_adc()
> Right, this really feels like it's fixing the wrong thing.

I check the cs_change flag in the spi_transfer_one_message(). The 
cs_change flag is used in two cases. If the transfer is the last one it 
is used to keep the CS assert. If the transfer is not the last one it's 
used to generate the 10us pulse and then de-assert the CS. In my case 
the transfer is the last one and it can work correctly if I re-assert 
the CS after restart the SSP in the pxa2xx_spi_transfer_one() which is 
called from spi_transfer_one_message(). From my experiments both 
cs_change flag cases in the spi_transfer_one_message() can't help the 
problem. I am not sure if I fully understand your point.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] [RFC] spi: pxa2xx: Do cs if restart the SSP during pxa2xx_spi_transfer_one()

[Xiao, Jin]

Hi

On 3/8/2019 12:09 AM, Mark Brown wrote:
> On Thu, Mar 07, 2019 at 05:26:53PM +0200, Jarkko Nikula wrote:
>> On 3/7/19 9:24 AM, xiao jin wrote:
>>> The patch is to do cs again if spi-pxa2xx restar the SSP during
>>> pxa2xx_spi_transfer_one()
>> Hmm.. please correct me if I'm wrong but pxa2xx_spi_unprepare_transfer() is
>> called always when there is no more messages pending and the spi core should
>> have deasserted the CS already?
> Yes.

I try to describe the situation in more detail.

             cpu0                         cpu1

     => spi_transfer_one_message

     => __spi_pump_messages

     => __spi_sync

     => spi_sync

     => spi_sync_transfer.constprop.2

     => spi_write

     => fm1388_spi_burst_write

     => fm1388_fw_loaded

     => request_firmware_work_func

     => process_one_work

                 => pxa2xx_spi_unprepare_transfer

                 => spi_pump_messages

                 => kthread_worker_fn

Yes, the spi core has de-asserted the CS before the 
pxa2xx_spi_unprepare_transfer(). The problem on my side is that the new 
transfer will restart the SSP in pxa2xx_spi_transfer_one(). The spi core 
has asserted the CS again before restart the SSP.  In my test the CS 
assert before the restart SSP can't ensure the spi transfer working 
correctly.

>>> @@ -1056,6 +1057,11 @@ static int pxa2xx_spi_transfer_one(struct spi_controller *master,
>>>    	if ((pxa2xx_spi_read(drv_data, SSCR0) != cr0)
>>>    	    || (pxa2xx_spi_read(drv_data, SSCR1) & change_mask)
>>>    	    != (cr1 & change_mask)) {
>>> +		/* It needs to deassert the chip selection
>>> +		 * firstly before restart the SPP */
>>> +		need_cs_change = true;
>>> +		cs_deassert(spi);
>> I think code comes here at the beginning of each transfer so will be hit
>> multiple times before pxa2xx_spi_unprepare_transfer() if SPI message
>> consists of multiple transfers.
>> This makes me wondering if the device driver setting up the "struct
>> spi_transfer" is maybe missing the cs_change flag set for transfers before
>> last one in case HW needs CS toggling between transfers? For instance what
>> following drivers are doing with the cs_change flag:
>> drivers/char/tpm/tpm_tis_spi.c: tpm_tis_spi_transfer()
>> drivers/input/touchscreen/ad7877.c: ad7877_read(), ad7877_read_adc()
> Right, this really feels like it's fixing the wrong thing.

I check the cs_change flag in the spi_transfer_one_message(). The 
cs_change flag is used in two cases. If the transfer is the last one it 
is used to keep the CS assert. If the transfer is not the last one it's 
used to generate the 10us pulse and then de-assert the CS. In my case 
the transfer is the last one and it can work correctly if I re-assert 
the CS after restart the SSP in the pxa2xx_spi_transfer_one() which is 
called from spi_transfer_one_message(). From my experiments both 
cs_change flag cases in the spi_transfer_one_message() can't help the 
problem. I am not sure if I fully understand your point.

Re: [PATCH] [RFC] spi: pxa2xx: Do cs if restart the SSP during pxa2xx_spi_transfer_one()

[Jarkko Nikula]

Hi Jin

On 3/8/19 9:28 AM, Xiao, Jin wrote:
> 
> Yes, the spi core has de-asserted the CS before the 
> pxa2xx_spi_unprepare_transfer(). The problem on my side is that the new 
> transfer will restart the SSP in pxa2xx_spi_transfer_one(). The spi core 
> has asserted the CS again before restart the SSP.  In my test the CS 
> assert before the restart SSP can't ensure the spi transfer working 
> correctly.
> 
I wanted to ask have you found any other fix or reason than the patches 
you have sent?

I've been trying to debug why the commit d5898e19c0d7 ("spi: pxa2xx: Use 
core message processing loop") is causing the regression you are seeing.

I've been testing this by sending two or more messages each consisting 
of two 4-byte transfers through the spidev. SPI mode 3 and clock is 1 
MHz. I see only slight timing difference between a commit before and at 
d5898e19c0d7 when the chip select is active.

8ae55af38817: CS act to CLK running ~40 us. CLK idle to CS deact ~26 us
d5898e19c0d7: CS act to CLK running ~34 us. CLK idle to CS deact ~18 us

There is more difference/variability during the times when the CS is not 
active between messages. Which is understandable since there happens 
message passing from userspace, queing, etc.

At 8ae55af38817 total time over 2 messages varies from ~600 us to 1200 
~us and at d5898e19c0d7 from ~500 us to 1100 us. Then at v4.19 it goes a 
bit slover and varies between ~700 to 1600 us. Most probably due some 
other change than in SPI subsystem as there is not commit either in SPI 
core or spi-pxa2xx explaining it. Or just some random config change in 
my kernel configuration between 4.17 and 4.19.

What I don't understand why additional CS toggling makes it work for you 
in case SSP was stopped in pxa2xx_spi_unprepare_transfer() and restarted 
again in pxa2xx_spi_transfer_one(). This SSP stopping and restarting is 
not visible in the SPI signals since clock is already stopped when FIFO 
gets empty.

I guess the reason why you are seeing pxa2xx_spi_unprepare_transfer() 
only called sometimes is that in those cases where it is not called a 
new SPI message got queued while processing the current one and in other 
cases queue became empty before new message was queued.

In both cases there shouldn't be difference in CS handling. Which makes 
me scratching my head why additional CS toggling is fixing the issue in 
case there is SSP restart. And which was due more time elapsed between 
messages and queue went empty.

-- 
Jarkko

Re: [PATCH] [RFC] spi: pxa2xx: Do cs if restart the SSP during pxa2xx_spi_transfer_one()

[Jarkko Nikula]

Hi Jin

On 3/8/19 9:28 AM, Xiao, Jin wrote:
> 
> Yes, the spi core has de-asserted the CS before the 
> pxa2xx_spi_unprepare_transfer(). The problem on my side is that the new 
> transfer will restart the SSP in pxa2xx_spi_transfer_one(). The spi core 
> has asserted the CS again before restart the SSP.  In my test the CS 
> assert before the restart SSP can't ensure the spi transfer working 
> correctly.
> 
I wanted to ask have you found any other fix or reason than the patches 
you have sent?

I've been trying to debug why the commit d5898e19c0d7 ("spi: pxa2xx: Use 
core message processing loop") is causing the regression you are seeing.

I've been testing this by sending two or more messages each consisting 
of two 4-byte transfers through the spidev. SPI mode 3 and clock is 1 
MHz. I see only slight timing difference between a commit before and at 
d5898e19c0d7 when the chip select is active.

8ae55af38817: CS act to CLK running ~40 us. CLK idle to CS deact ~26 us
d5898e19c0d7: CS act to CLK running ~34 us. CLK idle to CS deact ~18 us

There is more difference/variability during the times when the CS is not 
active between messages. Which is understandable since there happens 
message passing from userspace, queing, etc.

At 8ae55af38817 total time over 2 messages varies from ~600 us to 1200 
~us and at d5898e19c0d7 from ~500 us to 1100 us. Then at v4.19 it goes a 
bit slover and varies between ~700 to 1600 us. Most probably due some 
other change than in SPI subsystem as there is not commit either in SPI 
core or spi-pxa2xx explaining it. Or just some random config change in 
my kernel configuration between 4.17 and 4.19.

What I don't understand why additional CS toggling makes it work for you 
in case SSP was stopped in pxa2xx_spi_unprepare_transfer() and restarted 
again in pxa2xx_spi_transfer_one(). This SSP stopping and restarting is 
not visible in the SPI signals since clock is already stopped when FIFO 
gets empty.

I guess the reason why you are seeing pxa2xx_spi_unprepare_transfer() 
only called sometimes is that in those cases where it is not called a 
new SPI message got queued while processing the current one and in other 
cases queue became empty before new message was queued.

In both cases there shouldn't be difference in CS handling. Which makes 
me scratching my head why additional CS toggling is fixing the issue in 
case there is SSP restart. And which was due more time elapsed between 
messages and queue went empty.

-- 
Jarkko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel