[U-Boot-Users] [PATCH] SPI relocation fix
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];
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
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
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
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
Vladimir's patch works fine on my MPC852T.
SPI and SMC1 functional.
However I do not use I2C and SMC2.
Regards, David
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
participants (4)
-
David Ho -
David Ho -
Vladimir Gurevich -
Wolfgang Denk