Re: R: spi_async and heavy CPU occupation by ksoftirqd

Re: R: spi_async and heavy CPU occupation by ksoftirqd

[apaterniani.swapp-IWqWACnzNjyonA0d6jMUrA]

I'll back in my office on Aug 27.

- Andrea




____________________________________________________________
ADSL 4 MB FLAT
navighi GRATIS per tutto il 2007 
Affrettati! l'offerta è valida sino al 23 agosto 2007 
http://abbonati.tiscali.it/adsl/prodotti/tc/4flat/


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

Re: spi_async and heavy CPU occupation by ksoftirqd

[llandre]

I think I found the cause.
Since I needed to enable the RDY control (SPI_CONTROL_DRCTL_2) as 
required by the SPI slave (Quantum QT510), the actual SPI transactions 
may occur after the driver wrote the TX FIFO (if (write(drv_data))).
Thus

while ((read(drv_data) == 0) && limit--);

waits until the actual transaction have been completed, that happens 
when the slave is ready to accept data.
So I'm afraid I need modify the MXL driver in order to support this case.

-- 
llandre

DAVE Electronics System House - R&D Department
web:   http://www.dave-tech.it
email: r&d2-4VKA1VU3ct/j+vYz1yj4TQ@public.gmane.org


> Is there any logged error message?
> May be that limit reaches 0 but in this case an error is logged (trailing bytes read failed).
> 
> - Andrea
> 
> 
> 
> 
> 
>> -----Messaggio originale-----
>> Da: llandre [mailto:r&d2-4VKA1VU3ct/j+vYz1yj4TQ@public.gmane.org]
>> Inviato: venerdì 10 agosto 2007 10.52
>> A: David Brownell
>> Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Andrea Paterniani
>> Oggetto: Re: [spi-devel-general] spi_async and heavy CPU occupation by
>> ksoftirqd
>>
>>
>>> That seems to be the most precise tool you have, unless you
>>> have access to the JTAG-style hardware tools giving access
>>> to the on-chip instruction trace buffers found in many ARMs.
>> Unfortunatley trace port is not available so I have to use the old
>> fashioned debug pin technique.
>>
>>>> or does the kernel provides some
>>>> tools to do that? Could oprofile come to help?
>>> Statistical profiling probably wouldn't help.  I'm not sure that
>>> anything like gprof is working in the kernel.  Folk that I know
>>> have done that level analysis tend to rely on hardware trace tools.
>> I see.
>>
>>>>> I'd expect that using spi_sync() wouldn't save much, but it might
>>>>> be worth verifying that.
>>>> Initially my driver used spi_sync. When I realized the huge CPU
>>>> occupation I modified it in order to use spi_async instead but, again,
>>>> nothing changed.
>>> And that should have been a HUGE clue that the way you were calling
>>> it wasn't a factor at all.
>> I moved the set/clear pin functions inside the iMXL master driver and I
>> found the problem is at line 804 inside the interrupt handler:
>>
>> 	qt510_debug_pin(1);
>> 	while ((read(drv_data) == 0) && limit--);
>> 	qt510_debug_pin(0);
>>
>> Usually the high pulse lasts about 10ms. Occasionally (about 1 time out
>> of 3) it lasts about 300 ms ! That would explain the ksoftirqd abnormal
>> CPU occupation.
>> I hope Andrea can come to help.
>>
>>
>>
>> --
>> llandre
>>
>> DAVE Electronics System House - R&D Department
>> web:   http://www.dave-tech.it
>> email: r&d2-4VKA1VU3ct/j+vYz1yj4TQ@public.gmane.org
> 
> 
> 



-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

R: spi_async and heavy CPU occupation by ksoftirqd

[Andrea Paterniani]

You're right the current version of the driver doesn't support RDY control.
Infact SPI_DEFAULT_CONTROL is defined with SPI_CONTROL_DRCTL_0.
Also setup function doesn't managed DRCTL changes.

Then you probably changes SPI_DEFAULT_CONTROL to use SPI_CONTROL_DRCTL_2.

Please do your suggestions for driver modification in order to release a driver patch.

- Andrea



