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

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

[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 <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/
 
 else
 
diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile
index aee7e32..f073e1c 100644
--- a/drivers/usb/common/Makefile
+++ b/drivers/usb/common/Makefile
@@ -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
diff --git a/include/configs/km/kmp204x-common.h b/include/configs/km/kmp204x-common.h
index 028623d..5bdda22 100644
--- a/include/configs/km/kmp204x-common.h
+++ b/include/configs/km/kmp204x-common.h
@@ -406,6 +406,7 @@ int get_scl(void);
 #endif
 
 #define __USB_PHY_TYPE	utmi
+#define CONFIG_USB_EHCI_FSL
 
 /*
  * Environment Configuration
-- 
2.7.4

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

[Marek Vasut]

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/

>  else
>  
> diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile
> index aee7e32..f073e1c 100644
> --- a/drivers/usb/common/Makefile
> +++ b/drivers/usb/common/Makefile
> @@ -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.

> diff --git a/include/configs/km/kmp204x-common.h b/include/configs/km/kmp204x-common.h
> index 028623d..5bdda22 100644
> --- a/include/configs/km/kmp204x-common.h
> +++ b/include/configs/km/kmp204x-common.h
> @@ -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

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

[Sriram Dash]

>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/
>

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

>>  else
>>
>> diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile
>> index aee7e32..f073e1c 100644
>> --- a/drivers/usb/common/Makefile
>> +++ b/drivers/usb/common/Makefile
>> @@ -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.
>
>> diff --git a/include/configs/km/kmp204x-common.h
>> b/include/configs/km/kmp204x-common.h
>> index 028623d..5bdda22 100644
>> --- a/include/configs/km/kmp204x-common.h
>> +++ b/include/configs/km/kmp204x-common.h
>> @@ -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

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

[Marek Vasut]

On 06/28/2016 09: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/
>>
> 
> 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?

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.

-- 
Best regards,
Marek Vasut

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

[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/
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

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

[Sriram Dash]

>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.

> 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?

>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?

>Please take the lead to fix this issue.
>
>York

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

[york sun]

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
> 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

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

[Marek Vasut]

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