next pass of cleaning up micropatch.c

next pass of cleaning up micropatch.c

[Robert P. J. Day]

   numerous changes to micropatch.c:

   1) define patching macros for brevity
   2) make sections for each patch more modular
   3) comment out questionable verify_patch() routine for now, since
 	no one seems to call it


comments?  it's still not the final cut, just another step along
the way.

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




===== arch/ppc/8xx_io/micropatch.c 1.3 vs edited =====
--- 1.3/arch/ppc/8xx_io/micropatch.c	2004-10-07 18:28:32 -04:00
+++ edited/arch/ppc/8xx_io/micropatch.c	2004-10-08 08:34:58 -04:00
@@ -621,15 +621,29 @@
  };
  #endif

-/* Load the microcode patch.  This is called early in the CPM initialization
- * with the controller in the reset state.  We enable the processor after
- * we load the patch.
+/*
+ *  Shortcut macros for patching code.
   */
+
+#define PATCH2000 \
+	dp = (uint *)(commproc->cp_dpmem); \
+	for (i=0; i<(sizeof(patch_2000)/4); i++) \
+		*dp++ = patch_2000[i];
+
+#define PATCH2E00 \
+	dp = (uint *)&(commproc->cp_dpmem[0x0e00]); \
+	for (i=0; i<(sizeof(patch_2e00)/4); i++) \
+		*dp++ = patch_2e00[i];
+
+#define PATCH2F00 \
+	dp = (uint *)&(commproc->cp_dpmem[0x0f00]); \
+	for (i=0; i<(sizeof(patch_2f00)/4); i++) \
+		*dp++ = patch_2f00[i];
+
+
  void
