[U-Boot-Users] [PATCH] SPI relocation fix

[U-Boot-Users] [PATCH] SPI relocation fix

[David Ho]

Signed-off-by: David Ho <davidho@nanometrics.ca>

---

 cpu/mpc8xx/spi.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

3f60ba612e16635f35bf00e91fa821d36f294534
diff --git a/cpu/mpc8xx/spi.c b/cpu/mpc8xx/spi.c
index e318ed0..cb3469b 100644
--- a/cpu/mpc8xx/spi.c
+++ b/cpu/mpc8xx/spi.c
@@ -153,8 +153,11 @@ void spi_init_f (void)
 	spi  = (spi_t *)&cp->cp_dpmem[spi->spi_rpbase];
 #else
 	spi  = (spi_t *)&cp->cp_dparam[PROFF_SPI];
-	/* Disable relocation */
-	spi->spi_rpbase = 0;
+
+	if (!mpc8xx_new_core()) {
+		/* Disable relocation */
+		spi->spi_rpbase = 0;
+	}
 #endif
 
 /* 1 */
@@ -306,8 +309,11 @@ void spi_init_r (void)
 	spi  = (spi_t *)&cp->cp_dpmem[spi->spi_rpbase];
 #else
 	spi  = (spi_t *)&cp->cp_dparam[PROFF_SPI];
-	/* Disable relocation */
-	spi->spi_rpbase = 0;
+
+	if (!mpc8xx_new_core()) {
+		/* Disable relocation */
+		spi->spi_rpbase = 0;
+	}
 #endif
 
 	/* tx and rx buffer descriptors */
@@ -399,8 +405,11 @@ ssize_t spi_xfer (size_t count)
 	spi  = (spi_t *)&cp->cp_dpmem[spi->spi_rpbase];
 #else
 	spi  = (spi_t *)&cp->cp_dparam[PROFF_SPI];
-	/* Disable relocation */
-	spi->spi_rpbase = 0;
+
+	if (!mpc8xx_new_core()) {
+		/* Disable relocation */
+		spi->spi_rpbase = 0;
+	}
 #endif
 
 	tbdf = (cbd_t *) & cp->cp_dpmem[spi->spi_tbase];
-- 
1.2.4.g79f5-dirty

[U-Boot-Users] [PATCH] SPI relocation fix

[Wolfgang Denk]

In message <11437528271561-git-send-email-davidho@nanometrics.ca> you wrote:
> Signed-off-by: David Ho <davidho@nanometrics.ca>

No CHANGELOG entry. No description what you're doing and why.

> +	if (!mpc8xx_new_core()) {
> +		/* Disable relocation */
> +		spi->spi_rpbase = 0;
> +	}

Also, I don't like the name "mpc8xx_new_core". What's  "new"?  It  is
not  exactly new by today, and will definitely not be new any more in
a year from now.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Obviously, a major malfunction has occurred."
              -- Steve Nesbitt, voice of Mission Control, January 28,
                 1986, as the shuttle Challenger exploded within view
                 of the grandstands.

[U-Boot-Users] [PATCH] SPI relocation fix

[Vladimir Gurevich]

Hi Wolfgang,

Wolfgang Denk wrote:

>Also, I don't like the name "mpc8xx_new_core". What's  "new"?  It  is
>not  exactly new by today, and will definitely not be new any more in
>a year from now.
>  
>
This problem is supposed to be solved in a clean and generic way by the 
patch I submitted almost 2 moths ago.

Please, please, please! Take a look  (and there were two more). :)

Happy hacking,
Vladimir

[U-Boot-Users] [PATCH] SPI relocation fix

[David Ho]

Hi Wolfgang,

> This problem is supposed to be solved in a clean and generic way by 
> the patch I submitted almost 2 moths ago.

As you can see there are a number of people who requires this fix for 
the 866 family.
I really suggest this feature be expedited so others can benefit from a 
common implementation.
I am willing to test out Vladimir's patch on my board.

Something has to be done to continue progress for the 8xx family even if 
you are not actively working on it.  Please provide suggestions to help 
with this.

David

[U-Boot-Users] [PATCH] SPI relocation fix

[Wolfgang Denk]

Dear David,

in message <442D4947.8090304@nanometrics.ca> you wrote:
> 
> I am willing to test out Vladimir's patch on my board.

Please feel free to go on and test it. I'm looking  forward  to  hear
your test report. Please test it on older 8xx H/W, too.

