From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Wed, 29 Jun 2016 12:27:17 +0200 Subject: [U-Boot] [RFC PATCH v1] usb: spl: fix USB errata for FSL SPL targets In-Reply-To: References: <1467068555-12673-1-git-send-email-york.sun@nxp.com> <5771BC63.3020101@denx.de> Message-ID: <5773A285.80909@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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:marex at 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 >>>>>> CC: Sriram Dash >>>>>> CC: Rajesh Bhagat >>>>>> >>>>>> --- >>>>>> 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