[PATCH] to support choice of 8xx microcode patch

[PATCH] to support choice of 8xx microcode patch

[Robert P. J. Day]

   yes, this is a *full* and apparently working source code patch that 
applies cleanly against the latest "bk pull" of linuxppc-2.5.  the 
changes should be obvious.  there's still a lot of cruft in these 
files i'd dearly like to get rid of, but i wanted to keep the changes 
to a minimum for now.

   feedback welcome.  patch is the result of running:

   $ bk -r diffs -u

at the top of the source tree.

rday




===== arch/ppc/8xx_io/Kconfig 1.4 vs edited =====
--- 1.4/arch/ppc/8xx_io/Kconfig	2004-05-27 18:37:33 -04:00
+++ edited/arch/ppc/8xx_io/Kconfig	2004-09-25 06:40:34 -04:00
@@ -140,13 +140,28 @@

  	  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 patches"
+	default NO_UCODE_PATCH
+
+config	NO_UCODE_PATCH
+	bool "None"
+
+config	IIC_SPI_UCODE_PATCH
+	bool "IIC/SPI relocation patch"
+
+config	IIC_SPI_SMC1_UCODE_PATCH
+	bool "IIC/SPI/SMC1 relocation patch"
+
+config	USB_SOF_UCODE_PATCH
+	bool "USB/SOF relocation patch"
+
+endchoice
+
+config	UCODE_PATCH
+	bool
+	depends on !NO_UCODE_PATCH
+	default y

  endmenu

===== arch/ppc/8xx_io/commproc.c 1.16 vs edited =====
--- 1.16/arch/ppc/8xx_io/commproc.c	2004-07-15 19:49:03 -04:00
+++ edited/arch/ppc/8xx_io/commproc.c	2004-09-25 08:05:14 -04:00
@@ -114,6 +114,7 @@
  	commproc = (cpm8xx_t *)&imp->im_cpm;

  #ifdef CONFIG_UCODE_PATCH
+# warning "commproc.c: Processing a microcode patch by doing a reset."
  	/* Perform a reset.
  	*/
  	commproc->cp_cpcr = (CPM_CR_RST | CPM_CR_FLG);
===== arch/ppc/8xx_io/micropatch.c 1.2 vs edited =====
--- 1.2/arch/ppc/8xx_io/micropatch.c	2002-02-05 02:55:41 -05:00
+++ edited/arch/ppc/8xx_io/micropatch.c	2004-09-25 08:12:57 -04:00
@@ -19,18 +19,11 @@
  #include <asm/8xx_immap.h>
  #include <asm/commproc.h>

-/* Define this to get SMC patches as well.  You need to modify the uart
- * driver as well......
-#define USE_SMC_PATCH 1
+/*
+ * IIC/SPI relocation.
   */

-#ifdef CONFIG_USB_MPC8xx
-#define USE_USB_SOF_PATCH
-#endif
-
-#ifdef USE_IIC_PATCH
-#define PATCH_DEFINED
-	/* IIC/SPI */
+#ifdef CONFIG_IIC_SPI_UCODE_PATCH
  uint patch_2000[] = {
  	0x7FFFEFD9,
  	0x3FFD0000,
@@ -183,10 +176,12 @@
  };
  #endif

-#ifdef USE_SMC_PATCH
-#define PATCH_DEFINED
-/* SMC2/IIC/SPI Patch */
-/* This is the area from 0x2000 to 0x23ff.
+/*
+ *  IIC/SPI/SMC1 relocation.
+ */
+
+#ifdef CONFIG_IIC_SPI_SMC1_UCODE_PATCH
+/* This is the area from 0x2000 to 0x24ff.
  */
  uint patch_2000[] = {
  	0x3fff0000,
@@ -602,8 +597,7 @@
  };
  #endif

