Re: [U-Boot] [PATCH v3 3/7] [REPOST-1] gpio: Add Multi-Function-Pin configuration driver for Marvell SoCs

2010-12-02 Thread Wolfgang Denk 
Dear Prafulla Wadaskar,

In message f766e4f80769bd478052fb6533fa745d19a9291...@sc-vexch4.marvell.com 
you wrote:
 
  Since the mfp.c is for generic case, and should be made as generic at
  the beginning...
 
 Sure.. but I don't know how it will be on other SoCs?
 Apart from that what would be other impact?
 So in my opinion, let's keep it for future updates.

No, this is the wrong approach.

Already now you realize that this code does not reallys cale well with
future extensions, because you put all the logic in your code.

I think this is a bad approach here.

Instead, make the whole thing data driven - implement only minimal
code that waks through a table provided by the user.  Instead of
putting the logic in the code, try to put it in the data.

 With reference to your precise concern, I will create a macro in 
 asm/arch/mfp.h
 moving below code there.

 +  if ( mfpr_no  37)
 +   p_mfpr += (0x004c / 4) + mfpr_no;
 +  else if ( mfpr_no = 56)
 +   p_mfpr += (0x00e0 / 4) + (mfpr_no - 56);
 +  else
 +   p_mfpr += (mfpr_no - 37)

No, please don't.  It's ugly, not readable and not maintainable.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, MD: Wolfgang Denk  Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
You might not be as stupid as you look. This is not hard. Let's think
about this. I mean ... I'll think about this, and  you  can  join  in
when you know the words. - Terry Pratchett, _Men at Arms_
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH v3 3/7] [REPOST-1] gpio: Add Multi-Function-Pin configuration driver for Marvell SoCs

2010-12-01 Thread Prafulla Wadaskar 
Most of the Marvell SoCs has Multi Function Pin (MFP) configuration registers
For ex. ARMADA100.

These registers are programmed to expose the specific functionality
associated with respective SoC Pins

This driver provides configuration APIs,
using them, configuration need to be done in board specific code

for ex- following code configures MFPs 107 and 108 for UART_TX/RX functionality

int board_early_init_f(void)
{
u32 mfp_cfg[] = {
/* Console on UART1 */
MFP107_UART1_RXD,
MFP108_UART1_TXD,
MFP_EOC /*End of configureation*/
};
/* configure MFP's */
mfp_config(mfp_cfg);
return 0;
}

Signed-off-by: Prafulla Wadaskar prafu...@marvell.com
---
Please Ignore earlier patch, below bufix is added to this patch
 
Change log v3:
Fixed: Read-modify-Write a mfg register as per configuration
ANDed with MASK value before ORing for each configuration

