thoughts and questions on 8xx patches

thoughts and questions on 8xx patches

[Robert P. J. Day]

   based on my reading the source (and, in this case, 
arch/ppc/8xx_io/micropatch.c), i have a number of 
questions/observations on the whole ugly area of microcode patches, so 
i saved them all up and i'll ask them all at once.

   as a quick intro, the RISC Controller Config Register (RCCR) is 
what determines which regions of DPRAM can contain executable 
microcode.  according to the MPC850 manual, there are four 
possibilities for the ucode settings in that register (bits 14-15):

   00: no microcode execution
   01: 512 bytes at 0x2000, 256 bytes at 0x2f00
   10: 1K at 0x2000, 256 bytes at 0x2f00
   11: 2K at 0x2000, 512 bytes at 0x2e00

1) am i to assume that the two chunks of microcode written to these
    two different addresses are run independently?  and that it's
    *always* the case that, if there is any microcode at all, there
    will *always* be exactly two segments at the selected addresses?
    (that is, it's not possible to just write a patch to the first
    address and leave it at that.  or is it?  the docs don't make
    this clear.)

remaining questions deal with micropatch.c

2) in cpm_load_patch(), before writing microcode into dpmem, the
    RCCR register is set to disable microcode:

    commproc->cp_rccr = 0;

    and is reactivated later.  is this necessary while you're
    installing ucode?

3) next, in micropatch.c, microcode is directly copied from the
    patch arrays patch_2000[] and patch_2f00[] into the respective
    addresses in dpmem.  but who's to say that these arrays are
    always guaranteed to exist?  notice above that there could very
    well exist a patch that writes to 2000 and 2e00 instead.  it
    seems like a bad idea to just flat-out assume that those two
    arrays will always exist, unless there's something more subtle
    happening here that i missed.

4) if the USB SOF patch is being applied, i notice that the RCCR
    register is reset with the value 0x0009, which not only sets
    the microcode address sizes, but sets the RCCR bit for EIE --
    external interrupt enable.  i'm not even going to ask what that
    does, but it will come up later.  i'm just going to accept it as
    magic and move on.

5) if you've defined either SMC PATCH or IIC PATCH, then this
    routine defines the relocation base for the IIC and SPI structs:

    #define RPBASE 0x0400

    this is an error since, from above in that file, the size of the
    SMC/IIC/SPI patch is not 1024 bytes, but 1280, which means that
    the patch will clash with those relocated structures at offset
    0x0400.  redefine RPBASE to 0x0500 to fix this.

6) if you're applying the SMC patch, notice that this routine will
    also apply the patch array patch_2e00[].  now, how exactly does
    that co-operate with the patch_2f00[] array that has *already* been
    installed?  the 2e00 patch is awfully short, and *that's* the ucode
    that's going to be run.  what happens with the stuff that was
    installed at offset 0x2f00 earlier?  will it be run as well?  how?

7) finally, the function verify_patch() clearly does just that --
    verifies the patch contents.  at the top, it disables ucode
    execution (again, why?) with:

    commproc->cp_rccr = 0;

    but at the bottom, it resets the RCCR register with:

    commproc->cp_rccr = 0x0009;

    huh?  i'm pretty sure that's a bad idea, given that the restored
    value should reflect what was in that register before, rather
    than just hardcoding it to 0x0009.  this is fine if you're
    verifying the USB SOF patch, but it's not so good if you're
    verifying any other kind of patch.

    thoughts?

rday

Re: thoughts and questions on 8xx patches

[Dan Malek]

On Sep 20, 2004, at 4:05 PM, Robert P. J. Day wrote:

> 1) am i to assume that the two chunks of microcode written to these
>    two different addresses are run independently?

Nope.  Don't ever assume anything.  Read the microcode patch
instructions, load the code where it says, set registers as it says.

> 2) in cpm_load_patch(), before writing microcode into dpmem, the
>    RCCR register is set to disable microcode:
>
>    commproc->cp_rccr = 0;
>
>    and is reactivated later.  is this necessary while you're
>    installing ucode?

Some boot roms install versions of microcode patches that
we know nothing about.  This is the way to remove them and
install what our Linux drivers know.

> 3) next, in micropatch.c, microcode is directly copied from the
>    patch arrays patch_2000[] and patch_2f00[] into the respective
>    addresses in dpmem.  but who's to say that these arrays are
>    always guaranteed to exist?

Those were the locations used for the microcode patches known
at the time.  Again, it's where the instructions said to load the
patches.  We know nothing more about it.