> -----Messaggio originale-----
> Da: llandre [mailto:r&d2-4VKA1VU3ct/j+vYz1yj4TQ@public.gmane.org]
> Inviato: venerdì 10 agosto 2007 13.33
> A: Andrea Paterniani
> Cc: David Brownell; spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> Oggetto: Re: [spi-devel-general] spi_async and heavy CPU occupation by
> ksoftirqd
>
>
> I think I found the cause.
> Since I needed to enable the RDY control (SPI_CONTROL_DRCTL_2) as
> required by the SPI slave (Quantum QT510), the actual SPI transactions
> may occur after the driver wrote the TX FIFO (if (write(drv_data))).
> Thus
>
> while ((read(drv_data) == 0) && limit--);
>
> waits until the actual transaction have been completed, that happens
> when the slave is ready to accept data.
> So I'm afraid I need modify the MXL driver in order to support this case.
>
> --
> llandre
>
> DAVE Electronics System House - R&D Department
> web:   http://www.dave-tech.it
> email: r&d2-4VKA1VU3ct/j+vYz1yj4TQ@public.gmane.org
>
>
> > Is there any logged error message?
> > May be that limit reaches 0 but in this case an error is logged (trailing bytes read failed).
> >
> > - Andrea
> >
> >
> >
> >
> >
> >> -----Messaggio originale-----
> >> Da: llandre [mailto:r&d2-4VKA1VU3ct/j+vYz1yj4TQ@public.gmane.org]
> >> Inviato: venerdì 10 agosto 2007 10.52
> >> A: David Brownell
> >> Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Andrea Paterniani
> >> Oggetto: Re: [spi-devel-general] spi_async and heavy CPU occupation by
> >> ksoftirqd
> >>
> >>
> >>> That seems to be the most precise tool you have, unless you
> >>> have access to the JTAG-style hardware tools giving access
> >>> to the on-chip instruction trace buffers found in many ARMs.
> >> Unfortunatley trace port is not available so I have to use the old
> >> fashioned debug pin technique.
> >>
> >>>> or does the kernel provides some
> >>>> tools to do that? Could oprofile come to help?
> >>> Statistical profiling probably wouldn't help.  I'm not sure that
> >>> anything like gprof is working in the kernel.  Folk that I know
> >>> have done that level analysis tend to rely on hardware trace tools.
> >> I see.
> >>
> >>>>> I'd expect that using spi_sync() wouldn't save much, but it might
> >>>>> be worth verifying that.
> >>>> Initially my driver used spi_sync. When I realized the huge CPU
> >>>> occupation I modified it in order to use spi_async instead but, again,
> >>>> nothing changed.
> >>> And that should have been a HUGE clue that the way you were calling
> >>> it wasn't a factor at all.
> >> I moved the set/clear pin functions inside the iMXL master driver and I
> >> found the problem is at line 804 inside the interrupt handler:
> >>
> >> 	qt510_debug_pin(1);
> >> 	while ((read(drv_data) == 0) && limit--);
> >> 	qt510_debug_pin(0);
> >>
> >> Usually the high pulse lasts about 10ms. Occasionally (about 1 time out
> >> of 3) it lasts about 300 ms ! That would explain the ksoftirqd abnormal
> >> CPU occupation.
> >> I hope Andrea can come to help.
> >>
> >>
> >>
> >> --
> >> llandre
> >>
> >> DAVE Electronics System House - R&D Department
> >> web:   http://www.dave-tech.it
> >> email: r&d2-4VKA1VU3ct/j+vYz1yj4TQ@public.gmane.org
> >
> >
> >
>
>


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

Re: R: spi_async and heavy CPU occupation by ksoftirqd

[llandre]

AFAIK introducing support for RDY in the driver is not a trivial matter.
Since
a) I just need to perform 1-byte transactions to communicate with 
Quantum QT510
b) I don't have much time to spend on this issue (customer needs to 
assemble a demo quickly)
I'm afraid I'll have to write a dirty patch just to make my specific 
configuration work.

I'll keep you informed anyway.

Regards,
llandre

DAVE Electronics System House - R&D Department
web:   http://www.dave-tech.it
email: r&d2-4VKA1VU3ct/j+vYz1yj4TQ@public.gmane.org