Best regards,

Wolfgang Denk

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

[U-Boot-Users] [PATCH] SPI relocation fix

[David Ho]

Vladimir's patch works fine on my MPC852T.

SPI and SMC1 functional.

However I do not use I2C and SMC2.

Regards,
David

[U-Boot-Users] [PATCH] SPI relocation fix

[Vladimir Gurevich]

Hello David,

David Ho wrote:

>Vladimir's patch works fine on my MPC852T.
>
>SPI and SMC1 functional.
>
>However I do not use I2C and SMC2.
>  
>
Thanks a lot for the testing (and for the good news).

Can you tell me which mode did you actually test? For example, have you 
tried to relocate SPI or SMC1 parameter areas or just left them where 
they were? (both modes are broken in the current U-boot anyway).

Actually, the main CPU I used was MPC852T as well, so I am am pretty 
confident that any combination should work there provided that you 
configured it according to the doc.

Do you happen to have any other 8xx CPUs to test with?

Happy hacking,
Vladimir

[U-Boot-Users] [PATCH] SPI relocation fix

[David Ho]

Hi Vladimir,

> Do you happen to have any other 8xx CPUs to test with?

I had an EP860 board to play with before I moved on to our custom
MPC852T.  However after a few server changes I have lost my u-boot
port to the board.

I wouldn't mind giving a go on my EP860 if someone has a u-boot port
handy for that.

Thanks, David

[U-Boot-Users] [PATCH] SPI relocation fix

[David Ho]

Hi Vladimir,

This is required to relocate SPI on the MPC852T.  Presumably, similar
fix is required for relocating I2C.

Regards,
David

On 3/31/06, Vladimir Gurevich <vag@paulidav.org> wrote:
> Hello David,
>
> David Ho wrote:
>
> >Vladimir's patch works fine on my MPC852T.
> >
> >SPI and SMC1 functional.
> >
> >However I do not use I2C and SMC2.
> >
> >
> Thanks a lot for the testing (and for the good news).
>
> Can you tell me which mode did you actually test? For example, have you
> tried to relocate SPI or SMC1 parameter areas or just left them where
> they were? (both modes are broken in the current U-boot anyway).
>
> Actually, the main CPU I used was MPC852T as well, so I am am pretty
> confident that any combination should work there provided that you
> configured it according to the doc.
>
> Do you happen to have any other 8xx CPUs to test with?
>
> Happy hacking,
> Vladimir
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: spi-reloc.patch
Type: application/octet-stream
Size: 839 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20060403/7249a833/attachment.obj 

[U-Boot-Users] [PATCH] SPI relocation fix

[Vladimir Gurevich]

Hello David,

David Ho wrote:

>This is required to relocate SPI on the MPC852T.  Presumably, similar
>fix is required for relocating I2C.
>  
>
I assume you've applied my patch, correct?

Normally, at the powerup  rpbase should be set to the default values, so 
no programming is required. Otherwise, the microcode patch downloading 
code is programming them correctly.

Are you sure you really need these? Could it be that you somehow hve 
overwritten the parameter area? What are the values in rpbase if you do 
not set them explicitly?

As I said, I do use MPC852T and everything works fine.

Happy hacking,
Vladimir

[U-Boot-Users] [PATCH] SPI relocation fix

[David Ho]

> Normally, at the powerup  rpbase should be set to the default values, so
> no programming is required.

Yes, this is true if you are using the default SPI PRAM location.

> Otherwise, the microcode patch downloading
> code is programming them correctly.

MPC852T skips the microcode patch does it not?

> Are you sure you really need these? Could it be that you somehow hve
> overwritten the parameter area? What are the values in rpbase if you do
> not set them explicitly?
>

For the case where you intentionally want to move SPI to elsewhere
(using ENET on SCC2), I will have to define CFG_SPI_DPMEM_OFFSET. 
since CFG_SPI_DPMEM_OFFSET is not assigned to spi->rpbase for MPC852T
( microcode patch is skipped), SPI is not relocated using your patch.

In my case I tested relocating SPI to 0x1800.

You can try that on your MPC852T.

Regards, David

[U-Boot-Users] [PATCH] SPI relocation fix

[Vladimir Gurevich]

Hello David,

David Ho wrote:

>MPC852T skips the microcode patch does it not?
>  
>
Well, it depends on the actual combination of the parameters, but the 
idea was that cpm_load_patch() will be called nonetheless and will 
program the rpbase whether or not the actual patch is required.

