kirkwood spi_claim/release_bus support

PATCH Superseded Valentin Longchamp 4 participants 24 messages May 16, 2012 – Jun 1, 2012

Subsystems (2)

Arm (32-bit) SPI

lore

Share & tools

lore

Valentin Longchamp

author

2012-05-16 10:53 UTC #0 lore raw

cover kirkwood spi_claim/release_bus support

This series adds generic support for the spi_claim/release_bus functions for the kirkwood processors.

The implementation was already discussed in another thread following my first board specific submission of the patch.

The series adds two functions to the kirkwood mpp code to be able to temporarily save and then restore the mpp configuration.

Valentin Longchamp (3):
kirkwood: add kirkwood_mpp_save/restore functions spi/kirkwood: support spi_claim/release_bus functions spi/kirkwood: add weak functions board_spi_claim/release_bus

 arch/arm/cpu/arm926ejs/kirkwood/mpp.c    |   18 +++++++++++
 arch/arm/include/asm/arch-kirkwood/mpp.h |    2 +
 arch/arm/include/asm/arch-kirkwood/spi.h |    9 ++++++
 drivers/spi/kirkwood_spi.c               |   47 +++++++++++++++++++++++++++++-
 4 files changed, 75 insertions(+), 1 deletions(-)

Valentin Longchamp

author

2012-05-16 10:53 UTC #1 lore raw

PATCH 1/3 kirkwood: add kirkwood_mpp_save/restore functions

These 2 functions can be used in pair if one needs to set a mpp configuration only for a given time and then switch back to the previous mpp config.

Signed-off-by Valentin Longchamp <[email protected]>

cc Holger Brunck <[email protected]>

cc Prafulla Wadaskar <[email protected]>

---
 arch/arm/cpu/arm926ejs/kirkwood/mpp.c    |   18 ++++++++++++++++++
 arch/arm/include/asm/arch-kirkwood/mpp.h |    2 ++
 2 files changed, 20 insertions(+), 0 deletions(-)

arch/arm/cpu/arm926ejs/kirkwood/mpp.c +18 -0

index 3da6c98..43f5053 100644
@@ -80,3 +80,21 @@ void kirkwood_mpp_conf(u32 *mpp_list)
 	debug("\n");
 
 }
+
+u32 mpp_regs[MPP_NR_REGS];
+
+void kirkwood_mpp_save(void)
+{
+	int i;
+
+	for (i = 0; i < MPP_NR_REGS; i++)
+		mpp_regs[i] = readl(MPP_CTRL(i));
+}
+
+void kirkwood_mpp_restore(void)
+{
+	int i;
+
+	for (i = 0; i < MPP_NR_REGS; i++)
+		writel(mpp_regs[i], MPP_CTRL(i));
+}

arch/arm/include/asm/arch-kirkwood/mpp.h +2 -0

index b3c090e..da65b4d 100644
@@ -313,5 +313,7 @@
 #define MPP_MAX			49
 
 void kirkwood_mpp_conf(unsigned int *mpp_list);
+void kirkwood_mpp_save(void);
+void kirkwood_mpp_restore(void);
 
 #endif

Signature · 1 line

1.7.1

Michael Walle

2012-05-28 22:07 UTC #3 lore raw

PATCH 1/3 kirkwood: add kirkwood_mpp_save/restore functions

Am Donnerstag 24 Mai 2012, 10:26:44 schrieb Prafulla Wadaskar:

Quoted 6 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+
+void kirkwood_mpp_save(void)
This should be
void kirkwood_mpp_save(unsigned int *mpp_ctrl, int len)

Quoted 11 lines

arch/arm/cpu/arm926ejs/kirkwood/mpp.c void kirkwood_mpp_conf(u32 *mpp_list) · line 94
Quoted 8 lines
arch/arm/cpu/arm926ejs/kirkwood/mpp.c +8 -0
+{
+	int i;
+
+	for (i = 0; i < MPP_NR_REGS; i++)
+		mpp_regs[i] = readl(MPP_CTRL(i));
+}
+
+void kirkwood_mpp_restore(void)
Same here
void kirkwood_mpp_restore(unsigned int *mpp_ctrl, int len)

or even better kirkwood_mpp_restore(const unsigned int *mpp_ctrl, int len)

btw. sometimes "unsigned int" is used and sometimes "u32", could we agree on one? Eg. kirkwood_mpp_conf() is declared with unsigned int but defined with u32.

Signature · 1 line

Michael

Prafulla Wadaskar

2012-05-29 12:42 UTC #4 lore raw

PATCH 1/3 kirkwood: add kirkwood_mpp_save/restore functions

Quoted 28 lines

-----Original Message-----
From: Michael Walle [mailto:michael at walle.cc]
Sent: 29 May 2012 03:38
To: u-boot at lists.denx.de
Cc: Prafulla Wadaskar; Valentin Longchamp; holger.brunck at keymile.com
Subject: Re: [U-Boot] [PATCH 1/3] kirkwood: add kirkwood_mpp_save/restore functions
Am Donnerstag 24 Mai 2012, 10:26:44 schrieb Prafulla Wadaskar:
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+
+void kirkwood_mpp_save(void)
This should be
void kirkwood_mpp_save(unsigned int *mpp_ctrl, int len)
Quoted 11 lines
arch/arm/cpu/arm926ejs/kirkwood/mpp.c void kirkwood_mpp_conf(u32 *mpp_list) · line 94
Quoted 8 lines
arch/arm/cpu/arm926ejs/kirkwood/mpp.c +8 -0
+{
+	int i;
+
+	for (i = 0; i < MPP_NR_REGS; i++)
+		mpp_regs[i] = readl(MPP_CTRL(i));
+}
+
+void kirkwood_mpp_restore(void)
Same here
void kirkwood_mpp_restore(unsigned int *mpp_ctrl, int len)

Quoted 8 lines

or even better kirkwood_mpp_restore(const unsigned int *mpp_ctrl, int len)
btw. sometimes "unsigned int" is used and sometimes "u32", could we agree on
one? Eg. kirkwood_mpp_conf() is declared with unsigned int but defined with
u32.

Hi Michael

Thanks for your comments.

Yes, we should use u32.
And const will be a problem since mpp_ctrl will be array that will be dynamically modified.

Regards..
Prafulla . . .

Michael Walle

2012-05-29 17:02 UTC #5 lore raw

PATCH 1/3 kirkwood: add kirkwood_mpp_save/restore functions

Am Dienstag 29 Mai 2012, 14:42:08 schrieb Prafulla Wadaskar:

Quoted 2 lines

And const will be a problem since mpp_ctrl will be array that will be dynamically modified.

But not within kirkwood_mpp_restore(), right? So an mpp_ctrl array would be implicitly casted from u32* to const u32*.

so we would have the following declarations:
kirkwood_mpp_restore(const u32 *mpp_list)
kirkwood_mpp_conf(const u32 *mpp_list)
kirkwood_mpp_save(u32 *mpp_list)

where only the latter modifies mpp_list.

Signature · 1 line

michael

Valentin Longchamp

author

2012-05-29 08:44 UTC #6 lore raw

PATCH 1/3 kirkwood: add kirkwood_mpp_save/restore functions

In reply to Prafulla Wadaskar

Quoted 31 lines · Prafulla Wadaskar