-cpm_load_patch(volatile immap_t *immr)
-{
-#ifdef CONFIG_UCODE_PATCH
-	volatile uint		*dp;
+cpm_load_patch(volatile immap_t *immr) {
+	volatile uint		*dp;		/* Dual-ported RAM. */
  	volatile cpm8xx_t	*commproc;
  	volatile iic_t		*iip;
  	volatile spi_t		*spp;
@@ -638,42 +652,22 @@

  	commproc = (cpm8xx_t *)&immr->im_cpm;

-	/* We work closely with commproc.c.  We know it only allocates
-	 * from data only space.
-	 * For this particular patch, we only use the bottom 512 bytes
-	 * and the upper 256 byte extension.  We will use the space
-	 * starting at 1K for the relocated parameters, as the general
-	 * CPM allocation won't come from that area.
-	 */
-	commproc->cp_rccr = 0;
-
-	/* Copy the patch into DPRAM.
-	 *
-	 * ADDENDUM:  I am somewhat nervous about the next few lines,
-	 * as they imply that *any* patch will *always* consist of at
-	 * least the patch_2000[] and patch_2f00[] arrays, and it's 
-	 * not clear to me that that's true.  More to come here as I
-	 * figure this out.
-	*/
-	dp = (uint *)(commproc->cp_dpmem);
-	for (i=0; i<(sizeof(patch_2000)/4); i++)
-		*dp++ = patch_2000[i];
-
-	dp = (uint *)&(commproc->cp_dpmem[0x0f00]);
-	for (i=0; i<(sizeof(patch_2f00)/4); i++)
-		*dp++ = patch_2f00[i];
-
  #ifdef CONFIG_USB_SOF_UCODE_PATCH
-
-	/* Enable uCode fetches from DPRAM. */
+	commproc->cp_rccr = 0;
+	PATCH2000
+	PATCH2F00
  	commproc->cp_rccr = 0x0009;

-	printk("USB uCode patch installed\n");
-#endif /* CONFIG_USB_SOF_PATCH */
+	printk("USB SOF microcode patch installed\n");
+#endif /* CONFIG_USB_SOF_UCODE_PATCH */

  #if defined(CONFIG_I2C_SPI_UCODE_PATCH) || \
      defined(CONFIG_I2C_SPI_SMC1_UCODE_PATCH)

+	commproc->cp_rccr = 0;
+	PATCH2000
+	PATCH2F00
+
  	iip = (iic_t *)&commproc->cp_dparam[PROFF_IIC];
  # define RPBASE 0x0500
  	iip->iic_rpbase = RPBASE;
@@ -684,63 +678,42 @@
  	spp = (spi_t *)&commproc->cp_dparam[PROFF_SPI];
  	spp->spi_rpbase = i;

-# if defined(CONFIG_I2C_SPI_SMC1_UCODE_PATCH)
-	dp = (uint *)&(commproc->cp_dpmem[0x0e00]);
-	for (i=0; i<(sizeof(patch_2e00)/4); i++)
-		*dp++ = patch_2e00[i];
+# if defined(CONFIG_I2C_SPI_UCODE_PATCH)
+	commproc->cp_cpmcr1 = 0x802a;
+	commproc->cp_cpmcr2 = 0x8028;
+	commproc->cp_cpmcr3 = 0x802e;
+	commproc->cp_cpmcr4 = 0x802c;
+	commproc->cp_rccr = 1;

-	/* Enable the traps to get to it.
-	*/
+	printk("I2C/SPI microcode patch installed.\n");
+# endif /* CONFIG_I2C_SPI_UCODE_PATCH */
+
+# if defined(CONFIG_I2C_SPI_SMC1_UCODE_PATCH)
+	PATCH2E00
  	commproc->cp_cpmcr1 = 0x8080;
  	commproc->cp_cpmcr2 = 0x808a;
  	commproc->cp_cpmcr3 = 0x8028;
  	commproc->cp_cpmcr4 = 0x802a;
-
-	/* Enable uCode fetches from DPRAM.
-	*/
  	commproc->cp_rccr = 3;

  	smp = (smc_uart_t *)&commproc->cp_dparam[PROFF_SMC1];
  	smp->smc_rpbase = 0x1FC0;

-	printk("I2C/SPI/SMC1 ucode patch installed.\n");
+	printk("I2C/SPI/SMC1 microcode patch installed.\n");
  # endif /* CONFIG_I2C_SPI_SMC1_UCODE_PATCH) */

-# if defined(CONFIG_I2C_SPI_UCODE_PATCH)
-	/* Enable the traps to get to it.
-	*/
-	commproc->cp_cpmcr1 = 0x802a;
-	commproc->cp_cpmcr2 = 0x8028;
-	commproc->cp_cpmcr3 = 0x802e;
-	commproc->cp_cpmcr4 = 0x802c;
-
-	/* Enable uCode fetches from DPRAM.
-	*/
-	commproc->cp_rccr = 1;
-
-	printk("I2C/SPI ucode patch installed.\n");
-# endif /* CONFIG_I2C_SPI_UCODE_PATCH */
-
-	/* Relocate the IIC and SPI parameter areas.  These have to
-	 * aligned on 32-byte boundaries.
-	 */
-	iip = (iic_t *)&commproc->cp_dparam[PROFF_IIC];
-	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
-#endif /* CONFIG_UCODE_PATCH */
+#endif /* some variation of the I2C/SPI patch was selected */
  }

+/*
+ *  Take this entire routine out, since no one calls it and its 
+ * logic is suspect.
+ */
+
+#if 0
  void
  verify_patch(volatile immap_t *immr)
  {
-#ifdef CONFIG_UCODE_PATCH
  	volatile uint		*dp;
  	volatile cpm8xx_t	*commproc;
  	int i;
@@ -769,5 +742,5 @@
  		}

  	commproc->cp_rccr = 0x0009;
-#endif /* CONFIG_UCODE_PATCH */
  }
+#endif

Re: next pass of cleaning up micropatch.c

[Dan Malek]

