[U-Boot-Users] [PATCH] mpc85xx: make the MxMR register in upmconfig as a parameter

[U-Boot-Users] [PATCH] mpc85xx: make the MxMR register in upmconfig as a parameter

[Sebastian Siewior]

The default value for the MxMR register is not always the right one.
This patch adds the value of MxMR register as an additional
parameter (plus a few defines instead of hex coded values).

Signed-off-by: Sebastian Siewior <bigeasy@linutronix.de>
---
 cpu/mpc85xx/cpu.c |   17 +++++++++++------
 include/common.h  |    4 ++++
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/cpu/mpc85xx/cpu.c b/cpu/mpc85xx/cpu.c
index 2373b4a..081e804 100644
--- a/cpu/mpc85xx/cpu.c
+++ b/cpu/mpc85xx/cpu.c
@@ -314,16 +314,21 @@ int dma_xfer(void *dest, uint count, void *src) {
 	return dma_check();
 }
 #endif
+
+#define MXMR_OP_NORMAL	(0x00000000)
+#define MXMR_OP_WRITE	(0x10000000)
+#define MXMR_OP_READ	(0x20000000)
+#define MXMR_OP_RUN	(0x30000000)
+
 /*
- * Configures a UPM. Currently, the loop fields in MxMR (RLF, WLF and TLF)
- * are hardcoded as "1"."size" is the number or entries, not a sizeof.
+ * Configures a UPM. The MxMR mode is the fourth parameter.
+ * "size" is the number or entries, not a sizeof.
  */
-void upmconfig (uint upm, uint * table, uint size)
+void upmconfig(uint upm, uint *table, uint size, unsigned int mxmr_mode)
 {
 	int i, mdr, mad, old_mad = 0;
 	volatile u32 *mxmr;
 	volatile ccsr_lbc_t *lbc = (void *)(CFG_MPC85xx_LBC_ADDR);
-	int loopval = 0x00004440;
 	volatile u32 *brp,*orp;
 	volatile u8* dummy = NULL;
 	int upmmask;
@@ -364,7 +369,7 @@ void upmconfig (uint upm, uint * table, uint size)
 
 	for (i = 0; i < size; i++) {
 		/* 1 */
-		out_be32(mxmr, loopval | 0x10000000 | i); /* OP_WRITE */
+		out_be32(mxmr, mxmr_mode | MXMR_OP_WRITE | i);
 		/* 2 */
 		out_be32(&lbc->mdr, table[i]);
 		/* 3 */
@@ -377,5 +382,5 @@ void upmconfig (uint upm, uint * table, uint size)
 		} while (mad <= old_mad && !(!mad && i == (size-1)));
 		old_mad = mad;
 	}
-	out_be32(mxmr, loopval); /* OP_NORMAL */
+	out_be32(mxmr, mxmr_mode | MXMR_OP_NORMAL);
 }
diff --git a/include/common.h b/include/common.h
index 10b997e..0c0ef5b 100644
--- a/include/common.h
+++ b/include/common.h
@@ -441,7 +441,11 @@ void		ppcDWstore(unsigned int *addr, unsigned int *value);
 int	checkcpu      (void);
 int	checkicache   (void);
 int	checkdcache   (void);
+#ifdef CONFIG_MPC85xx
+void	upmconfig     (unsigned int, unsigned int *, unsigned int, unsigned int);
+#else
 void	upmconfig     (unsigned int, unsigned int *, unsigned int);
+#endif
 ulong	get_tbclk     (void);
 void	reset_cpu     (ulong addr);
 #if defined (CONFIG_OF_LIBFDT) && defined (CONFIG_OF_BOARD_SETUP)
-- 
1.5.5.2

[U-Boot-Users] [PATCH] mpc85xx: make the MxMR register in upmconfig as a parameter

[Andy Fleming]

On Mon, Jul 14, 2008 at 5:54 AM, Sebastian Siewior
<bigeasy@linutronix.de> wrote:
> The default value for the MxMR register is not always the right one.
> This patch adds the value of MxMR register as an additional
> parameter (plus a few defines instead of hex coded values).
>
> Signed-off-by: Sebastian Siewior <bigeasy@linutronix.de>


I'm not convinced this is the right solution.  Anytime we put a
cpu-specific #ifdef for a function definition, we should think long
and hard about why.  Maybe instead of an argument, we should make
mxmr_mode a config value.  Also, unless I'm misreading this patch,
you've broken *every* board with this patch, since there's no change
to any of the invokers of upmconfig to supply the fourth argument.