-#ifdef USE_USB_SOF_PATCH
-#define PATCH_DEFINED
+#ifdef CONFIG_USB_SOF_UCODE_PATCH
  uint patch_2000[] = {
  	0x7fff0000,
  	0x7ffd0000,
@@ -630,14 +624,35 @@
   * with the controller in the reset state.  We enable the processor after
   * we load the patch.
   */
+
+#ifndef CONFIG_UCODE_PATCH
+
+/*
+ *  Um ... why are we defining null patch functions here when this
+ * file won't even be included in the build process unless a patch
+ * is selected?  Seems kind of redundant.
+ */
+
+void
+cpm_load_patch(volatile immap_t *immr)
+{ }
+
+void
+verify_patch(volatile immap_t *immr)
+{ }
+
+#else	/* we have a patch, get to work. */
+
+# warning "micropatch.c: Defining cpm_load_patch() and verify_patch()."
+
  void
  cpm_load_patch(volatile immap_t *immr)
  {
-#ifdef PATCH_DEFINED
  	volatile uint		*dp;
  	volatile cpm8xx_t	*commproc;
  	volatile iic_t		*iip;
  	volatile spi_t		*spp;
+	volatile smc_uart_t	*smp;	/* new for SMC1 relocation */
  	int	i;

  	commproc = (cpm8xx_t *)&immr->im_cpm;
@@ -653,6 +668,17 @@

  	/* Copy the patch into DPRAM.
  	*/
+
+	/*
+	 *   I'm nervous about the assumption that *every* potential 
+	 * patch will always incorporate a patch_2000[] and patch_2f00[]
+	 * array.  It's entirely possible that a patch might include,
+	 * instead, patch_2000[] and patch_2e00[] instead.  I'd be happier
+	 * to see this code spread out across the different choices, with
+	 * each patch choice looking after just its own arrays.  But
+	 * I'll leave this alone for now.
+	 */
+
  	dp = (uint *)(commproc->cp_dpmem);
  	for (i=0; i<(sizeof(patch_2000)/4); i++)
  		*dp++ = patch_2000[i];
@@ -661,29 +687,26 @@
  	for (i=0; i<(sizeof(patch_2f00)/4); i++)
  		*dp++ = patch_2f00[i];

-#ifdef USE_USB_SOF_PATCH
-#if 0 /* usb patch should not relocate iic */
-	iip = (iic_t *)&commproc->cp_dparam[PROFF_IIC];
-#define RPBASE 0x0030
-	iip->iic_rpbase = RPBASE;
-
-	/* Put SPI above the IIC, also 32-byte aligned.
-	*/
-	i = (RPBASE + sizeof(iic_t) + 31) & ~31;
-	spp = (spi_t *)&commproc->cp_dparam[PROFF_SPI];
-	spp->spi_rpbase = i;
-#endif
+#ifdef CONFIG_USB_SOF_UCODE_PATCH

  	/* Enable uCode fetches from DPRAM. */
  	commproc->cp_rccr = 0x0009;

  	printk("USB uCode patch installed\n");
-#endif /* USE_USB_SOF_PATCH */

-#if defined(USE_SMC_PATCH) || defined(USE_IIC_PATCH)
+#endif /* CONFIG_USB_SOF_UCODE_PATCH */
+
+#if 	defined(CONFIG_IIC_SPI_UCODE_PATCH) || \
+	defined(CONFIG_IIC_SPI_SMC1_UCODE_PATCH)

  	iip = (iic_t *)&commproc->cp_dparam[PROFF_IIC];
-#define RPBASE 0x0400
+
+# if	defined(CONFIG_IIC_SPI_SMC1_UCODE_PATCH)
+# define RPBASE 0x0500	/* SMC1 relocation patch is 1280 bytes long. */
+# else
+# define RPBASE 0x0400
+# endif
+
  	iip->iic_rpbase = RPBASE;

  	/* Put SPI above the IIC, also 32-byte aligned.
@@ -692,7 +715,7 @@
  	spp = (spi_t *)&commproc->cp_dparam[PROFF_SPI];
  	spp->spi_rpbase = i;

-#ifdef USE_SMC_PATCH
+# ifdef CONFIG_IIC_SPI_SMC1_UCODE_PATCH
  	dp = (uint *)&(commproc->cp_dpmem[0x0e00]);
  	for (i=0; i<(sizeof(patch_2e00)/4); i++)
  		*dp++ = patch_2e00[i];
@@ -707,9 +730,15 @@
  	/* Enable uCode fetches from DPRAM.
  	*/
  	commproc->cp_rccr = 3;
-#endif

-#ifdef USE_IIC_PATCH
+	smp = (smc_uart_t *)&commproc->cp_dparam[PROFF_SMC1];
+	smp->smc_rpbase = 0x1FC0;
+
+	printk("SMC1 relocated to 0x%x\n", smp->smc_rpbase);
+
+# endif
+
+# ifdef CONFIG_IIC_SPI_UCODE_PATCH
  	/* Enable the traps to get to it.
  	*/
  	commproc->cp_cpmcr1 = 0x802a;
@@ -722,7 +751,7 @@
  	commproc->cp_rccr = 1;

  	printk("I2C uCode patch installed\n");
-#endif
+# endif

  	/* Relocate the IIC and SPI parameter areas.  These have to
  	 * aligned on 32-byte boundaries.
@@ -736,14 +765,12 @@
  	spp = (spi_t *)&commproc->cp_dparam[PROFF_SPI];
  	spp->spi_rpbase = i;

-#endif /* USE_SMC_PATCH || USE_IIC_PATCH */
-#endif /* PATCH_DEFINED */
+#endif /* IIC/SPI or IIC/SPI/SMC1 relocation patch. */
  }

  void
  verify_patch(volatile immap_t *immr)
  {
-#ifdef PATCH_DEFINED
  	volatile uint		*dp;
  	volatile cpm8xx_t	*commproc;
  	int i;
@@ -772,6 +799,5 @@
  		}

  	commproc->cp_rccr = 0x0009;
-#endif /* PATCH_DEFINED */
  }