REPOST-V1:
I am sorry for the repost-V1 :-(
MASK values changed to relevent (copy paste mistake)

Regards..
Prafulla . .

 drivers/gpio/Makefile |1 +
 drivers/gpio/mfp.c|  110 +
 include/mfp.h |   97 +++
 3 files changed, 208 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpio/mfp.c
 create mode 100644 include/mfp.h

diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 398024c..f6903d5 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -27,6 +27,7 @@ LIB   := $(obj)libgpio.o
 
 COBJS-$(CONFIG_AT91_GPIO)  += at91_gpio.o
 COBJS-$(CONFIG_KIRKWOOD_GPIO)  += kw_gpio.o
+COBJS-$(CONFIG_MFP)+= mfp.o
 COBJS-$(CONFIG_MXC_GPIO)   += mxc_gpio.o
 COBJS-$(CONFIG_PCA953X)+= pca953x.o
 COBJS-$(CONFIG_S5P)+= s5p_gpio.o
diff --git a/drivers/gpio/mfp.c b/drivers/gpio/mfp.c
new file mode 100644
index 000..f26af97
--- /dev/null
+++ b/drivers/gpio/mfp.c
@@ -0,0 +1,110 @@
+/*
+ * (C) Copyright 2010
+ * Marvell Semiconductor www.marvell.com
+ * Written-by: Prafulla Wadaskar prafu...@marvell.com,
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+ * MA 02110-1301 USA
+ */
+
+#include common.h
+#include asm/io.h
+#include mfp.h
+#include asm/arch/mfp.h
+#ifdef CONFIG_ARMADA100
+#include asm/arch/armada100.h
+#define MFPR_BASE  ARMD1_MFPR_BASE;
+#else
+#error Unsupported SoC...
+#endif
+
+/*
+ * mfp_config
+ *
+ * On most of Marvell SoCs (ex. ARMADA100) there is Multi-Funtion-Pin
+ * configuration registers to configure each GPIO/Function pin on the
+ * SoC.
+ *
+ * This function reads the array of values for
+ * MFPR_X registers and programms them into respective
+ * Multi-Function Pin registers.
+ * It supports - Alternate Function Selection programming.
+ *
+ * Whereas,
+ * The Configureation value is constructed using ARMD_MFP()
+ * array consists of 32bit values as-
+ * Bits 31-16  : Mfp instance number (i.e. MFPR no. to be programmed)
+ * Bits 15-13  : PULL_UP/PULL_DOWN selection
+ * Bits 11:10  : Pin Driver strength
+ * Bits 6-4: Edge detection configuration
+ * Bits 2-0: Alternate Function Selection
+ *
+ * For more details please refer respective Product Software Manual
+ */
+void mfp_config(u32 *mfp_cfgs)
+{
+   u32 *p_mfpr = NULL;
+   u32 val, cfg_val, mfpr_no;
+
+   do {
+   cfg_val = *mfp_cfgs++;
+   /* exit if End of configuration table detected */
+   if (cfg_val == MFP_EOC)
+   break;
+   /* abstract mfpr tobe programmed from configuration value */
+   mfpr_no = (cfg_val  MFP_PINNO_MASK)  16;
+   BUG_ON(mfpr_no = MFP_PIN_MAX);
+
+   /* the offset address are divided in three regions and not
+* consecutive, this corrects the same (Ref: Specs: A1.1) */
+   p_mfpr = (u32 *)MFPR_BASE;
+   if ( mfpr_no  37)
+   p_mfpr += (0x004c / 4) + mfpr_no;
+   else if ( mfpr_no = 56)
+   p_mfpr += (0x00e0 / 4) + (mfpr_no - 56);
+   else
+   p_mfpr += (mfpr_no - 37);
+   /*p_mfpr contains address of register

Re: [U-Boot] [PATCH v3 3/7] [REPOST-1] gpio: Add Multi-Function-Pin configuration driver for Marvell SoCs

2010-12-01 Thread Lei Wen 
Hi Prafulla,

On Thu, Dec 2, 2010 at 12:01 AM, Prafulla Wadaskar prafu...@marvell.com wrote:
 Most of the Marvell SoCs has Multi Function Pin (MFP) configuration registers
 For ex. ARMADA100.

 These registers are programmed to expose the specific functionality
 associated with respective SoC Pins

 This driver provides configuration APIs,
 using them, configuration need to be done in board specific code

 for ex- following code configures MFPs 107 and 108 for UART_TX/RX 
 functionality

 int board_early_init_f(void)
 {
        u32 mfp_cfg[] = {
                /* Console on UART1 */
                MFP107_UART1_RXD,
                MFP108_UART1_TXD,
                MFP_EOC         /*End of configureation*/
        };
        /* configure MFP's */
        mfp_config(mfp_cfg);
        return 0;
 }

 Signed-off-by: Prafulla Wadaskar prafu...@marvell.com
 ---
 Please Ignore earlier patch, below bufix is added to this patch

 Change log v3:
 Fixed: Read-modify-Write a mfg register as per configuration
 ANDed with MASK value before ORing for each configuration

 REPOST-V1:
 I am sorry for the repost-V1 :-(
 MASK values changed to relevent (copy paste mistake)

 Regards..
 Prafulla . .

  drivers/gpio/Makefile |    1 +
  drivers/gpio/mfp.c    |  110 
 +
  include/mfp.h         |   97 +++
  3 files changed, 208 insertions(+), 0 deletions(-)
  create mode 100644 drivers/gpio/mfp.c
  create mode 100644 include/mfp.h

 diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
 index 398024c..f6903d5 100644
 --- a/drivers/gpio/Makefile
 +++ b/drivers/gpio/Makefile
 @@ -27,6 +27,7 @@ LIB   := $(obj)libgpio.o

  COBJS-$(CONFIG_AT91_GPIO)      += at91_gpio.o
  COBJS-$(CONFIG_KIRKWOOD_GPIO)  += kw_gpio.o
 +COBJS-$(CONFIG_MFP)            += mfp.o
  COBJS-$(CONFIG_MXC_GPIO)       += mxc_gpio.o
  COBJS-$(CONFIG_PCA953X)                += pca953x.o
  COBJS-$(CONFIG_S5P)            += s5p_gpio.o
 diff --git a/drivers/gpio/mfp.c b/drivers/gpio/mfp.c
 new file mode 100644
 index 000..f26af97
 --- /dev/null
 +++ b/drivers/gpio/mfp.c
 @@ -0,0 +1,110 @@
 +/*
 + * (C) Copyright 2010
 + * Marvell Semiconductor www.marvell.com
 + * Written-by: Prafulla Wadaskar prafu...@marvell.com,
 + *
 + * See file CREDITS for list of people who contributed to this
 + * project.
 + *
 + * This program is free software; you can redistribute it and/or
 + * modify it under the terms of the GNU General Public License as
 + * published by the Free Software Foundation; either version 2 of
 + * the License, or (at your option) any later version.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program; if not, write to the Free Software
 + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
 + * MA 02110-1301 USA
 + */
 +
 +#include common.h
 +#include asm/io.h
 +#include mfp.h
 +#include asm/arch/mfp.h
 +#ifdef CONFIG_ARMADA100
 +#include asm/arch/armada100.h
 +#define MFPR_BASE      ARMD1_MFPR_BASE;
 +#else
 +#error Unsupported SoC...
 +#endif
 +
 +/*
 + * mfp_config
 + *
 + * On most of Marvell SoCs (ex. ARMADA100) there is Multi-Funtion-Pin
 + * configuration registers to configure each GPIO/Function pin on the
 + * SoC.
 + *
 + * This function reads the array of values for
 + * MFPR_X registers and programms them into respective
 + * Multi-Function Pin registers.
 + * It supports - Alternate Function Selection programming.
 + *
 + * Whereas,
 + * The Configureation value is constructed using ARMD_MFP()
 + * array consists of 32bit values as-
 + * Bits 31-16  : Mfp instance number (i.e. MFPR no. to be programmed)
 + * Bits 15-13  : PULL_UP/PULL_DOWN selection
 + * Bits 11:10  : Pin Driver strength
 + * Bits 6-4    : Edge detection configuration
 + * Bits 2-0    : Alternate Function Selection
 + *
 + * For more details please refer respective Product Software Manual
 + */
 +void mfp_config(u32 *mfp_cfgs)
 +{
 +       u32 *p_mfpr = NULL;
 +       u32 val, cfg_val, mfpr_no;
 +
 +       do {
 +               cfg_val = *mfp_cfgs++;
 +               /* exit if End of configuration table detected */
 +               if (cfg_val == MFP_EOC)
 +                       break;
 +               /* abstract mfpr tobe programmed from configuration value */
 +               mfpr_no = (cfg_val  MFP_PINNO_MASK)  16;
 +               BUG_ON(mfpr_no = MFP_PIN_MAX);
 +
 +               /* the offset address are divided in three regions and not
 +                * consecutive, this corrects the same (Ref: Specs: A1.1) */
 +               p_mfpr = (u32 *)MFPR_BASE;
 +               if ( mfpr_no  37)
 +                       p_mfpr += (0x004c / 4) + mfpr_no;
 +              

Re: [U-Boot] [PATCH v3 3/7] [REPOST-1] gpio: Add Multi-Function-Pin configuration driver for Marvell SoCs

2010-12-01 Thread Prafulla Wadaskar 


 -Original Message-
 From: Lei Wen [mailto:adrian.w...@gmail.com]
 Sent: Wednesday, December 01, 2010 7:51 PM
 To: Prafulla Wadaskar
 Cc: u-boot@lists.denx.de; Eric Miao; Manas Saksena; Lei Wen; Yu Tang;
 Ashish Karkare; Kiran Vedere; Prabhanjan Sarnaik
 Subject: Re: [U-Boot] [PATCH v3 3/7] [REPOST-1] gpio: Add Multi-Function-
 Pin configuration driver for Marvell SoCs
 
 Hi Prafulla,
...snip...
  +
  +               /* the offset address are divided in three regions and
 not
  +                * consecutive, this corrects the same (Ref: Specs:
 A1.1) */
  +               p_mfpr = (u32 *)MFPR_BASE;
  +               if ( mfpr_no  37)
  +                       p_mfpr += (0x004c / 4) + mfpr_no;
  +               else if ( mfpr_no = 56)
  +                       p_mfpr += (0x00e0 / 4) + (mfpr_no - 56);
  +               else
  +                       p_mfpr += (mfpr_no - 37);
 
 This three regions is only meaningful for armada100, so when we use
 the mfp.c to other cpu, like pxa920,
 how could we reuse this setting? Obviously, pxa920 is different with
 armada100 regions arrangement, with
 the start mfp of ND_IO[15].

Currently mfp is added for armada100, I don't know what will be diffs with 
other SoCs.
When other SoC will come up, we will modify this file :-)

Regards..
Prafulla . .
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 3/7] [REPOST-1] gpio: Add Multi-Function-Pin configuration driver for Marvell SoCs

2010-12-01 Thread Lei Wen 
Hi Prafulla,

On Thu, Dec 2, 2010 at 2:54 PM, Prafulla Wadaskar prafu...@marvell.com wrote:


 -Original Message-
 From: Lei Wen [mailto:adrian.w...@gmail.com]
 Sent: Wednesday, December 01, 2010 7:51 PM
 To: Prafulla Wadaskar
 Cc: u-boot@lists.denx.de; Eric Miao; Manas Saksena; Lei Wen; Yu Tang;
 Ashish Karkare; Kiran Vedere; Prabhanjan Sarnaik
 Subject: Re: [U-Boot] [PATCH v3 3/7] [REPOST-1] gpio: Add Multi-Function-
 Pin configuration driver for Marvell SoCs

 Hi Prafulla,
 ...snip...
  +
  +               /* the offset address are divided in three regions and
 not
  +                * consecutive, this corrects the same (Ref: Specs:
 A1.1) */
  +               p_mfpr = (u32 *)MFPR_BASE;
  +               if ( mfpr_no  37)
  +                       p_mfpr += (0x004c / 4) + mfpr_no;
  +               else if ( mfpr_no = 56)
  +                       p_mfpr += (0x00e0 / 4) + (mfpr_no - 56);
  +               else
  +                       p_mfpr += (mfpr_no - 37);

 This three regions is only meaningful for armada100, so when we use
 the mfp.c to other cpu, like pxa920,
 how could we reuse this setting? Obviously, pxa920 is different with
 armada100 regions arrangement, with
 the start mfp of ND_IO[15].

 Currently mfp is added for armada100, I don't know what will be diffs with 
 other SoCs.
 When other SoC will come up, we will modify this file :-)

Since the mfp.c is for generic case, and should be made as generic at
the beginning...
Current this is only hard coding, I think provide some kind of mapping
would be good.

Best regards,
Lei
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 3/7] [REPOST-1] gpio: Add Multi-Function-Pin configuration driver for Marvell SoCs