>For the case where you intentionally want to move SPI to elsewhere
>(using ENET on SCC2), I will have to define CFG_SPI_DPMEM_OFFSET. 
>since CFG_SPI_DPMEM_OFFSET is not assigned to spi->rpbase for MPC852T
>( microcode patch is skipped), SPI is not relocated using your patch.
>  
>
That's exactly the case I describe above. Could you make sure 
cpm_load_patch() is being called?

Happy hacking,
Vladimir

[U-Boot-Users] [PATCH] SPI relocation fix

[David Ho]

> Well, it depends on the actual combination of the parameters, but the
> idea was that cpm_load_patch() will be called nonetheless and will
> program the rpbase whether or not the actual patch is required.
>
> >For the case where you intentionally want to move SPI to elsewhere
> >(using ENET on SCC2), I will have to define CFG_SPI_DPMEM_OFFSET.
> >since CFG_SPI_DPMEM_OFFSET is not assigned to spi->rpbase for MPC852T
> >( microcode patch is skipped), SPI is not relocated using your patch.
> >
> >
> That's exactly the case I describe above. Could you make sure
> cpm_load_patch() is being called?

Already did

cpu/mpc8xx/cpu_init.c

#if defined(CFG_MPC8XX_RELOCATION_UPATCH)
        cpm_load_patch (immr);  /* load mpc8xx  microcode patch */
#endif

The condition for which this is defined as follows, which is not
behaving according to your idea.

if defined(CFG_MPC850_SMC_UPATCH)     || \
    defined(CFG_MPC850_I2C_SPI_UPATCH) || \
    defined(CFG_MPC850_I2C_SMC_UPATCH) || \
    defined(CFG_MPC850_SPI_SMC_UPATCH) || \
    defined(CFG_MPC860_SMC_UPATCH)     || \
    defined(CFG_MPC860_I2C_SPI_UPATCH) || \
    defined(CFG_MPC860_I2C_SMC_UPATCH) || \
    defined(CFG_MPC860_SPI_SMC_UPATCH)

#  define CFG_MPC8XX_RELOCATION_UPATCH   1
#endif

[U-Boot-Users] [PATCH] SPI relocation fix

[Vladimir Gurevich]

Hello David,

David Ho wrote:

>cpu/mpc8xx/cpu_init.c
>
>#if defined(CFG_MPC8XX_RELOCATION_UPATCH)
>        cpm_load_patch (immr);  /* load mpc8xx  microcode patch */
>#endif
>  
>
OK, so let's fix that instead. I think my original intention was to call 
this function unconditionally, because it s designed to touch only those 
registers that are needed for a specific mode. For example in your case, 
it will only program rpbase without trying to load any microcode.

Thanks for pointing it out.

Happy hacking,
Vladimir

[U-Boot-Users] [PATCH] SPI relocation fix

[David Ho]

 >
> OK, so let's fix that instead. I think my original intention was to call
> this function unconditionally, because it s designed to touch only those
> registers that are needed for a specific mode. For example in your case,
> it will only program rpbase without trying to load any microcode.
>
> Thanks for pointing it out.

One thing I want to point out is that you probably don't want to copy
microcode to DPRAM for the MPC866 family, which is always executed in
cpm_load_patch.

David

[U-Boot-Users] [PATCH] SPI relocation fix

[Vladimir Gurevich]

Hello David,

David Ho wrote:

>One thing I want to point out is that you probably don't want to copy
>microcode to DPRAM for the MPC866 family, which is always executed in
>cpm_load_patch.
>  
>
But the body of UcodeCopy will be empty in this case, wouldn't it?

Happy hacking,
Vladimir

[U-Boot-Users] [PATCH] SPI relocation fix

[David Ho]

On 4/3/06, Vladimir Gurevich <vag@paulidav.org> wrote:
> But the body of UcodeCopy will be empty in this case, wouldn't it?

Indeed, good point.

David

[U-Boot-Users] [PATCH] SPI relocation fix

[Wolfgang Denk]

In message <44317CBD.7030802@paulidav.org> you wrote:
> 
> But the body of UcodeCopy will be empty in this case, wouldn't it?

Please don't call empty functions if you can avoid it. It adds to the
code size, and makes understanding the code much harder.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The first 90% of a project takes 90% of the time, the last 10%  takes
the other 90% of the time.

[U-Boot-Users] [PATCH] SPI relocation fix

