[U-Boot] [PATCH v2] spi/kirkwood: add weak functions board_spi_bus_claim/release

[Valentin Longchamp]

On 04/03/2012 08:35 AM, Prafulla Wadaskar wrote:
> 
> 
>> -----Original Message-----
>> From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
>> Sent: 02 April 2012 19:07
>> To: Prafulla Wadaskar
>> Cc: u-boot at lists.denx.de; Gerlando Falauto; Holger Brunck
>> Subject: Re: [PATCH v2] spi/kirkwood: add weak functions
>> board_spi_bus_claim/release
>>
>> Dear Prafulla,
>>
>> On 03/30/2012 02:58 PM, Prafulla Wadaskar wrote:
>>>> -----Original Message-----
>>>> From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
>>>> Sent: 30 March 2012 17:45
>>>> To: Prafulla Wadaskar
>>>> Cc: u-boot at lists.denx.de; Gerlando Falauto; Holger Brunck
>>>> Subject: Re: [PATCH v2] spi/kirkwood: add weak functions
>>>> board_spi_bus_claim/release
>>>>
>>>> Hi Prafulla,
>>>>
>>>> For the simplicity of the discussion, I have removed everything in
>> the
>>>> discussion that is not relevant for the current open point.
>>>>
>>>> On 03/30/2012 01:34 PM, Prafulla Wadaskar wrote:
>>>>> In Kirkwood specific claim_bus API, you will backup default
>>>> configuration (which is NF in your case) for these particular pins
>>>> (SPI_SI, SPI_SCK, SPI_CSn, MOSI, MISO).
>>>>
>>>> But which MPP are these particular pins ?
>>>>
>>>> Let's have a look at a single signal, SPI_SCK for instance. From
>> the
>>>> 88F6281
>>>> Hardware Spec [1], page 53, SPI_SCK can be MPP[2], MPP[10]. How can
>>>> the generic
>>>> driver know which one actually is wired to the SPI device SCK pin
>> on
>>>> the
>>>> currently running hardware (when none is configured as then, since
>> by
>>>> default
>>>> for us MPP[2] is NF_IO[4] and MPP[10] is UA0_TXD ) ? This is a
>> board
>>>> specific !
>>>>
>>>> If you tell me how I easily can find this out in the kirkwood
>> driver,
>>>
>>> Dear Valentin
>>>
>>> Please make a use of CONFIG_SF_DEFAULT_CS for this.
>>
>> Is this really the correct #define to use ? From the documentation,
>> this is
>> supposed to set the Serial Flash CS. OK our SPI controller is used for
>> serial
>> flash access, but this may be used for something else.
> 
> Dear Valentin
> 
> I don't think there should be any issue, finally those are defined to configure CS (one of mpp) for arch specific SPI driver. And we are just extending its usage for other SPI signals.
> 
>>
>> So I would not use this #define, nor any that is related to Serial
>> Flash support.
>>
>>> By default this is defined to 0, lets map it to our default use case
>> i.e. using MPP0-3 for default SPI signals
>>>
>>> One may pre-define this in his board config as per specific need,
>> and we can use this effectively in Kirkwood_spi driver.
>>> i.e.
>>>  bit0 to be used to configure SPI_CSn
>>>  bit1 to be used to configure SPI_MOSI... and so on
>>>
>>> so if my needs are to use
>>> 1. MPP7 as SPI_CSn I will define CONFIG_SF_DEFAULT_CS to 0x01
>>> 2. MPP6 as SPI_MOSI I will define CONFIG_SF_DEFAULT_CS to 0x02
>>> 2. MPP6 as SPI_MOSI and MPP7 as SPI_CSn, I will define
>> CONFIG_SF_DEFAULT_CS to 0x03
>>> .. and so on.
>>>
>>
>> Yeah I see what you mean and that's what I had figured out. But I
>> don't think
>> this is a simple solution:
> 
> But this is better than providing weak APIs to avoid code duplication.

I agree with you about the code duplication, that's definitely the weak point of
my solution.

> 
>>
>> 1) We would have to add another CONFIG_SYS_XY for this purpose (as
>> explained above)
> 
> Okay, this is also a better solution.
> 
>>
>> 2) The two spi_claim_bus and spi_release_bus functions should
>> implement some bit
>> masking on the new CONFIG_SYS_XY to determine 4 pins that have to be
>> configured.
>> The number of code lines would not be huge, but that would still be a
>> lot more
>> than the 10 lines that my board specific functions would require. 10
>> sequencial
>> c line codes/board are from my point of view acceptable (and it would
>> be only
>> for our board, since I see no other board using this).
> 
> I don't think just from your board point of view, just think if there are 15 more boards being mainlined needs this support.

You mean there are 15 more boards that would implement the feature you tell us
below we should have not implemented ?

> 
>>
>> OK genericity is nice, but I guess we would be the only ones using
>> this code,
> 
> Today... who knows tomorrow :-), if I would have supported this feature in the beginning, I would have followed this method to expose this feature, that you would have used :-)

Using something does not mean that's its design was the correct one ...

> 
>> and it would not be worth it in this case. Additionnaly, I like the
>> fact that
>> anything that has to do with the mpp configurations stays in one
>> single board
>> specific .c file.
> 
> I agree, but this is dynamic usage of MPPs, spi_claim/release are SPI functions, so let SPI driver handle it.
> 
> Even I can say : you should not have designed a board which shares the same mpps for two peripheral which are actively used (not either/or)

So you mean, we should not have designed a board that uses the feature you want
me to implement a generic way because there is a s/w framework in place for ?
That's not a fair argument ...

> 
> Any ways, these are requirements, s/w has framework in place, so why not to use it in generic way?
> 

Anyways, you are the custodian and even if I'm still not completely convinced by
your arguments I will do it your way. Let's end this discussion here and next
time I will come back to you about it, it will be with a patch doing bit masking
on a new CONFIG_SYS_KW_SPI_MPP to know which MPP are used by the SPI controller.

Valentin