[PATCH] first in a series to enhance microcode patches

[PATCH] first in a series to enhance microcode patches

[Robert P. J. Day]

   (after a short discussion with tom rini, we're going to ignore any 
previous patch submissions of mine WRT microcode patches and start 
fresh.  first trivial patch, just to start things off.)

   purpose of patch:

   1) adds a relocation pointer to smc_uart_t
   2) redeclares reserved chunks in structures to be in terms of a
      standard char array, rather than the hideous combination of uint,
      ushort, and so on.  (a purely aesthetic fix, admittedly.)

more patches to follow shortly.





--- linuxppc-2.5/include/asm-ppc/commproc.h	2004-09-16 13:08:12.000000000 
-0400
+++ linuxppc-2.5-new/include/asm-ppc/commproc.h	2004-09-16 13:40:52.000000000 
-0400
@@ -145,6 +145,8 @@
  	ushort	smc_brkec;	/* rcv'd break condition counter */
  	ushort	smc_brkcr;	/* xmt break count register */
  	ushort	smc_rmask;	/* Temporary bit mask */
+	char	res1[8];	/* Reserved */
+	ushort	smc_rpbase;	/* Relocation pointer */
  } smc_uart_t;

  /* Function code bits.
@@ -475,8 +477,7 @@
  */
  typedef struct scc_uart {
  	sccp_t	scc_genscc;
-	uint	scc_res1;	/* Reserved */
-	uint	scc_res2;	/* Reserved */
+	char	res1[8];	/* Reserved */
  	ushort	scc_maxidl;	/* Maximum idle chars */
  	ushort	scc_idlc;	/* temp idle counter */
  	ushort	scc_brkcr;	/* Break count register */
@@ -560,9 +561,9 @@
  	ushort	iic_tbptr;	/* Internal */
  	ushort	iic_tbc;	/* Internal */
  	uint	iic_txtmp;	/* Internal */
-	uint	iic_res;	/* reserved */
+	char	res1[4];	/* Reserved */
    	ushort	iic_rpbase;	/* Relocation pointer */
-	ushort	iic_res2;	/* reserved */
+	char	res2[2];	/* Reserved */
  } iic_t;

  #define BD_IIC_START		((ushort)0x0400)

Re: [PATCH] first in a series to enhance microcode patches

[Dan Malek]

On Oct 5, 2004, at 12:32 PM, Robert P. J. Day wrote:

>   (after a short discussion with tom rini, we're going to ignore any 
> previous patch submissions of mine WRT microcode patches and start 
> fresh.  first trivial patch, just to start things off.)

No, send a whole patch that _does_ something.  Let's see all of these
changes at once.  By itself, this patch is useless and doesn't add any
features, it just wastes our time discussing it.

>   2) redeclares reserved chunks in structures to be in terms of a
>      standard char array, rather than the hideous combination of uint,
>      ushort, and so on.  (a purely aesthetic fix, admittedly.)

Just for information, most of the original data structures were all
machine generated with some minor manual touch ups.  I certainly
wasn't going to type in all of that stuff and risk mistakes with offsets
and sizes.

Thanks.

	-- Dan

Re: [PATCH] first in a series to enhance microcode patches

[Tom Rini]

On Tue, Oct 05, 2004 at 03:20:37PM -0400, Dan Malek wrote:

> 
> On Oct 5, 2004, at 12:32 PM, Robert P. J. Day wrote:
> 
> >  (after a short discussion with tom rini, we're going to ignore any 
> >previous patch submissions of mine WRT microcode patches and start 
> >fresh.  first trivial patch, just to start things off.)
> 
> No, send a whole patch that _does_ something.  Let's see all of these
> changes at once.  By itself, this patch is useless and doesn't add any
> features, it just wastes our time discussing it.

A series of inter-dependant patches might be better, but I like small,
easy to understand patches.

-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [PATCH] first in a series to enhance microcode patches

[Robert P. J. Day]

On Tue, 5 Oct 2004, Tom Rini wrote:

> On Tue, Oct 05, 2004 at 03:20:37PM -0400, Dan Malek wrote:

>> No, send a whole patch that _does_ something.  Let's see all of these
>> changes at once.  By itself, this patch is useless and doesn't add any
>> features, it just wastes our time discussing it.
>
> A series of inter-dependant patches might be better, but I like small,
> easy to understand patches.

you know, it's a good thing i'm severely bipolar. :-)  somebody needs 
to make a decision -- i'll go with whatever it is.

rday

Re: [PATCH] first in a series to enhance microcode patches

[Tom Rini]

On Tue, Oct 05, 2004 at 04:00:45PM -0400, Robert P. J. Day wrote:

> On Tue, 5 Oct 2004, Tom Rini wrote:
> 
> >On Tue, Oct 05, 2004 at 03:20:37PM -0400, Dan Malek wrote:
> 
> >>No, send a whole patch that _does_ something.  Let's see all of these
> >>changes at once.  By itself, this patch is useless and doesn't add any
> >>features, it just wastes our time discussing it.
> >
> >A series of inter-dependant patches might be better, but I like small,
> >easy to understand patches.
> 
> you know, it's a good thing i'm severely bipolar. :-)  somebody needs 
> to make a decision -- i'll go with whatever it is.