[Vladimir Gurevich]

Hello Wolfgang,

Wolfgang Denk wrote:

>Please don't call empty functions if you can avoid it. It adds to the
>code size, and makes understanding the code much harder.
>  
>
No problem. I can easily #ifdef the call to UcodeCopy rather than the 
body itself, since it's predicated on the need for the patch. But 
cpm_load_patch() should still has to be called unconditionally and if no 
relocation is required at all, it will be empty. This is a small price 
to pay for the ability to handle different cases automatically without 
the need to worry about whether a microcode patch is actually needed 
(because depending on the CPU and actual settings there might be no 
relocation or relocation with or without a patch).

I believe that we need some centralized code, because certain 
combination of patches/devices do affect other devices (that's the 
nature of microcode patches provided my Motorola) that might not really 
be used by a certain board, but still have to be programmed correctly. 
For example, the combined I2C/SPI/SMC relocation patch requires correct 
rpbase settings on both SMC1 and SMC2 regardless of whether you plan to 
use them or not.

Unfortunately, all these dependancies are a little difficult to express 
via C preprocessor (at least not without making the configuration much 
more complex and error prone). You can see that  the code in 
mpc8xx_upatch.h got pretty complex on its own.

Regards,
Vladimir

[U-Boot-Users] [PATCH] SPI relocation fix

[Wolfgang Denk]

Dear Vladimir,

in message <44318AB9.2070209@paulidav.org> you wrote:
> 
> No problem. I can easily #ifdef the call to UcodeCopy rather than the 
> body itself, since it's predicated on the need for the patch. But 
> cpm_load_patch() should still has to be called unconditionally and if no 

Ummm... why would I want to call this function if I don't need it?

> relocation is required at all, it will be empty. This is a small price 
> to pay for the ability to handle different cases automatically without 
> the need to worry about whether a microcode patch is actually needed 
> (because depending on the CPU and actual settings there might be no 
> relocation or relocation with or without a patch).

Ummm... The function name "CPM load patch" suggests that it  is  used
to load a microcode patch. 

> I believe that we need some centralized code, because certain 
> combination of patches/devices do affect other devices (that's the 
> nature of microcode patches provided my Motorola) that might not really 
> be used by a certain board, but still have to be programmed correctly. 
> For example, the combined I2C/SPI/SMC relocation patch requires correct 
> rpbase settings on both SMC1 and SMC2 regardless of whether you plan to 
> use them or not.
> 
> Unfortunately, all these dependancies are a little difficult to express 
> via C preprocessor (at least not without making the configuration much 
> more complex and error prone). You can see that  the code in 
> mpc8xx_upatch.h got pretty complex on its own.

Then let's at least rename the function to have a name that says what
it's supposed to do.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The universe is all a spin-off of the Big Bang.

[U-Boot-Users] [PATCH] SPI relocation fix

[Vladimir Gurevich]

Hello Wolfgang,

Wolfgang Denk wrote:

>Then let's at least rename the function to have a name that says what
>it's supposed to do.
>  
>
Sounds reasonable. How about cpm_parameter_relocate() ?

Do you want me to re-submit the whole thing then?

Regards,
Vladimir

[U-Boot-Users] [PATCH] SPI relocation fix

[Wolfgang Denk]

In message <443168CF.9030908@paulidav.org> you wrote:
> 
> >cpu/mpc8xx/cpu_init.c
> >
> >#if defined(CFG_MPC8XX_RELOCATION_UPATCH)
> >        cpm_load_patch (immr);  /* load mpc8xx  microcode patch */
> >#endif
> >  
> >
> OK, so let's fix that instead. I think my original intention was to call 
> this function unconditionally, because it s designed to touch only those 

Please be careful. Do not add to the code  size  of  existing  boards
that don't need all this.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Many aligators will be slain, but the swamp will remain.

[U-Boot-Users] [PATCH] SPI relocation fix

[Vladimir Gurevich]

Hello David,

If you like, I'd like you to take a look at the patch I submitted to the 
list about two months ago. It not only takes care of your problem but 
provides uniform support for various types of parameter area relocations 
across all existing MPC8xx CPUs.

I am attaching this patch here, since not all the archives keep the patches.

Happy hacking,
Vladimir

-------------- next part --------------
A non-text attachment was scrubbed...
Name: mpc8xx_upatch.diff.gz
Type: application/x-gzip
Size: 16241 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20060330/8d8518ed/attachment.bin