> You're right the current version of the driver doesn't support RDY control.
> Infact SPI_DEFAULT_CONTROL is defined with SPI_CONTROL_DRCTL_0.
> Also setup function doesn't managed DRCTL changes.
> 
> Then you probably changes SPI_DEFAULT_CONTROL to use SPI_CONTROL_DRCTL_2.
> 
> Please do your suggestions for driver modification in order to release a driver patch.
> 
> - Andrea


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

Re: R: spi_async and heavy CPU occupation by ksoftirqd

[llandre]

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

Atatched is a dirty ugly patch that seems to work for 1-byte transfers I 
use with QT510.
In case RDY control is enabled, transfer is completed by

interrupt_data_ready (new irq handler)
pump_transfers (tasklet)

in this order.


Regards,
llandre

DAVE Electronics System House - R&D Department
web:   http://www.dave-tech.it
email: r&d2-4VKA1VU3ct/j+vYz1yj4TQ@public.gmane.org


> AFAIK introducing support for RDY in the driver is not a trivial matter.
> Since
> a) I just need to perform 1-byte transactions to communicate with 
> Quantum QT510
> b) I don't have much time to spend on this issue (customer needs to 
> assemble a demo quickly)
> I'm afraid I'll have to write a dirty patch just to make my specific 
> configuration work.
> 
> I'll keep you informed anyway.
> 
> Regards,
> llandre
> 
> DAVE Electronics System House - R&D Department
> web:   http://www.dave-tech.it
> email: r&d2-4VKA1VU3ct/j+vYz1yj4TQ@public.gmane.org
> 
> 
> 
>> You're right the current version of the driver doesn't support RDY control.
>> Infact SPI_DEFAULT_CONTROL is defined with SPI_CONTROL_DRCTL_0.
>> Also setup function doesn't managed DRCTL changes.
>>
>> Then you probably changes SPI_DEFAULT_CONTROL to use SPI_CONTROL_DRCTL_2.
>>
>> Please do your suggestions for driver modification in order to release a driver patch.
>>
>> - Andrea
> 
> 
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc.
> Still grepping through log files to find problems?  Stop.
> Now Search log events and configuration files using AJAX and a browser.
> Download your FREE copy of Splunk now >>  http://get.splunk.com/
> _______________________________________________
> spi-devel-general mailing list
> spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/spi-devel-general
> 
> 


[-- Attachment #2: spi-imx-rdy.patch --]
[-- Type: text/plain, Size: 4151 bytes --]

diff --git a/drivers/spi/spi_imx.c b/drivers/spi/spi_imx.c
index 25c22ff..2b9ab9c 100644
--- a/drivers/spi/spi_imx.c
+++ b/drivers/spi/spi_imx.c
@@ -148,6 +148,7 @@
 #define SPI_DEFAULT_ENABLE_LOOPBACK	(0)
 #define SPI_DEFAULT_ENABLE_DMA		(0)
 #define SPI_DEFAULT_PERIOD_WAIT		(8)
+#define SPI_IS_RDY_ENABLED			((SPI_DEFAULT_CONTROL & SPI_CONTROL_DRCTL_2) ? 1 : 0)
 /*-------------------------------------------------------------------------*/
 
 
@@ -281,7 +282,7 @@ static int flush(struct driver_data *drv_data)
 		while (readl(regs + SPI_INT_STATUS) & SPI_STATUS_RR)
 			d = readl(regs + SPI_RXDATA);
 	} while ((readl(regs + SPI_CONTROL) & SPI_CONTROL_XCH) && limit--);
-
+	
 	return limit;
 }
 
@@ -754,6 +755,20 @@ static irqreturn_t interrupt_wronly_transfer(struct driver_data *drv_data)
 	return handled;
 }
 