Could you also explain in greater detail what you are trying to do,
here?  My familiarity with the LBC code is fairly low.

Andy

[U-Boot-Users] [PATCH] mpc85xx: make the MxMR register in upmconfig as a parameter

[Sebastian Siewior]

* Andy Fleming | 2008-07-14 19:27:08 [-0500]:

>On Mon, Jul 14, 2008 at 5:54 AM, Sebastian Siewior
><bigeasy@linutronix.de> wrote:
>> The default value for the MxMR register is not always the right one.
>> This patch adds the value of MxMR register as an additional
>> parameter (plus a few defines instead of hex coded values).
>>
>> Signed-off-by: Sebastian Siewior <bigeasy@linutronix.de>
>
>
>I'm not convinced this is the right solution.  Anytime we put a
>cpu-specific #ifdef for a function definition, we should think long
>and hard about why.  Maybe instead of an argument, we should make
>mxmr_mode a config value.  Also, unless I'm misreading this patch,
>you've broken *every* board with this patch, since there's no change
>to any of the invokers of upmconfig to supply the fourth argument.
Other sollutions are fine with me :) I did not change any board specific
code, because I did not find any users (with 85xx cpus). Still possible
that I missed some....
Another sollution might be to add upconfig_mxmr() and let upmconfig call
that function with the default value however this sounds nasty.

>Could you also explain in greater detail what you are trying to do,
>here?  My familiarity with the LBC code is fairly low.
The MxMR register specifies the mode in which the UPM is used. I have to
program two UPMs with different content of this register: in one case I
have to set UWPL & GPL4 bits in the other case the default value would
do).

If you prefer a define for this register the way we deal with BRx/ORx
than I could send a patch that does this. What should be done in case
the user is going to program UPMA but did not specify MAMR? The default
value or build error? Since this value as well as the UPM tables depend
very much on the hardware, the user should been told by his hardware
guys what to do :)

>Andy
Sebastian

[U-Boot-Users] [PATCH] mpc85xx: make the MxMR register in upmconfig as a parameter

[Wolfgang Grandegger]

Sebastian Siewior wrote:
> * Andy Fleming | 2008-07-14 19:27:08 [-0500]:
> 
>> On Mon, Jul 14, 2008 at 5:54 AM, Sebastian Siewior
>> <bigeasy@linutronix.de> wrote:
>>> The default value for the MxMR register is not always the right one.
>>> This patch adds the value of MxMR register as an additional
>>> parameter (plus a few defines instead of hex coded values).
>>>
>>> Signed-off-by: Sebastian Siewior <bigeasy@linutronix.de>
>>
>> I'm not convinced this is the right solution.  Anytime we put a
>> cpu-specific #ifdef for a function definition, we should think long
>> and hard about why.  Maybe instead of an argument, we should make
>> mxmr_mode a config value.  Also, unless I'm misreading this patch,
>> you've broken *every* board with this patch, since there's no change
>> to any of the invokers of upmconfig to supply the fourth argument.
> Other sollutions are fine with me :) I did not change any board specific
> code, because I did not find any users (with 85xx cpus). Still possible
> that I missed some....

upmconfig was introduced with the socrates board but our internal 
version now also uses a configurable MXMR value. The TQM85xx boards and 
likely more should be converted as well.

> Another sollution might be to add upconfig_mxmr() and let upmconfig call
> that function with the default value however this sounds nasty.
> 
>> Could you also explain in greater detail what you are trying to do,
>> here?  My familiarity with the LBC code is fairly low.
> The MxMR register specifies the mode in which the UPM is used. I have to
> program two UPMs with different content of this register: in one case I
> have to set UWPL & GPL4 bits in the other case the default value would
> do).
> 
> If you prefer a define for this register the way we deal with BRx/ORx
> than I could send a patch that does this. What should be done in case
> the user is going to program UPMA but did not specify MAMR? The default
> value or build error? Since this value as well as the UPM tables depend
> very much on the hardware, the user should been told by his hardware
> guys what to do :)

I think upmconfig() should only touch the relevant bits of the MxMR 
register. For the TQM8548 NAND support (see board/tqc/tqm85xx/nand.c) I 
implemented it as Linux does:

         clrsetbits_be32(&lbc->mbmr, MxMR_MAD_MSK,
                         MxMR_OP_WARR | (addr & MxMR_MAD_MSK));

         out_be32 (&lbc->mdr, val);

         /* dummy access to perform write */
         out_8 ((void __iomem *)CFG_NAND0_BASE, 0);

         clrbits_be32(&lbc->mbmr, MxMR_OP_WARR);


