[U-Boot] [U-Boot 3/3] rockchip: rk3288: enable rockusb support on rk3288 based device

[Simon Glass]

Hi Eddie,

On 4 May 2017 at 19:15, Eddie Cai <eddie.cai.linux at gmail.com> wrote:
> 2017-05-05 0:49 GMT+08:00 Simon Glass <sjg at chromium.org>:
>> Hi,
>>
>> On 4 May 2017 at 03:19, Lukasz Majewski <lukma at denx.de> wrote:
>>> Hi Eddie, Simon,
>>>
>>>> Hi Simon
>>>>
>>>> 2017-05-03 18:09 GMT+08:00 Simon Glass <sjg at chromium.org>:
>>>> > Hi Eddie,
>>>> >
>>>> > On 2 May 2017 at 04:37, Eddie Cai <eddie.cai.linux at gmail.com> wrote:
>>>> >> Hi Simon
>>>> >> 2017-03-20 10:30 GMT+08:00 Simon Glass <sjg at chromium.org>:
>>>> >>> Hi Eddie.
>>>> >>>
>>>> >>> On 15 March 2017 at 01:56, Eddie Cai <eddie.cai.linux at gmail.com>
>>>> >>> wrote:
>>>> >>>> this patch enable rockusb support on rk3288 based device.
>>>> >>>>
>>>> >>>> Signed-off-by: Eddie Cai <eddie.cai.linux at gmail.com>
>>>> >>>> ---
>>>> >>>>  include/configs/rk3288_common.h | 4 ++++
>>>> >>>>  1 file changed, 4 insertions(+)
>>>> >>>
>>>> >>> I think this should be done in Kconfig.
>>>> >> since rockusb used so widely on rockchip soc based devices. every
>>>> >> rockchip soc based
>>>> >> device should support it. So I would like to put it in
>>>> >> rk3288_common.h or even rockchip-common.h.
>>>> >> what do you think?
>>>> >
>>>> > We are moving to removing the board config headers so cannot add new
>>>> > non-Kconfig CONFIG options.
>>>> >
>>>> > You can add it to arch/arm/Kconfig - e.g. with 'imply CONFIG_....'
>>>> > under 'config ARCH_ROCKCHIP'.
>>>> USB_FUNCTION_ROCKUSB depend on USB_GADGET, USB_GADGET_DOWNLOAD,
>>>> CONFIG_G_DNL_VENDOR_NUM, CONFIG_G_DNL_PRODUCT_NUM
>>>> should i move those config to arch/arm/Kconfig? how to define
>>>> CONFIG_G_DNL_VENDOR_NUM
>>>> and CONFIG_G_DNL_PRODUCT_NUM when select it?
>>>
>>> I would prefer to keep those CONFIG options as they are now (and are in
>>> sync with other gadgets).
>>
>> These two are new though right?
>>
>> CONFIG_CMD_ROCKUSB
>> CONFIG_USB_FUNCTION_ROCKUSB
>>
>> So they should be added to Kconfig. If they 'depend' on non-Kconfig
>> options, that's fine IMO. We can't express that until the other
>> options are converted.
>>
>> Let's convert them soon!
> Does it means i can keep it in rk3288_common.h?

Anything new you add must go into defconfig. If it depends on a
non-Kconfig option, then just leave out the depend. We cannot add new
things to the config whitelist, so you should get a build error if you
try to add a new option outside Kconfig.

>>
>>>
>>> The problem here is that the gadget infrastructure is a bit
>>> "problematic" from the device model point of view.
>>>
>>> It has his own "structure" borrowed from Linux kernel.
>>>
>>> I've poked around and it seems like adding it to DM requires calling
>>> "g_dnl_register()" with embedded name of the gadget (like
>>> "usb_dnl_dfu").
>>>
>>> And such call is required for each different gadget (like mass storage,
>>> dfu, fastboot).
>>>
>>> Or maybe we should add "stub" DM support to only indicate supported
>>> gadgets (like dfu, ums, etc) and leave as much code untouched as
>>> possible?
>>
>> I'm sure this can be resolved in some way. I admit I have not looked
>> at it but was thinking of attacking usb_tty sometime, so perhaps might
>> see if I can figure it out.
>>
>>>
>>>> >
>>>> > Please help to remove any options you can from the headers.
>>>> >
>>>> >>>
>>>> >>>>
>>>> >>>> diff --git a/include/configs/rk3288_common.h
>>>> >>>> b/include/configs/rk3288_common.h index b5606d4..b19a34d 100644
>>>> >>>> --- a/include/configs/rk3288_common.h
>>>> >>>> +++ b/include/configs/rk3288_common.h
>>>> >>>> @@ -74,6 +74,10 @@
>>>> >>>>  #define CONFIG_FASTBOOT_BUF_ADDR       CONFIG_SYS_LOAD_ADDR
>>>> >>>>  #define CONFIG_FASTBOOT_BUF_SIZE       0x08000000
>>>> >>>>
>>>> >>>> +/* rockusb  */
>>>> >>>> +#define CONFIG_CMD_ROCKUSB
>>>> >>>> +#define CONFIG_USB_FUNCTION_ROCKUSB
>>>> >>>> +
>>>> >>>>  /* usb mass storage */
>>>> >>>>  #define CONFIG_USB_FUNCTION_MASS_STORAGE
>>>> >>>>  #define CONFIG_CMD_USB_MASS_STORAGE
>>>> >>>> --
>>>> >>>> 2.7.4
>>>> >
>>>> > Regards,
>>>> > Simon
>>
>> Regards,
>> Simon

Regards,
Simon