Quoted 31 lines
-----Original Message-----
From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
Sent: 16 May 2012 16:24
To: Prafulla Wadaskar; holger.brunck at keymile.com
Cc: Valentin Longchamp; u-boot at lists.denx.de; Holger Brunck; Prafulla Wadaskar
Subject: [PATCH 1/3] kirkwood: add kirkwood_mpp_save/restore functions
These 2 functions can be used in pair if one needs to set a mpp configuration only for a given time and then switch back to the previous
mpp config.
Signed-off-by Valentin Longchamp <[email protected]>
cc Holger Brunck <[email protected]>
cc Prafulla Wadaskar <[email protected]>
---
 arch/arm/cpu/arm926ejs/kirkwood/mpp.c    |   18 ++++++++++++++++++
 arch/arm/include/asm/arch-kirkwood/mpp.h |    2 ++
 2 files changed, 20 insertions(+), 0 deletions(-)
arch/arm/cpu/arm926ejs/kirkwood/mpp.c +0 -0
index 3da6c98..43f5053 100644
@@ -80,3 +80,21 @@ void kirkwood_mpp_conf(u32 *mpp_list)
 	debug("\n");
 

Quoted 6 lines

arch/arm/cpu/arm926ejs/kirkwood/mpp.c void kirkwood_mpp_conf(u32 *mpp_list) · line 84
Quoted 3 lines
arch/arm/cpu/arm926ejs/kirkwood/mpp.c +2 -0
 }
+
+u32 mpp_regs[MPP_NR_REGS];
This is optional feature only used in this specific case. No global here, this should be part of caller function.

I wanted this patch to be independant of the SPI claim bus that _currently_ is the only one using it, but who knows in the future.

Anyway if this isn't global here, it will have to be global in the kirkwood_spi.c driver since it would have to be shared between spi_claim_bus and spi_release_bus.

Quoted 5 lines

arch/arm/cpu/arm926ejs/kirkwood/mpp.c void kirkwood_mpp_conf(u32 *mpp_list) · line 86
Quoted 3 lines
arch/arm/cpu/arm926ejs/kirkwood/mpp.c +2 -0
+
+void kirkwood_mpp_save(void)
This should be
void kirkwood_mpp_save(unsigned int *mpp_ctrl, int len)

Here we save _all_ mpp registers, with direct access to the registers. With your proposed solution, I would save it in a dynamically allocated table of size len.

That's fine for me, but I would then need to export MPP_NR_REGS, because that is what I would pass as len arg, is that OK ?

Quoted 32 lines

arch/arm/cpu/arm926ejs/kirkwood/mpp.c void kirkwood_mpp_conf(u32 *mpp_list) · line 94
Quoted 9 lines
arch/arm/cpu/arm926ejs/kirkwood/mpp.c +8 -0
+{
+	int i;
+
+	for (i = 0; i < MPP_NR_REGS; i++)
+		mpp_regs[i] = readl(MPP_CTRL(i));
+}
+
+void kirkwood_mpp_restore(void)
Same here
void kirkwood_mpp_restore(unsigned int *mpp_ctrl, int len)
Quoted 12 lines
arch/arm/cpu/arm926ejs/kirkwood/mpp.c +6 -0
+{
+	int i;
+
+	for (i = 0; i < MPP_NR_REGS; i++)
+		writel(mpp_regs[i], MPP_CTRL(i));
+}
arch/arm/include/asm/arch-kirkwood/mpp.h +0 -0
index b3c090e..da65b4d 100644
arch/arm/include/asm/arch-kirkwood/mpp.h line 317
Quoted 5 lines
arch/arm/include/asm/arch-kirkwood/mpp.h +2 -0
@@ -313,5 +313,7 @@
 #define MPP_MAX			49
 
 void kirkwood_mpp_conf(unsigned int *mpp_list);
+void kirkwood_mpp_save(void);
+void kirkwood_mpp_restore(void);
Regards..
Prafulla . . .

Prafulla Wadaskar

2012-05-29 10:12 UTC #7 lore raw

PATCH 1/3 kirkwood: add kirkwood_mpp_save/restore functions

Quoted 10 lines

-----Original Message-----
From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
Sent: 29 May 2012 14:15
To: Prafulla Wadaskar
Cc: holger.brunck at keymile.com; u-boot at lists.denx.de
Subject: Re: [PATCH 1/3] kirkwood: add kirkwood_mpp_save/restore functions

...snip...

Quoted 6 lines

Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
+
+void kirkwood_mpp_save(void)
This should be
void kirkwood_mpp_save(unsigned int *mpp_ctrl, int len)

Quoted 8 lines

Here we save _all_ mpp registers, with direct access to the registers. With your
proposed solution, I would save it in a dynamically allocated table of size len.
That's fine for me, but I would then need to export MPP_NR_REGS, because that is
what I would pass as len arg, is that OK ?

I think in your case you need configuration of 4 MPPs, i.e. from MPP6 to MPP11, so you may declare array of length 7 and backup and restore the same MPPs using len = 6.

No Need to backup and restore all MPPs.

Regards...
Prafulla . . .

Valentin Longchamp

author

2012-05-29 11:28 UTC #8 lore raw

PATCH 1/3 kirkwood: add kirkwood_mpp_save/restore functions

Quoted 20 lines

Quoted 8 lines
-----Original Message-----
From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
Sent: 29 May 2012 14:15
To: Prafulla Wadaskar
Cc: holger.brunck at keymile.com; u-boot at lists.denx.de
Subject: Re: [PATCH 1/3] kirkwood: add kirkwood_mpp_save/restore functions
...snip...
Quoted 11 lines
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
+
+void kirkwood_mpp_save(void)
This should be
void kirkwood_mpp_save(unsigned int *mpp_ctrl, int len)
Here we save _all_ mpp registers, with direct access to the registers. With your
proposed solution, I would save it in a dynamically allocated table of size len.

Quoted 8 lines

Quoted 3 lines
That's fine for me, but I would then need to export MPP_NR_REGS, because that is
what I would pass as len arg, is that OK ?
I think in your case you need configuration of 4 MPPs, i.e. from MPP6 to MPP11, so you may declare array of length 7 and backup and restore the same MPPs using len = 6.
No Need to backup and restore all MPPs.

Sorry, but this is exactly what you did in the kirkwood_mpp_conf function, you read and write all the registers every time you need to change only one pin, I took it from there for consistency:

Quoted 4 lines

4efb77d4 cpu/arm926ejs/kirkwood/mpp.c          (Prafulla Wadaskar  2009-06-20 11:01:53 +0200  76)       for (i = 0; i < MPP_NR_REGS; i++) {
4efb77d4 cpu/arm926ejs/kirkwood/mpp.c          (Prafulla Wadaskar  2009-06-20 11:01:53 +0200  77)               writel(mpp_ctrl[i], MPP_CTRL(i));
4efb77d4 cpu/arm926ejs/kirkwood/mpp.c          (Prafulla Wadaskar  2009-06-20 11:01:53 +0200  78)               debug(" %08x", mpp_ctrl[i]);
4efb77d4 cpu/arm926ejs/kirkwood/mpp.c          (Prafulla Wadaskar  2009-06-20 11:01:53 +0200  79)       }

And the way you did it is logical, if it was not done like that, a lot of reading/masking/rewriting would be needed, and this for every single pin, so it would be much less efficient than just reading all the regs and write them all back.

Prafulla Wadaskar

2012-05-29 12:06 UTC #9 lore raw

PATCH 1/3 kirkwood: add kirkwood_mpp_save/restore functions

Quoted 59 lines