>   .....notice above that there could very
>    well exist a patch that writes to 2000 and 2e00 instead.

When that happens, we'll have to adjust for it.

> ...... i'm not even going to ask what that
>    does, but it will come up later.  i'm just going to accept it as
>    magic and move on.

Don't ask later.  It's all magic.  Just follow the instructions.

>    thoughts?

Stop trying to understand the undocumented details of the CPM :-)
There have been many patches over the years, some may not
be available anymore, some we may have never used.  The
current Linux drivers may not be up to date with the more recent
versions of microcode patches.

At one time, we had a bunch of little patches.  IIC only, SMC only,
USB SOF.  Then, some of them were combined so you could do
multiple relocations at once, like IIC and SMCs.  I don't know if
we have ever done that.  I know Wolfgang has been keeping
more up to date with his development trees, so it wouldn't hurt
to look there.


	-- Dan

Re: thoughts and questions on 8xx patches

[Robert P. J. Day]

On Mon, 20 Sep 2004, Dan Malek wrote:

> On Sep 20, 2004, at 4:05 PM, Robert P. J. Day wrote:

>> ...... i'm not even going to ask what that
>>    does, but it will come up later.  i'm just going to accept it as
>>    magic and move on.
>
> Don't ask later.  It's all magic.  Just follow the instructions.

hold on a minute.  while i can appreciate not digging too deeply into 
the magic that is a microcode patch, a number of the questions i asked 
were pretty fundamental ones that should be answerable, completely 
independent of the actual patch.  at least, they should be answerable 
to the extent that the source code in micropatch.c makes a certain 
amount of sense.  so, given that i've been told on occasion to RTFS to 
get the answers to my questions, and i've gone and RTFS, it's not 
unreasonable that i get to ask some questions about TFS. :-)

and some of those fundamental questions, based on reading TFS, would 
be:

1) what does it mean to have two different areas for the microcode
    patch in DPRAM?  the manual describes the second area as an
    "extension"?  does that mean that execution simply flows from the
    first area to the second?  (frankly, that would make no sense to
    me, but what do i know?)

    and it would be useful to understand to know why the SMC patch
    writes a little info into offset 0x2e00 and far more into 0x2f00,
    which means that the patch written into that alleged 512-byte
    "extension" isn't even contiguous.  now *that's* confusing, and
    it shouldn't just be dismissed as magic.

2) is the cpm_load_patch() routine justified in assuming that,
    regardless of the patch, there will *always* be a 0x2000 and 0x2f00
    array to load?  why should that be the case when one scenario is
    that the extension might just as well start at 0x2e00 instead?

    again, that's a pretty straightforward question that's independent
    of the *function* of the patch.  it's just asking about the basic
    format.

apart from the underlying magic, are there *any* rules that patches
have to follow, just to make micropatch.c more comprehensible, or to
at least be able to see when something just doesn't look right?

(as an aside, you didn't comment on a couple of other things, so i'm 
not sure if they had any validity.  that is, what is obviously a bad 
value for RPBASE, and the fact that verify_patch() hardcodes the value 
commproc->cp_rccr = 0x0009 at the end, which seems pretty clearly a 
bad idea.  can we agree that those things seem to be a problem? 
because if they aren't i'm more confused than ever.)

> At one time, we had a bunch of little patches.  IIC only, SMC only,
> USB SOF.  Then, some of them were combined so you could do
> multiple relocations at once, like IIC and SMCs.  I don't know if
> we have ever done that.  I know Wolfgang has been keeping
> more up to date with his development trees, so it wouldn't hurt
> to look there.

in fact, i've looked at his trees pretty carefully, and i like the way 
he's broken out his patches as individual .c files.  based on that, i 
hacked up a 2.6-based Kconfig file which gives the user a menu of 
patches to choose from, so there's none of this "you have to manually
define USE_SMC_PATCH to get this patch" silliness.

instead, you get something under the MPC8xx menu like (assuming you 
only ever get to install a single patch):

Microcode patches -->
 	[ ] None	(the default)
 	[ ] I2C/SPI
 	[ ] I2C/SPI/SMC

and so on.

if patches aren't going away any time soon, then it only makes sense 
to make their selection as simple and as comprehensible as possible, 
and not require the user to actually edit files to define selection 
macros for them.

rday

Re: thoughts and questions on 8xx patches

[Dan Malek]

On Sep 20, 2004, at 8:15 PM, Robert P. J. Day wrote:

> 1) what does it mean to have two different areas for the microcode
>    patch in DPRAM?