+static irqreturn_t interrupt_data_ready(struct driver_data *drv_data){
+	void __iomem *regs = drv_data->regs;
+
+	/* Read rx data */
+	read(drv_data);
+	/* Disable irq */
+	writel(readl(regs + SPI_INT_STATUS) & ~SPI_INTEN,
+		regs + SPI_INT_STATUS);
+	/* Schedule transfer tasklet */
+	tasklet_schedule(&drv_data->pump_transfers);
+	
+	return IRQ_HANDLED;
+}
+
 static irqreturn_t interrupt_transfer(struct driver_data *drv_data)
 {
 	struct spi_message *msg = drv_data->cur_msg;
@@ -763,7 +778,7 @@ static irqreturn_t interrupt_transfer(struct driver_data *drv_data)
 	unsigned long limit;
 
 	status = readl(regs + SPI_INT_STATUS);
-
+	
 	while (status & (SPI_STATUS_TH | SPI_STATUS_RO)) {
 		dev_dbg(&drv_data->pdev->dev,
 			"interrupt_transfer - status = 0x%08X\n", status);
@@ -785,23 +800,30 @@ static irqreturn_t interrupt_transfer(struct driver_data *drv_data)
 
 			msg->state = ERROR_STATE;
 			tasklet_schedule(&drv_data->pump_transfers);
-
+			
 			return IRQ_HANDLED;
 		}
 
 		/* Pump data */
 		read(drv_data);
 		if (write(drv_data)) {
-			writel(readl(regs + SPI_INT_STATUS) & ~SPI_INTEN,
-				regs + SPI_INT_STATUS);
+			if (SPI_IS_RDY_ENABLED) {
+				/* RX IRQ handler will complete the transfer */
+				drv_data->transfer_handler = interrupt_data_ready;
+				writel((readl(regs + SPI_INT_STATUS) & ~SPI_INTEN) | SPI_INTEN_RE,
+					regs + SPI_INT_STATUS);
+				goto skip_rx_poll;
+			} else {
+				writel(readl(regs + SPI_INT_STATUS) & ~SPI_INTEN,
+					regs + SPI_INT_STATUS);
+			}
 
 			dev_dbg(&drv_data->pdev->dev,
 				"interrupt_transfer - end of tx\n");
-
+			
 			/* Read trailing bytes */
 			limit = loops_per_jiffy << 1;
 			while ((read(drv_data) == 0) && limit--);
-
 			if (limit == 0)
 				dev_err(&drv_data->pdev->dev,
 					"interrupt_transfer - "
@@ -810,6 +832,7 @@ static irqreturn_t interrupt_transfer(struct driver_data *drv_data)
 				dev_dbg(&drv_data->pdev->dev,
 					"interrupt_transfer - end of rx\n");
 
+skip_rx_poll:
 			/* End of transfer, update total byte transfered */
 			msg->actual_length += drv_data->len;
 
@@ -821,9 +844,11 @@ static irqreturn_t interrupt_transfer(struct driver_data *drv_data)
 			/* Move to next transfer */
 			msg->state = next_transfer(drv_data);
 
-			/* Schedule transfer tasklet */
-			tasklet_schedule(&drv_data->pump_transfers);
-
+			if (!SPI_IS_RDY_ENABLED) {
+				/* Schedule transfer tasklet */
+				tasklet_schedule(&drv_data->pump_transfers);
+			}
+			
 			return IRQ_HANDLED;
 		}
 
@@ -832,7 +857,7 @@ static irqreturn_t interrupt_transfer(struct driver_data *drv_data)
 		/* We did something */
 		handled = IRQ_HANDLED;
 	}
-
+	
 	return handled;
 }
 
@@ -949,7 +974,7 @@ static void pump_transfers(unsigned long data)
 
 	/* Assert device chip-select */
 	drv_data->cs_control(SPI_CS_ASSERT);
-
+	
 	/* DMA cannot read/write SPI FIFOs other than 16 bits at a time; hence
 	   if bits_per_word is less or equal 8 PIO transfers are performed.
 	   Moreover DMA is convinient for transfer length bigger than FIFOs
@@ -1089,7 +1114,7 @@ static void pump_messages(struct work_struct *work)
 	/* Setup the SPI using the per chip configuration */
 	drv_data->cur_chip = spi_get_ctldata(drv_data->cur_msg->spi);
 	restore_state(drv_data);
-
+	
 	/* Mark as busy and launch transfers */
 	tasklet_schedule(&drv_data->pump_transfers);
 }

[-- Attachment #3: Type: text/plain, Size: 315 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

[-- Attachment #4: Type: text/plain, Size: 210 bytes --]

_______________________________________________
spi-devel-general mailing list
spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/spi-devel-general