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

[Simon Glass]

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!

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