Ah.  I assumed you were sending out one chunk at a time something you
had 'done', sorry.  To be clear, what I prefer is a series of patches
that get something done.  A single small cleanup like you posted is
fine, but if you have it as part of a larger change, just include it in
the series.  'Documentation/SubmittingPatches' is probably a good
reference, subsituting linux-kernel with linuxppc-embedded :)

-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [PATCH] first in a series to enhance microcode patches

[Robert P. J. Day]

On Tue, 5 Oct 2004, Dan Malek wrote:

> No, send a whole patch that _does_ something.  Let's see all of these
> changes at once.  By itself, this patch is useless and doesn't add any
> features, it just wastes our time discussing it.

ok, not a problem.  i'll submit it any way the powers that be prefer, 
i just wanted explicit instructions on how.  give me a day and i'll 
have a full, working patch.  that does something.

>>   2) redeclares reserved chunks in structures to be in terms of a
>>      standard char array, rather than the hideous combination of uint,
>>      ushort, and so on.  (a purely aesthetic fix, admittedly.)
>
> Just for information, most of the original data structures were all
> machine generated with some minor manual touch ups.  I certainly
> wasn't going to type in all of that stuff and risk mistakes with offsets
> and sizes.

that's definitely understandable.  it's just potentially confusing to 
have a structure's reserved chunks declared as some combination of 
uchar, ushort, uint and/or ulong, when it's obviously more 
comprehensible to make each reserved chunk a standard array of char 
whose size is obvious at a glance.

just for fun,

   $ cd include/asm-ppc
   $ grep -i reserved *.h

man.  the standards for declaring reserved space are all over the map, 
including this one:

mpc52xx.h:      volatile u32    reserved[4];    /* MMAP_CTRL + 0x3c .. 
0x48 */
mpc52xx.h:      volatile u32    reserved1;      /* INTR + 0x20 */
mpc52xx.h:      volatile u32    reserved2;      /* INTR + 0x34 */
...

   now *that* kind of creeps me out.  why is reserved space being 
declared as "volatile"?  yeesh.

   i may not understand what's happening here but, IMHO, if something 
is declared as reserved, that should be an indication that *nobody* is 
using it.  if it's being used for anything, then it shouldn't be 
labelled as "reserved"; it should have a name.  to be both volatile 
and reserved just makes me queasy. but that's just me.

rday

Re: [PATCH] first in a series to enhance microcode patches

[Wolfgang Denk]

In message <Pine.LNX.4.60.0410051543230.3549@localhost.localdomain> you wrote:
> 
> that's definitely understandable.  it's just potentially confusing to 
> have a structure's reserved chunks declared as some combination of 
> uchar, ushort, uint and/or ulong, when it's obviously more 
> comprehensible to make each reserved chunk a standard array of char 
> whose size is obvious at a glance.

Actually this might not be confusing, but making the code  easier  to
read,  to  understand,  and  maybe  one day to extend - remember that
these struct definitions are direct translations of Motorola provided
documentation - and I tend to  believe  that  the  chip  manufacturer
knows  more about the internals of his chips than you or me. One day,
a "uint reserved_xxx;" may turn into a new, shiny 32 bit register.

>    now *that* kind of creeps me out.  why is reserved space being 
> declared as "volatile"?  yeesh.

It does not hurt, and it makes it easier to adapt the code  when  new
register definitions pop up in a later version of the chip?


Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd@denx.de
Intel told us the Pentium would have "RISK" features...

Re: [PATCH] first in a series to enhance microcode patches

[Robert P. J. Day]

On Tue, 5 Oct 2004, Wolfgang Denk wrote:

> In message <Pine.LNX.4.60.0410051543230.3549@localhost.localdomain> you wrote:
>>
>> that's definitely understandable.  it's just potentially confusing to
>> have a structure's reserved chunks declared as some combination of
>> uchar, ushort, uint and/or ulong, when it's obviously more
>> comprehensible to make each reserved chunk a standard array of char
>> whose size is obvious at a glance.
>
> Actually this might not be confusing, but making the code  easier  to
> read,  to  understand,  and  maybe  one day to extend - remember that
> these struct definitions are direct translations of Motorola provided
> documentation - and I tend to  believe  that  the  chip  manufacturer
> knows  more about the internals of his chips than you or me. One day,
> a "uint reserved_xxx;" may turn into a new, shiny 32 bit register.

from "Documentation/SubmittingPatches", at the very end:

   4) Don't over-design.

   Don't try to anticipate nebulous future cases which may or may not
   be useful:  "Make it as simple as you can, and no simpler"

it seems that, if that's good advice for patches, it should be good 
advice for the code proper.  i do appreciate your point, but if at 
some point, a shiny new register suddenly appears, that strikes me as 
a significant enough change that mods to the header file shouldn't be 
considered a big deal.

anyway, just my $0.02.

rday

Re: [PATCH] first in a series to enhance microcode patches