On Oct 8, 2004, at 8:44 AM, Robert P. J. Day wrote:
> + *  Shortcut macros for patching code.
>   */
> +
> +#define PATCH2000 \
> +	dp = (uint *)(commproc->cp_dpmem); \
> +	for (i=0; i<(sizeof(patch_2000)/4); i++) \
> +		*dp++ = patch_2000[i];
> +
> +#define PATCH2E00 \
> +	dp = (uint *)&(commproc->cp_dpmem[0x0e00]); \
> +	for (i=0; i<(sizeof(patch_2e00)/4); i++) \
> +		*dp++ = patch_2e00[i];
> +
> +#define PATCH2F00 \
> +	dp = (uint *)&(commproc->cp_dpmem[0x0f00]); \
> +	for (i=0; i<(sizeof(patch_2f00)/4); i++) \
> +		*dp++ = patch_2f00[i];

Please get rid of these macros and place the code where it
belongs.  They add no value and just make it harder to
read the code and understand what it does.

Thanks.


	-- Dan

Re: next pass of cleaning up micropatch.c

[Robert P. J. Day]

On Fri, 8 Oct 2004, Tom Rini wrote:

> On Fri, Oct 08, 2004 at 11:34:06AM -0400, Dan Malek wrote:
>> On Oct 8, 2004, at 8:44 AM, Robert P. J. Day wrote:
>>> + *  Shortcut macros for patching code.
>>>  */
>>> +
>>> +#define PATCH2000 \
>>> +	dp = (uint *)(commproc->cp_dpmem); \
>>> +	for (i=0; i<(sizeof(patch_2000)/4); i++) \
>>> +		*dp++ = patch_2000[i];
>>> +
>>> +#define PATCH2E00 \
>>> +	dp = (uint *)&(commproc->cp_dpmem[0x0e00]); \
>>> +	for (i=0; i<(sizeof(patch_2e00)/4); i++) \
>>> +		*dp++ = patch_2e00[i];
>>> +
>>> +#define PATCH2F00 \
>>> +	dp = (uint *)&(commproc->cp_dpmem[0x0f00]); \
>>> +	for (i=0; i<(sizeof(patch_2f00)/4); i++) \
>>> +		*dp++ = patch_2f00[i];
>>
>> Please get rid of these macros and place the code where it
>> belongs.  They add no value and just make it harder to
>> read the code and understand what it does.
>
> I agree.  If there were more patches it might make sense to write a
> do_microcode_patch2(N) macro, but PATCH2NNN isn't "readable" and it's
> only 3 patches.

fair enough, but keep in mind, the whole point was that what you're 
looking at is the minimal *infrastructure* for possibly adding more 
patches down the road.  right *now*, there's only three because those 
are the only ones that were in micropatch.c at the moment.  there's 
certainly a lot more available at freescale that can be added as time 
goes by.

i'll put the actual code back in, but you might have second thoughts 
when we're up to 8 or 10 patches some day. :-)

rday

Re: next pass of cleaning up micropatch.c

[Tom Rini]

On Fri, Oct 08, 2004 at 11:34:06AM -0400, Dan Malek wrote:
> On Oct 8, 2004, at 8:44 AM, Robert P. J. Day wrote:
> >+ *  Shortcut macros for patching code.
> >  */
> >+
> >+#define PATCH2000 \
> >+	dp = (uint *)(commproc->cp_dpmem); \
> >+	for (i=0; i<(sizeof(patch_2000)/4); i++) \
> >+		*dp++ = patch_2000[i];
> >+
> >+#define PATCH2E00 \
> >+	dp = (uint *)&(commproc->cp_dpmem[0x0e00]); \
> >+	for (i=0; i<(sizeof(patch_2e00)/4); i++) \
> >+		*dp++ = patch_2e00[i];
> >+
> >+#define PATCH2F00 \
> >+	dp = (uint *)&(commproc->cp_dpmem[0x0f00]); \
> >+	for (i=0; i<(sizeof(patch_2f00)/4); i++) \
> >+		*dp++ = patch_2f00[i];
> 
> Please get rid of these macros and place the code where it
> belongs.  They add no value and just make it harder to
> read the code and understand what it does.

I agree.  If there were more patches it might make sense to write a
do_microcode_patch2(N) macro, but PATCH2NNN isn't "readable" and it's
only 3 patches.

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

Re: next pass of cleaning up micropatch.c

[Tom Rini]

