[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Stefan Roese]

Hi Ben,

On Saturday 04 November 2006 03:11, Kim Phillips wrote:
> Please pull from 'master' branch of:
>
>   http://opensource.freescale.com/pub/scm/u-boot-83xx.git
>
> to receive the following updates (essentially MPC8349mITX and MPC8360EMDS
> support):
>
> Ben Warren:
>       Add support for multiple I2C buses
>       Multi-bus I2C implementation of MPC834x
>       Additional MPC8349 support for multibus i2c

I'm commenting on the I2C code you submitted, which is now included in the
git tree of Kim Phillips. Sorry for the late review, but I have some more
or less cosmetic comments:

> ------------------------------- common/cmd_i2c.c
> ------------------------------- index c543bb5..62378af 100644
> @@ -101,8 +101,31 @@ static uchar	i2c_mm_last_chip;
>  static uint	i2c_mm_last_addr;
>  static uint	i2c_mm_last_alen;
>
> +/* If only one I2C bus is present, the list of devices to ignore when
> + * the probe command is issued is represented by a 1D array of addresses.
> + * When multiple buses are present, the list is an array of bus-address
> + * pairs.  The following macros take care of this */
> +
>  #if defined(CFG_I2C_NOPROBES)
> +#if defined(CONFIG_I2C_MULTI_BUS)
> +static struct
> +{
> +	uchar	bus;
> +	uchar	addr;
> +} i2c_no_probes[] = CFG_I2C_NOPROBES;
> +#define GET_BUS_NUM	i2c_get_bus_num()
> +#define COMPARE_BUS(b,i)	(i2c_no_probes[(i)].bus == (b))
> +#define COMPARE_ADDR(a,i)	(i2c_no_probes[(i)].addr == (a))
> +#define NO_PROBE_ADDR(i)	i2c_no_probes[(i)].addr
> +#else		/* single bus */
>  static uchar i2c_no_probes[] = CFG_I2C_NOPROBES;
> +#define GET_BUS_NUM	0
> +#define COMPARE_BUS(b,i)	((b) == 0)	/* Make compiler happy */
> +#define COMPARE_ADDR(a,i)	(i2c_no_probes[(i)] == (a))
> +#define NO_PROBE_ADDR(i)	i2c_no_probes[(i)]
> +#endif	/* CONFIG_MULTI_BUS */
> +
> +#define NUM_ELEMENTS_NOPROBE
> (sizeof(i2c_no_probes)/sizeof(i2c_no_probes[0])) #endif
>
>  static int
> @@ -538,14 +561,17 @@ int do_i2c_probe (cmd_tbl_t *cmdtp, int
>  	int j;
>  #if defined(CFG_I2C_NOPROBES)
>  	int k, skip;
> -#endif
> +	uchar bus = GET_BUS_NUM;
> +#endif	/* NOPROBES */
>
>  	puts ("Valid chip addresses:");
>  	for(j = 0; j < 128; j++) {
>  #if defined(CFG_I2C_NOPROBES)
>  		skip = 0;
> -		for (k = 0; k < sizeof(i2c_no_probes); k++){
> -			if (j == i2c_no_probes[k]){
> +		for(k=0; k < NUM_ELEMENTS_NOPROBE; k++)
> +		{

Please move the "{" brace into the "for" loop line. And please also
insert a space between "for" and the "(" parenthesis. So the line above
should look like this:

		for (k=0; k < NUM_ELEMENTS_NOPROBE; k++) {

It seems that you use this "brace style" throughout the complete patch.
This is not according to the U-Boot coding styles (and Linux by the way).
So could you please rework your patch and merge it with Kim again? Or
perhaps Kim can rework the patch according to the U-Boot/Linux coding
style (Lindent?).

Thanks.

> +			if(COMPARE_BUS(bus, k) && COMPARE_ADDR(j, k))
> +			{
>  				skip = 1;
>  				break;
>  			}
> @@ -561,8 +587,11 @@ int do_i2c_probe (cmd_tbl_t *cmdtp, int
>
>  #if defined(CFG_I2C_NOPROBES)
>  	puts ("Excluded chip addresses:");
> -	for( k = 0; k < sizeof(i2c_no_probes); k++ )
> -		printf(" %02X", i2c_no_probes[k] );
> +	for(k=0; k < NUM_ELEMENTS_NOPROBE; k++)
> +	{
> +		if(COMPARE_BUS(bus,k))

Again, please insert a space after the "if".

> +			printf(" %02X", NO_PROBE_ADDR(k));
> +	}
>  	putc ('\n');
>  #endif
>
> @@ -873,6 +902,104 @@ int do_sdram  ( cmd_tbl_t *cmdtp, int fl
>  }
>  #endif	/* CFG_CMD_SDRAM */
>
> +#if defined(CONFIG_I2C_CMD_TREE)
> +#if defined(CONFIG_I2C_MULTI_BUS)
> +int do_i2c_bus_num(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
> +{
> +	int bus_idx, ret=0;
> +
> +	if (argc == 1)  /* querying current setting */
> +	{
> +		printf("Current bus is %d\n", i2c_get_bus_num());
> +	}
> +	else
> +	{

That should be

	} else {

> +		bus_idx = simple_strtoul(argv[1], NULL, 10);
> +		printf("Setting bus to %d\n", bus_idx);
> +		ret = i2c_set_bus_num(bus_idx);
> +		if(ret)
> +		{
> +			printf("Failure changing bus number (%d)\n", ret);
> +		}

Single line statements don't have braces.

And so on...

Please "talk" with Kim on how to clean up this patch.

Thanks.

Best regards,
Stefan

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Ben Warren]

Hi Stefan,

--- Stefan Roese <sr@denx.de> wrote:

> Hi Ben,
> 
> On Saturday 04 November 2006 03:11, Kim Phillips
> wrote:
> > Please pull from 'master' branch of:
> >
> >  
>
http://opensource.freescale.com/pub/scm/u-boot-83xx.git
> >
> > to receive the following updates (essentially
> MPC8349mITX and MPC8360EMDS
> > support):
> >
> > Ben Warren:
> >       Add support for multiple I2C buses
> >       Multi-bus I2C implementation of MPC834x
> >       Additional MPC8349 support for multibus i2c
> 
> I'm commenting on the I2C code you submitted, which
> is now included in the
> git tree of Kim Phillips. Sorry for the late review,
> but I have some more
> or less cosmetic comments:

Sorry about the formatting.  I've been influenced by
many coding 'standards' over the years and haven't
quite adapted to the kernel way yet...  Old habits die
hard. 

To keep things moving, I'll fix this up and forward to
Kim on Monday morning.  I'm sure he has better things
to do than fix my sloppy styling.

Speaking of keeping things moving - I submitted a SPI
driver for the MPC834x several months back.  I'm sure
it has the same formatting problems.  Will it be
faster for me to resubmit?

regards,
Ben

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Stefan Roese]

Hi Ben,

On Monday 27 November 2006 03:45, Ben Warren wrote:
> Sorry about the formatting.  I've been influenced by
> many coding 'standards' over the years and haven't
> quite adapted to the kernel way yet...  Old habits die
> hard.

;-)

> To keep things moving, I'll fix this up and forward to
> Kim on Monday morning.  I'm sure he has better things
> to do than fix my sloppy styling.

Good idea. If I remember correctly, Kim is not available until mid of this 
week.

> Speaking of keeping things moving - I submitted a SPI
> driver for the MPC834x several months back.  I'm sure
> it has the same formatting problems.  Will it be
> faster for me to resubmit?

Yes. You would save us at least one round of code review & resubmit. It's also 
easier to have a patch based on the current git tree.

Best regards,
Stefan

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Timur Tabi]

Stefan Roese wrote:

> I'm commenting on the I2C code you submitted, which is now included in the
> git tree of Kim Phillips. Sorry for the late review, but I have some more
> or less cosmetic comments:

I will make the requested changes and put them into our 83xx tree.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Joakim Tjernlund]

> -----Original Message-----
> From: u-boot-users-bounces at lists.sourceforge.net 
> [mailto:u-boot-users-bounces at lists.sourceforge.net] On Behalf 
> Of Stefan Roese
> Sent: 27 November 2006 07:22
> To: u-boot-users at lists.sourceforge.net
> Cc: Kim Phillips; Ben Warren; Wolfgang Denk
> Subject: Re: [U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)
> 
> Hi Ben,
> 
> On Monday 27 November 2006 03:45, Ben Warren wrote:
> > Sorry about the formatting.  I've been influenced by
> > many coding 'standards' over the years and haven't
> > quite adapted to the kernel way yet...  Old habits die
> > hard.
> 
> ;-)
> 
> > To keep things moving, I'll fix this up and forward to
> > Kim on Monday morning.  I'm sure he has better things
> > to do than fix my sloppy styling.
> 
> Good idea. If I remember correctly, Kim is not available 
> until mid of this 
> week.
> 
> > Speaking of keeping things moving - I submitted a SPI
> > driver for the MPC834x several months back.  I'm sure
> > it has the same formatting problems.  Will it be
> > faster for me to resubmit?
> 
> Yes. You would save us at least one round of code review & 
> resubmit. It's also 
> easier to have a patch based on the current git tree.
> 
> Best regards,
> Stefan

I think that the I2C_READ and I2C_WRITE #defines in fsl_i2c.h conflicts
with
soft I2C.

 Jocke

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Timur Tabi]

Joakim Tjernlund wrote:

> I think that the I2C_READ and I2C_WRITE #defines in fsl_i2c.h conflicts
> with
> soft I2C.

Can you be more specific?  These two macros are defined in a variety of ways in 
U-Boot.  Soft I2C is not used on any Freescale parts (AFAIK).

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Ben Warren]

On Tue, 2006-11-28 at 12:04 -0600, Timur Tabi wrote:
> Joakim Tjernlund wrote:
> 
> > I think that the I2C_READ and I2C_WRITE #defines in fsl_i2c.h conflicts
> > with
> > soft I2C.
> 
> Can you be more specific?  These two macros are defined in a variety of ways in 
> U-Boot.  Soft I2C is not used on any Freescale parts (AFAIK).
> 
While it's not used on any Freescale evaluation boards, it could
certainly be implemented on boards with Freescale CPUs.  I'm not sure
why you'd bit-bang I2C if you have nice hardware controllers, but there
may be situations where this makes sense.  On the other hand, I don't
know if SOFT_I2C and HARD_I2C can co-exist.

Either way, I don't think we should preclude the use of SOFT I2C on
Freescale CPUs.

regards,
Ben

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Jerry Van Baren]

Ben Warren wrote:
> On Tue, 2006-11-28 at 12:04 -0600, Timur Tabi wrote:
>> Joakim Tjernlund wrote:
>>
>>> I think that the I2C_READ and I2C_WRITE #defines in fsl_i2c.h conflicts
>>> with
>>> soft I2C.
>> Can you be more specific?  These two macros are defined in a variety of ways in 
>> U-Boot.  Soft I2C is not used on any Freescale parts (AFAIK).
>>
> While it's not used on any Freescale evaluation boards, it could
> certainly be implemented on boards with Freescale CPUs.  I'm not sure
> why you'd bit-bang I2C if you have nice hardware controllers, but there
> may be situations where this makes sense.  On the other hand, I don't
> know if SOFT_I2C and HARD_I2C can co-exist.
> 
> Either way, I don't think we should preclude the use of SOFT I2C on
> Freescale CPUs.
> 
> regards,
> Ben

It is used on some boards with Mot/Freescale CPUs, the WindRiver (EST) 
SBC8260 for example (although configurable hard vs. soft IIRC).  The 
hardware i2c can be more effort than it is worth compared to bit-banging 
it.  Perhaps the new QUICCs are easier to use, perhaps y'all are just 
smarter than we were back then.  :-)