-
+#endif /* CONFIG_UCODE_PATCH */
===== drivers/serial/cpm_uart/cpm_uart_core.c 1.5 vs edited =====
--- 1.5/drivers/serial/cpm_uart/cpm_uart_core.c	2004-07-15 19:29:21 -04:00
+++ edited/drivers/serial/cpm_uart/cpm_uart_core.c	2004-09-25 08:04:08 -04:00
@@ -743,6 +743,21 @@
  	pinfo->smcup->smc_rbase = (u_char *)pinfo->rx_bd_base - DPRAM_BASE;
  	pinfo->smcup->smc_tbase = (u_char *)pinfo->tx_bd_base - DPRAM_BASE;

+/*
+ *  Apparently, include the following for ANY patch that is relocating SMC1.
+ */
+
+#if defined(CONFIG_IIC_SPI_SMC1_UCODE_PATCH)
+# warning "cpm_uart_core.c: Processing IIC/SPI/SMC1 ucode patch."
+
+	up->smc_rbptr = pinfo->smcup->smc_rbase;
+	up->smc_tbptr = pinfo->smcup->smc_tbase;
+	up->smc_rstate = 0;
+	up->smc_tstate = 0;
+	up->smc_brkcr = 1;              /* number of break chars */
+	up->smc_brkec = 0;
+#endif
+
  	/* Set up the uart parameters in the
  	 * parameter ram.
  	 */
===== drivers/serial/cpm_uart/cpm_uart_cpm1.c 1.5 vs edited =====
--- 1.5/drivers/serial/cpm_uart/cpm_uart_cpm1.c	2004-07-19 12:47:25 -04:00
+++ edited/drivers/serial/cpm_uart/cpm_uart_cpm1.c	2004-09-25 07:05:55 -04:00
@@ -194,8 +194,20 @@
  	cpm_uart_nr = 0;
  #ifdef CONFIG_SERIAL_CPM_SMC1
  	cpm_uart_ports[UART_SMC1].smcp = &cpmp->cp_smc[0];
+
+	/*
+	 *  Make the following change for any patch that relocates SMC1.
+	 */
+
+# if defined(CONFIG_IIC_SPI_SMC1_UCODE_PATCH)
+#  warning "cpm_uart_cpm1.c: Processing I2C/SPI/SMC1 ucode patch."
+	cpm_uart_ports[UART_SMC1].smcup =
+	    (smc_uart_t *) & cpmp->cp_dparam[0x3C0];  /* new address */
+# else
  	cpm_uart_ports[UART_SMC1].smcup =
  	    (smc_uart_t *) & cpmp->cp_dparam[PROFF_SMC1];
+# endif
+
  	cpm_uart_ports[UART_SMC1].port.mapbase =
  	    (unsigned long)&cpmp->cp_smc[0];
  	cpm_uart_ports[UART_SMC1].smcp->smc_smcm |= (SMCM_RX | SMCM_TX);
===== include/asm-ppc/commproc.h 1.14 vs edited =====
--- 1.14/include/asm-ppc/commproc.h	2004-07-15 19:49:03 -04:00
+++ edited/include/asm-ppc/commproc.h	2004-09-25 06:26:10 -04:00
@@ -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.

