usb: spl: fix USB errata for FSL SPL targets

RFC PATCH New York Sun 3 participants 8 messages Jun 27, 2016 – Jun 29, 2016 USB

lore

Share & tools

lore

York Sun

author

2016-06-27 23:02 UTC #0 lore raw

RFC PATCH 1/1 usb: spl: fix USB errata for FSL SPL targets

Commit 9262367 moved USB errata workaround to a C file but didn't build it for SPL targets.

Signed-off-by York Sun <[email protected]>

CC Sriram Dash <[email protected]>

CC Rajesh Bhagat <[email protected]>

---
Please review this patch. It fixed the compiling errors introduced by 9262367. Not sure if this is the way USB errata should be handled.

 drivers/Makefile                    | 7 +++++++
 drivers/usb/common/Makefile         | 8 ++++++--
 include/configs/km/kmp204x-common.h | 1 +
 3 files changed, 14 insertions(+), 2 deletions(-)

drivers/Makefile +7 -0

index 1723958..88774ba 100644
@@ -39,6 +39,13 @@ obj-$(CONFIG_OMAP_USB_PHY) += usb/phy/
 obj-$(CONFIG_SPL_SATA_SUPPORT) += block/
 obj-$(CONFIG_SPL_USB_HOST_SUPPORT) += block/
 obj-$(CONFIG_SPL_MMC_SUPPORT) += block/
+ifdef CONFIG_USB_EHCI_FSL
+CONFIG_SPL_USB_ERRATA = y
+endif
+ifdef CONFIG_USB_XHCI_FSL
+CONFIG_SPL_USB_ERRATA = y
+endif
+obj-$(CONFIG_SPL_USB_ERRATA) += usb/common/
 
 else

drivers/usb/common/Makefile +6 -2

index aee7e32..f073e1c 100644
@@ -4,5 +4,9 @@
 #
 
 obj-$(CONFIG_DM_USB) += common.o
-obj-$(CONFIG_USB_EHCI_FSL) += fsl-dt-fixup.o fsl-errata.o
-obj-$(CONFIG_USB_XHCI_FSL) += fsl-dt-fixup.o fsl-errata.o
+obj-$(CONFIG_USB_EHCI_FSL) += fsl-errata.o
+obj-$(CONFIG_USB_XHCI_FSL) += fsl-errata.o
+ifndef CONFIG_SPL_BUILD
+obj-$(CONFIG_USB_EHCI_FSL) += fsl-dt-fixup.o
+obj-$(CONFIG_USB_XHCI_FSL) += fsl-dt-fixup.o
+endif

include/configs/km/kmp204x-common.h +1 -0

index 028623d..5bdda22 100644
@@ -406,6 +406,7 @@ int get_scl(void);
 #endif
 
 #define __USB_PHY_TYPE	utmi