gvb

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Pantelis Antoniou]

On 28 ??? 2006, at 8:17 ??, Ben Warren wrote:

> On Tue, 2006-11-28 at 12:04 -0600, Timur Tabi wrote:
>> Joakim Tjernlund wrote:
>>
>>> I think that the I2C_READ and I2C_WRITE #defines in fsl_i2c.h  
>>> conflicts
>>> with
>>> soft I2C.
>>
>> Can you be more specific?  These two macros are defined in a  
>> variety of ways in
>> U-Boot.  Soft I2C is not used on any Freescale parts (AFAIK).
>>
> While it's not used on any Freescale evaluation boards, it could
> certainly be implemented on boards with Freescale CPUs.  I'm not sure
> why you'd bit-bang I2C if you have nice hardware controllers, but  
> there
> may be situations where this makes sense.  On the other hand, I don't
> know if SOFT_I2C and HARD_I2C can co-exist.
>
> Either way, I don't think we should preclude the use of SOFT I2C on
> Freescale CPUs.
>
> regards,
> Ben
>

In the same note, definitely, definitely there are designs with multiple
I2C buses. Just because something does not exist on an evaluation board,
it doesn't mean it's not used in the real world.

So please make sure that both HW I2C & SW I2C can coexist.

-- Pantelis

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Joakim Tjernlund]

