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

[Prafulla Wadaskar]

> -----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.

> 
> 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.

> 
> 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 :-)

> 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)

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

Regards..
Prafulla . . .

> 
> Regards
> 
> --
> Valentin Longchamp
> Embedded Software Engineer
> Hardware and Chip Integration
> ______________________________________
> KEYMILE AG
> Schwarzenburgstr. 73
> CH-3097 Liebefeld
> Phone +41 31 377 1318
> Fax   +41 31 377 1212
> valentin.longchamp at keymile.com
> www.keymile.com
> ______________________________________
> KEYMILE: A Specialist as a Partner