[U-Boot] [RFC PATCH v1] usb: spl: fix USB errata for FSL SPL targets

[Marek Vasut <marex@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 <york.sun@nxp.com>
>>>>>> CC: Sriram Dash <sriram.dash@nxp.com>
>>>>>> CC: Rajesh Bhagat <rajesh.bhagat@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