> -----Original Message-----
> From: Ben Warren [mailto:bwarren at qstreams.com] 
> Sent: den 28 november 2006 19:18
> To: Timur Tabi
> Cc: Joakim Tjernlund; Stefan Roese; 
> u-boot-users at lists.sourceforge.net; Kim Phillips; Wolfgang Denk
> Subject: Re: [U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)
> 
> On Tue, 2006-11-28 at 12:04 -0600, Timur Tabi wrote:
> > Joakim Tjernlund wrote:
> > 
> > > I think that the I2C_READ and I2C_WRITE #defines in 
> fsl_i2c.h conflicts
> > > with
> > > soft I2C.
> > 
> > Can you be more specific?  These two macros are defined in 
> a variety of ways in 
> > U-Boot.  Soft I2C is not used on any Freescale parts (AFAIK).
> > 
> While it's not used on any Freescale evaluation boards, it could
> certainly be implemented on boards with Freescale CPUs.  I'm not sure
> why you'd bit-bang I2C if you have nice hardware controllers, 
> but there
> may be situations where this makes sense.  On the other hand, I don't
> know if SOFT_I2C and HARD_I2C can co-exist.

I use it in u-boot to send an I2C reset sequence* 
which the hardware controller can't do(or so Freescale tells me).
Futhermore, one can not use the I2C pins as GPIO on
these parts, so I had to connect two pins from PORT D** to
the I2C bus to do the bitbanging. If someone knows if this
can be done differently, I am all ears.

