| Message ID | 1332755932-21259-1-git-send-email-valentin.longchamp@keymile.com |
|---|---|
| State | Rejected |
| Delegated to: | Prafulla Wadaskar |
| Headers | show |
Prafulla, On 03/26/2012 11:58 AM, Valentin Longchamp wrote: > Some kirkwood based boards may need to implement such function due to > some HW designs. I see no feedback from your side on this patch. I think you should go through the marvell tree: - the spi_claim/release_bus function are already implemented in the SPI subsystem - this patch touches only a kirkwood driver - there is no spi u-boot tree from what I see Please keep me up to date about the status of this patch Regards Valentin > > Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com> > cc: Gerlando Falauto <gerlando.falauto@keymile.com> > cc: Prafulla Wadaskar <prafulla@marvell.com> > cc: Holger Brunck <holger.brunck@keymile.com> > --- > drivers/spi/kirkwood_spi.c | 12 +++++++++++- > 1 files changed, 11 insertions(+), 1 deletions(-) > > diff --git a/drivers/spi/kirkwood_spi.c b/drivers/spi/kirkwood_spi.c > index db8ba8b..058dae2 100644 > --- a/drivers/spi/kirkwood_spi.c > +++ b/drivers/spi/kirkwood_spi.c > @@ -86,13 +86,23 @@ void spi_free_slave(struct spi_slave *slave) > free(slave); > } > > -int spi_claim_bus(struct spi_slave *slave) > +__attribute__((weak)) int board_spi_claim_bus(struct spi_slave *slave) > { > return 0; > } > > +int spi_claim_bus(struct spi_slave *slave) > +{ > + return board_spi_claim_bus(slave); > +} > + > +__attribute__((weak)) void board_spi_release_bus(struct spi_slave *slave) > +{ > +} > + > void spi_release_bus(struct spi_slave *slave) > { > + board_spi_release_bus(slave); > } > > #ifndef CONFIG_SPI_CS_IS_VALID
> -----Original Message----- > From: Valentin Longchamp [mailto:valentin.longchamp@keymile.com] > Sent: 27 March 2012 18:58 > 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 > > Prafulla, > > On 03/26/2012 11:58 AM, Valentin Longchamp wrote: > > Some kirkwood based boards may need to implement such function due > to > > some HW designs. > > I see no feedback from your side on this patch. I think you should go > through > the marvell tree: > > - the spi_claim/release_bus function are already implemented in the > SPI subsystem > - this patch touches only a kirkwood driver > - there is no spi u-boot tree from what I see > > Please keep me up to date about the status of this patch Hi Valentin, I have gone through this patch and related implementation in your other patch_series. http://lists.denx.de/pipermail/u-boot/2012-March/120716.html Basically spi_claim_bus and spi_release_bus are not supported in current Kirkwood spi driver. These are needed if someone wish to share the same interface pins with some other peripheral (that is your use case) But this is not board specific whereas, it should be feature enhancement for Kirkwood spi driver. You should add this support very similar to multiple CS pin selection support added to the Kirkwood driver, no external (board specific triggers needed) Here are my suggestions: 1. Configure these mpps in your board specific files as NF pins. 2. Populate below logic for claim/release bus feature in Kirkwood spi driver. 2.a. When spi_claim_bus will be called, backup current mpps status and reconfigure these mpps for SPI in Kirkwood_spi driver. 2.b. When spi_release_bus will be called, reconfigure with backed up mfg as SPI pins 2.c. Add check for to avoid multiple claim for same bus Regards.. Prafulla . . .
Hi Prafulla, On 03/28/2012 09:48 AM, Prafulla Wadaskar wrote: >> On 03/26/2012 11:58 AM, Valentin Longchamp wrote: >>> Some kirkwood based boards may need to implement such function due >> to >>> some HW designs. >> >> I see no feedback from your side on this patch. I think you should go >> through >> the marvell tree: >> >> - the spi_claim/release_bus function are already implemented in the >> SPI subsystem >> - this patch touches only a kirkwood driver >> - there is no spi u-boot tree from what I see >> >> Please keep me up to date about the status of this patch > > Hi Valentin, > I have gone through this patch and related implementation in your other patch_series. > http://lists.denx.de/pipermail/u-boot/2012-March/120716.html > > Basically spi_claim_bus and spi_release_bus are not supported in current Kirkwood spi driver. > These are needed if someone wish to share the same interface pins with some other peripheral (that is your use case) Correct, this is exactly our use case: we have the NAND Flash Controller and the SPI controller that share the same pins. > > But this is not board specific whereas, it should be feature enhancement for Kirkwood spi driver. This is correct for the mpp part of spi_claim_bus. If you look at the actual implementation that we do in our board specific function, there is an additional step that is needed by our board design. > > You should add this support very similar to multiple CS pin selection support added to the Kirkwood driver, no external (board specific triggers needed) > > Here are my suggestions: > 1. Configure these mpps in your board specific files as NF pins. > 2. Populate below logic for claim/release bus feature in Kirkwood spi driver. > 2.a. When spi_claim_bus will be called, backup current mpps status and reconfigure these mpps for SPI in Kirkwood_spi driver. > 2.b. When spi_release_bus will be called, reconfigure with backed up mfg as SPI pins > 2.c. Add check for to avoid multiple claim for same bus > OK, I agree with this, but I would add: 2.d. call weak attribute functions boad_spi_claim/release_bus at the end of spi_claim/release_bus functions
> -----Original Message----- > From: Valentin Longchamp [mailto:valentin.longchamp@keymile.com] > Sent: 29 March 2012 18:20 > 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, > > On 03/28/2012 09:48 AM, Prafulla Wadaskar wrote: > >> On 03/26/2012 11:58 AM, Valentin Longchamp wrote: > >>> Some kirkwood based boards may need to implement such function due > >> to > >>> some HW designs. > >> > >> I see no feedback from your side on this patch. I think you should > go > >> through > >> the marvell tree: > >> > >> - the spi_claim/release_bus function are already implemented in the > >> SPI subsystem > >> - this patch touches only a kirkwood driver > >> - there is no spi u-boot tree from what I see > >> > >> Please keep me up to date about the status of this patch > > > > Hi Valentin, > > I have gone through this patch and related implementation in your > other patch_series. > > http://lists.denx.de/pipermail/u-boot/2012-March/120716.html > > > > Basically spi_claim_bus and spi_release_bus are not supported in > current Kirkwood spi driver. > > These are needed if someone wish to share the same interface pins > with some other peripheral (that is your use case) > > Correct, this is exactly our use case: we have the NAND Flash > Controller and the > SPI controller that share the same pins. > > > > > But this is not board specific whereas, it should be feature > enhancement for Kirkwood spi driver. > > This is correct for the mpp part of spi_claim_bus. If you look at the > actual > implementation that we do in our board specific function, there is an > additional > step that is needed by our board design. > > > > > You should add this support very similar to multiple CS pin > selection support added to the Kirkwood driver, no external (board > specific triggers needed) > > > > Here are my suggestions: > > 1. Configure these mpps in your board specific files as NF pins. > > 2. Populate below logic for claim/release bus feature in Kirkwood > spi driver. > > 2.a. When spi_claim_bus will be called, backup current mpps status > and reconfigure these mpps for SPI in Kirkwood_spi driver. > > 2.b. When spi_release_bus will be called, reconfigure with backed up > mfg as SPI pins > > 2.c. Add check for to avoid multiple claim for same bus > > > > OK, I agree with this, but I would add: > 2.d. call weak attribute functions boad_spi_claim/release_bus at the > end of > spi_claim/release_bus functions With above logic, SPI driver will manage the show cleanly. Then, why do you need these weak attribute functions? Regards.. Prafulla . . .
On 03/29/2012 04:21 PM, Prafulla Wadaskar wrote: >> On 03/28/2012 09:48 AM, Prafulla Wadaskar wrote: >>>> On 03/26/2012 11:58 AM, Valentin Longchamp wrote: >>>>> Some kirkwood based boards may need to implement such function due >> >> Correct, this is exactly our use case: we have the NAND Flash >> Controller and the >> SPI controller that share the same pins. >> >>> >>> But this is not board specific whereas, it should be feature >> enhancement for Kirkwood spi driver. >> >> This is correct for the mpp part of spi_claim_bus. If you look at the >> actual >> implementation that we do in our board specific function, there is an >> additional >> step that is needed by our board design. >> >>> >>> You should add this support very similar to multiple CS pin >> selection support added to the Kirkwood driver, no external (board >> specific triggers needed) >>> >>> Here are my suggestions: >>> 1. Configure these mpps in your board specific files as NF pins. >>> 2. Populate below logic for claim/release bus feature in Kirkwood >> spi driver. >>> 2.a. When spi_claim_bus will be called, backup current mpps status >> and reconfigure these mpps for SPI in Kirkwood_spi driver. >>> 2.b. When spi_release_bus will be called, reconfigure with backed up >> mfg as SPI pins >>> 2.c. Add check for to avoid multiple claim for same bus >>> >> >> OK, I agree with this, but I would add: >> 2.d. call weak attribute functions boad_spi_claim/release_bus at the >> end of >> spi_claim/release_bus functions > > With above logic, SPI driver will manage the show cleanly. > Then, why do you need these weak attribute functions? > Because this is in our case not sufficient: we have an external device that takes care of "disabling" the the SPI bus from the "bus" when we do some NF accesses (and vice-versa), so that the SPI devices do not try interpret the NF signal toggling as SPI accesses. This external "mux" is driver by a GPIO and that's what I want to put in these board weak attribute functions. They belong to spi_claim/release_bus but really are specific to our device/boards and that's why I would need such functions.
Hi Prafulla, On 03/29/2012 02:49 PM, Valentin Longchamp wrote: > On 03/28/2012 09:48 AM, Prafulla Wadaskar wrote: >> Basically spi_claim_bus and spi_release_bus are not supported in current Kirkwood spi driver. >> These are needed if someone wish to share the same interface pins with some other peripheral (that is your use case) > > Correct, this is exactly our use case: we have the NAND Flash Controller and the > SPI controller that share the same pins. > >> >> But this is not board specific whereas, it should be feature enhancement for Kirkwood spi driver. > > This is correct for the mpp part of spi_claim_bus. If you look at the actual > implementation that we do in our board specific function, there is an additional > step that is needed by our board design. > I have started to implement this, and now I see that with your approach of doing the mpp part in the driver does not work and my proposed solution of doing this with board specific functions is the correct one: The SPI_SI, SPI_SCK, SPI_CSn all can be used with different mpp configuration. This is a board design parameter. How can the driver know which one is used on the board ? Requesting all of them is not an option and adding some configuration would be a significant effort while the problem is already tackled with the board specific functions.
> -----Original Message----- > From: Valentin Longchamp [mailto:valentin.longchamp@keymile.com] > Sent: 29 March 2012 21:15 > 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, > > On 03/29/2012 02:49 PM, Valentin Longchamp wrote: > > On 03/28/2012 09:48 AM, Prafulla Wadaskar wrote: > >> Basically spi_claim_bus and spi_release_bus are not supported in > current Kirkwood spi driver. > >> These are needed if someone wish to share the same interface pins > with some other peripheral (that is your use case) > > > > Correct, this is exactly our use case: we have the NAND Flash > Controller and the > > SPI controller that share the same pins. > > > >> > >> But this is not board specific whereas, it should be feature > enhancement for Kirkwood spi driver. > > > > This is correct for the mpp part of spi_claim_bus. If you look at > the actual > > implementation that we do in our board specific function, there is > an additional > > step that is needed by our board design. > > > > I have started to implement this, and now I see that with your > approach of doing > the mpp part in the driver does not work and my proposed solution of > doing this > with board specific functions is the correct one: > > The SPI_SI, SPI_SCK, SPI_CSn all can be used with different mpp > configuration. > This is a board design parameter. How can the driver know which one is > used on > the board ? Dear Valentin You should keep by default NF configuration in your board configuration (kwmpp_config), so this becomes your default mpp configuration. If SPI flash is being accessed, all spi_flash calls are guarded with claim_bus and release_bus APIs. 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). and in release_bus API, you will reconfigure backed-up mpp configuration (which will be NF in your case). With this you don't need km_hw_spi_bus_claim() or do_spi_toggle() in your board specific code any more. How come this doesn't work? It should work, if not let's debug the problem. Regards.. Prafulla . . .
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, I will be
happy to implement your proposed solution. Otherwise, I think we should stick
with the board specific function.
[1]
http://www.marvell.com/embedded-processors/kirkwood/assets/HW_88F6281_OpenSource.pdf
Regards
> -----Original Message----- > From: Valentin Longchamp [mailto:valentin.longchamp@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. 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. Regards.. Prafulla . . . > I will be > happy to implement your proposed solution. Otherwise, I think we > should stick > with the board specific function. > > [1] > http://www.marvell.com/embedded- > processors/kirkwood/assets/HW_88F6281_OpenSource.pdf > > 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@keymile.com > www.keymile.com > ______________________________________ > KEYMILE: A Specialist as a Partner
Dear Prafulla, On 03/30/2012 02:58 PM, Prafulla Wadaskar wrote: >> -----Original Message----- >> From: Valentin Longchamp [mailto:valentin.longchamp@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. 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: 1) We would have to add another CONFIG_SYS_XY for this purpose (as explained above) 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). OK genericity is nice, but I guess we would be the only ones using this code, 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. Regards
> -----Original Message----- > From: Valentin Longchamp [mailto:valentin.longchamp@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.longchamp@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. > > 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@keymile.com > www.keymile.com > ______________________________________ > KEYMILE: A Specialist as a Partner
On 04/03/2012 08:35 AM, Prafulla Wadaskar wrote: > > >> -----Original Message----- >> From: Valentin Longchamp [mailto:valentin.longchamp@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.longchamp@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
> -----Original Message----- > From: Valentin Longchamp [mailto:valentin.longchamp@keymile.com] > Sent: 04 April 2012 12:32 > 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 > ...snip... > > > > > 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. Dear Valentin, What ever you have implemented for spi_claim/release, I have suggested to move it to driver specific code so that it can be reused. That is a good feature that Kirkwood_spi driver is missing. I am thankful to you that you have addressed this through your requirement. BTW: it's not matter of custodian :-) no one is great on this earth!! We can keep discussion on convincing each other, but that is not our objective here. Let's keep evolving u-boot code for it's better usability. Thanks for your understanding and closure on this discussion. Regards. Prafulla . . .
diff --git a/drivers/spi/kirkwood_spi.c b/drivers/spi/kirkwood_spi.c index db8ba8b..058dae2 100644 --- a/drivers/spi/kirkwood_spi.c +++ b/drivers/spi/kirkwood_spi.c @@ -86,13 +86,23 @@ void spi_free_slave(struct spi_slave *slave) free(slave); } -int spi_claim_bus(struct spi_slave *slave) +__attribute__((weak)) int board_spi_claim_bus(struct spi_slave *slave) { return 0; } +int spi_claim_bus(struct spi_slave *slave) +{ + return board_spi_claim_bus(slave); +} + +__attribute__((weak)) void board_spi_release_bus(struct spi_slave *slave) +{ +} + void spi_release_bus(struct spi_slave *slave) { + board_spi_release_bus(slave); } #ifndef CONFIG_SPI_CS_IS_VALID
Some kirkwood based boards may need to implement such function due to some HW designs. Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com> cc: Gerlando Falauto <gerlando.falauto@keymile.com> cc: Prafulla Wadaskar <prafulla@marvell.com> cc: Holger Brunck <holger.brunck@keymile.com> --- drivers/spi/kirkwood_spi.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-)