On Fri, Oct 08, 2004 at 11:45:59AM -0400, Robert P. J. Day wrote:
> On Fri, 8 Oct 2004, Tom Rini wrote:
> >On Fri, Oct 08, 2004 at 11:34:06AM -0400, Dan Malek wrote:
> >>On Oct 8, 2004, at 8:44 AM, Robert P. J. Day wrote:
> >>>+ *  Shortcut macros for patching code.
> >>> */
> >>>+
> >>>+#define PATCH2000 \
> >>>+	dp = (uint *)(commproc->cp_dpmem); \
> >>>+	for (i=0; i<(sizeof(patch_2000)/4); i++) \
> >>>+		*dp++ = patch_2000[i];
> >>>+
> >>>+#define PATCH2E00 \
> >>>+	dp = (uint *)&(commproc->cp_dpmem[0x0e00]); \
> >>>+	for (i=0; i<(sizeof(patch_2e00)/4); i++) \
> >>>+		*dp++ = patch_2e00[i];
> >>>+
> >>>+#define PATCH2F00 \
> >>>+	dp = (uint *)&(commproc->cp_dpmem[0x0f00]); \
> >>>+	for (i=0; i<(sizeof(patch_2f00)/4); i++) \
> >>>+		*dp++ = patch_2f00[i];
> >>
> >>Please get rid of these macros and place the code where it
> >>belongs.  They add no value and just make it harder to
> >>read the code and understand what it does.
> >
> >I agree.  If there were more patches it might make sense to write a
> >do_microcode_patch2(N) macro, but PATCH2NNN isn't "readable" and it's
> >only 3 patches.
> 
> fair enough, but keep in mind, the whole point was that what you're 
> looking at is the minimal *infrastructure* for possibly adding more 
> patches down the road.  right *now*, there's only three because those 
> are the only ones that were in micropatch.c at the moment.  there's 
> certainly a lot more available at freescale that can be added as time 
> goes by.
> 
> i'll put the actual code back in, but you might have second thoughts 
> when we're up to 8 or 10 patches some day. :-)

When we get to 8 or 10 patches, we can figure out what a
do_microcode_patch*() macro might look like :)

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

Re: next pass of cleaning up micropatch.c

[Robert P. J. Day]

On Fri, 8 Oct 2004, Tom Rini wrote:

> When we get to 8 or 10 patches, we can figure out what a
> do_microcode_patch*() macro might look like :)

i'm already designing one. :-P

rday

Re: next pass of cleaning up micropatch.c

[Dan Malek]

On Oct 8, 2004, at 11:45 AM, Robert P. J. Day wrote:

> i'll put the actual code back in, but you might have second thoughts 
> when we're up to 8 or 10 patches some day. :-)

No, I won't.  I'm an old school engineer that believes writing software
is as much art as engineering.  Macros have their place, but only
when appropriate.  The only thing I hate more than macros is
code in include files.  Instead of those macros, I would have written
a simple function that takes a microcode patch array, #define'd
those hard coded addresses with logical names and passed it
into that function.

Thanks.


	-- Dan

Re: next pass of cleaning up micropatch.c

[Robert P. J. Day]

On Fri, 8 Oct 2004, Dan Malek wrote:

>
> On Oct 8, 2004, at 11:45 AM, Robert P. J. Day wrote:
>
>> i'll put the actual code back in, but you might have second thoughts when 
>> we're up to 8 or 10 patches some day. :-)
>
> ... Instead of those macros, I would have written a simple function 
> that takes a microcode patch array, #define'd those hard coded 
> addresses with logical names and passed it into that function.

nice idea, but sadly, doomed to failure.  note that the code to copy 
the patch arrays *requires* you to be able to take the sizeof() those 
arrays to know *exactly* how much to copy.  if you pass the array to a 
function by its address, i'm pretty sure you lose the ability to take 
its sizeof() anymore, isn't that right?

in short, you either need the code or a macro representing the code. 
a function won't help you here.

rday

Re: next pass of cleaning up micropatch.c

[Wolfgang Denk]