so I removed I2C_READ/I2C_WRITE from asm-ppc/i2c.h
and renamed I2C_READ to I2C_READ_BIT(same for write)
locally in cpu/mpc83xx/i2c.c(now renamed to fsl-i2c.c)

* The reset sequence in soft i2c isn't complete. One has
  to send a start in each clock cycle as well.

** Something isn't working properly with PORTD in ODR mode.
   I have change direction from output to input for SDA in
   I2C_TRISTATE. It should be enough to set SDA high since
   its open drain.

 Jocke
> 
> Either way, I don't think we should preclude the use of SOFT I2C on
> Freescale CPUs.

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Timur Tabi]

Pantelis Antoniou wrote:

> In the same note, definitely, definitely there are designs with multiple
> I2C buses. Just because something does not exist on an evaluation board,
> it doesn't mean it's not used in the real world.

The new fsl_i2c.c supports multiple HW I2C buses.  Well, it supports only 2 
buses, but I have plans to fix that.

> So please make sure that both HW I2C & SW I2C can coexist.

I'm going to need some help with that.  All I'm hearing so far are comments that 
the two should work together.  I don't see how any of the I2C changes I made 
affect soft I2C.  On top of that, I've never used soft I2C, so I don't even know 
how it's supposed to work.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Wolfgang Denk]

In message <1164737878.31193.38.camel@saruman.qstreams.net> you wrote:
>
> > Can you be more specific?  These two macros are defined in a variety of ways in 
> > U-Boot.  Soft I2C is not used on any Freescale parts (AFAIK).
> > 
> While it's not used on any Freescale evaluation boards, it could
> certainly be implemented on boards with Freescale CPUs.  I'm not sure
> why you'd bit-bang I2C if you have nice hardware controllers, but there

...because  the  bitbanging  code  is  much  smaller  and  easier  to
implement and debug than the code that uses the HW controller?

> may be situations where this makes sense.  On the other hand, I don't
> know if SOFT_I2C and HARD_I2C can co-exist.

No, theu=y are exclusive. But it should be possible to select any  of
these interfaces.

> Either way, I don't think we should preclude the use of SOFT I2C on
> Freescale CPUs.

Agreed.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Wagner's music is better than it sounds."               - Mark Twain

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Wolfgang Denk]

In message <456C7A18.1090309@freescale.com> you wrote:
> 
> Can you be more specific?  These two macros are defined in a variety of ways in 
> U-Boot.  Soft I2C is not used on any Freescale parts (AFAIK).

But it should be possible to use it. Please don't use incompatible
definitions.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
When a man sits with a pretty girl for  an  hour,  it  seems  like  a
minute.  But let him sit on a hot stove for a minute -- and it's lon-
ger than any hour. That's relativity.              -- Albert Einstein

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Timur Tabi]

Wolfgang Denk wrote:
> In message <456C7A18.1090309@freescale.com> you wrote:
>> Can you be more specific?  These two macros are defined in a variety of ways in 
>> U-Boot.  Soft I2C is not used on any Freescale parts (AFAIK).
> 
> But it should be possible to use it. Please don't use incompatible
> definitions.