2010-12-01 Thread Prafulla Wadaskar 


 -Original Message-
 From: Lei Wen [mailto:adrian.w...@gmail.com]
 Sent: Thursday, December 02, 2010 12:29 PM
 To: Prafulla Wadaskar
 Cc: u-boot@lists.denx.de; Eric Miao; Manas Saksena; Lei Wen; Yu Tang;
 Ashish Karkare; Kiran Vedere; Prabhanjan Sarnaik
 Subject: Re: [U-Boot] [PATCH v3 3/7] [REPOST-1] gpio: Add Multi-Function-
 Pin configuration driver for Marvell SoCs
 
 Hi Prafulla,
 
 On Thu, Dec 2, 2010 at 2:54 PM, Prafulla Wadaskar prafu...@marvell.com
 wrote:
 
 
  -Original Message-
  From: Lei Wen [mailto:adrian.w...@gmail.com]
  Sent: Wednesday, December 01, 2010 7:51 PM
  To: Prafulla Wadaskar
  Cc: u-boot@lists.denx.de; Eric Miao; Manas Saksena; Lei Wen; Yu Tang;
  Ashish Karkare; Kiran Vedere; Prabhanjan Sarnaik
  Subject: Re: [U-Boot] [PATCH v3 3/7] [REPOST-1] gpio: Add Multi-
 Function-
  Pin configuration driver for Marvell SoCs
 
  Hi Prafulla,
  ...snip...
   +
   +               /* the offset address are divided in three regions
 and
  not
   +                * consecutive, this corrects the same (Ref: Specs:
  A1.1) */
   +               p_mfpr = (u32 *)MFPR_BASE;
   +               if ( mfpr_no  37)
   +                       p_mfpr += (0x004c / 4) + mfpr_no;
   +               else if ( mfpr_no = 56)
   +                       p_mfpr += (0x00e0 / 4) + (mfpr_no - 56);
   +               else
   +                       p_mfpr += (mfpr_no - 37);
 
  This three regions is only meaningful for armada100, so when we use
  the mfp.c to other cpu, like pxa920,
  how could we reuse this setting? Obviously, pxa920 is different with
  armada100 regions arrangement, with
  the start mfp of ND_IO[15].
 
  Currently mfp is added for armada100, I don't know what will be diffs
 with other SoCs.
  When other SoC will come up, we will modify this file :-)
 
 Since the mfp.c is for generic case, and should be made as generic at
 the beginning...

Sure.. but I don't know how it will be on other SoCs?
Apart from that what would be other impact?
So in my opinion, let's keep it for future updates.

With reference to your precise concern, I will create a macro in asm/arch/mfp.h 
moving below code there.

+  if ( mfpr_no  37)
+   p_mfpr += (0x004c / 4) + mfpr_no;
+  else if ( mfpr_no = 56)
+   p_mfpr += (0x00e0 / 4) + (mfpr_no - 56);
+  else
+   p_mfpr += (mfpr_no - 37)

This will help to isolate it as SoC specific implementation, I will post 
REPOST-2 for the same

Thanks and Regards..
Prafulla . . 
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot