On 04/03/2012 08:35 AM, Prafulla Wadaskar wrote: > > >> -----Original Message----- >> From: Valentin Longchamp [mailto:valentin.longch...@keymile.com] >> Sent: 02 April 2012 19:07 >> To: Prafulla Wadaskar >> Cc: u-boot@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.longch...@keymile.com] >>>> Sent: 30 March 2012 17:45 >>>> To: Prafulla Wadaskar >>>> Cc: u-boot@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