So you're saying that in MPC8349ITX.h, I should be able to delete this line:

#define CONFIG_HARC_I2C

and replace it with this line:

#define CONFIG_SOFT_I2C

and everything should still work????

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Tolunay Orkun]

Wolfgang Denk wrote:
> In message <1164737878.31193.38.camel@saruman.qstreams.net> you wrote:
>>> Can you be more specific?  These two macros are defined in a variety of ways in 
>>> U-Boot.  Soft I2C is not used on any Freescale parts (AFAIK).
>>>
>> While it's not used on any Freescale evaluation boards, it could
>> certainly be implemented on boards with Freescale CPUs.  I'm not sure
>> why you'd bit-bang I2C if you have nice hardware controllers, but there
> 
> ...because  the  bitbanging  code  is  much  smaller  and  easier  to
> implement and debug than the code that uses the HW controller?
> 
>> may be situations where this makes sense.  On the other hand, I don't
>> know if SOFT_I2C and HARD_I2C can co-exist.
> 
> No, theu=y are exclusive. But it should be possible to select any  of
> these interfaces.

How do we handle the case that there is one hard I2C interface and 
another soft I2C interface (bus) via a pair of GPIO port pins? I have a 
PPC405EP based custom board that has such a case and I was looking 
forward to enabling multiple I2C bus support in U-Boot via both SOFT_I2C 
as well as HARD_I2C defined.

Tolunay

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Joakim Tjernlund]

On Tue, 2006-11-28 at 15:08 -0600, Timur Tabi wrote:
> Wolfgang Denk wrote:
> > In message <456C7A18.1090309@freescale.com> you wrote:
> >> Can you be more specific?  These two macros are defined in a variety of ways in 
> >> U-Boot.  Soft I2C is not used on any Freescale parts (AFAIK).
> > 
> > But it should be possible to use it. Please don't use incompatible
> > definitions.
> 
> So you're saying that in MPC8349ITX.h, I should be able to delete this line:
> 
> #define CONFIG_HARC_I2C
> 
> and replace it with this line:
> 
> #define CONFIG_SOFT_I2C
> 
> and everything should still work????

No, see attached patch(s)

Not tested in your tree as I don't use that one (yet)

 Jocke
-------------- next part --------------

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Ben Warren]

On Tue, 2006-11-28 at 15:08 -0600, Timur Tabi wrote:
> Wolfgang Denk wrote:
> > In message <456C7A18.1090309@freescale.com> you wrote:
> >> Can you be more specific?  These two macros are defined in a variety of ways in 
> >> U-Boot.  Soft I2C is not used on any Freescale parts (AFAIK).
> > 
> > But it should be possible to use it. Please don't use incompatible
> > definitions.
> 
> So you're saying that in MPC8349ITX.h, I should be able to delete this line:
> 
> #define CONFIG_HARC_I2C
> 
> and replace it with this line:
> 
> #define CONFIG_SOFT_I2C
> 
> and everything should still work????
> 
No.  To implement Soft I2C (bit-banging) you need to provide information
about the I/O that you're using, among other things.

regards,
Ben

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Timur Tabi]

Joakim Tjernlund wrote:

> No, see attached patch(s)

Ah, I see.

> Not tested in your tree as I don't use that one (yet)

Git didn't like your patches, for some reason, so I had to apply them by hand, 
but everything seems to be okay.  I will apply them to our tree for Wolfgang's 
convenience.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Joakim Tjernlund]

> -----Original Message-----
> From: u-boot-users-bounces at lists.sourceforge.net 
> [mailto:u-boot-users-bounces at lists.sourceforge.net] On Behalf 
> Of Timur Tabi
> Sent: den 28 november 2006 23:16
> To: joakim.tjernlund at transmode.se
> Cc: u-boot-users at lists.sourceforge.net; Stefan Roese; Ben 
> Warren; Wolfgang Denk; Kim Phillips
> Subject: Re: [U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)
> 
> Joakim Tjernlund wrote:
> 
> > No, see attached patch(s)
> 
> Ah, I see.

:)

> 
> > Not tested in your tree as I don't use that one (yet)
> 
> Git didn't like your patches, for some reason, so I had to 
> apply them by hand, 
> but everything seems to be okay.  I will apply them to our 
> tree for Wolfgang's 
> convenience.

