driver/ifc:Change accessor function to take care of endianness
Subsystems (3)
PATCH v2 1/1 driver/ifc:Change accessor function to take care of endianness
IFC registers can be of type Little Endian or big Endian depending upon Freescale SoC. Here SoC defines the register type of IFC IP.
So update acessor functions with common IFC acessor functions to take care both type of endianness.
---
Changes for v2:
- fix spelling mistakes
README | 6 + arch/powerpc/include/asm/config_mpc85xx.h | 3 + drivers/mtd/nand/fsl_ifc_nand.c | 243 +++++++++++++++-------------- drivers/mtd/nand/fsl_ifc_spl.c | 60 +++---- include/fsl_ifc.h | 42 +++-- 5 files changed, 190 insertions(+), 164 deletions(-)
README +6 -0
@@ -472,6 +472,12 @@ The following options need to be configured: Board config to use DDR3. It can be enabled for SoCs with Freescale DDR3 controllers. + CONFIG_SYS_FSL_IFC_BE + Defines the IFC controller register space as Big Endian + + CONFIG_SYS_FSL_IFC_LE + Defines the IFC controller register space as Little Endian + - Intel Monahans options: CONFIG_SYS_MONAHANS_RUN_MODE_OSC_RATIO
arch/powerpc/include/asm/config_mpc85xx.h +3 -0
@@ -22,6 +22,9 @@#define FSL_DDR_VER_4_7 47#define FSL_DDR_VER_5_0 50 +/* IP endianness */ +#define CONFIG_SYS_FSL_IFC_BE +/* Number of TLB CAM entries we have on FSL Book-E chips */ #if defined(CONFIG_E500MC )#define CONFIG_SYS_NUM_TLBCAMS 64
drivers/mtd/nand/fsl_ifc_nand.c +122 -121
421 lines not on this page · raw message
drivers/mtd/nand/fsl_ifc_spl.c +30 -30
100 lines not on this page · raw message
include/fsl_ifc.h +29 -13
55 lines not on this page · raw message
Signature · 1 line
1.7.9.5
PATCH v2 1/1 driver/ifc:Change accessor function to take care of endianness
Dear Prabhakar Kushwaha,
Quoted 2 lines · In message <[email protected]> you…
IFC registers can be of type Little Endian or big Endian depending upon Freescale SoC. Here SoC defines the register type of IFC IP.
As is, you are only adding dead code, as there is no place anywhere in the mainline code that defines CONFIG_SYS_FSL_IFC_LE
drivers/mtd/nand/fsl_ifc_nand.c static void set_addr(struct mtd_info *mtd, int column, int page_addr, int oob) · line 234
Quoted 5 lines
drivers/mtd/nand/fsl_ifc_nand.c +2
-2/* Program ROW0/COL0 */ -out_be32 (&ifc->ifc_nand.row0, page_addr); -out_be32 (&ifc->ifc_nand.col0, (oob ?IFC_NAND_COL_MS :0 ) | column); +ifc_out32 (&ifc->ifc_nand.row0, page_addr); +ifc_out32 (&ifc->ifc_nand.col0, (oob ?IFC_NAND_COL_MS :0 ) | column);
I seriously dislike the idea of introducing special I/O accessors for a single device driver. If more drivers would follow that example, we will soon have a serious mess.
If would probably be useful to understand what you are trying to do if we could see where you actually need this, so we can then try and figure out a better implementation.
In the current form this should not be added to mainline.
Best regards,
Wolfgang Denk
Signature · 4 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de "The whole world is about three drinks behind." - Humphrey Bogart
PATCH v2 1/1 driver/ifc:Change accessor function to take care of endianness
Quoted 8 lines · Wolfgang Denk
Dear Prabhakar Kushwaha,
Quoted 2 lines · In message <[email protected]> you…
IFC registers can be of type Little Endian or big Endian depending upon Freescale SoC. Here SoC defines the register type of IFC IP.
As is, you are only adding dead code, as there is no place anywhere in the mainline code that defines
CONFIG_SYS_FSL_IFC_LE
Yes, consider it RFC until we have patches for a target that needs LE.
Quoted 9 lines
drivers/mtd/nand/fsl_ifc_nand.c static void set_addr(struct mtd_info *mtd, int column, int page_addr, int oob) · line 234
Quoted 5 lines
drivers/mtd/nand/fsl_ifc_nand.c +2
-2/* Program ROW0/COL0 */ -out_be32 (&ifc->ifc_nand.row0, page_addr); -out_be32 (&ifc->ifc_nand.col0, (oob ?IFC_NAND_COL_MS :0 ) | column); +ifc_out32 (&ifc->ifc_nand.row0, page_addr); +ifc_out32 (&ifc->ifc_nand.col0, (oob ?IFC_NAND_COL_MS :0 ) | column);I seriously dislike the idea of introducing special I/O accessors for a single device driver. If more drivers would follow that example, we will soon have a serious mess.
As the changelog says, we have chips coming out on which these registers are little-endian, and thus we can't hardcode big-endian in the driver.
I don't know whether there's something more generic we could key off of than IFC, but in Linux (and maybe U-Boot) we'll want to make this driven by the device tree rather than at compile time (it's not as simple as PPC versus ARM), so getting the knowledge from something other than the IFC node would be awkward -- and it would be good to keep this code similar between U-Boot and Linux.
What sort of mess are you envisioning? This isn't implementing accessors from scratch; it's just a wrapper. It's local to IFC code.
-Scott
PATCH v2 1/1 driver/ifc:Change accessor function to take care of endianness
Dear Scott,
Quoted 5 lines · In message <[email protected]> you wrote:
Quoted 3 lines
As is, you are only adding dead code, as there is no place anywhere in the mainline code that defines
CONFIG_SYS_FSL_IFC_LEYes, consider it RFC until we have patches for a target that needs LE.
This shouldbe noted in the Subject: then!
Quoted 7 lines
Quoted 3 lines
I seriously dislike the idea of introducing special I/O accessors for a single device driver. If more drivers would follow that example, we will soon have a serious mess.
As the changelog says, we have chips coming out on which these registers are little-endian, and thus we can't hardcode big-endian in the driver.
I'm not discussing the need for a way to switch endianess, only the current implementation.
Quoted 2 lines
What sort of mess are you envisioning? This isn't implementing accessors from scratch; it's just a wrapper. It's local to IFC code.
I fear that more IP blocks will follow that have similar requirements, and if we implemnt similar wrappers for each of them separately, we will have a mess of hard to maintain code. For example, it will then be impossible to share common parts of code because driver A uses A_out32() and driver B uses B_out32().
We should rather try and find a generic solution where the same functions can be used by mulltiple drivers which have the same needs.
Best regards,
Wolfgang Denk
Signature · 5 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de An age is called Dark not because the light fails to shine, but because people refuse to see it. -- James Michener, "Space"
PATCH v2 1/1 driver/ifc:Change accessor function to take care of endianness
Dear Scott,
Quoted 9 lines · In message <[email protected]> I wrote:
I fear that more IP blocks will follow that have similar requirements, and if we implemnt similar wrappers for each of them separately, we will have a mess of hard to maintain code. For example, it will then be impossible to share common parts of code because driver A uses
A_out32()and driver B usesB_out32().We should rather try and find a generic solution where the same functions can be used by mulltiple drivers which have the same needs.
On second thought, I also think we should avoid solutions where the BE/LE test has to be done for each and every I/O accessor call again and again. We should rather do this just once, and for example set function pointers as needed (hoping that this driver will only be needed after relocation, so we have writable data segment).
Best regards,
Wolfgang Denk
Signature · 8 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de Mike's Law: For a lumber company employing two men and a cut-off saw, the marginal product of labor for any number of additional workers equals zero until the acquisition of another cut-off saw. Let's not even consider a chainsaw. - Mike Dennison [You could always schedule the saw, though - ed.]
PATCH v2 1/1 driver/ifc:Change accessor function to take care of endianness
Dear Wolfgang,
Quoted 10 lines · Wolfgang Denk
Dear Scott,
Quoted 7 lines · In message <[email protected]> I wrote:
I fear that more IP blocks will follow that have similar requirements, and if we implemnt similar wrappers for each of them separately, we will have a mess of hard to maintain code. For example, it will then be impossible to share common parts of code because driver A uses
A_out32()and driver B usesB_out32().
Quoted 8 lines
Quoted 2 lines
We should rather try and find a generic solution where the same functions can be used by mulltiple drivers which have the same needs.
On second thought, I also think we should avoid solutions where the BE/LE test has to be done for each and every I/O accessor call again and again. We should rather do this just once, and for example set function pointers as needed (hoping that this driver will only be needed after relocation, so we have writable data segment).
I like the idea of setting it just once, but I don't see how to implement it. A pointer is probably not the solution, because we do need some drivers before relocation.
York
PATCH v2 1/1 driver/ifc:Change accessor function to take care of endianness
Dear York,
Quoted 10 lines · In message <[email protected]> you wrote:
Quoted 6 lines
On second thought, I also think we should avoid solutions where the BE/LE test has to be done for each and every I/O accessor call again and again. We should rather do this just once, and for example set function pointers as needed (hoping that this driver will only be needed after relocation, so we have writable data segment).
I like the idea of setting it just once, but I don't see how to implement it. A pointer is probably not the solution, because we do need some drivers before relocation.
"some drivers before relocation" - how many which are these?
Also, is it really necessary to make the decision about endianess at runtime? We don't have multi-board support in U-Boot yet, so when you build an image you know exactly which SoC you are building for, so you should be able to make the selection at compile time?
Best regards,
Wolfgang Denk
Signature · 4 lines
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de To be is to program.
PATCH v2 1/1 driver/ifc:Change accessor function to take care of endianness
Quoted 14 lines · Wolfgang Denk
Dear York,
Quoted 10 lines · In message <[email protected]> you wrote:
Quoted 6 lines
On second thought, I also think we should avoid solutions where the BE/LE test has to be done for each and every I/O accessor call again and again. We should rather do this just once, and for example set function pointers as needed (hoping that this driver will only be needed after relocation, so we have writable data segment).
I like the idea of setting it just once, but I don't see how to implement it. A pointer is probably not the solution, because we do need some drivers before relocation.
Quoted 6 lines
"some drivers before relocation" - how many which are these?
Also, is it really necessary to make the decision about endianess at runtime? We don't have multi-board support in U-Boot yet, so when you build an image you know exactly which SoC you are building for, so you should be able to make the selection at compile time?
It is done at compile time in this patch.
-Scott
PATCH v2 1/1 driver/ifc:Change accessor function to take care of endianness
Quoted 11 lines · Scott Wood
Quoted 10 lines · Wolfgang Denk
Dear York,
Quoted 7 lines · In message <[email protected]> you wrote:
Quoted 6 lines
On second thought, I also think we should avoid solutions where the BE/LE test has to be done for each and every I/O accessor call again and again. We should rather do this just once, and for example set function pointers as needed (hoping that this driver will only be needed after relocation, so we have writable data segment).
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
I like the idea of setting it just once, but I don't see how to implement it. A pointer is probably not the solution, because we do need some drivers before relocation.
"some drivers before relocation" - how many which are these?
IFC, DDR, I2C (only 32-bit controller is concerned), GUT
Quoted 8 lines
Quoted 5 lines
Also, is it really necessary to make the decision about endianess at runtime? We don't have multi-board support in U-Boot yet, so when you build an image you know exactly which SoC you are building for, so you should be able to make the selection at compile time?
It is done at compile time in this patch.
No. It is not necessary to do it at run time. It would be easier to use a switch to decide at compiling time. It does involve many changes to implement the wrapper.
York
PATCH v2 1/1 driver/ifc:Change accessor function to take care of endianness
Quoted 26 lines · York Sun
Quoted 11 lines · Scott Wood
Quoted 10 lines · Wolfgang Denk
Dear York,
Quoted 7 lines · In message <[email protected]> you wrote:
Quoted 6 lines
On second thought, I also think we should avoid solutions where the BE/LE test has to be done for each and every I/O accessor call again and again. We should rather do this just once, and for example set function pointers as needed (hoping that this driver will only be needed after relocation, so we have writable data segment).
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
I like the idea of setting it just once, but I don't see how to implement it. A pointer is probably not the solution, because we do need some drivers before relocation.
"some drivers before relocation" - how many which are these?
IFC, DDR, I2C (only 32-bit controller is concerned), GUT
Quoted 6 lines
Quoted 5 lines
Also, is it really necessary to make the decision about endianess at runtime? We don't have multi-board support in U-Boot yet, so when you build an image you know exactly which SoC you are building for, so you should be able to make the selection at compile time?
Quoted 6 lines
Quoted 2 lines
It is done at compile time in this patch.
No. It is not necessary to do it at run time. It would be easier to use a switch to decide at compiling time. It does involve many changes to implement the wrapper.
Do we all agree on this patch?
York
PATCH v2 1/1 driver/ifc:Change accessor function to take care of endianness
In reply to Wolfgang DenkQuoted 9 lines · Wolfgang Denk
Dear Scott,
Quoted 6 lines · In message <[email protected]> I wrote:
I fear that more IP blocks will follow that have similar requirements, and if we implemnt similar wrappers for each of them separately, we will have a mess of hard to maintain code. For example, it will then be impossible to share common parts of code because driver A uses
A_out32()and driver B usesB_out32().
Quoted 9 lines
Quoted 2 lines
We should rather try and find a generic solution where the same functions can be used by mulltiple drivers which have the same needs.
On second thought, I also think we should avoid solutions where the BE/LE test has to be done for each and every I/O accessor call again and again. We should rather do this just once, and for example set function pointers as needed (hoping that this driver will only be needed after relocation, so we have writable data segment).
Thanks Wolfgang for this suggestion.
As far I understand, you are suggesting to have global structure with function pointer to accessor function depending upon IP's endiannes.. but i afraid, for IFC we do have scenario where these accessor function are used before relocation.
Regards,
Prabhakar
PATCH v2 1/1 driver/ifc:Change accessor function to take care of endianness
In reply to Prabhakar KushwahaQuoted 11 lines · Prabhakar Kushwaha
IFC registers can be of type Little Endian or big Endian depending upon Freescale SoC. Here SoC defines the register type of IFC IP.
So update acessor functions with common IFC acessor functions to take care both type of endianness.
Signed-off-by Prabhakar Kushwaha <[email protected]>---
Changes for v2:
- fix spelling mistakes
Applied to u-boot-mpc85xx master branch.
York
PATCH v2 1/1 driver/ifc:Change accessor function to take care of endianness
Quoted 7 lines · York Sun
Quoted 6 lines · Prabhakar Kushwaha
IFC registers can be of type Little Endian or big Endian depending upon Freescale SoC. Here SoC defines the register type of IFC IP.
So update acessor functions with common IFC acessor functions to take care both type of endianness.
Quoted 7 lines
Quoted 5 lines
Signed-off-by Prabhakar Kushwaha <[email protected]>---
Changes for v2:
- fix spelling mistakesApplied to u-boot-mpc85xx master branch.
I thought this was supposed to wait until there was a user.
-Scott
PATCH v2 1/1 driver/ifc:Change accessor function to take care of endianness
Quoted 14 lines · Scott Wood
Quoted 13 lines · York Sun
Quoted 11 lines · Prabhakar Kushwaha
IFC registers can be of type Little Endian or big Endian depending upon Freescale SoC. Here SoC defines the register type of IFC IP.
So update acessor functions with common IFC acessor functions to take care both type of endianness.
Signed-off-by Prabhakar Kushwaha <[email protected]>---
Changes for v2:
- fix spelling mistakes
Quoted 4 lines
Quoted 1 line
Applied to u-boot-mpc85xx master branch.
I thought this was supposed to wait until there was a user.
We have one. I am clearing the path to push patches for LS2100.
York