It means that's where the microcode was built to run.

>    and it would be useful to understand to know why the SMC patch
>    writes a little info into offset 0x2e00 and far more into 0x2f00,

You would have to ask the person that wrote it.

> 2) is the cpm_load_patch() routine justified in assuming that,
>    regardless of the patch, there will *always* be a 0x2000 and 0x2f00
>    array to load?

No, it was just done that way because that is the microcode we
had to load and that's where it belonged.

> apart from the underlying magic, are there *any* rules that patches
> have to follow,

You can only run one patch at a time.

> (as an aside, you didn't comment on a couple of other things, so i'm 
> not sure if they had any validity.  that is, what is obviously a bad 
> value for RPBASE,

The only comment I can make is there have been several different 
versions
of CPM roms on the different processor variants, the microcode patches 
are
specific to those, and perhaps the last change was for a specific 
processor
that doesn't match what you have.

Various IIC/SMC relocation patches have been used, and the USB SOF
patch has been used on the various processors.  At the time they were
used, they worked.

> if patches aren't going away any time soon, then it only makes sense 
> to make their selection as simple and as comprehensible as possible, 
> and not require the user to actually edit files to define selection 
> macros for them.

If you have the patches and can test them, go for it.


	-- Dan

Re: thoughts and questions on 8xx patches

[Robert P. J. Day]

   rather than get into more detailed discussion on microcode patches, 
here's a (partial) patch that represents what i'd really, really, 
*really* like to see.

==========================================================================
--- linuxppc-2.5/arch/ppc/8xx_io/Kconfig	2004-09-21 05:54:40.112334040 -0400
+++ linuxppc-2.5-new/arch/ppc/8xx_io/Kconfig	2004-09-21 06:00:56.836063328 -0400
@@ -140,13 +140,23 @@

  	  If in doubt, say N here.

-config UCODE_PATCH
-	bool "I2C/SPI Microcode Patch"
-	help
-	  Motorola releases microcode updates for their 8xx CPM modules.  The
-	  microcode update file has updates for IIC, SMC and USB.  Currently only
-	  the USB update is available by default, if the MPC8xx USB option is
-	  enabled.  If in doubt, say 'N' here.
+choice
+	prompt "Microcode patch selection"
+	default NO_UCODE_PATCH
+
+config NO_UCODE_PATCH
+	bool "None"
+
+config USB_SOF_PATCH
+	bool "USB SOF patch"
+
+config I2C_SPI_PATCH
+	bool "I2C/SPI relocation patch"
+
+config I2C_SPI_SMC1_PATCH
+	bool "I2C/SPI/SMC1 relocation patch"
+
+endchoice

  endmenu
==========================================================================

   it adds a simple choice entry to the MPC8xx menu, from which you can 
select the appropriate patch -- it's as easy as that.

   note that that's not the entire list of possible patches -- i have 
the list from wolfgang's tree that distinguishes between 850 and 8xx 
(non-850) patches in some cases so the list is actually longer than 
this one.  but, really, selecting a patch should be this easy -- pick 
one and check the box.  and this makes it trivial to add or delete 
patches appropriately as time goes by.

   obviously, this menu requires changes in the underlying micropatch.c 
file (which aren't shown here), but i'm curious about whether this 
approach would overly offend anyone.

   i'm willing to create the overall patch to support this, but not if 
it's just going to be tossed because of philosophical positions.


rday

Re: thoughts and questions on 8xx patches

[Wolfgang Denk]

In message <Pine.LNX.4.60.0409210605090.9033@dell.enoriver.com> you wrote:
> 
>    rather than get into more detailed discussion on microcode patches, 
> here's a (partial) patch that represents what i'd really, really, 
> *really* like to see.
...
> +config I2C_SPI_PATCH
> +	bool "I2C/SPI relocation patch"
> +
> +config I2C_SPI_SMC1_PATCH
> +	bool "I2C/SPI/SMC1 relocation patch"

Why? As far as I understand the I2C/SPI patch has been obsolteted  by
the I2C/SPI/SMC1 patch. So only the latter is needed.

>    it adds a simple choice entry to the MPC8xx menu, from which you can 
> select the appropriate patch -- it's as easy as that.

No, it is not that easy.

>    note that that's not the entire list of possible patches -- i have 
> the list from wolfgang's tree that distinguishes between 850 and 8xx 
> (non-850) patches in some cases so the list is actually longer than 
> this one.  but, really, selecting a patch should be this easy -- pick 

Indeed. And AFAICT there is no way to get the processor version  from
any  other CONFIG option that the board name, so this would become an
awfully long list.

> one and check the box.  and this makes it trivial to add or delete 
> patches appropriately as time goes by.
> 
>    obviously, this menu requires changes in the underlying micropatch.c 
> file (which aren't shown here), but i'm curious about whether this 
> approach would overly offend anyone.

This has been discussed many times before. 

>    i'm willing to create the overall patch to support this, but not if 
> it's just going to be tossed because of philosophical positions.

It's technical issues, too. There was a decision not to configure for
a specific variant of the 8xx processor, and so it becomes cumbersome
to add "convenient" configuration options  which  depend  on  exactly
this information.

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
Do you suppose the reason the ends of the `Intel Inside'  logo  don't
match up is that it was drawn on a Pentium?

Re: thoughts and questions on 8xx patches

[Robert P. J. Day]

On Tue, 21 Sep 2004, Wolfgang Denk wrote:

> In message <Pine.LNX.4.60.0409210605090.9033@dell.enoriver.com> you wrote:
>>
>>    rather than get into more detailed discussion on microcode patches,
>> here's a (partial) patch that represents what i'd really, really,
>> *really* like to see.
> ...
>> +config I2C_SPI_PATCH
>> +	bool "I2C/SPI relocation patch"
>> +
>> +config I2C_SPI_SMC1_PATCH
>> +	bool "I2C/SPI/SMC1 relocation patch"
>
> Why? As far as I understand the I2C/SPI patch has been obsolteted  by
> the I2C/SPI/SMC1 patch. So only the latter is needed.

what i was showing was just an example, it didn't need to represent 
*exactly* the set of choices that would be in the final menu.  if the 
above is the case, that's fine.  but there's still then, at the very 
least, the list of possible patches that are in *your* 2.4 kernel 
source tree.  i wasn't submitting a final list of patches, just a 
suggested *format* for selection, that's all.  don't get ahead of me 
here.

>>    it adds a simple choice entry to the MPC8xx menu, from which you can
>> select the appropriate patch -- it's as easy as that.
>
> No, it is not that easy.

yes, it is.

>>    note that that's not the entire list of possible patches -- i have
>> the list from wolfgang's tree that distinguishes between 850 and 8xx
>> (non-850) patches in some cases so the list is actually longer than
>> this one.  but, really, selecting a patch should be this easy -- pick
>
> Indeed. And AFAICT there is no way to get the processor version  from
> any  other CONFIG option that the board name, so this would become an
> awfully long list.

in what way?  the denx 2.4 kernel source tree defines all of 5 
patches.  i don't see that as an overly long list, and given that the 
default selection would be "None", people who don't even know what a 
patch is would never have to make that decision.

at the moment, your denx tree supports a number of patches that 
require users to edit source files and manually define constants. in 
what way is this a better idea than picking from a drop down choice 
list?

(and i never suggested that the config process automatically detect 
the 850 or non-850-ness of the processor.  that would be a manual 
selection by the user, just as it is in your source tree at the 
moment.  and it's *still* a short list.)

>> one and check the box.  and this makes it trivial to add or delete
>> patches appropriately as time goes by.
>>
>>    obviously, this menu requires changes in the underlying micropatch.c
>> file (which aren't shown here), but i'm curious about whether this
>> approach would overly offend anyone.
>
> This has been discussed many times before.

yes, it has, and the conversation has typically gone something like 
this:

me: ... general whine about something i don't like that could be
 	improved ...
reply: "yes, that's a problem, you're right, so stop whining and
 	submit a patch."
me: "um ... ok, what about a patch like this?"
reply: "no."

   and around and around we go ...

rday

Re: thoughts and questions on 8xx patches

[Robert P. J. Day]

On Tue, 21 Sep 2004, Wolfgang Denk wrote:

> In message <Pine.LNX.4.60.0409210605090.9033@dell.enoriver.com> you wrote:
>>
>>    rather than get into more detailed discussion on microcode patches,
>> here's a (partial) patch that represents what i'd really, really,
>> *really* like to see.
> ...
>> +config I2C_SPI_PATCH
>> +	bool "I2C/SPI relocation patch"
>> +
>> +config I2C_SPI_SMC1_PATCH
>> +	bool "I2C/SPI/SMC1 relocation patch"
>
> Why? As far as I understand the I2C/SPI patch has been obsolteted  by
> the I2C/SPI/SMC1 patch. So only the latter is needed.

uh huh.  even though, as i've already pointed out, the code in 
micropatch.c in *both* your source tree and the linuxppc-2.5 tree is 
broken in that, if you applied the SMC patch, it would have (AFAICT) 
caused a conflict because of an erroneously low value of RPBASE.

it's a bit presumptuous to declare that a broken patch has obsoleted 
one that actually works, don't you think?

rday

Re: thoughts and questions on 8xx patches

[Wolfgang Denk]

In message <Pine.LNX.4.60.0409210700390.9187@dell.enoriver.com> you wrote:
>
> > Why? As far as I understand the I2C/SPI patch has been obsolteted  by
> > the I2C/SPI/SMC1 patch. So only the latter is needed.
> 
> uh huh.  even though, as i've already pointed out, the code in 
> micropatch.c in *both* your source tree and the linuxppc-2.5 tree is 
> broken in that, if you applied the SMC patch, it would have (AFAICT) 
> caused a conflict because of an erroneously low value of RPBASE.
> 
> it's a bit presumptuous to declare that a broken patch has obsoleted 
> one that actually works, don't you think?

You are misinterpreting things. The patch is one thing - it  is  more
or  less  a  black  box  suppied  with usage instructions by the chip
manufacturer. micropatch.c is some code  in  the  Linux  kernel  that
attempts  to  implement the instructions that come with the microcode
patch. The fact that there may be errors in micropatch.c has  nothing
to  do with the fact that one version of the microcode patch may have
obsoleted other versions.

These things have nothing to do with each other.

As  I  alrady  wrote  you  privately  I  think  you  are  right  that
micropatch.c  is  broken  for  the  recent  versions of the microcode
patches. This  does  NOT  mean  that  such  recent  versions  of  the
microcode patches don;t obsolete older versions of the same patches.

Please don;t mix up unrelated things.

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
A rolling stone gathers momentum.

Re: thoughts and questions on 8xx patches

[Wolfgang Denk]

In message <Pine.LNX.4.60.0409210644150.9187@dell.enoriver.com> you wrote:
> 
> > In message <Pine.LNX.4.60.0409210605090.9033@dell.enoriver.com> you wrote:
> >>
> >>    rather than get into more detailed discussion on microcode patches,
> >> here's a (partial) patch that represents what i'd really, really,
     ^^^^^^^^^^^^^^^^^^^^^^^^
...
> what i was showing was just an example, it didn't need to represent 
> *exactly* the set of choices that would be in the final menu.  if the 

Ummm. To me "partial patch" means that this is an excerpt of  exactly
the  patch you want to see applied. Please write "example" if this is
what you mean.

> above is the case, that's fine.  but there's still then, at the very 
> least, the list of possible patches that are in *your* 2.4 kernel 
> source tree.  i wasn't submitting a final list of patches, just a 

Please forget our tree for this discussionhere, it is  not  relevant.
Also,  I  wrote  you before that the uCode stuff was only tested in a
small number of specific configurations, and that it is KNOWN  TO  BE
BROKEN for many configurations, including most newer 8xx processors.

> suggested *format* for selection, that's all.  don't get ahead of me 
> here.

It looked like a ready-to-sumbit patch, and you announced it as such.

> >>    it adds a simple choice entry to the MPC8xx menu, from which you can
> >> select the appropriate patch -- it's as easy as that.
> >
> > No, it is not that easy.
> 
> yes, it is.

OK. I give up here. I've explained  the  complexity  (like  processor
variants,  like  incompatible  changes of the relocation base address
pointer between versions, etc.) several times  before.  You  seem  to
know better...

> > Indeed. And AFAICT there is no way to get the processor version  from
> > any  other CONFIG option that the board name, so this would become an
> > awfully long list.
> 
> in what way?  the denx 2.4 kernel source tree defines all of 5 
> patches.  i don't see that as an overly long list, and given that the 
> default selection would be "None", people who don't even know what a 
> patch is would never have to make that decision.

It was _you_ who asked for a "simple" configuration  where  you  just
enable  a uCode patch, and where the kernel config tool does not even
offer selections that don;t apply to a specific board. It  was  _you_
who  posted  a  "(partial) patch" which did not mention that you will
violate your own requirements.

> at the moment, your denx tree supports a number of patches that 
> require users to edit source files and manually define constants. in 
> what way is this a better idea than picking from a drop down choice 
> list?

It is not better. Nobody ever claimed that. See above.

> (and i never suggested that the config process automatically detect 
> the 850 or non-850-ness of the processor.  that would be a manual 

Yes, you did. Quote Robert P. J. Day (22 Jul 2004 08:00:01 -0400):

	...
	   perhaps overly ambitious, but it would be *really* cool if the
	patches presented to the developer were tied to the underlying
	processor.  that is, once you've selected, say, RPXlite, the only
	patches displayed in the menu should be those relevant to the rpxlite.
	...

> me: "um ... ok, what about a patch like this?"
> reply: "no."

Just look at this discussion today: you post a "(partial) patch", and
when I point out problems with it you say, "umm, that's not the  real
patch,  it's  just  an example, but it needs to be fixed and extended
and actually you goot make it working first." I claim that it's  more
complex  than  this,  you deny this, and then add that of course more
manual selections need to be added.

I have to admit that I don't understand what _exactly_ you suggest.

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
It is easier to write an incorrect program than understand a  correct
one.

Re: thoughts and questions on 8xx patches

[Robert P. J. Day]

On Tue, 21 Sep 2004, Wolfgang Denk wrote:

> In message <Pine.LNX.4.60.0409210700390.9187@dell.enoriver.com> you wrote:
>>
>>> Why? As far as I understand the I2C/SPI patch has been obsolteted  by
>>> the I2C/SPI/SMC1 patch. So only the latter is needed.
>>
>> uh huh.  even though, as i've already pointed out, the code in
>> micropatch.c in *both* your source tree and the linuxppc-2.5 tree is
>> broken in that, if you applied the SMC patch, it would have (AFAICT)
>> caused a conflict because of an erroneously low value of RPBASE.
>>
>> it's a bit presumptuous to declare that a broken patch has obsoleted
>> one that actually works, don't you think?
>
> You are misinterpreting things. The patch is one thing - it  is  more
> or  less  a  black  box  suppied  with usage instructions by the chip
> manufacturer. micropatch.c is some code  in  the  Linux  kernel  that
> attempts  to  implement the instructions that come with the microcode
> patch. The fact that there may be errors in micropatch.c has  nothing
> to  do with the fact that one version of the microcode patch may have
> obsoleted other versions.
>
> These things have nothing to do with each other.

   ok, i was definitely unclear about the point i was trying to make, 
so let me rephrase.  i understand that the patch itself is independent 
from the code to copy/install it -- your point is well taken, and i 
accept that the ucode patch itself works just fine.  but, in this 
context, it's premature to say that it clearly "obsoletes" the older 
patch given that micropatch.c could never have installed it 
successfully in the first place.  in short, it could not possibly have 
been tested in the current situation.  that's all i meant.  anyone who 
took your advice and moved up to the newer patch and installed it 
would have been in for a rude awakening.

   and, besides, who's to say that the new I2C/SPI/SMC1 patch 
"obsoletes" the older I2C/SPI patch?  what if someone *wants* just the 
older relocation and has no need to move SMC1?  the older patch works. 
why shouldn't someone be allowed to choose it?  why should an older, 
simpler and proved-to-be-working patch be obsoleted by a newer, more 
complicated patch with additional functionality someone might not 
want?  why should you, or anyone else, make that decision for them?

   and, while i'm waiting for the caffeine to kick in, a more general 
observation.  i've been told, more than once, that my suggestions 
regarding applying microcode patches are not viable -- that it's "not 
that easy".  but no one seems prepared to explain exactly why.

   when i started out on this path, i asked some initial, fairly dumb 
questions.  then, after putting in the time to read the actual source 
code and the MPC850 manual, i got a much better understanding of how 
ucode patches were applied.  but there are still some mysterious 
corners that are not well documented, if they're documented at all. 
and when i ask questions about these corners, all i get is, "don't 
ask, it's all just magic, just accept it".

   in other words, the dialogue goes something like this:

me: "here's a suggestion to make ucode patches easier."
reply: "it's not that easy."
me: "why?"
reply: "we can't tell you, just take our word for it."

   i've proposed a simple, *workable* solution for selecting ucode 
patches from a short, established list.  if you don't think it will 
work, by all means, explain why, without resorting to "it's not that 
easy" or "it's just magic, don't ask."

rday

Re: thoughts and questions on 8xx patches

[Robert P. J. Day]

On Tue, 21 Sep 2004, Wolfgang Denk wrote:

> In message <Pine.LNX.4.60.0409210644150.9187@dell.enoriver.com> you wrote:
>>
>>> In message <Pine.LNX.4.60.0409210605090.9033@dell.enoriver.com> you wrote:
>>>>
>>>>    rather than get into more detailed discussion on microcode patches,
>>>> here's a (partial) patch that represents what i'd really, really,
>     ^^^^^^^^^^^^^^^^^^^^^^^^
> ...
>> what i was showing was just an example, it didn't need to represent
>> *exactly* the set of choices that would be in the final menu.  if the
>
> Ummm. To me "partial patch" means that this is an excerpt of  exactly
> the  patch you want to see applied. Please write "example" if this is
> what you mean.

ok, sorry, what i meant was that it was a patch that would simply 
demonstrate what the config menu would look like.  anyone who was 
interested could apply it, see how the config menu changed, then just 
un-apply the patch after they got the idea.  it wasn't submitted as an 
official patch, so i apologize for the confusion.

so, regarding terminology, i should have called that an "example 
patch"?  gotcha.  profuse apologies here.

> It looked like a ready-to-sumbit patch, and you announced it as such.

yup, my mistake.  i'll be more careful about my choice of words in the 
future.

> I have to admit that I don't understand what _exactly_ you suggest.

from list discussions some time back, i already gave up on the idea of 
the configuration being able to detect the exact processor and taking 
that into account.  we've already established that, and i'm happy with 
that.

all i'm suggesting now is a drop down choice menu of known patches 
relevant to that architecture, that's all.  in some cases, there would 
be two choices, as in:

 	[ ] I2C/SPI for 850
  	[ ] I2C/SPI for 8xx (non-850)

with underlying help menus to clarify the situation.  AFAICT, this 
would just mirror the available choices in the denx 2.4 kernel tree. 
i can't imagine that this list would get overly long.  from what i can 
see, the denx tree supports the following list of patches:

 	USB SOF
 	I2C/SPI for 850
 	I2C/SPI for 8xx (non-850)
 	I2C/SPI/SMC1 for 850
 	I2C/SPI/SMC1 for 8xx (non-850)

and, unless i'm reading this badly, that's the entire list.  all of 
five available patches.  i don't think that's overly long, and it 
would be fairly simple to implement.

so ... why is this such an objectionable thing?

rday

Re: thoughts and questions on 8xx patches

[Wolfgang Denk]

In message <Pine.LNX.4.60.0409210741150.9337@dell.enoriver.com> you wrote:
> 
> > These things have nothing to do with each other.
> 
>    ok, i was definitely unclear about the point i was trying to make, 
> so let me rephrase.  i understand that the patch itself is independent 
> from the code to copy/install it -- your point is well taken, and i 
> accept that the ucode patch itself works just fine.  but, in this 
> context, it's premature to say that it clearly "obsoletes" the older 
> patch given that micropatch.c could never have installed it 

I repeat: these things have nothing to do with each other.

The uCode patch may even be used in a completely different  OS  where
there  is no such thing like micropatch.c. and still the I2C/SPI/SMC1
obsoletes the older I2C/SPI  patch.  Your  terms  "presumptuous"  and
"premature" are inappropriate.

> been tested in the current situation.  that's all i meant.  anyone who 
> took your advice and moved up to the newer patch and installed it 
> would have been in for a rude awakening.

We were discussiong a NEW implementation for 2.6,  right?  Of  course
you  will  have  to fix the known problems. Anybody who is trying the
old code on - say - a MPC859 will experience much severe problems.

>    and, besides, who's to say that the new I2C/SPI/SMC1 patch 
> "obsoletes" the older I2C/SPI patch?  what if someone *wants* just the 

Motorola/Freescale ?

> older relocation and has no need to move SMC1?  the older patch works. 

What does it hurt?

> observation.  i've been told, more than once, that my suggestions 
> regarding applying microcode patches are not viable -- that it's "not 
> that easy".  but no one seems prepared to explain exactly why.

Arg... You must have bad memory. For example:

> Subject: Re: more questions about 8xx microcode patches
> From: Wolfgang Denk <wd@denx.de>
> Date: Wed, 28 Jul 2004 00:30:28 +0200
> To: "Robert P. J. Day" <rpjday@mindspring.com>
> Cc: Embedded Linux PPC list <linuxppc-embedded@lists.linuxppc.org>
...
> Just to add to the complexity: are you aware that the  parameter  RAM
> relocation  may  be  different between 8xx processors? For example it
> seems that on the 866 family  you  can  relocate  PRAM  even  without
> loading any uCode by just writing the correct offset to (for example)
> the spi_rpbase register.

I also explained to you that the assumption that *_rpbase == 0  means
that no uCode patch is installed is wrong, and that code which writes
0 into this register will crash on such processors.

And so on. You received similar explanations why selecting a specific
processor model is complicated. 

>    i've proposed a simple, *workable* solution for selecting ucode 
> patches from a short, established list.  if you don't think it will 
> work, by all means, explain why, without resorting to "it's not that 
> easy" or "it's just magic, don't ask."

It will not work, because in the example you showed it does not  take
into  account the complexity of this issue. It's not magic, but there
exisit many differenc combinations which you did not deal with.  Just
two examples: (1) differenrt versions of the uCode patch are required
for  MPC850  vs.  other  processors, and (2) you may need to relocate
your parameter RAM on systems which actually don't use any  microcode
for this (like MPC866 and other processors of this family).

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
"The POP3 server service depends on the SMTP  server  service,  which
failed to start because of the following error: The operation comple-
ted successfully."                         -- Windows NT Server v3.51

Re: thoughts and questions on 8xx patches

[Wolfgang Denk]

In message <Pine.LNX.4.60.0409210808250.9449@dell.enoriver.com> you wrote:
> 
> from list discussions some time back, i already gave up on the idea of 
> the configuration being able to detect the exact processor and taking 
> that into account.  we've already established that, and i'm happy with 
> that.

OK.

> all i'm suggesting now is a drop down choice menu of known patches 
> relevant to that architecture, that's all.  in some cases, there would 
> be two choices, as in:
> 
>  	[ ] I2C/SPI for 850
>   	[ ] I2C/SPI for 8xx (non-850)

Plus there is also the case of "need to relocate parameter RAM  on  a
MPC852/859/866/...  processor".  In  this  case  no microcode patch i
sneeded, and the current code will crash the system.

More cases may be necessary.

> i can't imagine that this list would get overly long.  from what i can 
> see, the denx tree supports the following list of patches:

Please forget this. I asked you several times NOT  to  use  this  old
code  for  reference.  It  is  known to be broken in several configu-
rations.

> and, unless i'm reading this badly, that's the entire list.  all of 

No, it's  not.  It  will  not  work  on  most  of  the  newer  MPC8xx
processors.

> so ... why is this such an objectionable thing?

Because it is (1) incomplete and (2) broken.

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
There are two ways to write error-free programs. Only the  third  one
works.

Re: thoughts and questions on 8xx patches

[Robert P. J. Day]

On Tue, 21 Sep 2004, Wolfgang Denk wrote:

> In message <Pine.LNX.4.60.0409210808250.9449@dell.enoriver.com> you wrote:
>>
>> from list discussions some time back, i already gave up on the idea of
>> the configuration being able to detect the exact processor and taking
>> that into account.  we've already established that, and i'm happy with
>> that.
>
> OK.
>
>> all i'm suggesting now is a drop down choice menu of known patches
>> relevant to that architecture, that's all.  in some cases, there would
>> be two choices, as in:
>>
>>  	[ ] I2C/SPI for 850
>>   	[ ] I2C/SPI for 8xx (non-850)
>
> Plus there is also the case of "need to relocate parameter RAM  on  a
> MPC852/859/866/...  processor".  In  this  case  no microcode patch i
> sneeded, and the current code will crash the system.
>
> More cases may be necessary.
...
>> and, unless i'm reading this badly, that's the entire list.  all of
>
> No, it's  not.  It  will  not  work  on  most  of  the  newer  MPC8xx
> processors.
>
>> so ... why is this such an objectionable thing?
>
> Because it is (1) incomplete and (2) broken.

ok, so i'll back off and ask one more question.  even without a 
*current* list of microcode patches, is there any value to even 
building in the infrastructure for such a thing in the config menu, so 
that patches can be added quickly and (relatively) painlessly as they 
come up?  and old ones tossed out as they become obsolete?  if there's 
no value to this at all, i'll just let it drop and move on.

rday

Re: thoughts and questions on 8xx patches

[Dan Malek]

On Sep 21, 2004, at 8:56 AM, Robert P. J. Day wrote:

> ok, so i'll back off and ask one more question.

Create and test the patch that works for you.  Don't be creating
options and code for things you can't test and verify.  Those
of us that have lived through the generations of microcode
patches and processor variants know you can't guess what
the future will bring.  If someone else has a need to extend
the work you have done, they will do that to meet their
requirements.

If you make the options available and write the code to
match, people are going to believe it works.  If you can't
test it, they will struggle with it and we will have these
"... this is obviously broken ..." discussions all over again.

Thanks.


	-- Dan