-----Original Message-----
From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
Sent: 29 May 2012 16:59
To: Prafulla Wadaskar
Cc: holger.brunck at keymile.com; u-boot at lists.denx.de
Subject: Re: [PATCH 1/3] kirkwood: add kirkwood_mpp_save/restore functions
Quoted 8 lines
Quoted 8 lines
-----Original Message-----
From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
Sent: 29 May 2012 14:15
To: Prafulla Wadaskar
Cc: holger.brunck at keymile.com; u-boot at lists.denx.de
Subject: Re: [PATCH 1/3] kirkwood: add kirkwood_mpp_save/restore functions
Quoted 8 lines
...snip...
Quoted 7 lines
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
+
+void kirkwood_mpp_save(void)
This should be
void kirkwood_mpp_save(unsigned int *mpp_ctrl, int len)
Here we save _all_ mpp registers, with direct access to the
registers.
Quoted 2 lines
Quoted 2 lines
With your
proposed solution, I would save it in a dynamically allocated table
of
Quoted 7 lines
Quoted 5 lines
size len.
That's fine for me, but I would then need to export MPP_NR_REGS, because that is
what I would pass as len arg, is that OK ?
I think in your case you need configuration of 4 MPPs, i.e. from
MPP6 to MPP11, so you may declare array of length 7 and backup and restore the same MPPs using len = 6.
Quoted 3 lines
No Need to backup and restore all MPPs.
Sorry, but this is exactly what you did in the kirkwood_mpp_conf function, you
read and write all the registers every time you need to change only one pin, I
took it from there for consistency:
Quoted 1 line
4efb77d4 cpu/arm926ejs/kirkwood/mpp.c (Prafulla Wadaskar
2009-06-20 11:01:53 +0200  76)       for (i = 0; i < MPP_NR_REGS; i++)
{
Quoted 1 line
4efb77d4 cpu/arm926ejs/kirkwood/mpp.c (Prafulla Wadaskar
2009-06-20 11:01:53 +0200  77)               writel(mpp_ctrl[i],
MPP_CTRL(i));
Quoted 1 line
4efb77d4 cpu/arm926ejs/kirkwood/mpp.c (Prafulla Wadaskar
2009-06-20 11:01:53 +0200  78)               debug(" %08x",
mpp_ctrl[i]);
Quoted 1 line
4efb77d4 cpu/arm926ejs/kirkwood/mpp.c (Prafulla Wadaskar

Quoted 8 lines

2009-06-20 11:01:53 +0200  79)       }
And the way you did it is logical, if it was not done like that, a lot of
reading/masking/rewriting would be needed, and this for every single pin, so it
would be much less efficient than just reading all the regs and write them all back.

Yes, but the idea is that mpp_config function is called just once during initialization, we have exposed a simple array to do this configuration.

