On 06/29/2016 06:12 AM, york sun wrote: > On 06/28/2016 08:53 PM, Sriram Dash wrote: >>> From: york sun >>> On 06/28/2016 12:02 AM, Sriram Dash wrote: >>>>> From: Marek Vasut [mailto:ma...@denx.de] On 06/28/2016 01:02 AM, York >>>>> Sun wrote: >>>>>> Commit 9262367 moved USB errata workaround to a C file but didn't >>>>>> build it for SPL targets. >>>>>> >>>>>> Signed-off-by: York Sun <york....@nxp.com> >>>>>> CC: Sriram Dash <sriram.d...@nxp.com> >>>>>> CC: Rajesh Bhagat <rajesh.bha...@nxp.com> >>>>>> >>>>>> --- >>>>>> 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(-) >>>>>> >>>>>> diff --git a/drivers/Makefile b/drivers/Makefile index >>>>>> 1723958..88774ba 100644 >>>>>> --- a/drivers/Makefile >>>>>> +++ b/drivers/Makefile >>>>>> @@ -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/ >>> >>> Better. >>> >>>>> >>>> >>>> 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? >>>> >>> >>> 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/ >> >> 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. > >> >>> 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. > >> >>> 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 >> obj-$(CONFIG_OF_BOARD_SETUP) += fsl-dt-fixup.o This is also a NAK, how the heck can this depend on OF_BOARD_SETUP? >> What is your opinion? > > That looks reasonable. Please test some targets with and without SPL, > powerpc and ARM. > > York > -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
