[PATCH] PXA: Colibri320: Add M41T00 RTC support

[PATCH] PXA: Colibri320: Add M41T00 RTC support

[Marek Vasut]

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
---
 arch/arm/mach-pxa/colibri-pxa320.c       |   16 ++++++++++++++++
 arch/arm/mach-pxa/colibri-pxa3xx.c       |   14 ++++++++++++++
 arch/arm/mach-pxa/include/mach/colibri.h |    7 +++++++
 3 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-pxa/colibri-pxa320.c b/arch/arm/mach-pxa/colibri-pxa320.c
index 99e850d..0bc5f98 100644
--- a/arch/arm/mach-pxa/colibri-pxa320.c
+++ b/arch/arm/mach-pxa/colibri-pxa320.c
@@ -236,6 +236,21 @@ static void __init colibri_pxa320_init_uart(void)
 	pxa3xx_mfp_config(ARRAY_AND_SIZE(colibri_pxa320_uart_pin_config));
 }
 
+#if defined(CONFIG_RTC_DRV_DS1307) || defined(CONFIG_RTC_DRV_DS1307_MODULE)
+static mfp_cfg_t colibri_pxa320_i2c_pin_config[] __initdata = {
+	GPIO32_I2C_SCL,
+	GPIO33_I2C_SDA,
+};
+
+static void __init colibri_pxa320_init_rtc(void)
+{
+	pxa3xx_mfp_config(ARRAY_AND_SIZE(colibri_pxa320_i2c_pin_config));
+	colibri_pxa3xx_init_rtc();
+}
+#else
+static inline void colibri_pxa320_init_rtc(void) {}
+#endif
+
 void __init colibri_pxa320_init(void)
 {
 	pxa_set_ffuart_info(NULL);
@@ -252,6 +267,7 @@ void __init colibri_pxa320_init(void)
 				mfp_to_gpio(MFP_PIN_GPIO28));
 	colibri_pxa320_init_uart();
 	colibri_pxa320_init_udc();
+	colibri_pxa320_init_rtc();
 }
 
 MACHINE_START(COLIBRI320, "Toradex Colibri PXA320")
diff --git a/arch/arm/mach-pxa/colibri-pxa3xx.c b/arch/arm/mach-pxa/colibri-pxa3xx.c
index 199afa2..2bbeae6 100644
--- a/arch/arm/mach-pxa/colibri-pxa3xx.c
+++ b/arch/arm/mach-pxa/colibri-pxa3xx.c
@@ -26,6 +26,7 @@
 #include <mach/mmc.h>
 #include <mach/pxafb.h>
 #include <plat/pxa3xx_nand.h>
+#include <plat/i2c.h>
 
 #include "generic.h"
 #include "devices.h"
@@ -198,3 +199,16 @@ void __init colibri_pxa3xx_init_nand(void)
 }
 #endif
 
+#if defined(CONFIG_RTC_DRV_DS1307) || defined(CONFIG_RTC_DRV_DS1307_MODULE)
+static struct i2c_board_info __initdata colibri_pxa3xx_i2c_devs[] = {
+	{
+		I2C_BOARD_INFO("m41t00", 0x68),
+	},
+};
+
+void __init colibri_pxa3xx_init_rtc(void)
+{
+	pxa_set_i2c_info(NULL);
+	i2c_register_board_info(0, ARRAY_AND_SIZE(colibri_pxa3xx_i2c_devs));
+}
+#endif
diff --git a/arch/arm/mach-pxa/include/mach/colibri.h b/arch/arm/mach-pxa/include/mach/colibri.h
index 58dada1..d5dc04d 100644
--- a/arch/arm/mach-pxa/include/mach/colibri.h
+++ b/arch/arm/mach-pxa/include/mach/colibri.h
@@ -51,6 +51,13 @@ extern void colibri_pxa3xx_init_nand(void);
 static inline void colibri_pxa3xx_init_nand(void) {}
 #endif
 
+#if defined(CONFIG_RTC_DRV_DS1307) || defined(CONFIG_RTC_DRV_DS1307_MODULE)
+extern void colibri_pxa3xx_init_rtc(void);
+#else
+static inline void colibri_pxa3xx_init_rtc(void) {}
+#endif
+
+
 /* physical memory regions */
 #define COLIBRI_SDRAM_BASE	0xa0000000      /* SDRAM region */
 
-- 
1.7.1

[PATCH] PXA: Colibri320: Add M41T00 RTC support

[pieterg]

On Saturday 31 July 2010 04:30:53 Marek Vasut wrote:
> +#if defined(CONFIG_RTC_DRV_DS1307) ||
> defined(CONFIG_RTC_DRV_DS1307_MODULE) 
> +static mfp_cfg_t colibri_pxa320_i2c_pin_config[] __initdata = { 
> +	GPIO32_I2C_SCL, 
> +	GPIO33_I2C_SDA,
> +};

Should the i2c pins really depend on the DS1307 config?
On my board, I use a different rtc. So I do need the I2C pins to be 
configured, but I don't need the DS1307.