eeh? I just did them aginst your tree with git-format-patch.
Are you using an old git perhaps? I think something changed
with git's diff format.

 Jocke

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Timur Tabi]

Joakim Tjernlund wrote:

> eeh? I just did them aginst your tree with git-format-patch.
> Are you using an old git perhaps? I think something changed
> with git's diff format.

I do have one patch in my sandbox that's not in the tree, but even so, I can't 
explain why git-apply and git-am complained.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Joakim Tjernlund]

On Tue, 2006-11-28 at 16:16 -0600, Timur Tabi wrote:
> Joakim Tjernlund wrote:
> 
> > No, see attached patch(s)
> 
> Ah, I see.
> 
> > Not tested in your tree as I don't use that one (yet)
> 
> Git didn't like your patches, for some reason, so I had to apply them by hand, 
> but everything seems to be okay.  I will apply them to our tree for Wolfgang's 
> convenience.
> 

While I am at it, I would also like to see this in u-boot
We use I2C as HRCW since we wan't to haw our flash reset connetced to
HRESET, otherwise you might be unable to boot if the flash is in non
read array mode when the board resets.
We also need to have the version info in the begining of the flash so
we can identify what version of u-boot we have installed. 
-------------- next part --------------

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Jerry Van Baren]

Joakim Tjernlund wrote:
> On Tue, 2006-11-28 at 16:16 -0600, Timur Tabi wrote:
>> Joakim Tjernlund wrote:
>>
>>> No, see attached patch(s)
>> Ah, I see.
>>
>>> Not tested in your tree as I don't use that one (yet)
>> Git didn't like your patches, for some reason, so I had to apply them by hand, 
>> but everything seems to be okay.  I will apply them to our tree for Wolfgang's 
>> convenience.
>>
> 
> While I am at it, I would also like to see this in u-boot
> We use I2C as HRCW since we wan't to haw our flash reset connetced to
> HRESET, otherwise you might be unable to boot if the flash is in non
> read array mode when the board resets.
> We also need to have the version info in the begining of the flash so
> we can identify what version of u-boot we have installed. 
> 
> ------------------------------------------------------------------------
> 
> From 89b60f21af0d04959d93ccb70fd781c8aba9e66c Mon Sep 17 00:00:00 2001
> From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> Date: Tue, 28 Nov 2006 23:42:31 +0100
> Subject: [PATCH] Make HRCW and version info in data segment configurable.
> 
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---
>  cpu/mpc83xx/start.S |   28 +++++++++++++++++-----------
>  1 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/cpu/mpc83xx/start.S b/cpu/mpc83xx/start.S
> index 0f27bb6..44bca26 100644
> --- a/cpu/mpc83xx/start.S
> +++ b/cpu/mpc83xx/start.S
> @@ -77,20 +77,12 @@
>  	END_GOT
>  
>  /*
> - * Version string - must be in data segment because MPC83xx uses the
> - * first 256 bytes for the Hard Reset Configuration Word table (see
> - * below).  Similarly, can't have the U-Boot Magic Number as the first
> - * thing in the image - don't know how this will affect the image tools,
> - * but I guess I'll find out soon.
> + * MPC83xx can use the first 0x40 bytes for the Hard Reset Configuration Word 
> + * table (see below) if so configured.
>   */
> -	.data
> -	.globl	version_string
> -version_string:
> -	.ascii U_BOOT_VERSION
> -	.ascii " (", __DATE__, " - ", __TIME__, ")"
> -	.ascii " ", CONFIG_IDENT_STRING, "\0"
>  
>  	.text
> +#ifndef CFG_HRCW_IN_I2C_EEPROM
>  #define _HRCW_TABLE_ENTRY(w)		\
>  	.fill	8,1,(((w)>>24)&0xff);	\
>  	.fill	8,1,(((w)>>16)&0xff);	\
> @@ -99,7 +91,21 @@ version_string:
>  
>  	_HRCW_TABLE_ENTRY(CFG_HRCW_LOW)
>  	_HRCW_TABLE_ENTRY(CFG_HRCW_HIGH)
> +#endif
>  
> +/*
> + * Version string - May be in data segment if one wants to reserve the
> + * space left to address 0x100 for future expansion of HRCW bytes.
> + */
> +#ifdef CFG_VERSION_STRING_IN_DATA
> +	.data
> +#endif
> +        .long   0x27051956              /* U-Boot Magic Number */
> +	.globl	version_string
> +version_string:
> +	.ascii U_BOOT_VERSION
> +	.ascii " (", __DATE__, " - ", __TIME__, ")"
> +	.ascii " ", CONFIG_IDENT_STRING, "\0"
>  
>  #ifndef CONFIG_DEFAULT_IMMR
>  #error CONFIG_DEFAULT_IMMR must be defined

