[U-Boot] [PATCH v2] spi/kirkwood: add weak functions board_spi_bus_claim/release
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(-)
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
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:
- Configure these mpps in your board specific files as NF pins.
- 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:
- Configure these mpps in your board specific files as NF pins.
- 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:
- Configure these mpps in your board specific files as NF pins.
- 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_OpenSo...
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
- MPP7 as SPI_CSn I will define CONFIG_SF_DEFAULT_CS to 0x01
- MPP6 as SPI_MOSI I will define CONFIG_SF_DEFAULT_CS to 0x02
- 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
- MPP7 as SPI_CSn I will define CONFIG_SF_DEFAULT_CS to 0x01
- MPP6 as SPI_MOSI I will define CONFIG_SF_DEFAULT_CS to 0x02
- 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.
- We would have to add another CONFIG_SYS_XY for this purpose (as
explained above)
Okay, this is also a better solution.
- 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
- MPP7 as SPI_CSn I will define CONFIG_SF_DEFAULT_CS to 0x01
- MPP6 as SPI_MOSI I will define CONFIG_SF_DEFAULT_CS to 0x02
- 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.
- We would have to add another CONFIG_SYS_XY for this purpose (as
explained above)
Okay, this is also a better solution.
- 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 . . .
participants (2)
-
Prafulla Wadaskar -
Valentin Longchamp