A few weeks ago I submitted

[PATCH 4/5] colibri-pxa3xx: add i2c support

which initializes the i2c gpio's, depending on CONFIG_I2C || 
CONFIG_I2C_MODULE

(but the patch was rejected, argueing that people might want to use 
different gpio's for i2c)

Rgds, Pieter

[PATCH] PXA: Colibri320: Add M41T00 RTC support

[Marek Vasut]

Dne So 31. ?ervence 2010 09:26:03 pieterg napsal(a):
> On Saturday 31 July 2010 04:30:53 Marek Vasut wrote:
> > +#if defined(CONFIG_RTC_DRV_DS1307) ||
> > defined(CONFIG_RTC_DRV_DS1307_MODULE)
> > +static mfp_cfg_t colibri_pxa320_i2c_pin_config[] __initdata = {
> > +	GPIO32_I2C_SCL,
> > +	GPIO33_I2C_SDA,
> > +};
> 
> Should the i2c pins really depend on the DS1307 config?
> On my board, I use a different rtc. So I do need the I2C pins to be
> configured, but I don't need the DS1307.
> 
> A few weeks ago I submitted
> 
> [PATCH 4/5] colibri-pxa3xx: add i2c support
> 
> which initializes the i2c gpio's, depending on CONFIG_I2C ||
> CONFIG_I2C_MODULE
> 
> (but the patch was rejected, argueing that people might want to use
> different gpio's for i2c)
> 
> Rgds, Pieter

Well I'd like to rework the colibri pxa3xx stuff later, but I'll have to discuss 
it with Dan. I believe reworking it the same way as pxa270 colibri would be nice 
(and it'd solve your problem too as you use custom board I think?).

Actually, there is one thing that puzzles me. Eric/Dan, shall I stick all the 
MFP configs into one (or two, one for board and once for cpu card) array or keep 
it split in multiple smaller arrays for each device? I think putting it all in 
one place would be more readable.

Cheers

[PATCH] PXA: Colibri320: Add M41T00 RTC support

[Daniel Mack]

On Sat, Jul 31, 2010 at 09:33:05AM +0200, Marek Vasut wrote:
> Dne So 31. ?ervence 2010 09:26:03 pieterg napsal(a):
> > On Saturday 31 July 2010 04:30:53 Marek Vasut wrote:
> > > +#if defined(CONFIG_RTC_DRV_DS1307) ||
> > > defined(CONFIG_RTC_DRV_DS1307_MODULE)
> > > +static mfp_cfg_t colibri_pxa320_i2c_pin_config[] __initdata = {
> > > +	GPIO32_I2C_SCL,
> > > +	GPIO33_I2C_SDA,
> > > +};
> > 
> > Should the i2c pins really depend on the DS1307 config?
> > On my board, I use a different rtc. So I do need the I2C pins to be
> > configured, but I don't need the DS1307.

Yes, I think you're right. The I2C pins should only be set up if I2C
support for PXA is in fact enabled. If someone manages to build a kernel
which has support for this driver (which should depend on the I2C core
as its transport layer) but not for PXA I2C, that's an unfortunate
exception.

You could think about something like

  select I2C_PXA if RTC_DRV_DS1307

in the Colibri block of mach-pxa/Kconfig, but that's somewhat overdone
maybe.

> Well I'd like to rework the colibri pxa3xx stuff later, but I'll have to discuss 
> it with Dan. I believe reworking it the same way as pxa270 colibri would be nice 
> (and it'd solve your problem too as you use custom board I think?).

Yes, that'd be nice.

> Actually, there is one thing that puzzles me. Eric/Dan, shall I stick all the 
> MFP configs into one (or two, one for board and once for cpu card) array or keep 
> it split in multiple smaller arrays for each device? I think putting it all in 
> one place would be more readable.

Hmm, GPIOs for peripherals that are assembled on the module itself can
safely be configured unconditionally as it is impossible to use them for
anything else. For GPIOs that are allocted by a specific base board,
things are different though, especially for the the Colibri eval board
with it hundreds of jumpers on it. It's a convenience feature to have
them free for general purpose use just by disabling a certain config
directive.

Daniel

[PATCH] PXA: Colibri320: Add M41T00 RTC support

[Russell King - ARM Linux]

On Sat, Jul 31, 2010 at 09:33:05AM +0200, Marek Vasut wrote:
> Actually, there is one thing that puzzles me. Eric/Dan, shall I stick all the 
> MFP configs into one (or two, one for board and once for cpu card) array or keep 
> it split in multiple smaller arrays for each device? I think putting it all in 
> one place would be more readable.

I've always been of the opinion that the pin configuration should be setup
independently of what drivers are built into the kernel or not.

Consider a pin which is used as an output to drive some high impedance
signal (eg, an input on another device) and it doesn't have any pull-up
or pull down.  If on reset, this signal is also configured as a high
impedance input, it will float.

Inputs which float cause increased current drain (that's a fact of the
high impedance FET input configurations - a floating signal ends up
partially turning on both N and P FETs at both ends, which allows
current to flow between supply and ground.  This is also why the current
consumption of devices is related to their clock frequency and disabling
clocks saves power.)

So, it is desirable to have the pin configuration correct for the entire
board, irrespective of the driver configuration.

[PATCH] PXA: Colibri320: Add M41T00 RTC support

[Russell King - ARM Linux]

On Sat, Jul 31, 2010 at 11:05:54AM +0200, Daniel Mack wrote:
> Hmm, GPIOs for peripherals that are assembled on the module itself can
> safely be configured unconditionally as it is impossible to use them for
> anything else. For GPIOs that are allocted by a specific base board,
> things are different though, especially for the the Colibri eval board
> with it hundreds of jumpers on it. It's a convenience feature to have
> them free for general purpose use just by disabling a certain config
> directive.

It would probably make sense for their to be a primary table which at
least ensures that all pins are setup to safe default values.  You
really don't want the unconnected pins left floating, especially if
they come out to some vulnerable connector or unconnected jumper pin.

I'd suggest they're configured as inputs but with a pull-up or pull-down,
not forgetting to consider the low power mode pull setting as well.

[PATCH] PXA: Colibri320: Add M41T00 RTC support

[Marek Vasut]

Dne So 31. ?ervence 2010 11:05:54 Daniel Mack napsal(a):
> On Sat, Jul 31, 2010 at 09:33:05AM +0200, Marek Vasut wrote:
> > Dne So 31. ?ervence 2010 09:26:03 pieterg napsal(a):
> > > On Saturday 31 July 2010 04:30:53 Marek Vasut wrote:
> > > > +#if defined(CONFIG_RTC_DRV_DS1307) ||
> > > > defined(CONFIG_RTC_DRV_DS1307_MODULE)
> > > > +static mfp_cfg_t colibri_pxa320_i2c_pin_config[] __initdata = {
> > > > +	GPIO32_I2C_SCL,
> > > > +	GPIO33_I2C_SDA,
> > > > +};
> > > 
> > > Should the i2c pins really depend on the DS1307 config?
> > > On my board, I use a different rtc. So I do need the I2C pins to be
> > > configured, but I don't need the DS1307.
> 
> Yes, I think you're right. The I2C pins should only be set up if I2C
> support for PXA is in fact enabled. If someone manages to build a kernel
> which has support for this driver (which should depend on the I2C core
> as its transport layer) but not for PXA I2C, that's an unfortunate
> exception.
> 
> You could think about something like
> 
>   select I2C_PXA if RTC_DRV_DS1307
> 
> in the Colibri block of mach-pxa/Kconfig, but that's somewhat overdone
> maybe.

You're right, probably. I'll see what I can do about it (maybe I'll just queue 
this patch after the rework).

> 
> > Well I'd like to rework the colibri pxa3xx stuff later, but I'll have to
> > discuss it with Dan. I believe reworking it the same way as pxa270
> > colibri would be nice (and it'd solve your problem too as you use custom
> > board I think?).
> 
> Yes, that'd be nice.

It's a mess, true indeed. But now I need to get some sleep :-)
> 
> > Actually, there is one thing that puzzles me. Eric/Dan, shall I stick all
> > the MFP configs into one (or two, one for board and once for cpu card)
> > array or keep it split in multiple smaller arrays for each device? I
> > think putting it all in one place would be more readable.
> 
> Hmm, GPIOs for peripherals that are assembled on the module itself can
> safely be configured unconditionally as it is impossible to use them for
> anything else. For GPIOs that are allocted by a specific base board,
> things are different though, especially for the the Colibri eval board
> with it hundreds of jumpers on it. It's a convenience feature to have
> them free for general purpose use just by disabling a certain config
> directive.

Ok, I'd agree on this ;-)
> 
> Daniel

[PATCH] PXA: Colibri320: Add M41T00 RTC support

[Eric Miao]

On Sat, Jul 31, 2010 at 5:22 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sat, Jul 31, 2010 at 11:05:54AM +0200, Daniel Mack wrote:
>> Hmm, GPIOs for peripherals that are assembled on the module itself can
>> safely be configured unconditionally as it is impossible to use them for
>> anything else. For GPIOs that are allocted by a specific base board,
>> things are different though, especially for the the Colibri eval board
>> with it hundreds of jumpers on it. It's a convenience feature to have
>> them free for general purpose use just by disabling a certain config
>> directive.
>
> It would probably make sense for their to be a primary table which at
> least ensures that all pins are setup to safe default values. ?You
> really don't want the unconnected pins left floating, especially if
> they come out to some vulnerable connector or unconnected jumper pin.
>
> I'd suggest they're configured as inputs but with a pull-up or pull-down,
> not forgetting to consider the low power mode pull setting as well.
>

Yeah, that's also my opinion. So for those pins which can be connected
to different usages (e.g. to two or more different expansion boards), I'd
like to have the baseboard code to configure them safely (e.g. inputs with
pull as Russell suggested), and perform another mfp config in the
expansion board code.