I could test the new patch on the TQM8548 and Socrates board.

BTW: upmconfig() should also work as is on the MPC83xx and MPC86xx, IIRC 
but it's not obvious where to put the code if we go for a more generic 
FSL UPM solution.

Wolfgang.

[U-Boot-Users] [PATCH] mpc85xx: make the MxMR register in upmconfig as a parameter

[Sebastian Siewior]

* Wolfgang Grandegger | 2008-07-15 13:28:40 [+0200]:

>Sebastian Siewior wrote:
>>* Andy Fleming | 2008-07-14 19:27:08 [-0500]:
>>
>>>On Mon, Jul 14, 2008 at 5:54 AM, Sebastian Siewior
>>><bigeasy@linutronix.de> wrote:
>>>>The default value for the MxMR register is not always the right one.
>>>>This patch adds the value of MxMR register as an additional
>>>>parameter (plus a few defines instead of hex coded values).
>>>>
>>>>Signed-off-by: Sebastian Siewior <bigeasy@linutronix.de>
>>>
>>>I'm not convinced this is the right solution.  Anytime we put a
>>>cpu-specific #ifdef for a function definition, we should think long
>>>and hard about why.  Maybe instead of an argument, we should make
>>>mxmr_mode a config value.  Also, unless I'm misreading this patch,
>>>you've broken *every* board with this patch, since there's no change
>>>to any of the invokers of upmconfig to supply the fourth argument.
>>Other sollutions are fine with me :) I did not change any board specific
>>code, because I did not find any users (with 85xx cpus). Still possible
>>that I missed some....
>
>upmconfig was introduced with the socrates board but our internal 
>version now also uses a configurable MXMR value. The TQM85xx boards and 
>likely more should be converted as well.

Socrates uses upmconfig, TQM85xx uses its own implementation. Why
TQM85xx boards? There is only one, isn't it?

>>If you prefer a define for this register the way we deal with BRx/ORx
>>than I could send a patch that does this. What should be done in case
>>the user is going to program UPMA but did not specify MAMR? The default
>>value or build error? Since this value as well as the UPM tables depend
>>very much on the hardware, the user should been told by his hardware
>>guys what to do :)
>
>I think upmconfig() should only touch the relevant bits of the MxMR 
>register. For the TQM8548 NAND support (see board/tqc/tqm85xx/nand.c) I 
>implemented it as Linux does:
>
>        clrsetbits_be32(&lbc->mbmr, MxMR_MAD_MSK,
>                        MxMR_OP_WARR | (addr & MxMR_MAD_MSK));
>
>        out_be32 (&lbc->mdr, val);
>
>        /* dummy access to perform write */
>        out_8 ((void __iomem *)CFG_NAND0_BASE, 0);
>
>        clrbits_be32(&lbc->mbmr, MxMR_OP_WARR);
>
Okey, so you prefer to clear only MAD bits and MODE bits and leave
everythign else untouched. And where should de define them? In
cpu/mpc85xx/cpu_init.c right after the OR/BR settings?

Why does Linux program UPMs? Isn't this one-time-setup?

>Wolfgang.

Sebastian

[U-Boot-Users] [PATCH] mpc85xx: make the MxMR register in upmconfig as a parameter

[Wolfgang Grandegger]

Sebastian Siewior wrote:
> * Wolfgang Grandegger | 2008-07-15 13:28:40 [+0200]:
> 
>> Sebastian Siewior wrote:
>>> * Andy Fleming | 2008-07-14 19:27:08 [-0500]:
>>>
>>>> On Mon, Jul 14, 2008 at 5:54 AM, Sebastian Siewior
>>>> <bigeasy@linutronix.de> wrote:
>>>>> The default value for the MxMR register is not always the right one.
>>>>> This patch adds the value of MxMR register as an additional
>>>>> parameter (plus a few defines instead of hex coded values).
>>>>>
>>>>> Signed-off-by: Sebastian Siewior <bigeasy@linutronix.de>
>>>> I'm not convinced this is the right solution.  Anytime we put a
>>>> cpu-specific #ifdef for a function definition, we should think long
>>>> and hard about why.  Maybe instead of an argument, we should make
>>>> mxmr_mode a config value.  Also, unless I'm misreading this patch,
>>>> you've broken *every* board with this patch, since there's no change
>>>> to any of the invokers of upmconfig to supply the fourth argument.
>>> Other sollutions are fine with me :) I did not change any board specific
>>> code, because I did not find any users (with 85xx cpus). Still possible
>>> that I missed some....
>> upmconfig was introduced with the socrates board but our internal 
>> version now also uses a configurable MXMR value. The TQM85xx boards and 
>> likely more should be converted as well.
> 
> Socrates uses upmconfig, TQM85xx uses its own implementation. Why
> TQM85xx boards? There is only one, isn't it?