Hi Timur,

I don't believe you want to do this: you are removing the HRCW entirely 
from flash if you have it in I2C (CFG_HRCW_IN_I2C_EEPROM).  Unless I'm 
missing something (possible), I don't see why you even need a 
configuration option CFG_HRCW_IN_I2C_EEPROM.

* You can chose to ignore the flash-HRCW and use the I2C one simply by 
pulling a config pin up/down on power up (if I understand the manuals 
correctly), so you can store your HRCW in both places and chose which 
you use via a hardware configuration jumper.  This is a Very Good 
Thing[tm].  Suppressing the flash version simply because you chose not 
to use it is not something _I_ want to do on my 8360.

* If you put your HRCW in I2C _and_ in flash, you will be able to boot 
via the flash HRCW if your I2C version gets hosed.  If you don't, you 
will be hosed.

* Having a redundant HRCW in flash is essentially free: it costs you 64 
bytes of otherwise unused memory (well, other than the version string 
which you moved there, but there should be room for both).

My suggestion for putting the u-boot version string in a fixed place in 
flash was to put it _after_ the HRCW (e.g. at offset 0x40).  0x60 (96) 
bytes should be plenty for the version string.  Instead, you've unwisely 
(IMHO) _replaced_ the HRCW.  Browsing some of the other PowerPC CPU 
types, the ones that can appear to put the version string at the very 
start of flash.  This implies putting the version string in flash at 
offset 0x40 is OK (I was concerned about relocation fixup issues with 
pointers to flash vs. RAM after relocation).

On a related topic, the comment at the head of start.S says
/*
  * Version string - must be in data segment because MPC83xx uses the
  * first 256 bytes for the Hard Reset Configuration Word table (see
  * below).  Similarly, can't have the U-Boot Magic Number as the first
  * thing in the image - don't know how this will affect the image tools,
  * but I guess I'll find out soon.
  */
I see the comment was cut'n'pasted from the 8260 start.S.  Actually only 
the first 64 (0x40) bytes of memory are used for the HRCW, _not_ all 256 
bytes.

Looks like I (or somebody) needs to submit a clean up patch for that 
comment for the 8260 and 8360.  Sigh.  I just _had_ to look.  Curiosity 
will be the death of me yet.  ;-)

gvb

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Wolfgang Denk]

In message <456CA564.8060509@freescale.com> you wrote:
>
> > But it should be possible to use it. Please don't use incompatible
> > definitions.
> 
> So you're saying that in MPC8349ITX.h, I should be able to delete this line:
> 
> #define CONFIG_HARC_I2C
> 
> and replace it with this line:
> 
> #define CONFIG_SOFT_I2C
> 
> and everything should still work????

Yes - assuming you provide the required definitions for port pin
initialization and access in you rboard config file.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A penny saved is a penny to squander.
- Ambrose Bierce

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Wolfgang Denk]

In message <456CA573.4020103@orkun.us> you wrote:
>
> How do we handle the case that there is one hard I2C interface and 
> another soft I2C interface (bus) via a pair of GPIO port pins? I have a 
> PPC405EP based custom board that has such a case and I was looking 
> forward to enabling multiple I2C bus support in U-Boot via both SOFT_I2C 
> as well as HARD_I2C defined.

Good question. I guess you're the first one who wants to do this,  so
you  will  have  to  sort  out  the  problems. I agree that this is a
reasonable request, it's  just  that  it  has  never  been  attempted
before.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I came home the other night and tried to open the door  with  my  car
keys...and  the  building started up. So I took it out for a drive. A
cop pulled me over for speeding. He asked me where I  live...  "Right
here".                                                - Steven Wright

[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Joakim Tjernlund]

