On Tue, Jun 04, 2013 at 04:02:34PM +0200, Michal Simek wrote:
> The ISR currently consumes the rx buffer data and re-enables transmission
> from within interrupt context. This is bad because if the interrupt
> occurs again before the ISR exits, the new interrupt will be erroneously
> cleared by the still completing ISR.
> Simplified the ISR by just setting the completion variable and exiting with
> no action. Then just looped the transmit functionality in
> xilinx_spi_txrx_bufs().
Applied but this is a bit sad, having to defer the refill to process
context means that we're adding extra latency which takes us further
away from being able to saturate the bus. There ought to be a way to
avoid the issue though I can't think of a non-racy one - I guess level
triggered interrupts aren't an option?
On Tue, Jun 04, 2013 at 04:02:35PM +0200, Michal Simek wrote:
> From: Michal Simek <[email protected]>
>
> mmc_spi driver tests if dma is available
> through spi->master->dev.parent->dma_mask.
> Microblaze supports DMA but xilinx_spi IP doesn't.
> That's why clear dma_mask in the driver.
> + /* clear the dma_mask, to try to disable use of dma */
> + master->dev.dma_mask = 0;
> +
This looks like a bodge in the wrong place. Either the device
registration is incorrect in that it advertises DMA when none is
available or the SPI driver ought to be offering the MMC driver a more
sensible way of advertising this limitation. My first thought is the
former but I didn't check where dma_mask is getting set.
On Tue, Jun 04, 2013 at 04:02:36PM +0200, Michal Simek wrote:
> Do not load endian value from platform data
> and rather autodetect it.
Applied, thanks.
On 06/04/2013 07:36 PM, Mark Brown wrote:
> On Tue, Jun 04, 2013 at 04:02:35PM +0200, Michal Simek wrote:
>> From: Michal Simek <[email protected]>
>>
>> mmc_spi driver tests if dma is available
>> through spi->master->dev.parent->dma_mask.
>> Microblaze supports DMA but xilinx_spi IP doesn't.
>> That's why clear dma_mask in the driver.
>
>> + /* clear the dma_mask, to try to disable use of dma */
>> + master->dev.dma_mask = 0;
>> +
>
> This looks like a bodge in the wrong place. Either the device
> registration is incorrect in that it advertises DMA when none is
> available or the SPI driver ought to be offering the MMC driver a more
> sensible way of advertising this limitation. My first thought is the
> former but I didn't check where dma_mask is getting set.
I have looked at history of this change and we have done it 3 years ago
based on one custom configuration.
It is shame that I don't have hw to test this but there was
something wrong in connection to mmc_spi.c.
I will try to find out hw for this to test but probably
won't be available. :-( And I will just revert this change in my tree.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
>
>>
>> =============
>> spi: xilinx - minimize iomem reads
>>
>> If this IP core is accessed through bridges like PCI-e, reads are rather
>> costly. Doing many reads or read-modify-writes is thus long and strenuous
>> on the CPU (active waiting).
>>
>> The transfer workflow of this driver allows some assumptions to be made and
>> exploited to minimize reads as much as possible.
>>
>> These two assumptions are made:
>> - since we are in control of the CR register, cache it so we don't have to
>> read it all the time to modify it.
>
> Makes sense.
I have made an attempt at it can you check if you get any performance
improvemets
on your setup.
http://www.spinics.net/lists/linux-spi/msg05963.html
Thanks,
Shubhrajyoti