+#define CONFIG_USB_EHCI_FSL
 
 /*
  * Environment Configuration

Signature · 1 line

2.7.4

Marek Vasut

2016-06-27 23:53 UTC #1 lore raw

RFC PATCH 1/1 usb: spl: fix USB errata for FSL SPL targets

Quoted 21 lines · York Sun

Commit 9262367 moved USB errata workaround to a C file but didn't build it for SPL targets.
Signed-off-by York Sun <[email protected]>
CC Sriram Dash <[email protected]>
CC Rajesh Bhagat <[email protected]>
---
Please review this patch. It fixed the compiling errors introduced by 9262367. Not sure if this is the way USB errata should be handled.
 drivers/Makefile                    | 7 +++++++
 drivers/usb/common/Makefile         | 8 ++++++--
 include/configs/km/kmp204x-common.h | 1 +
 3 files changed, 14 insertions(+), 2 deletions(-)
drivers/Makefile +0 -0
index 1723958..88774ba 100644

drivers/Makefile obj-$(CONFIG_OMAP_USB_PHY) += usb/phy/ · line 48

Quoted 10 lines

drivers/Makefile +7 -0

@@ -39,6 +39,13 @@ obj-$(CONFIG_OMAP_USB_PHY) += usb/phy/
 obj-$(CONFIG_SPL_SATA_SUPPORT) += block/
 obj-$(CONFIG_SPL_USB_HOST_SUPPORT) += block/
 obj-$(CONFIG_SPL_MMC_SUPPORT) += block/
+ifdef CONFIG_USB_EHCI_FSL
+CONFIG_SPL_USB_ERRATA = y
+endif
+ifdef CONFIG_USB_XHCI_FSL
+CONFIG_SPL_USB_ERRATA = y
+endif
+obj-$(CONFIG_SPL_USB_ERRATA) += usb/common/

I really dislike the naming here, I'd say just do

obj-$(CONFIG_USB_EHCI_FSL) += usb/common/
obj-$(CONFIG_USB_XHCI_FSL) += usb/common/

Quoted 8 lines

drivers/Makefile +0 -0
 else
 
drivers/usb/common/Makefile +0 -0
index aee7e32..f073e1c 100644
@@ -4,5 +4,9 @@
 #

drivers/usb/common/Makefile line 12

Quoted 10 lines

drivers/usb/common/Makefile +6 -2

 
 obj-$(CONFIG_DM_USB) += common.o
-obj-$(CONFIG_USB_EHCI_FSL) += fsl-dt-fixup.o fsl-errata.o
-obj-$(CONFIG_USB_XHCI_FSL) += fsl-dt-fixup.o fsl-errata.o
+obj-$(CONFIG_USB_EHCI_FSL) += fsl-errata.o
+obj-$(CONFIG_USB_XHCI_FSL) += fsl-errata.o
+ifndef CONFIG_SPL_BUILD
+obj-$(CONFIG_USB_EHCI_FSL) += fsl-dt-fixup.o
+obj-$(CONFIG_USB_XHCI_FSL) += fsl-dt-fixup.o
+endif

Either the filename is misnamed or this is real broken design. I would expect that a filename with -dt- in it's name should depend on CONFIG_*FDT* , but not on CONFIG_SPL_BUILD.

Quoted 13 lines

include/configs/km/kmp204x-common.h +1 -0

index 028623d..5bdda22 100644
@@ -406,6 +406,7 @@ int get_scl(void);
 #endif
 
 #define __USB_PHY_TYPE	utmi
+#define CONFIG_USB_EHCI_FSL
 
 /*
  * Environment Configuration

Signature · 2 lines

Best regards,
Marek Vasut

Sriram Dash

2016-06-28 07:02 UTC #2 lore raw

RFC PATCH 1/1 usb: spl: fix USB errata for FSL SPL targets

Quoted 36 lines

From: Marek Vasut [mailto:marex at denx.de]
Quoted 21 lines · York Sun
Commit 9262367 moved USB errata workaround to a C file but didn't build it for SPL targets.
Signed-off-by York Sun <[email protected]>
CC Sriram Dash <[email protected]>
CC Rajesh Bhagat <[email protected]>
---
Please review this patch. It fixed the compiling errors introduced by 9262367. Not sure if this is the way USB errata should be handled.
 drivers/Makefile                    | 7 +++++++
 drivers/usb/common/Makefile         | 8 ++++++--
 include/configs/km/kmp204x-common.h | 1 +
 3 files changed, 14 insertions(+), 2 deletions(-)
drivers/Makefile index renamed from drivers/Makefile +0 -0
1723958..88774ba 100644
drivers/Makefile obj-$(CONFIG_OMAP_USB_PHY) += usb/phy/ · line 48
Quoted 10 lines
drivers/Makefile +7 -0
@@ -39,6 +39,13 @@ obj-$(CONFIG_OMAP_USB_PHY) += usb/phy/
 obj-$(CONFIG_SPL_SATA_SUPPORT) += block/
 obj-$(CONFIG_SPL_USB_HOST_SUPPORT) += block/
 obj-$(CONFIG_SPL_MMC_SUPPORT) += block/
+ifdef CONFIG_USB_EHCI_FSL
+CONFIG_SPL_USB_ERRATA = y
+endif
+ifdef CONFIG_USB_XHCI_FSL
+CONFIG_SPL_USB_ERRATA = y
+endif
+obj-$(CONFIG_SPL_USB_ERRATA) += usb/common/
I really dislike the naming here, I'd say just do

Quoted 3 lines

obj-$(CONFIG_USB_EHCI_FSL) += usb/common/
obj-$(CONFIG_USB_XHCI_FSL) += usb/common/

Hello York/Marek,

IMO, the build for SPL is failing in PPC as the cmd_errata is not getting the definition of the has_erratum_aNNNNNN functions. So, instead of EHCI or XHCI flags, i think we can use CONFIG_CMD_ERRATA for SPL build for the errata applicability.

+obj-$(CONFIG_CMD_ERRATA) += usb/common/fsl-errata.o

What is your opinion?

Regards,
Sriram

Quoted 39 lines

Quoted 7 lines
drivers/Makefile +0 -0
 else
 
drivers/usb/common/Makefile +0 -0
index aee7e32..f073e1c 100644
drivers/usb/common/Makefile line 10
Quoted 9 lines
drivers/usb/common/Makefile +4 -2
@@ -4,5 +4,9 @@
 #
 
 obj-$(CONFIG_DM_USB) += common.o
-obj-$(CONFIG_USB_EHCI_FSL) += fsl-dt-fixup.o fsl-errata.o
-obj-$(CONFIG_USB_XHCI_FSL) += fsl-dt-fixup.o fsl-errata.o
+obj-$(CONFIG_USB_EHCI_FSL) += fsl-errata.o
+obj-$(CONFIG_USB_XHCI_FSL) += fsl-errata.o ifndef CONFIG_SPL_BUILD
+obj-$(CONFIG_USB_EHCI_FSL) += fsl-dt-fixup.o
+obj-$(CONFIG_USB_XHCI_FSL) += fsl-dt-fixup.o endif
Either the filename is misnamed or this is real broken design. I would expect that a filename with -dt- in it's name should depend on CONFIG_*FDT* , but not on CONFIG_SPL_BUILD.
include/configs/km/kmp204x-common.h int get_scl(void); #endif · line 412
Quoted 13 lines
include/configs/km/kmp204x-common.h +1 -0
index 028623d..5bdda22 100644
@@ -406,6 +406,7 @@ int get_scl(void);  #endif
 
 #define __USB_PHY_TYPE	utmi
+#define CONFIG_USB_EHCI_FSL
 
 /*
  * Environment Configuration
 
--
Best regards,
Marek Vasut

Marek Vasut

2016-06-28 08:02 UTC #3 lore raw

RFC PATCH 1/1 usb: spl: fix USB errata for FSL SPL targets

Quoted 42 lines · Sriram Dash

Quoted 36 lines
From: Marek Vasut [mailto:marex at denx.de]
Quoted 21 lines · York Sun
Commit 9262367 moved USB errata workaround to a C file but didn't build it for SPL targets.
Signed-off-by York Sun <[email protected]>
CC Sriram Dash <[email protected]>
CC Rajesh Bhagat <[email protected]>
---
Please review this patch. It fixed the compiling errors introduced by 9262367. Not sure if this is the way USB errata should be handled.
 drivers/Makefile                    | 7 +++++++
 drivers/usb/common/Makefile         | 8 ++++++--
 include/configs/km/kmp204x-common.h | 1 +
 3 files changed, 14 insertions(+), 2 deletions(-)
drivers/Makefile index renamed from drivers/Makefile +0 -0
1723958..88774ba 100644
drivers/Makefile obj-$(CONFIG_OMAP_USB_PHY) += usb/phy/ · line 48
Quoted 10 lines
drivers/Makefile +7 -0
@@ -39,6 +39,13 @@ obj-$(CONFIG_OMAP_USB_PHY) += usb/phy/
 obj-$(CONFIG_SPL_SATA_SUPPORT) += block/
 obj-$(CONFIG_SPL_USB_HOST_SUPPORT) += block/
 obj-$(CONFIG_SPL_MMC_SUPPORT) += block/
+ifdef CONFIG_USB_EHCI_FSL
+CONFIG_SPL_USB_ERRATA = y
+endif
+ifdef CONFIG_USB_XHCI_FSL
+CONFIG_SPL_USB_ERRATA = y
+endif
+obj-$(CONFIG_SPL_USB_ERRATA) += usb/common/
I really dislike the naming here, I'd say just do
Quoted 3 lines
obj-$(CONFIG_USB_EHCI_FSL) += usb/common/
obj-$(CONFIG_USB_XHCI_FSL) += usb/common/
Hello York/Marek,

Quoted 8 lines

IMO, the build for SPL is failing in PPC as the cmd_errata is not getting the definition of the has_erratum_aNNNNNN functions. So, instead of EHCI or XHCI flags, i think we can use CONFIG_CMD_ERRATA for SPL build for the errata applicability.
+obj-$(CONFIG_CMD_ERRATA) += usb/common/fsl-errata.o
What is your opinion?

I think naming macro which is NOT enabling a command CONFIG_CMD_ERRATA is horrible and needs renaming in near future. Otherwise, I'll just wait for York's opinion on the above.

Signature · 2 lines

Best regards,
Marek Vasut

york sun

author

2016-06-28 15:47 UTC #4 lore raw

RFC PATCH 1/1 usb: spl: fix USB errata for FSL SPL targets

In reply to Sriram Dash

Quoted 36 lines · Sriram Dash

Quoted 36 lines
From: Marek Vasut [mailto:marex at denx.de]
Quoted 21 lines · York Sun
Commit 9262367 moved USB errata workaround to a C file but didn't build it for SPL targets.
Signed-off-by York Sun <[email protected]>
CC Sriram Dash <[email protected]>
CC Rajesh Bhagat <[email protected]>
---
Please review this patch. It fixed the compiling errors introduced by 9262367. Not sure if this is the way USB errata should be handled.
  drivers/Makefile                    | 7 +++++++
  drivers/usb/common/Makefile         | 8 ++++++--
  include/configs/km/kmp204x-common.h | 1 +
  3 files changed, 14 insertions(+), 2 deletions(-)
drivers/Makefile index renamed from drivers/Makefile +0 -0
1723958..88774ba 100644
drivers/Makefile obj-$(CONFIG_OMAP_USB_PHY) += usb/phy/ · line 48
Quoted 10 lines
drivers/Makefile +7 -0
@@ -39,6 +39,13 @@ obj-$(CONFIG_OMAP_USB_PHY) += usb/phy/
  obj-$(CONFIG_SPL_SATA_SUPPORT) += block/
  obj-$(CONFIG_SPL_USB_HOST_SUPPORT) += block/
  obj-$(CONFIG_SPL_MMC_SUPPORT) += block/
+ifdef CONFIG_USB_EHCI_FSL
+CONFIG_SPL_USB_ERRATA = y
+endif
+ifdef CONFIG_USB_XHCI_FSL
+CONFIG_SPL_USB_ERRATA = y
+endif
+obj-$(CONFIG_SPL_USB_ERRATA) += usb/common/
I really dislike the naming here, I'd say just do

Quoted 2 lines

Quoted 2 lines
obj-$(CONFIG_USB_EHCI_FSL) += usb/common/
obj-$(CONFIG_USB_XHCI_FSL) += usb/common/

Better.

Quoted 4 lines

Quoted 1 line
Hello York/Marek,

Quoted 9 lines

IMO, the build for SPL is failing in PPC as the cmd_errata is not getting the definition of the has_erratum_aNNNNNN functions. So, instead of EHCI or XHCI flags, i think we can use CONFIG_CMD_ERRATA for SPL build for the errata applicability.
+obj-$(CONFIG_CMD_ERRATA) += usb/common/fsl-errata.o
What is your opinion?

Sriram,

I think it is not a good idea. CONFIG_CMD_ERRATA has nothing to do with USB errata specifically. It means to enable the command "errata", and nothing more.

What's broken here is the mechanism to detect if an erratum applies to a particular SoC version. Before your change, the functions were included from the header file. You moved those functions into a C file. That's fine. We need to build this file for both normal image and SPL. I don't think the workarounds are optional, unless USB is not used at all for SPL.

I think Marek's comments make sense.
1) obj-$(CONFIG_USB_EHCI_FSL) += usb/common/

    obj-$(CONFIG_USB_XHCI_FSL) += usb/common/

For this to work, we need to fix kmp204x-common.h, which I noticed CONFIG_USB_EHCI_FSL is not defined. Please examine if this macro should be defined at SoC level, instead of board level.
2) Use a proper macro for fsl-dt-fixup.o
The entry point is fdt_fixup_dr_usb(), which is only called by ft_board_setup() when CONFIG_OF_BOARD_SETUP is defined.

Please take the lead to fix this issue.

York

Sriram Dash

2016-06-29 03:53 UTC #5 lore raw

RFC PATCH 1/1 usb: spl: fix USB errata for FSL SPL targets

Quoted 68 lines

From: york sun
Quoted 36 lines · Sriram Dash
Quoted 36 lines
From: Marek Vasut [mailto:marex at denx.de] On 06/28/2016 01:02 AM, York
Quoted 21 lines · Sun
Commit 9262367 moved USB errata workaround to a C file but didn't build it for SPL targets.
Signed-off-by York Sun <[email protected]>
CC Sriram Dash <[email protected]>
CC Rajesh Bhagat <[email protected]>
---
Please review this patch. It fixed the compiling errors introduced by 9262367. Not sure if this is the way USB errata should be handled.
  drivers/Makefile                    | 7 +++++++
  drivers/usb/common/Makefile         | 8 ++++++--
  include/configs/km/kmp204x-common.h | 1 +
  3 files changed, 14 insertions(+), 2 deletions(-)
drivers/Makefile index renamed from drivers/Makefile +0 -0
1723958..88774ba 100644
drivers/Makefile obj-$(CONFIG_OMAP_USB_PHY) += usb/phy/ · line 48
Quoted 10 lines
drivers/Makefile +7 -0
@@ -39,6 +39,13 @@ obj-$(CONFIG_OMAP_USB_PHY) += usb/phy/
  obj-$(CONFIG_SPL_SATA_SUPPORT) += block/
  obj-$(CONFIG_SPL_USB_HOST_SUPPORT) += block/
  obj-$(CONFIG_SPL_MMC_SUPPORT) += block/
+ifdef CONFIG_USB_EHCI_FSL
+CONFIG_SPL_USB_ERRATA = y
+endif
+ifdef CONFIG_USB_XHCI_FSL
+CONFIG_SPL_USB_ERRATA = y
+endif
+obj-$(CONFIG_SPL_USB_ERRATA) += usb/common/
I really dislike the naming here, I'd say just do
Quoted 2 lines
Quoted 2 lines
obj-$(CONFIG_USB_EHCI_FSL) += usb/common/
obj-$(CONFIG_USB_XHCI_FSL) += usb/common/
Better.
Quoted 4 lines
Quoted 1 line
Hello York/Marek,
Quoted 9 lines
IMO, the build for SPL is failing in PPC as the cmd_errata is not getting the definition of the has_erratum_aNNNNNN functions. So, instead of EHCI or XHCI flags, i think we can use CONFIG_CMD_ERRATA for SPL build for the errata applicability.
+obj-$(CONFIG_CMD_ERRATA) += usb/common/fsl-errata.o
What is your opinion?
Sriram,
I think it is not a good idea. CONFIG_CMD_ERRATA has nothing to do with USB errata specifically. It means to enable the command "errata", and nothing more.
What's broken here is the mechanism to detect if an erratum applies to a particular SoC version. Before your change, the functions were included from the header file. You moved those functions into a C file. That's fine. We need to build this file for both normal image and SPL. I don't think the workarounds are optional, unless USB is not used at all for SPL.

Quoted 3 lines

I think Marek's comments make sense.
1) obj-$(CONFIG_USB_EHCI_FSL) += usb/common/
obj-$(CONFIG_USB_XHCI_FSL) += usb/common/

OK. This looks generic also.

Quoted 3 lines

For this to work, we need to fix
kmp204x-common.h, which I noticed CONFIG_USB_EHCI_FSL is not defined. Please examine if this macro should be defined at SoC level, instead of board level.

The kmp204x-common does not need to define CONFIG_USB_EHCI_FSL. Currently, the board is not using the USB. But, they are using CONFIG_CMD_ERRATA. So, instead of having a flag of CONFIG_USB_EHCI_FSL in the board specific file, drivers/usb/common/Makefile should include fsl-errata.o for the config CONFIG_CMD_ERRATA. What is your opinion?

Quoted 4 lines

2) Use a proper macro for fsl-dt-fixup.o The entry point is fdt_fixup_dr_usb(), which is only called by
ft_board_setup() when CONFIG_OF_BOARD_SETUP is defined.

Yes. You are correct. fdt_fixup_dr_usb is only called when CONFIG_OF_BOARD_SETUP is defined. So, I guess it will be fine for drivers/usb/common/Makefile

obj-$(CONFIG_USB_EHCI_FSL) += fsl-errata.o
obj-$(CONFIG_USB_XHCI_FSL) += fsl-errata.o
obj-$( CONFIG_CMD_ERRATA) += fsl-errata.o
obj-$(CONFIG_OF_BOARD_SETUP) += fsl-dt-fixup.o

What is your opinion?

Quoted 3 lines

Please take the lead to fix this issue.
York

york sun

author

2016-06-29 04:12 UTC #6 lore raw

RFC PATCH 1/1 usb: spl: fix USB errata for FSL SPL targets

Quoted 68 lines · Sriram Dash

Quoted 68 lines
From: york sun
Quoted 36 lines · Sriram Dash
Quoted 36 lines
From: Marek Vasut [mailto:marex at denx.de] On 06/28/2016 01:02 AM, York
Quoted 21 lines · Sun
Commit 9262367 moved USB errata workaround to a C file but didn't build it for SPL targets.
Signed-off-by York Sun <[email protected]>
CC Sriram Dash <[email protected]>
CC Rajesh Bhagat <[email protected]>
---
Please review this patch. It fixed the compiling errors introduced by 9262367. Not sure if this is the way USB errata should be handled.
   drivers/Makefile                    | 7 +++++++
   drivers/usb/common/Makefile         | 8 ++++++--
   include/configs/km/kmp204x-common.h | 1 +
   3 files changed, 14 insertions(+), 2 deletions(-)
drivers/Makefile index renamed from drivers/Makefile +0 -0
1723958..88774ba 100644
drivers/Makefile obj-$(CONFIG_OMAP_USB_PHY) += usb/phy/ · line 48
Quoted 10 lines
drivers/Makefile +7 -0
@@ -39,6 +39,13 @@ obj-$(CONFIG_OMAP_USB_PHY) += usb/phy/
   obj-$(CONFIG_SPL_SATA_SUPPORT) += block/
   obj-$(CONFIG_SPL_USB_HOST_SUPPORT) += block/
   obj-$(CONFIG_SPL_MMC_SUPPORT) += block/
+ifdef CONFIG_USB_EHCI_FSL
+CONFIG_SPL_USB_ERRATA = y
+endif
+ifdef CONFIG_USB_XHCI_FSL
+CONFIG_SPL_USB_ERRATA = y
+endif
+obj-$(CONFIG_SPL_USB_ERRATA) += usb/common/
I really dislike the naming here, I'd say just do
Quoted 2 lines
Quoted 2 lines
obj-$(CONFIG_USB_EHCI_FSL) += usb/common/
obj-$(CONFIG_USB_XHCI_FSL) += usb/common/
Better.
Quoted 4 lines
Quoted 1 line
Hello York/Marek,
Quoted 9 lines
IMO, the build for SPL is failing in PPC as the cmd_errata is not getting the definition of the has_erratum_aNNNNNN functions. So, instead of EHCI or XHCI flags, i think we can use CONFIG_CMD_ERRATA for SPL build for the errata applicability.
+obj-$(CONFIG_CMD_ERRATA) += usb/common/fsl-errata.o
What is your opinion?
Sriram,
I think it is not a good idea. CONFIG_CMD_ERRATA has nothing to do with USB errata specifically. It means to enable the command "errata", and nothing more.
What's broken here is the mechanism to detect if an erratum applies to a particular SoC version. Before your change, the functions were included from the header file. You moved those functions into a C file. That's fine. We need to build this file for both normal image and SPL. I don't think the workarounds are optional, unless USB is not used at all for SPL.

Quoted 5 lines

Quoted 3 lines
I think Marek's comments make sense.
1) obj-$(CONFIG_USB_EHCI_FSL) += usb/common/
    obj-$(CONFIG_USB_XHCI_FSL) += usb/common/
OK. This looks generic also.

If an SoC has the errata, it should implement the workaround, regardless if the U-Boot uses USB. Kernel may use it later. So we should treat it that way.

Quoted 10 lines

Quoted 4 lines
For this to work, we need to fix
kmp204x-common.h, which I noticed CONFIG_USB_EHCI_FSL is not defined. Please examine if this macro should be defined at SoC level, instead of board level.
The kmp204x-common does not need to define CONFIG_USB_EHCI_FSL. Currently, the board is not using the USB. But, they are using CONFIG_CMD_ERRATA. So, instead of having a flag of CONFIG_USB_EHCI_FSL in the board specific file, drivers/usb/common/Makefile should include fsl-errata.o for the config CONFIG_CMD_ERRATA. What is your opinion?

I don't agree. CMD_ERRATA is to enable the command to show which erratum workaround has been implemented. It shouldn't gate if the actual workaround code runs. Even kmp204x boards doesn't use USB, the workaround still needs to be implemented, in case kernel uses USB later. I was suggesting to put a macro in SoC config file if that fits the purpose.

Quoted 9 lines

Quoted 5 lines
2) Use a proper macro for fsl-dt-fixup.o The entry point is fdt_fixup_dr_usb(), which is only called by
ft_board_setup() when CONFIG_OF_BOARD_SETUP is defined.
Yes. You are correct. fdt_fixup_dr_usb is only called when CONFIG_OF_BOARD_SETUP is defined. So, I guess it will be fine for drivers/usb/common/Makefile

Quoted 6 lines

obj-$(CONFIG_USB_EHCI_FSL) += fsl-errata.o
obj-$(CONFIG_USB_XHCI_FSL) += fsl-errata.o
obj-$( CONFIG_CMD_ERRATA) += fsl-errata.o
obj-$(CONFIG_OF_BOARD_SETUP) += fsl-dt-fixup.o
What is your opinion?

That looks reasonable. Please test some targets with and without SPL, powerpc and ARM.

York

Marek Vasut

2016-06-29 10:27 UTC #7 lore raw

RFC PATCH 1/1 usb: spl: fix USB errata for FSL SPL targets

Quoted 90 lines · york sun

Quoted 68 lines · Sriram Dash
Quoted 68 lines
From: york sun
Quoted 36 lines · Sriram Dash
Quoted 36 lines
From: Marek Vasut [mailto:marex at denx.de] On 06/28/2016 01:02 AM, York
Quoted 21 lines · Sun
Commit 9262367 moved USB errata workaround to a C file but didn't build it for SPL targets.
Signed-off-by York Sun <[email protected]>
CC Sriram Dash <[email protected]>
CC Rajesh Bhagat <[email protected]>
---
Please review this patch. It fixed the compiling errors introduced by 9262367. Not sure if this is the way USB errata should be handled.
   drivers/Makefile                    | 7 +++++++
   drivers/usb/common/Makefile         | 8 ++++++--
   include/configs/km/kmp204x-common.h | 1 +
   3 files changed, 14 insertions(+), 2 deletions(-)
drivers/Makefile index renamed from drivers/Makefile +0 -0
1723958..88774ba 100644
drivers/Makefile obj-$(CONFIG_OMAP_USB_PHY) += usb/phy/ · line 48
Quoted 10 lines
drivers/Makefile +7 -0
@@ -39,6 +39,13 @@ obj-$(CONFIG_OMAP_USB_PHY) += usb/phy/
   obj-$(CONFIG_SPL_SATA_SUPPORT) += block/
   obj-$(CONFIG_SPL_USB_HOST_SUPPORT) += block/
   obj-$(CONFIG_SPL_MMC_SUPPORT) += block/
+ifdef CONFIG_USB_EHCI_FSL
+CONFIG_SPL_USB_ERRATA = y
+endif
+ifdef CONFIG_USB_XHCI_FSL
+CONFIG_SPL_USB_ERRATA = y
+endif
+obj-$(CONFIG_SPL_USB_ERRATA) += usb/common/
I really dislike the naming here, I'd say just do
Quoted 2 lines
Quoted 2 lines
obj-$(CONFIG_USB_EHCI_FSL) += usb/common/
obj-$(CONFIG_USB_XHCI_FSL) += usb/common/
Better.
Quoted 4 lines
Quoted 1 line
Hello York/Marek,
Quoted 9 lines
IMO, the build for SPL is failing in PPC as the cmd_errata is not getting the definition of the has_erratum_aNNNNNN functions. So, instead of EHCI or XHCI flags, i think we can use CONFIG_CMD_ERRATA for SPL build for the errata applicability.
+obj-$(CONFIG_CMD_ERRATA) += usb/common/fsl-errata.o
What is your opinion?
Sriram,
I think it is not a good idea. CONFIG_CMD_ERRATA has nothing to do with USB errata specifically. It means to enable the command "errata", and nothing more.
What's broken here is the mechanism to detect if an erratum applies to a particular SoC version. Before your change, the functions were included from the header file. You moved those functions into a C file. That's fine. We need to build this file for both normal image and SPL. I don't think the workarounds are optional, unless USB is not used at all for SPL.
Quoted 5 lines
Quoted 3 lines
I think Marek's comments make sense.
1) obj-$(CONFIG_USB_EHCI_FSL) += usb/common/
    obj-$(CONFIG_USB_XHCI_FSL) += usb/common/
OK. This looks generic also.
If an SoC has the errata, it should implement the workaround, regardless if the U-Boot uses USB. Kernel may use it later. So we should treat it that way.
Quoted 10 lines
Quoted 4 lines
For this to work, we need to fix
kmp204x-common.h, which I noticed CONFIG_USB_EHCI_FSL is not defined. Please examine if this macro should be defined at SoC level, instead of board level.
The kmp204x-common does not need to define CONFIG_USB_EHCI_FSL. Currently, the board is not using the USB. But, they are using CONFIG_CMD_ERRATA. So, instead of having a flag of CONFIG_USB_EHCI_FSL in the board specific file, drivers/usb/common/Makefile should include fsl-errata.o for the config CONFIG_CMD_ERRATA. What is your opinion?

Quoted 18 lines

I don't agree. CMD_ERRATA is to enable the command to show which erratum workaround has been implemented. It shouldn't gate if the actual workaround code runs. Even kmp204x boards doesn't use USB, the workaround still needs to be implemented, in case kernel uses USB later. I was suggesting to put a macro in SoC config file if that fits the purpose.
Quoted 12 lines
Quoted 5 lines
2) Use a proper macro for fsl-dt-fixup.o The entry point is fdt_fixup_dr_usb(), which is only called by
ft_board_setup() when CONFIG_OF_BOARD_SETUP is defined.
Yes. You are correct. fdt_fixup_dr_usb is only called when CONFIG_OF_BOARD_SETUP is defined. So, I guess it will be fine for drivers/usb/common/Makefile
obj-$(CONFIG_USB_EHCI_FSL) += fsl-errata.o
obj-$(CONFIG_USB_XHCI_FSL) += fsl-errata.o
obj-$( CONFIG_CMD_ERRATA) += fsl-errata.o

This is a NAK

Quoted 1 line

Quoted 1 line
obj-$(CONFIG_OF_BOARD_SETUP) += fsl-dt-fixup.o

This is also a NAK, how the heck can this depend on OF_BOARD_SETUP?

Quoted 7 lines

Quoted 1 line
What is your opinion?
That looks reasonable. Please test some targets with and without SPL, powerpc and ARM.
York

Signature · 2 lines

Best regards,
Marek Vasut