In message <Pine.LNX.4.60.0410081643100.25430@dell.enoriver.com> you wrote:
> 
> nice idea, but sadly, doomed to failure.  note that the code to copy 
> the patch arrays *requires* you to be able to take the sizeof() those 
> arrays to know *exactly* how much to copy.  if you pass the array to a 
> function by its address, i'm pretty sure you lose the ability to take 
> its sizeof() anymore, isn't that right?

This is not right. Guess how memcpy() works - it's a function, btw.

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd@denx.de
God runs electromagnetics by wave theory on  Monday,  Wednesday,  and
Friday,  and the Devil runs them by quantum theory on Tuesday, Thurs-
day, and Saturday.                                   -- William Bragg

Re: next pass of cleaning up micropatch.c

[Robert P. J. Day]

On Sat, 9 Oct 2004, Wolfgang Denk wrote:

> In message <Pine.LNX.4.60.0410081643100.25430@dell.enoriver.com> you wrote:
>>
>> nice idea, but sadly, doomed to failure.  note that the code to copy
>> the patch arrays *requires* you to be able to take the sizeof() those
>> arrays to know *exactly* how much to copy.  if you pass the array to a
>> function by its address, i'm pretty sure you lose the ability to take
>> its sizeof() anymore, isn't that right?
>
> This is not right. Guess how memcpy() works - it's a function, btw.

sure.  and it accepts a size parameter, which is why it works.  i'm 
not sure what you're getting at.

rday

Re: next pass of cleaning up micropatch.c

[Wolfgang Denk]

In message <Pine.LNX.4.60.0410081908410.3331@localhost.localdomain> you wrote:
> 
> >> nice idea, but sadly, doomed to failure.  note that the code to copy
> >> the patch arrays *requires* you to be able to take the sizeof() those
...
> > This is not right. Guess how memcpy() works - it's a function, btw.
> 
> sure.  and it accepts a size parameter, which is why it works.  i'm 
> not sure what you're getting at.

I'm getting at your "doomed to failure" argument.

I'm surprised that you didn't think about the possibility to pass the
patch length as an argument to the function, that's all.  This  seems
all  too  obvious  to  me.  (If  you like you can this make an inline
function - then the code will be more or less identical  to  what  we
have now [which does not make the code a bit better, btw.])

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 only person who always got his work done by Friday
                                                 was Robinson Crusoe.

Re: next pass of cleaning up micropatch.c

[Robert P. J. Day]

On Sat, 9 Oct 2004, Wolfgang Denk wrote:

> I'm surprised that you didn't think about the possibility to pass the
> patch length as an argument to the function, that's all.  This  seems
> all  too  obvious  to  me.

oh, sure, if you know the length, it's easy.  and who wants to count 
those bytes?  not me.  that's why i long ago dropped the idea of 
writing a function.

personally, i still like the macro solution.  it seems to be 
appropriate for a 2-line loop, but i'm not going to start that
discussion again.  time to move on.

rday

Re: next pass of cleaning up micropatch.c

[Robert P. J. Day]

On Sat, 9 Oct 2004, Wolfgang Denk wrote:

> In message <Pine.LNX.4.60.0410081930510.3440@localhost.localdomain> you wrote:
>>
>> oh, sure, if you know the length, it's easy.  and who wants to count
>> those bytes?  not me.  that's why i long ago dropped the idea of
>> writing a function.
>
> You mean it is more difficult to use "sizeof(foo)" as a parameter  to
> a function than inside one of your macros?
>
> Wow. You see me surprised.

ok.  point taken.

rday

Re: next pass of cleaning up micropatch.c

[Wolfgang Denk]

In message <Pine.LNX.4.60.0410081930510.3440@localhost.localdomain> you wrote:
> 
> oh, sure, if you know the length, it's easy.  and who wants to count 
> those bytes?  not me.  that's why i long ago dropped the idea of 
> writing a function.

You mean it is more difficult to use "sizeof(foo)" as a parameter  to
a function than inside one of your macros?

Wow. You see me surprised.


I give up here.


Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd@denx.de
Of all the things I've lost, I miss my mind the most.