but save/restore will be called very frequently, may be for each SPI transaction. So there must be some optimisation. Secondly, we should only tweak only needed MPPs in run time, why all? It may lead to some other side effects (I don't know).

Regards..
Prafulla . . .

Valentin Longchamp

author

2012-05-29 12:50 UTC #10 lore raw

PATCH 1/3 kirkwood: add kirkwood_mpp_save/restore functions

Quoted 68 lines · Prafulla Wadaskar

Quoted 59 lines
-----Original Message-----
From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
Sent: 29 May 2012 16:59
To: Prafulla Wadaskar
Cc: holger.brunck at keymile.com; u-boot at lists.denx.de
Subject: Re: [PATCH 1/3] kirkwood: add kirkwood_mpp_save/restore functions
Quoted 8 lines
Quoted 8 lines
-----Original Message-----
From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
Sent: 29 May 2012 14:15
To: Prafulla Wadaskar
Cc: holger.brunck at keymile.com; u-boot at lists.denx.de
Subject: Re: [PATCH 1/3] kirkwood: add kirkwood_mpp_save/restore functions
Quoted 8 lines
...snip...
Quoted 7 lines
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
+
+void kirkwood_mpp_save(void)
This should be
void kirkwood_mpp_save(unsigned int *mpp_ctrl, int len)
Here we save _all_ mpp registers, with direct access to the
registers.
Quoted 2 lines
Quoted 2 lines
With your
proposed solution, I would save it in a dynamically allocated table
of
Quoted 7 lines
Quoted 5 lines
size len.
That's fine for me, but I would then need to export MPP_NR_REGS, because that is
what I would pass as len arg, is that OK ?
I think in your case you need configuration of 4 MPPs, i.e. from
MPP6 to MPP11, so you may declare array of length 7 and backup and restore the same MPPs using len = 6.
Quoted 3 lines
No Need to backup and restore all MPPs.
Sorry, but this is exactly what you did in the kirkwood_mpp_conf function, you
read and write all the registers every time you need to change only one pin, I
took it from there for consistency:
Quoted 1 line
4efb77d4 cpu/arm926ejs/kirkwood/mpp.c (Prafulla Wadaskar
2009-06-20 11:01:53 +0200  76)       for (i = 0; i < MPP_NR_REGS; i++)
{
Quoted 1 line
4efb77d4 cpu/arm926ejs/kirkwood/mpp.c (Prafulla Wadaskar
2009-06-20 11:01:53 +0200  77)               writel(mpp_ctrl[i],
MPP_CTRL(i));
Quoted 1 line
4efb77d4 cpu/arm926ejs/kirkwood/mpp.c (Prafulla Wadaskar
2009-06-20 11:01:53 +0200  78)               debug(" %08x",
mpp_ctrl[i]);
Quoted 1 line
4efb77d4 cpu/arm926ejs/kirkwood/mpp.c (Prafulla Wadaskar
Quoted 8 lines
2009-06-20 11:01:53 +0200  79)       }
And the way you did it is logical, if it was not done like that, a lot of
reading/masking/rewriting would be needed, and this for every single pin, so it
would be much less efficient than just reading all the regs and write them all back.

Quoted 6 lines

Yes, but the idea is that mpp_config function is called just once during initialization, we have exposed a simple array to do this configuration.
but save/restore will be called very frequently, may be for each SPI transaction. So there must be some optimisation.
Secondly, we should only tweak only needed MPPs in run time, why all? It may lead to some other side effects (I don't know).

With the proposed code, all are saved and restored, but not all are tweaked. Furthermore, maybe reading and writing back 7 registers and is more efficient than determining which one of the 7 have to be read/written back and then performing the accesses.

However, if you tell me that mpp_config function is supposed to be called just once during initialization, I will propose a new function that optimizes these reg accesses as you would prefer it, but the best optimization potential is to avoid to call the 2 mpp_config and mpp_save functions.

Prafulla Wadaskar

2012-05-29 13:15 UTC #11 lore raw

PATCH 1/3 kirkwood: add kirkwood_mpp_save/restore functions

Quoted 103 lines

-----Original Message-----
From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
Sent: 29 May 2012 18:20
To: Prafulla Wadaskar
Cc: holger.brunck at keymile.com; u-boot at lists.denx.de
Subject: Re: [PATCH 1/3] kirkwood: add kirkwood_mpp_save/restore functions
Quoted 25 lines · Prafulla Wadaskar
Quoted 25 lines
-----Original Message-----
From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
Sent: 29 May 2012 16:59
To: Prafulla Wadaskar
Cc: holger.brunck at keymile.com; u-boot at lists.denx.de
Subject: Re: [PATCH 1/3] kirkwood: add kirkwood_mpp_save/restore functions
Quoted 15 lines
Quoted 8 lines
-----Original Message-----
From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
Sent: 29 May 2012 14:15
To: Prafulla Wadaskar
Cc: holger.brunck at keymile.com; u-boot at lists.denx.de
Subject: Re: [PATCH 1/3] kirkwood: add kirkwood_mpp_save/restore functions
...snip...
Quoted 6 lines
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
+
+void kirkwood_mpp_save(void)
This should be
void kirkwood_mpp_save(unsigned int *mpp_ctrl, int len)
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line
Here we save _all_ mpp registers, with direct access to the
registers.
Quoted 2 lines
Quoted 2 lines
With your
proposed solution, I would save it in a dynamically allocated
table
Quoted 14 lines
Quoted 14 lines
of
Quoted 7 lines
Quoted 5 lines
size len.
That's fine for me, but I would then need to export MPP_NR_REGS, because that is
what I would pass as len arg, is that OK ?
I think in your case you need configuration of 4 MPPs, i.e. from
MPP6 to MPP11, so you may declare array of length 7 and backup and restore the same MPPs using len = 6.
Quoted 3 lines
No Need to backup and restore all MPPs.
Quoted 8 lines
Quoted 8 lines
Sorry, but this is exactly what you did in the kirkwood_mpp_conf function, you
read and write all the registers every time you need to change only one pin, I
took it from there for consistency:
Quoted 1 line
4efb77d4 cpu/arm926ejs/kirkwood/mpp.c (Prafulla Wadaskar
2009-06-20 11:01:53 +0200  76)       for (i = 0; i < MPP_NR_REGS;
i++)
Quoted 11 lines
Quoted 11 lines
{
Quoted 1 line
4efb77d4 cpu/arm926ejs/kirkwood/mpp.c (Prafulla Wadaskar
2009-06-20 11:01:53 +0200  77)               writel(mpp_ctrl[i],
MPP_CTRL(i));
Quoted 1 line
4efb77d4 cpu/arm926ejs/kirkwood/mpp.c (Prafulla Wadaskar
2009-06-20 11:01:53 +0200  78)               debug(" %08x",
mpp_ctrl[i]);
Quoted 1 line
4efb77d4 cpu/arm926ejs/kirkwood/mpp.c (Prafulla Wadaskar
2009-06-20 11:01:53 +0200  79)       }
And the way you did it is logical, if it was not done like that, a
lot
Quoted 2 lines
Quoted 2 lines
of
reading/masking/rewriting would be needed, and this for every
single
Quoted 2 lines
Quoted 2 lines
pin, so it
would be much less efficient than just reading all the regs and
write
Quoted 3 lines
Quoted 1 line
them all back.
Yes, but the idea is that mpp_config function is called just once
during initialization, we have exposed a simple array to do this configuration.
Quoted 2 lines
but save/restore will be called very frequently, may be for each SPI
transaction. So there must be some optimisation.
Quoted 1 line
Secondly, we should only tweak only needed MPPs in run time, why
all? It may lead to some other side effects (I don't know).
Quoted 1 line
With the proposed code, all are saved and restored, but not all are tweaked.
Furthermore, maybe reading and writing back 7 registers and is more efficient
than determining which one of the 7 have to be read/written back and then
performing the accesses.

Quoted 4 lines

However, if you tell me that mpp_config function is supposed to be called just
once during initialization, I will propose a new function that optimizes these

New ideas are always welcomed, let's keep is separate from this context.

Quoted 3 lines

reg accesses as you would prefer it, but the best optimization potential is to
avoid to call the 2 mpp_config and mpp_save functions.

I think, Kirkwood_mpp_config (already supported) finally writes the said MPP configurations to the SoC,

If we can have just Kirkwood_mpp_read() function that reads the MPP configuration for the said MPP will be enough from mpp.c prospective. This function should return the MPP config value in the same way it is being programmed by Kirkwood_mpp_config() function.

In SPI driver we will use these functions to handle the claim/release/backup the MPP attributes.

Regards..
Prafulla . . .

Marek Vasut

2012-05-29 14:47 UTC #12 lore raw

PATCH 1/3 kirkwood: add kirkwood_mpp_save/restore functions

In reply to Prafulla Wadaskar

Dear Prafulla Wadaskar,

this is offtopic in this thread, but I tried contacting you about thrice by now via email, maybe you didn't get those mails. To get quickly to the point, can you please try cleaning up the patches in patchwork?

Thanks!

Best regards,
Marek Vasut

Prafulla Wadaskar

2012-05-30 14:28 UTC #13 lore raw

[U-Boot] patchwork cleanup

Dear Marek

I did few cleanups in patchwork?
I will check if further more is needed.

Regards..
Prafulla . . .

Quoted 20 lines

-----Original Message-----
From: Marek Vasut [mailto:marek.vasut at gmail.com]
Sent: 29 May 2012 20:17
To: u-boot at lists.denx.de
Cc: Prafulla Wadaskar; Valentin Longchamp; holger.brunck at keymile.com
Subject: Re: [U-Boot] [PATCH 1/3] kirkwood: add kirkwood_mpp_save/restore functions
Dear Prafulla Wadaskar,
this is offtopic in this thread, but I tried contacting you about thrice by now
via email, maybe you didn't get those mails. To get quickly to the point, can
you please try cleaning up the patches in patchwork?
Thanks!
Best regards,
Marek Vasut

Marek Vasut

2012-06-01 15:27 UTC #14 lore raw

[U-Boot] patchwork cleanup

Dear Prafulla Wadaskar,

Quoted 4 lines

Dear Marek
I did few cleanups in patchwork?
I will check if further more is needed.

I just checked, it seems it's all gone now, only a few crumbles left :)

Sorry I kept pestering you so much.

Thanks a lot for cleaning it up!

Quoted 24 lines

Regards..
Prafulla . . .
Quoted 20 lines
-----Original Message-----
From: Marek Vasut [mailto:marek.vasut at gmail.com]
Sent: 29 May 2012 20:17
To: u-boot at lists.denx.de
Cc: Prafulla Wadaskar; Valentin Longchamp; holger.brunck at keymile.com
Subject: Re: [U-Boot] [PATCH 1/3] kirkwood: add kirkwood_mpp_save/restore functions
Dear Prafulla Wadaskar,
this is offtopic in this thread, but I tried contacting you about thrice by now
via email, maybe you didn't get those mails. To get quickly to the point, can
you please try cleaning up the patches in patchwork?
Thanks!
Best regards,
Marek Vasut

Best regards,
Marek Vasut

Valentin Longchamp

author

2012-05-16 10:53 UTC #15 lore raw

PATCH 2/3 spi/kirkwood: support spi_claim/release_bus functions

In reply to Valentin Longchamp

These two function nows ensure that the MPP is configured correctly for the SPI controller before any SPI access, and restore the initial configuration when the access is over.

Since the used pins for the SPI controller can differ (2 possibilities for each signal), the used pins are configured with CONFIG_SYS_KW_SPI_MPP.

Signed-off-by Valentin Longchamp <[email protected]>

cc Holger Brunck <[email protected]>

cc Prafulla Wadaskar <[email protected]>

---
 arch/arm/include/asm/arch-kirkwood/spi.h |    9 ++++++++
 drivers/spi/kirkwood_spi.c               |   34 ++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 0 deletions(-)

arch/arm/include/asm/arch-kirkwood/spi.h +9 -0

index 1d5043f..305c573 100644
@@ -37,6 +37,15 @@ struct kwspi_registers {
 	u32 irq_mask;	/* 0x10614 */
 };
 
+#define CSn_MPP7	0x1
+#define MOSI_MPP6	0x2
+#define SCK_MPP10	0x4
+#define MISO_MPP11	0x8
+
+#ifndef CONFIG_SYS_KW_SPI_MPP
+#define CONFIG_SYS_KW_SPI_MPP	0x0
+#endif
+
 #define KWSPI_CLKPRESCL_MASK	0x1f
 #define KWSPI_CSN_ACT		1 /* Activates serial memory interface */
 #define KWSPI_SMEMRDY		(1 << 1) /* SerMem Data xfer ready */

drivers/spi/kirkwood_spi.c +34 -0

index db8ba8b..0877915 100644
@@ -88,11 +88,45 @@ void spi_free_slave(struct spi_slave *slave)
 
 int spi_claim_bus(struct spi_slave *slave)
 {
+	u32 config;
+	u32 spi_mpp_config[5];
+
+	config = CONFIG_SYS_KW_SPI_MPP;
+
+	if (config & CSn_MPP7)
+		spi_mpp_config[0] = MPP7_SPI_SCn;
+	else
+		spi_mpp_config[0] = MPP0_SPI_SCn;
+
+	if (config & MOSI_MPP6)
+		spi_mpp_config[1] = MPP6_SPI_MOSI;
+	else
+		spi_mpp_config[1] = MPP1_SPI_MOSI;
+
+	if (config & SCK_MPP10)
+		spi_mpp_config[2] = MPP10_SPI_SCK;
+	else
+		spi_mpp_config[2] = MPP2_SPI_SCK;
+
+	if (config & MISO_MPP11)
+		spi_mpp_config[3] = MPP11_SPI_MISO;
+	else
+		spi_mpp_config[3] = MPP3_SPI_MISO;
+
+	spi_mpp_config[4] = 0;
+
+	/* save current mpp configuration */
+	kirkwood_mpp_save();
+
+	/* finally set chosen mpp spi configuration */
+	kirkwood_mpp_conf(spi_mpp_config);
+
 	return 0;
 }
 
 void spi_release_bus(struct spi_slave *slave)
 {
+	kirkwood_mpp_restore();
 }
 
 #ifndef CONFIG_SPI_CS_IS_VALID

Signature · 1 line

1.7.1

Prafulla Wadaskar

2012-05-24 08:35 UTC #16 lore raw

PATCH 2/3 spi/kirkwood: support spi_claim/release_bus functions

Quoted 35 lines

-----Original Message-----
From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
Sent: 16 May 2012 16:24
To: Prafulla Wadaskar; holger.brunck at keymile.com
Cc: Valentin Longchamp; u-boot at lists.denx.de; Holger Brunck; Prafulla Wadaskar
Subject: [PATCH 2/3] spi/kirkwood: support spi_claim/release_bus functions
These two function nows ensure that the MPP is configured correctly for
the SPI controller before any SPI access, and restore the initial configuration when the access is over.
Since the used pins for the SPI controller can differ (2 possibilities for each signal), the used pins are configured with CONFIG_SYS_KW_SPI_MPP.
Signed-off-by Valentin Longchamp <[email protected]>
cc Holger Brunck <[email protected]>
cc Prafulla Wadaskar <[email protected]>
---
 arch/arm/include/asm/arch-kirkwood/spi.h |    9 ++++++++
 drivers/spi/kirkwood_spi.c               |   34
++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 0 deletions(-)
arch/arm/include/asm/arch-kirkwood/spi.h +0 -0
index 1d5043f..305c573 100644

arch/arm/include/asm/arch-kirkwood/spi.h struct kwspi_registers { · line 43

Quoted 7 lines

arch/arm/include/asm/arch-kirkwood/spi.h +4 -0

@@ -37,6 +37,15 @@ struct kwspi_registers {
 	u32 irq_mask;	/* 0x10614 */
 };
 
+#define CSn_MPP7	0x1
+#define MOSI_MPP6	0x2
+#define SCK_MPP10	0x4
+#define MISO_MPP11	0x8

Let's define above as (1 << x) to make it more readable.

arch/arm/include/asm/arch-kirkwood/spi.h struct kwspi_registers { · line 46

Quoted 3 lines

arch/arm/include/asm/arch-kirkwood/spi.h +3 -0
+
+#ifndef CONFIG_SYS_KW_SPI_MPP
+#define CONFIG_SYS_KW_SPI_MPP	0x0

Some more documentation is needed, you need to explain how each bit we are using to configure the SPI-MPPs

Quoted 11 lines

arch/arm/include/asm/arch-kirkwood/spi.h +2 -0
+#endif
+
 #define KWSPI_CLKPRESCL_MASK	0x1f
 #define KWSPI_CSN_ACT		1 /* Activates serial memory interface
 */
#define KWSPI_SMEMRDY (1 << 1) /* SerMem Data xfer ready */
drivers/spi/kirkwood_spi.c +0 -0
index db8ba8b..0877915 100644

drivers/spi/kirkwood_spi.c void spi_free_slave(struct spi_slave *slave) · line 90

Quoted 3 lines

drivers/spi/kirkwood_spi.c +0 -0

@@ -88,11 +88,45 @@ void spi_free_slave(struct spi_slave *slave)
 
 int spi_claim_bus(struct spi_slave *slave)
 {

Instead define here
#ifdef CONFIG_SYS_KW_SPI_MPP, otherwise build with default.

Quoted 33 lines

drivers/spi/kirkwood_spi.c +33 -0

+	u32 config;
+	u32 spi_mpp_config[5];
+
+	config = CONFIG_SYS_KW_SPI_MPP;
+
+	if (config & CSn_MPP7)
+		spi_mpp_config[0] = MPP7_SPI_SCn;
+	else
+		spi_mpp_config[0] = MPP0_SPI_SCn;
+
+	if (config & MOSI_MPP6)
+		spi_mpp_config[1] = MPP6_SPI_MOSI;
+	else
+		spi_mpp_config[1] = MPP1_SPI_MOSI;
+
+	if (config & SCK_MPP10)
+		spi_mpp_config[2] = MPP10_SPI_SCK;
+	else
+		spi_mpp_config[2] = MPP2_SPI_SCK;
+
+	if (config & MISO_MPP11)
+		spi_mpp_config[3] = MPP11_SPI_MISO;
+	else
+		spi_mpp_config[3] = MPP3_SPI_MISO;
+
+	spi_mpp_config[4] = 0;
+
+	/* save current mpp configuration */
+	kirkwood_mpp_save();
+
+	/* finally set chosen mpp spi configuration */
+	kirkwood_mpp_conf(spi_mpp_config);
+

drivers/spi/kirkwood_spi.c void spi_free_slave(struct spi_slave *slave)

Quoted 10 lines

drivers/spi/kirkwood_spi.c +1 -1

 	return 0;
 }
 
 void spi_release_bus(struct spi_slave *slave)
 {
+	kirkwood_mpp_restore();
 }
 
 #ifndef CONFIG_SPI_CS_IS_VALID
--

Regards..
Prafulla . . .

Quoted 1 line

1.7.1

Valentin Longchamp

author

2012-05-29 08:32 UTC #17 lore raw

PATCH 2/3 spi/kirkwood: support spi_claim/release_bus functions

Quoted 36 lines · Prafulla Wadaskar

Quoted 36 lines
-----Original Message-----
From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
Sent: 16 May 2012 16:24
To: Prafulla Wadaskar; holger.brunck at keymile.com
Cc: Valentin Longchamp; u-boot at lists.denx.de; Holger Brunck; Prafulla Wadaskar
Subject: [PATCH 2/3] spi/kirkwood: support spi_claim/release_bus functions
These two function nows ensure that the MPP is configured correctly for
the SPI controller before any SPI access, and restore the initial configuration when the access is over.
Since the used pins for the SPI controller can differ (2 possibilities for each signal), the used pins are configured with CONFIG_SYS_KW_SPI_MPP.
Signed-off-by Valentin Longchamp <[email protected]>
cc Holger Brunck <[email protected]>
cc Prafulla Wadaskar <[email protected]>
---
 arch/arm/include/asm/arch-kirkwood/spi.h |    9 ++++++++
 drivers/spi/kirkwood_spi.c               |   34
++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 0 deletions(-)
arch/arm/include/asm/arch-kirkwood/spi.h +0 -0
index 1d5043f..305c573 100644
@@ -37,6 +37,15 @@ struct kwspi_registers {
 	u32 irq_mask;	/* 0x10614 */
 };
 

Quoted 6 lines

arch/arm/include/asm/arch-kirkwood/spi.h struct kwspi_registers { · line 43
Quoted 4 lines
arch/arm/include/asm/arch-kirkwood/spi.h +4 -0
+#define CSn_MPP7	0x1
+#define MOSI_MPP6	0x2
+#define SCK_MPP10	0x4
+#define MISO_MPP11	0x8
Let's define above as (1 << x) to make it more readable.

Quoted 6 lines

arch/arm/include/asm/arch-kirkwood/spi.h struct kwspi_registers { · line 46
Quoted 4 lines
arch/arm/include/asm/arch-kirkwood/spi.h +3 -0
+
+#ifndef CONFIG_SYS_KW_SPI_MPP
+#define CONFIG_SYS_KW_SPI_MPP	0x0
Some more documentation is needed, you need to explain how each bit we are using to configure the SPI-MPPs

Not sure I understand what you mean here. But I think that you mean that I would have to document that bit 1 is for CSn signal (MPP0 or MPP7), bit 2 for MOSI signal (MPP1 or MPP6) and so on ... OK will do it.

Would you want me to define CSn_MPP0 as 0x0 (or (0 << 0) ) and MOSI_MPP1 as 0x0
and so on as well ?

Quoted 13 lines

Quoted 13 lines
arch/arm/include/asm/arch-kirkwood/spi.h +2 -0
+#endif
+
 #define KWSPI_CLKPRESCL_MASK	0x1f
 #define KWSPI_CSN_ACT		1 /* Activates serial memory interface
 */
#define KWSPI_SMEMRDY (1 << 1) /* SerMem Data xfer ready */
drivers/spi/kirkwood_spi.c +0 -0
index db8ba8b..0877915 100644
@@ -88,11 +88,45 @@ void spi_free_slave(struct spi_slave *slave)
 

Quoted 5 lines

drivers/spi/kirkwood_spi.c void spi_free_slave(struct spi_slave *slave) · line 90
Quoted 2 lines
drivers/spi/kirkwood_spi.c +0 -0
 int spi_claim_bus(struct spi_slave *slave)
 {
Instead define here
#ifdef CONFIG_SYS_KW_SPI_MPP, otherwise build with default.

OK, if you prefer it this way, it is fine for me. This implies that I have to remove the above #ifndef CONFIG_SYS_KW_SPI_MPP and that the boards that want to use this will have to #define CONFIG_SYS_KW_SPI_MPP in their config.

Quoted 47 lines

Quoted 34 lines

drivers/spi/kirkwood_spi.c +33 -0

+	u32 config;
+	u32 spi_mpp_config[5];
+
+	config = CONFIG_SYS_KW_SPI_MPP;
+
+	if (config & CSn_MPP7)
+		spi_mpp_config[0] = MPP7_SPI_SCn;
+	else
+		spi_mpp_config[0] = MPP0_SPI_SCn;
+
+	if (config & MOSI_MPP6)
+		spi_mpp_config[1] = MPP6_SPI_MOSI;
+	else
+		spi_mpp_config[1] = MPP1_SPI_MOSI;
+
+	if (config & SCK_MPP10)
+		spi_mpp_config[2] = MPP10_SPI_SCK;
+	else
+		spi_mpp_config[2] = MPP2_SPI_SCK;
+
+	if (config & MISO_MPP11)
+		spi_mpp_config[3] = MPP11_SPI_MISO;
+	else
+		spi_mpp_config[3] = MPP3_SPI_MISO;
+
+	spi_mpp_config[4] = 0;
+
+	/* save current mpp configuration */
+	kirkwood_mpp_save();
+
+	/* finally set chosen mpp spi configuration */
+	kirkwood_mpp_conf(spi_mpp_config);
+

drivers/spi/kirkwood_spi.c void spi_free_slave(struct spi_slave *slave)

Quoted 10 lines

drivers/spi/kirkwood_spi.c +1 -1

 	return 0;
 }
 
 void spi_release_bus(struct spi_slave *slave)
 {
+	kirkwood_mpp_restore();
 }
 
 #ifndef CONFIG_SPI_CS_IS_VALID
--

Regards..
Prafulla . . .

Prafulla Wadaskar

2012-05-29 10:29 UTC #18 lore raw

PATCH 2/3 spi/kirkwood: support spi_claim/release_bus functions

Quoted 64 lines

-----Original Message-----
From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
Sent: 29 May 2012 14:02
To: Prafulla Wadaskar
Cc: holger.brunck at keymile.com; u-boot at lists.denx.de
Subject: Re: [PATCH 2/3] spi/kirkwood: support spi_claim/release_bus functions
Quoted 5 lines · Prafulla Wadaskar
Quoted 5 lines
-----Original Message-----
From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
Sent: 16 May 2012 16:24
To: Prafulla Wadaskar; holger.brunck at keymile.com
Cc: Valentin Longchamp; u-boot at lists.denx.de; Holger Brunck;
Prafulla
Quoted 10 lines
Quoted 10 lines
Wadaskar
Subject: [PATCH 2/3] spi/kirkwood: support spi_claim/release_bus functions
These two function nows ensure that the MPP is configured correctly for
the SPI controller before any SPI access, and restore the initial configuration when the access is over.
Since the used pins for the SPI controller can differ (2
possibilities
Quoted 21 lines
Quoted 21 lines
for each signal), the used pins are configured with CONFIG_SYS_KW_SPI_MPP.
Signed-off-by Valentin Longchamp <[email protected]>
cc Holger Brunck <[email protected]>
cc Prafulla Wadaskar <[email protected]>
---
 arch/arm/include/asm/arch-kirkwood/spi.h |    9 ++++++++
 drivers/spi/kirkwood_spi.c               |   34
++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 0 deletions(-)
arch/arm/include/asm/arch-kirkwood/spi.h +0 -0
index 1d5043f..305c573 100644
@@ -37,6 +37,15 @@ struct kwspi_registers {
 	u32 irq_mask;	/* 0x10614 */
 };
 
Quoted 6 lines
arch/arm/include/asm/arch-kirkwood/spi.h struct kwspi_registers { · line 43
Quoted 4 lines
arch/arm/include/asm/arch-kirkwood/spi.h +4 -0
+#define CSn_MPP7	0x1
+#define MOSI_MPP6	0x2
+#define SCK_MPP10	0x4
+#define MISO_MPP11	0x8
Let's define above as (1 << x) to make it more readable.
OK
Quoted 6 lines
arch/arm/include/asm/arch-kirkwood/spi.h struct kwspi_registers { · line 46
Quoted 4 lines
arch/arm/include/asm/arch-kirkwood/spi.h +3 -0
+
+#ifndef CONFIG_SYS_KW_SPI_MPP
+#define CONFIG_SYS_KW_SPI_MPP	0x0
Some more documentation is needed, you need to explain how each bit

Quoted 7 lines

we are using to configure the SPI-MPPs
Not sure I understand what you mean here. But I think that you mean that I would
have to document that bit 1 is for CSn signal (MPP0 or MPP7), bit 2 for MOSI
signal (MPP1 or MPP6) and so on ... OK will do it.

Exactly,

Quoted 4 lines

Would you want me to define CSn_MPP0 as 0x0 (or (0 << 0) ) and MOSI_MPP1 as 0x0
and so on as well ?

Use any four bits for four configuration, I would suggest to use bit0 to bit-3.

Regards..
Prafulla . . .

Quoted 74 lines

Quoted 9 lines
Review
Quoted 9 lines
arch/arm/include/asm/arch-kirkwood/spi.h +2 -0
+#endif
+
 #define KWSPI_CLKPRESCL_MASK	0x1f
 #define KWSPI_CSN_ACT		1 /* Activates serial memory interface
 */
#define KWSPI_SMEMRDY (1 << 1) /* SerMem Data xfer ready */
(quoted diff) +0 -0
b/drivers/spi/kirkwood_spi.c
Quoted 10 lines
drivers/spi/kirkwood_spi.c void spi_free_slave(struct spi_slave *slave) · line 90
Quoted 7 lines
index db8ba8b..0877915 100644
drivers/spi/kirkwood_spi.c +0 -0
@@ -88,11 +88,45 @@ void spi_free_slave(struct spi_slave *slave)
 
 int spi_claim_bus(struct spi_slave *slave)
 {
Instead define here
#ifdef CONFIG_SYS_KW_SPI_MPP, otherwise build with default.
OK, if you prefer it this way, it is fine for me. This implies that I have to
remove the above #ifndef CONFIG_SYS_KW_SPI_MPP and that the boards that want to
use this will have to #define CONFIG_SYS_KW_SPI_MPP in their config.
Quoted 47 lines
Quoted 34 lines
drivers/spi/kirkwood_spi.c +33 -0
+	u32 config;
+	u32 spi_mpp_config[5];
+
+	config = CONFIG_SYS_KW_SPI_MPP;
+
+	if (config & CSn_MPP7)
+		spi_mpp_config[0] = MPP7_SPI_SCn;
+	else
+		spi_mpp_config[0] = MPP0_SPI_SCn;
+
+	if (config & MOSI_MPP6)
+		spi_mpp_config[1] = MPP6_SPI_MOSI;
+	else
+		spi_mpp_config[1] = MPP1_SPI_MOSI;
+
+	if (config & SCK_MPP10)
+		spi_mpp_config[2] = MPP10_SPI_SCK;
+	else
+		spi_mpp_config[2] = MPP2_SPI_SCK;
+
+	if (config & MISO_MPP11)
+		spi_mpp_config[3] = MPP11_SPI_MISO;
+	else
+		spi_mpp_config[3] = MPP3_SPI_MISO;
+
+	spi_mpp_config[4] = 0;
+
+	/* save current mpp configuration */
+	kirkwood_mpp_save();
+
+	/* finally set chosen mpp spi configuration */
+	kirkwood_mpp_conf(spi_mpp_config);
+
drivers/spi/kirkwood_spi.c void spi_free_slave(struct spi_slave *slave)
Quoted 10 lines
drivers/spi/kirkwood_spi.c +1 -1
 	return 0;
 }
 
 void spi_release_bus(struct spi_slave *slave)
 {
+	kirkwood_mpp_restore();
 }
 
 #ifndef CONFIG_SPI_CS_IS_VALID
--
Regards..
Prafulla . . .

Valentin Longchamp

author

2012-05-29 11:32 UTC #19 lore raw

PATCH 2/3 spi/kirkwood: support spi_claim/release_bus functions

Quoted 70 lines · Prafulla Wadaskar

Quoted 62 lines
-----Original Message-----
From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
Sent: 29 May 2012 14:02
To: Prafulla Wadaskar
Cc: holger.brunck at keymile.com; u-boot at lists.denx.de
Subject: Re: [PATCH 2/3] spi/kirkwood: support spi_claim/release_bus functions
Quoted 5 lines · Prafulla Wadaskar
Quoted 5 lines
-----Original Message-----
From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
Sent: 16 May 2012 16:24
To: Prafulla Wadaskar; holger.brunck at keymile.com
Cc: Valentin Longchamp; u-boot at lists.denx.de; Holger Brunck;
Prafulla
Quoted 10 lines
Quoted 10 lines
Wadaskar
Subject: [PATCH 2/3] spi/kirkwood: support spi_claim/release_bus functions
These two function nows ensure that the MPP is configured correctly for
the SPI controller before any SPI access, and restore the initial configuration when the access is over.
Since the used pins for the SPI controller can differ (2
possibilities
Quoted 21 lines
Quoted 21 lines
for each signal), the used pins are configured with CONFIG_SYS_KW_SPI_MPP.
Signed-off-by Valentin Longchamp <[email protected]>
cc Holger Brunck <[email protected]>
cc Prafulla Wadaskar <[email protected]>
---
 arch/arm/include/asm/arch-kirkwood/spi.h |    9 ++++++++
 drivers/spi/kirkwood_spi.c               |   34
++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 0 deletions(-)
arch/arm/include/asm/arch-kirkwood/spi.h +0 -0
index 1d5043f..305c573 100644
@@ -37,6 +37,15 @@ struct kwspi_registers {
 	u32 irq_mask;	/* 0x10614 */
 };
 
Quoted 6 lines
arch/arm/include/asm/arch-kirkwood/spi.h struct kwspi_registers { · line 43
Quoted 4 lines
arch/arm/include/asm/arch-kirkwood/spi.h +4 -0
+#define CSn_MPP7	0x1
+#define MOSI_MPP6	0x2
+#define SCK_MPP10	0x4
+#define MISO_MPP11	0x8
Let's define above as (1 << x) to make it more readable.
OK
Quoted 6 lines
arch/arm/include/asm/arch-kirkwood/spi.h struct kwspi_registers { · line 46
Quoted 4 lines
arch/arm/include/asm/arch-kirkwood/spi.h +3 -0
+
+#ifndef CONFIG_SYS_KW_SPI_MPP
+#define CONFIG_SYS_KW_SPI_MPP	0x0
Some more documentation is needed, you need to explain how each bit
Quoted 7 lines
we are using to configure the SPI-MPPs
Not sure I understand what you mean here. But I think that you mean that I would
have to document that bit 1 is for CSn signal (MPP0 or MPP7), bit 2 for MOSI
signal (MPP1 or MPP6) and so on ... OK will do it.

Quoted 8 lines

Exactly,
Quoted 4 lines
Would you want me to define CSn_MPP0 as 0x0 (or (0 << 0) ) and MOSI_MPP1 as 0x0
and so on as well ?
Use any four bits for four configuration, I would suggest to use bit0 to bit-3.

That's already how it is implemented ...:

bit 0: selects pin for CSn (MPP0 if 0, MPP7 if 1) bit 1: selects pin for MOSI (MPP1 if 0, MPP6 if 1) bit 2: selects pin for SCK (MPP2 if 0, MPP10 if 1) bit 3: selects pin for MISO (MPP3 if 0, MPP11 if 1)

Quoted 78 lines

Regards..
Prafulla . . .
Quoted 74 lines
Quoted 9 lines
Review
Quoted 9 lines
arch/arm/include/asm/arch-kirkwood/spi.h +2 -0
+#endif
+
 #define KWSPI_CLKPRESCL_MASK	0x1f
 #define KWSPI_CSN_ACT		1 /* Activates serial memory interface
 */
#define KWSPI_SMEMRDY (1 << 1) /* SerMem Data xfer ready */
(quoted diff) +0 -0
b/drivers/spi/kirkwood_spi.c
Quoted 10 lines
drivers/spi/kirkwood_spi.c void spi_free_slave(struct spi_slave *slave) · line 90
Quoted 7 lines
index db8ba8b..0877915 100644
drivers/spi/kirkwood_spi.c +0 -0
@@ -88,11 +88,45 @@ void spi_free_slave(struct spi_slave *slave)
 
 int spi_claim_bus(struct spi_slave *slave)
 {
Instead define here
#ifdef CONFIG_SYS_KW_SPI_MPP, otherwise build with default.
OK, if you prefer it this way, it is fine for me. This implies that I have to
remove the above #ifndef CONFIG_SYS_KW_SPI_MPP and that the boards that want to
use this will have to #define CONFIG_SYS_KW_SPI_MPP in their config.
Quoted 47 lines
Quoted 34 lines
drivers/spi/kirkwood_spi.c +33 -0
+	u32 config;
+	u32 spi_mpp_config[5];
+
+	config = CONFIG_SYS_KW_SPI_MPP;
+
+	if (config & CSn_MPP7)
+		spi_mpp_config[0] = MPP7_SPI_SCn;
+	else
+		spi_mpp_config[0] = MPP0_SPI_SCn;
+
+	if (config & MOSI_MPP6)
+		spi_mpp_config[1] = MPP6_SPI_MOSI;
+	else
+		spi_mpp_config[1] = MPP1_SPI_MOSI;
+
+	if (config & SCK_MPP10)
+		spi_mpp_config[2] = MPP10_SPI_SCK;
+	else
+		spi_mpp_config[2] = MPP2_SPI_SCK;
+
+	if (config & MISO_MPP11)
+		spi_mpp_config[3] = MPP11_SPI_MISO;
+	else
+		spi_mpp_config[3] = MPP3_SPI_MISO;
+
+	spi_mpp_config[4] = 0;
+
+	/* save current mpp configuration */
+	kirkwood_mpp_save();
+
+	/* finally set chosen mpp spi configuration */
+	kirkwood_mpp_conf(spi_mpp_config);
+
drivers/spi/kirkwood_spi.c void spi_free_slave(struct spi_slave *slave)
Quoted 10 lines
drivers/spi/kirkwood_spi.c +1 -1
 	return 0;
 }
 
 void spi_release_bus(struct spi_slave *slave)
 {
+	kirkwood_mpp_restore();
 }
 
 #ifndef CONFIG_SPI_CS_IS_VALID
--
Regards..
Prafulla . . .

Valentin Longchamp

author

2012-05-16 10:53 UTC #20 lore raw

PATCH 3/3 spi/kirkwood: add weak functions board_spi_claim/release_bus

In reply to Valentin Longchamp

This allows a final, board specific, step in the claim/relase_bus function for the SPI controller, which may be needed for some hardware designs.

Signed-off-by Valentin Longchamp <[email protected]>

cc Holger Brunck <[email protected]>

cc Prafulla Wadaskar <[email protected]>

---
 drivers/spi/kirkwood_spi.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

drivers/spi/kirkwood_spi.c +12 -1

index 0877915..0e4db45 100644
@@ -86,6 +86,11 @@ void spi_free_slave(struct spi_slave *slave)
 	free(slave);
 }
 
+__attribute__((weak)) int board_spi_claim_bus(struct spi_slave *slave)
+{
+	return 0;
+}
+
 int spi_claim_bus(struct spi_slave *slave)
 {
 	u32 config;

@@ -121,12 +126,18 @@ int spi_claim_bus(struct spi_slave *slave)
 	/* finally set chosen mpp spi configuration */
 	kirkwood_mpp_conf(spi_mpp_config);
 
-	return 0;
+	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)
 {
 	kirkwood_mpp_restore();
+
+	board_spi_release_bus(slave);
 }
 
 #ifndef CONFIG_SPI_CS_IS_VALID

Signature · 1 line

1.7.1

Prafulla Wadaskar

NAK

2012-05-24 08:38 UTC #21 lore raw

PATCH 3/3 spi/kirkwood: add weak functions board_spi_claim/release_bus

Quoted 11 lines

-----Original Message-----
From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
Sent: 16 May 2012 16:24
To: Prafulla Wadaskar; holger.brunck at keymile.com
Cc: Valentin Longchamp; u-boot at lists.denx.de; Holger Brunck; Prafulla Wadaskar
Subject: [PATCH 3/3] spi/kirkwood: add weak functions board_spi_claim/release_bus

Quoted 3 lines

This allows a final, board specific, step in the claim/relase_bus function for the SPI controller, which may be needed for some hardware designs.

NAK, this is not needed if earlier two patches in the patch series are in place.

Regards..
Prafulla . . .

Valentin Longchamp

author

2012-05-29 08:32 UTC #22 lore raw

PATCH 3/3 spi/kirkwood: add weak functions board_spi_claim/release_bus

Quoted 9 lines · Prafulla Wadaskar

Quoted 9 lines
-----Original Message-----
From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
Sent: 16 May 2012 16:24
To: Prafulla Wadaskar; holger.brunck at keymile.com
Cc: Valentin Longchamp; u-boot at lists.denx.de; Holger Brunck; Prafulla Wadaskar
Subject: [PATCH 3/3] spi/kirkwood: add weak functions board_spi_claim/release_bus

Quoted 6 lines

Quoted 3 lines
This allows a final, board specific, step in the claim/relase_bus function for the SPI controller, which may be needed for some hardware designs.
NAK, this is not needed if earlier two patches in the patch series are in place.

In our case, this is still needed. As I had already explained you in the previous discussion, even with the generic approach, our hardware design requires one access to an additional signal (a GPIO) to configure an external HW multiplexer which is present to electrically remove the Nand Flash device from the signals used by the SPI bus and put it back when the accesses are over.

That's why my first implementation was only relying on these weak functions.

Prafulla Wadaskar

2012-05-29 12:13 UTC #23 lore raw

PATCH 3/3 spi/kirkwood: add weak functions board_spi_claim/release_bus

Quoted 41 lines

-----Original Message-----
From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
Sent: 29 May 2012 14:03
To: Prafulla Wadaskar
Cc: holger.brunck at keymile.com; u-boot at lists.denx.de
Subject: Re: [PATCH 3/3] spi/kirkwood: add weak functions board_spi_claim/release_bus
Quoted 5 lines · Prafulla Wadaskar
Quoted 5 lines
-----Original Message-----
From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
Sent: 16 May 2012 16:24
To: Prafulla Wadaskar; holger.brunck at keymile.com
Cc: Valentin Longchamp; u-boot at lists.denx.de; Holger Brunck;
Prafulla
Quoted 6 lines
Quoted 6 lines
Wadaskar
Subject: [PATCH 3/3] spi/kirkwood: add weak functions board_spi_claim/release_bus
This allows a final, board specific, step in the claim/relase_bus function for the SPI controller, which may be needed for some
hardware
Quoted 3 lines
Quoted 1 line
designs.
NAK, this is not needed if earlier two patches in the patch series
are in place.
Quoted 1 line
In our case, this is still needed. As I had already explained you in the
previous discussion, even with the generic approach, our hardware design
requires one access to an additional signal (a GPIO) to configure an external HW
multiplexer which is present to electrically remove the Nand Flash device from
the signals used by the SPI bus and put it back when the accesses are over.

Quoted 2 lines

That's why my first implementation was only relying on these weak functions.

Okay, got it, on your board, apart from MPPs, you need additional control.

BTW: if NF_CEn could have been used this additional GPIO would not have needed. But any ways we cannot change your h/w now :-)
So in that case it makes sense to expose these weak functions.

Regards..
Prafulla . . .