Re: [PATCH] SPI

[Vitaly Wool <vwool@ru.mvista.com>]

David Brownell wrote:

>>>You're imposing the same implementation strategy Mark Underwood was.
>>>I believe I persuaded him not to want that, pointing out three other
>>>implementation strategies that can be just as reasonable:
>>>
>>>  http://marc.theaimsgroup.com/?l=linux-kernel&m=112684135722116&w=2
>>>
>>>It'd be fine if for example your PNX controller driver worked that way
>>>internally.  But other drivers shouldn't be forced to allocate kernel
>>>threads when they don't need them.
>>>      
>>>
>>Hm, so does that imply that the whole -rt patches from 
>>Ingo/Sven/Daniel/etc. are implementing wrong strategy (interrupts in 
>>threads)?
>>    
>>
>
>In an RT context, it may make sense to impose a policy like that.
>
>But I recall those folk have said they're making things so that sanely
>behaved kernel code will work with no changes.  And also that not all
>kernels should be enabling RT support ...
>
>  
>
FYI in brief: for PREEMPT_RT case all the interrupt handlers are working 
in a separate thread each unless explicitly specified otherwise.
This strategy proven itself to be quite functional and not significantly 
impacting the performance.
So, typically this means allocation of five to eight threads for 
interrupt handlers.
We will definitely have less SPI busses => less kernel threads, so I 
doubt there's a rationale in your opinion.

>>How will your strategy work with that BTW?
>>    
>>
>
>If they meet their goals, it'll work just fine.  Sanely behaved
>implementations will continue to work, and not even notice.
>  
>
Hm. How are you going to call something from an interrupt context? I 
guess that's gonna happen from an interrupt handler.
So there're 2 ways for PREEMPT_RT:
1) you're playing according to the rules  and don't specify SA_NODELAY 
=> the interrupt handler is in thread => bigger latencies for SPI than 
if we had separate kernel threads for each bus
2) you're not following the rules as specify SA_NODELAY => you're 
working in interrupt context creating a significant load to the 
interrupt context and slowing down everything that *is* following the rules.
So either SPI performance suffers, or RT performance does. :(

>
>
>  
>
>>>>+  [ picture deleted ]
>>>>        
>>>>
>>>That seems wierd even if I assume "platform_bus" is just an example.
>>>For example there are two rather different "spi bus" notions there,
>>>and it looks like neither one is the physical parent of any SPI device ...
>>>      
>>>
>>Not sure if I understand you :(
>>    
>>
>
>Why couldn't for example SPI sit on a PCI bus?
>
>And call the two boxes different things, if they're really different.
>The framework I've posted has "spi_master" as a class implmented
>by certain controller drivers.  (Others might use "spi_slave", and
>both would be types of "SPI bus".)  That would at least clarify
>the confusion on the left half of that picture.
>
>
>
>  
>
>>>>+3.2 How do the SPI devices gets discovered and probed ?
>>>>   
>>>>        
>>>>
>>>Better IMO to have tables that get consulted when the SPI master controller
>>>drivers register the parent ... tables that are initialized by the board
>>>specific __init section code, early on.  (Or maybe by __setup commandline
>>>parameters.)
>>>
>>>Doing it the way you are prevents you from declaring all the SPI devices in
>>>a single out-of-the-way location like the arch/.../board-specific.c file,
>>>which is normally responsible for declaring devices that are hard-wired on
>>>a given board and can't be probed.
>>> 
>>>
>>>      
>>>
>>By what means does it prevent that?
>>    
>>
>
>Well "prevent" may be a bit strong, if you like hopping levels in
>the software stack.  I don't; without such hopping (or without a
>separate out-of-band mechanism like device tables), I don't see
>a way to solve that problem.
>  
>
Aren't the tables you're suggesting also kinda out-of-band stuff?

>
>  
>
>>>>+#define SPI_MAJOR	153
>>>>+
>>>>+...
>>>>+
>>>>+#define SPI_DEV_CHAR "spi-char"
>>>>   
>>>>
>>>>        
>>>>
>>I thought 153 was the official SPI device number.
>>    
>>
>
>So it is (at least for minors 0..15, so long as they use some
>API I can't find a spec for), but that wasn't the point.  The
>point is to keep that sort of driver-specific information from
>cluttering headers which are addressed to _every_ driver.
>  
>
Yup, thanks for clarifications.

Vitaly