The port for Socrates and TQM8548 have been developped concarently and I 
  was therefore not aware of an upmconfig for MPC85xx. TQM85xx modules 
use UPMC for CAN and the TQM8548 uses UPMB for NAND.

>>> If you prefer a define for this register the way we deal with BRx/ORx
>>> than I could send a patch that does this. What should be done in case
>>> the user is going to program UPMA but did not specify MAMR? The default
>>> value or build error? Since this value as well as the UPM tables depend
>>> very much on the hardware, the user should been told by his hardware
>>> guys what to do :)
>> I think upmconfig() should only touch the relevant bits of the MxMR 
>> register. For the TQM8548 NAND support (see board/tqc/tqm85xx/nand.c) I 
>> implemented it as Linux does:
>>
>>        clrsetbits_be32(&lbc->mbmr, MxMR_MAD_MSK,
>>                        MxMR_OP_WARR | (addr & MxMR_MAD_MSK));
>>
>>        out_be32 (&lbc->mdr, val);
>>
>>        /* dummy access to perform write */
>>        out_8 ((void __iomem *)CFG_NAND0_BASE, 0);
>>
>>        clrbits_be32(&lbc->mbmr, MxMR_OP_WARR);
>>
> Okey, so you prefer to clear only MAD bits and MODE bits and leave
> everythign else untouched. And where should de define them? In
> cpu/mpc85xx/cpu_init.c right after the OR/BR settings?

For the time being, I prefer to keep upmconfig() compatible with the 
others similar implementation:

   $ find . -type f | xargs grep upmconfig
   ./mpc83xx/cpu.c:void upmconfig (uint upm, uint *table, uint size)
   ./mpc8260/cpu.c:void upmconfig (uint upm, uint * table, uint size)
   ./mpc8xx/cpu.c:void upmconfig (uint upm, uint * table, uint size)
   ./mpc85xx/cpu.c:void upmconfig (uint upm, uint * table, uint size)

mxmr should be defined in the platform specific code, as currently all 
boards using upmconfig() do, if necessary. If we unify them, we have 
more choices.

 > Why does Linux program UPMs? Isn't this one-time-setup?

See http://lxr.linux.no/linux+v2.6.26/drivers/mtd/nand/fsl_upm.c. A 
similar implementation exists for U-Boot in drivers/mtd/nand/fsl_upm.c,
which is used by the TQM8548, for example.

Wolfgang.

[U-Boot-Users] [PATCH] mpc85xx: make the MxMR register in upmconfig as a parameter

[Wolfgang Grandegger]

Sebastian Siewior wrote:
> The default value for the MxMR register is not always the right one.
> This patch adds the value of MxMR register as an additional
> parameter (plus a few defines instead of hex coded values).
> 
> Signed-off-by: Sebastian Siewior <bigeasy@linutronix.de>
> ---
>  cpu/mpc85xx/cpu.c |   17 +++++++++++------
>  include/common.h  |    4 ++++
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/cpu/mpc85xx/cpu.c b/cpu/mpc85xx/cpu.c
> index 2373b4a..081e804 100644
> --- a/cpu/mpc85xx/cpu.c
> +++ b/cpu/mpc85xx/cpu.c
> @@ -314,16 +314,21 @@ int dma_xfer(void *dest, uint count, void *src) {
>  	return dma_check();
>  }
>  #endif
> +
> +#define MXMR_OP_NORMAL	(0x00000000)
> +#define MXMR_OP_WRITE	(0x10000000)
> +#define MXMR_OP_READ	(0x20000000)
> +#define MXMR_OP_RUN	(0x30000000)

Please use the the corresponding definitions from 
"include/asm-ppc/fsl_lbc.h".

[U-Boot-Users] [PATCH] mpc85xx: make the MxMR register in upmconfig as a parameter

[Sebastian Siewior]

Wolfgang Grandegger wrote:
>> +#define MXMR_OP_NORMAL    (0x00000000)
>> +#define MXMR_OP_WRITE    (0x10000000)
>> +#define MXMR_OP_READ    (0x20000000)
>> +#define MXMR_OP_RUN    (0x30000000)
> 
> Please use the the corresponding definitions from 
> "include/asm-ppc/fsl_lbc.h".
Ach okey. I do this with the new patch once Andy agreed how to pass the 
MxMR value.

Sebastian