Re: [PATCH] to support choice of 8xx microcode patch

[Wolfgang Denk]

In message <Pine.LNX.4.60.0409250834090.3738@dell.enoriver.com> you wrote:
> 
>    yes, this is a *full* and apparently working source code patch that 

For which CPU types did you test it?

Where do you select which CPU type gets  used,  and  adjust  for  the
corresponfding version of the uCode patch?


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 certain things men must do to remain men.
	-- Kirk, "The Ultimate Computer", stardate 4929.4

Re: [PATCH] to support choice of 8xx microcode patch

[Robert P. J. Day]

On Sat, 25 Sep 2004, Wolfgang Denk wrote:

> In message <Pine.LNX.4.60.0409250834090.3738@dell.enoriver.com> you wrote:
>>
>>    yes, this is a *full* and apparently working source code patch that
>
> For which CPU types did you test it?

mine.  850DE, and that's it.  i've verified that my board works with:

   a) no patch applied
   b) IIC/SPI/SMC1 patch applied, and no networking
   c) IIC/SPI/SMC1 patch applied, with ethernet on SCC3

> Where do you select which CPU type gets used, and adjust for the 
> corresponding version of the uCode patch?

   i don't, but that's not the point.  this kernel source patch is 
based on the code in the latest "bk pull" so, while it clearly has 
deficiencies, they're no more than the deficiencies that *already* 
*exist* in the source tree.  (i.e., i don't break anything that's not 
already broken. :-)

   i'm not claiming that this is the perfect solution to ucode patches. 
what i'm claiming is that it's a *better* approach than what's already 
there, which requires folks to hack the source files to define the 
appropriate macros.  in short, you lose *nothing* in the way of 
functionality compared to what's currently there, and you gain in the 
way of improved flexibility and usage.  if you're going to say, "well, 
that patch doesn't let me do X", i'm going to reply, "you can't do X 
at the moment, anyway, so you can't complain."  (what the patch *does* 
do, i believe, is allow functionality to be added much more easily 
than the current situation.)

   anyway, some observations/questions about the current state of 
things re: ucode patches (some of which is definite ugliness and 
redundancy in the code).  (and, BTW, i want to give credit to torsten 
demke, who sent me the basic patch to do the SMC1/SCC3 co-existence.)

1) obviously, the "#warning"s i put into various files can be removed
    later.


   related to arch/ppc/8xx_io/Kconfig:


2) the new structure of Kconfig makes it trivial to add/remove/change
    the selectable patches.


   related to arch/ppc/8xx_io/micropatch.c:


3) rather than having all the patch arrays lumped together in
    micropatch.c, i would prefer to see them as they are in the DENX
    source tree -- broken into separate files, one of which, of course,
    would be conditionally included based on the selected patch.  this
    would obviously be a trivial change.

4) regarding the functions cpm_load_patch() and verify_patch(), i have
    no idea why they're conditionally defined in micropatch.c, since
    that file isn't even part of the build unless a patch is selected,
    based on the "Makefile" in that directory.  kind of redundant.

5) i'm not even sure what verify_patch() is doing there, since
    no one else in the entire kernel source tree calls it.  but what
    the heck, i just left it there.

6) i'm *extremely* nervous about the first part of cpm_load_patch(),
    which just *assumes* that any patch will *always* incorporate both
    a patch_2000[] and patch_2f00[] array.  i don't see why this has to
    be true.  theoretically, a patch can consist of a patch_2000[] and
    a patch_2e00[] array, no?  certainly, that's a possibility based on
    the possible settings in commproc->cp_rccr, and i don't recall
    anyone clearing up that issue.

    rather than try to get clever by having this common code at the
    top of cpm_load_patch() to save lines, i'd prefer to break out each
    patch processing all by itself, just to make things clearer.  so
    what if you duplicate a few lines?  it's cleaner.

7) note that, if you relocate SMC1, you need to increase RPBASE based
    on the size of that patch's patch_2000[] array, from 0x400 to
    0x500.  any reason not to make that a constant change regardless?
    no big deal, just asking.