[Mark Chambers]

rday:
> >>
> >> that's definitely understandable.  it's just potentially confusing to
> >> have a structure's reserved chunks declared as some combination of
> >> uchar, ushort, uint and/or ulong, when it's obviously more
> >> comprehensible to make each reserved chunk a standard array of char
> >> whose size is obvious at a glance.
> >
wd:
> > Actually this might not be confusing, but making the code  easier  to
> > read,  to  understand,  and  maybe  one day to extend - remember that
> > these struct definitions are direct translations of Motorola provided
> > documentation - and I tend to  believe  that  the  chip  manufacturer
> > knows  more about the internals of his chips than you or me. One day,
> > a "uint reserved_xxx;" may turn into a new, shiny 32 bit register.
>
rday:
> it seems that, if that's good advice for patches, it should be good
> advice for the code proper.  i do appreciate your point, but if at
> some point, a shiny new register suddenly appears, that strikes me as
> a significant enough change that mods to the header file shouldn't be
> considered a big deal.
>
> anyway, just my $0.02.
>

Rob,

What about, "if it ain't broke, don't fix it".  If it were my code I
wouldn't
touch that stuff - at best you get pretty code, at worst you break
something.
Remember, these aren't 'real' structures, they're just templates to make
I/O addressing come out right.  Wolfgang is likely right, that Motorola
created the structures based on what they know about the internal
decoding logic.  So if in the future one needs to add new registers you
again minimize the chance of breaking something by not rearranging
the arrays correctly.  Then again, if Motorola adds registers (or they
may already be there, but not documentented) they will probably
release a new header file, which you'll have to clean up all over
again.

That's my two cents.

Mark C.

Re: [PATCH] first in a series to enhance microcode patches

[Robert P. J. Day]

On Wed, 6 Oct 2004, Mark Chambers wrote:

> Rob,
>
> What about, "if it ain't broke, don't fix it".  If it were my code I 
> wouldn't touch that stuff - at best you get pretty code, at worst 
> you break something.

oh, i wasn't suggesting going through removing the "volatile" 
qualifier from reserved spaces or anything insane like that.  i was 
just making an observation, that's all.  i have no intention of 
touching that stuff.  really.  i promise. :-)

rday

[PATCH] first in a series to enhance microcode patches

[Robert P. J. Day]

   resend of previous patch -- sorry, forgot the developer's cert of 
origin.  please be patient, i'm slowly getting the hang of this.

   purpose of patch:

   1) adds a relocation pointer to smc_uart_t
   2) redeclares reserved chunks in structures to be in terms of a
      standard char array, rather than the hideous combination of uint,
      ushort, and so on.

Signed-off-by: Robert P. J. Day <rpjday@mindspring.com>



--- linuxppc-2.5/include/asm-ppc/commproc.h	2004-09-16 13:08:12.000000000 
-0400
+++ linuxppc-2.5-new/include/asm-ppc/commproc.h	2004-09-16 13:40:52.000000000 
-0400
@@ -145,6 +145,8 @@
  	ushort	smc_brkec;	/* rcv'd break condition counter */
  	ushort	smc_brkcr;	/* xmt break count register */
  	ushort	smc_rmask;	/* Temporary bit mask */
+	char	res1[8];	/* Reserved */
+	ushort	smc_rpbase;	/* Relocation pointer */
  } smc_uart_t;

  /* Function code bits.
@@ -475,8 +477,7 @@
  */
  typedef struct scc_uart {
  	sccp_t	scc_genscc;
-	uint	scc_res1;	/* Reserved */
-	uint	scc_res2;	/* Reserved */
+	char	res1[8];	/* Reserved */
  	ushort	scc_maxidl;	/* Maximum idle chars */
  	ushort	scc_idlc;	/* temp idle counter */
  	ushort	scc_brkcr;	/* Break count register */
@@ -560,9 +561,9 @@
  	ushort	iic_tbptr;	/* Internal */
  	ushort	iic_tbc;	/* Internal */
  	uint	iic_txtmp;	/* Internal */
-	uint	iic_res;	/* reserved */
+	char	res1[4];	/* Reserved */
    	ushort	iic_rpbase;	/* Relocation pointer */
-	ushort	iic_res2;	/* reserved */
+	char	res2[2];	/* Reserved */
  } iic_t;

  #define BD_IIC_START		((ushort)0x0400)

Re: [PATCH] first in a series to enhance microcode patches

[Tom Rini]

On Tue, Oct 05, 2004 at 01:32:11PM -0400, Robert P. J. Day wrote:

>   resend of previous patch -- sorry, forgot the developer's cert of 
> origin.  please be patient, i'm slowly getting the hang of this.
> 
>   purpose of patch:
> 
>   1) adds a relocation pointer to smc_uart_t
>   2) redeclares reserved chunks in structures to be in terms of a
>      standard char array, rather than the hideous combination of uint,
>      ushort, and so on.
> 
> Signed-off-by: Robert P. J. Day <rpjday@mindspring.com>

Applied.

-- 
Tom Rini
http://gate.crashing.org/~trini/