> -----Original Message-----
> From: u-boot-users-bounces at lists.sourceforge.net 
> [mailto:u-boot-users-bounces at lists.sourceforge.net] On Behalf 
> Of Jerry Van Baren
> Sent: 29 November 2006 14:21
> To: u-boot-users at lists.sourceforge.net
> Subject: Re: [U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)
> 
> Joakim Tjernlund wrote:
> > On Tue, 2006-11-28 at 16:16 -0600, Timur Tabi wrote:
> >> Joakim Tjernlund wrote:
> >>
> >>> No, see attached patch(s)
> >> Ah, I see.
> >>
> >>> Not tested in your tree as I don't use that one (yet)
> >> Git didn't like your patches, for some reason, so I had to 
> apply them by hand, 
> >> but everything seems to be okay.  I will apply them to our 
> tree for Wolfgang's 
> >> convenience.
> >>
> > 
> > While I am at it, I would also like to see this in u-boot
> > We use I2C as HRCW since we wan't to haw our flash reset 
> connetced to
> > HRESET, otherwise you might be unable to boot if the flash is in non
> > read array mode when the board resets.
> > We also need to have the version info in the begining of 
> the flash so
> > we can identify what version of u-boot we have installed. 
> > 
> > 
> --------------------------------------------------------------
> ----------
> > 
> > From 89b60f21af0d04959d93ccb70fd781c8aba9e66c Mon Sep 17 
> 00:00:00 2001
> > From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > Date: Tue, 28 Nov 2006 23:42:31 +0100
> > Subject: [PATCH] Make HRCW and version info in data segment 
> configurable.
> > 
> > 
> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > ---
> >  cpu/mpc83xx/start.S |   28 +++++++++++++++++-----------
> >  1 files changed, 17 insertions(+), 11 deletions(-)
> > 
> > diff --git a/cpu/mpc83xx/start.S b/cpu/mpc83xx/start.S
> > index 0f27bb6..44bca26 100644
> > --- a/cpu/mpc83xx/start.S
> > +++ b/cpu/mpc83xx/start.S
> > @@ -77,20 +77,12 @@
> >  	END_GOT
> >  
> >  /*
> > - * Version string - must be in data segment because 
> MPC83xx uses the
> > - * first 256 bytes for the Hard Reset Configuration Word table (see
> > - * below).  Similarly, can't have the U-Boot Magic Number 
> as the first
> > - * thing in the image - don't know how this will affect 
> the image tools,
> > - * but I guess I'll find out soon.
> > + * MPC83xx can use the first 0x40 bytes for the Hard Reset 
> Configuration Word 
> > + * table (see below) if so configured.
> >   */
> > -	.data
> > -	.globl	version_string
> > -version_string:
> > -	.ascii U_BOOT_VERSION
> > -	.ascii " (", __DATE__, " - ", __TIME__, ")"
> > -	.ascii " ", CONFIG_IDENT_STRING, "\0"
> >  
> >  	.text
> > +#ifndef CFG_HRCW_IN_I2C_EEPROM
> >  #define _HRCW_TABLE_ENTRY(w)		\
> >  	.fill	8,1,(((w)>>24)&0xff);	\
> >  	.fill	8,1,(((w)>>16)&0xff);	\
> > @@ -99,7 +91,21 @@ version_string:
> >  
> >  	_HRCW_TABLE_ENTRY(CFG_HRCW_LOW)
> >  	_HRCW_TABLE_ENTRY(CFG_HRCW_HIGH)
> > +#endif
> >  
> > +/*
> > + * Version string - May be in data segment if one wants to 
> reserve the
> > + * space left to address 0x100 for future expansion of HRCW bytes.
> > + */
> > +#ifdef CFG_VERSION_STRING_IN_DATA
> > +	.data
> > +#endif
> > +        .long   0x27051956              /* U-Boot Magic Number */
> > +	.globl	version_string
> > +version_string:
> > +	.ascii U_BOOT_VERSION
> > +	.ascii " (", __DATE__, " - ", __TIME__, ")"
> > +	.ascii " ", CONFIG_IDENT_STRING, "\0"
> >  
> >  #ifndef CONFIG_DEFAULT_IMMR
> >  #error CONFIG_DEFAULT_IMMR must be defined
> 
> Hi Timur,
> 
> I don't believe you want to do this: you are removing the 
> HRCW entirely 
> from flash if you have it in I2C (CFG_HRCW_IN_I2C_EEPROM).  
> Unless I'm 
> missing something (possible), I don't see why you even need a 
> configuration option CFG_HRCW_IN_I2C_EEPROM.

[SNIP good arugments]

The only reason to possibly want this is if you want to keep the format
where the u-boot header is the first thing in the image. I can manage
to have HRCW first in flash, but not having the version string in data
segment.

Summary:
  Remove the CFG_HRCW_IN_I2C_EEPROM if you want, but keep
  CFG_VERSION_STRING_IN_DATA.

 Jocke