8) some definite redundancy currently in micropatch.c -- the code:

         iip->iic_rpbase = RPBASE;

         /* Put SPI above the IIC, also 32-byte aligned.
         */
         i = (RPBASE + sizeof(iic_t) + 31) & ~31;
         spp = (spi_t *)&commproc->cp_dparam[PROFF_SPI];
         spp->spi_rpbase = i;

    appears in two different places in cpm_load_patch().  sure seems
    unnecessary, but i left it in just in case.

9) if verify_patch() is going to stay there, can someone explain why
    it deactivates the patch selection bits at the top:

        commproc->cp_rccr = 0;

    and then hardcodes that value back at the bottom, regardless of
    what it was in the first place?

         commproc->cp_rccr = 0x0009;

    i can't believe that's correct, but i left it as is since no one
    calls it anyway.

thoughts?  as you can see, there's plenty of stuff that can be 
clarified/cleaned up/thrown away.

rday

Re: [PATCH] to support choice of 8xx microcode patch

[Robert P. J. Day]

On Sat, 25 Sep 2004, Wolfgang Denk wrote:

> In message <Pine.LNX.4.60.0409250834090.3738@dell.enoriver.com> you wrote:
>>
>>    yes, this is a *full* and apparently working source code patch that
>
> For which CPU types did you test it?
>
> Where do you select which CPU type gets  used,  and  adjust  for  the
> corresponfding version of the uCode patch?

are you referring to the distinction you make in the DENX 2.4 tree, 
where you distinguish between 850 and non-850 architectures?  since 
that appears to be done manually, i see no reason why the same 
couldn't be done in the patch i submitted.  it would be a trivial 
change to get the same effect.

rday

Re: [PATCH] to support choice of 8xx microcode patch

[Wolfgang Denk]

In message <Pine.LNX.4.60.0409251109150.4235@dell.enoriver.com> you wrote:
>
> > Where do you select which CPU type gets  used,  and  adjust  for  the
> > corresponfding version of the uCode patch?
> 
> are you referring to the distinction you make in the DENX 2.4 tree, 
> where you distinguish between 850 and non-850 architectures?  since 

This is one part of what needs to be done. Also,  the  MPC866  family
handles relocation differently.

> that appears to be done manually, i see no reason why the same 
> couldn't be done in the patch i submitted.  it would be a trivial 
> change to get the same effect.

You are right. But since you wrote  "this  is  a  *full*  ...  patch"
(emphasis  by  yourself)  I  somehow  expected  that you covered this
problem, too.

Sorry if this was a misunderstanding.

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 more data I punch in this card,  the lighter it becomes, and the
lower the mailing cost."
                     - Stan Kelly-Bootle, "The Devil's DP Dictionary"

Re: [PATCH] to support choice of 8xx microcode patch

[Robert P. J. Day]

On Sat, 25 Sep 2004, Wolfgang Denk wrote:

> In message <Pine.LNX.4.60.0409251109150.4235@dell.enoriver.com> you wrote:
>>
>>> Where do you select which CPU type gets  used,  and  adjust  for  the
>>> corresponfding version of the uCode patch?
>>
>> are you referring to the distinction you make in the DENX 2.4 tree,
>> where you distinguish between 850 and non-850 architectures?  since
>
> This is one part of what needs to be done. Also,  the  MPC866  family
> handles relocation differently.

not a problem.  hey, submit a patch. :-)  seriously, you can at least 
suggest what the Kconfig menu choices should look like.  i certainly 
don't feel comfortable doing that, and it's obvious how to add extra 
entries to Kconfig.

>> that appears to be done manually, i see no reason why the same
>> couldn't be done in the patch i submitted.  it would be a trivial
>> change to get the same effect.
>
> You are right. But since you wrote  "this  is  a  *full*  ...  patch"
> (emphasis  by  yourself)  I  somehow  expected  that you covered this
> problem, too.
>
> Sorry if this was a misunderstanding.

no problem.  what i meant by "full" was that it's a completely 
self-contained patch that should (theoretically) drop into place in 
the current linuxppc-2.5 bk tree and should work at least as well as 
what's already there.

i'm not submitting it for immediate inclusion since, as you saw from 
my interminable list of questions/observations, there's still lots of 
trash that can be cleaned up in the current tree.  but it would be 
nice if someone were to apply the patch and at least make sure it 
works in its current form, at least with what it's been designed to 
do.

the point was not to be complete, it was to build an infrastructure so 
that patches were far easier to work with, that's all.

rday