[U-Boot] [PATCH 2/7] usb: rockchip: implement skeleton for K_FW_GET_CHIP_VER command

Alberto Panizzo
2018-07-04T13:27:56Z

On Tue, Jul 03, 2018 at 11:33:45PM +0200, Lukasz Majewski wrote:
> Hi Alberto,
>
> > Chip Version is a string saved in BOOTROM address space Little Endian.
> >
> > Ex for rk3288: 0x33323041 0x32303134 0x30383133 0x56323030
> > which brings: 320A20140813V200
> >
> > Note that memory version do invert MSB/LSB so printing the char
> > buffer will show: A02341023180002V
> >
> > Signed-off-by: Alberto Panizzo <alberto@amarulasolutions.com>
> > ---
> > drivers/usb/gadget/f_rockusb.c | 38
> > +++++++++++++++++++++++++++++++++++++- 1 file changed, 37
> > insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/f_rockusb.c
> > b/drivers/usb/gadget/f_rockusb.c index a39ad51..7612871 100644
> > --- a/drivers/usb/gadget/f_rockusb.c
> > +++ b/drivers/usb/gadget/f_rockusb.c
> > @@ -532,6 +532,42 @@ static void cb_read_storage_id(struct usb_ep
> > *ep, struct usb_request *req) CSW_GOOD);
> > }
> >
> > +int __weak rk_get_bootrom_chip_version(unsigned int *chip_info, int
> > size) +{
> > + return 0;
> > +}
> > +
> > +static void cb_get_chip_version(struct usb_ep *ep, struct
> > usb_request *req) +{
> > + ALLOC_CACHE_ALIGN_BUFFER(struct fsg_bulk_cb_wrap, cbw,
> > + sizeof(struct fsg_bulk_cb_wrap));
> > + unsigned int chip_info[4], i;
> > +
> > + memset(chip_info, 0, sizeof(chip_info));
> > + rk_get_bootrom_chip_version(chip_info, 4);
> > +
> > + /*
> > + * Chip Version is a string saved in BOOTROM address space
> > Little Endian
> > + *
> > + * Ex for rk3288: 0x33323041 0x32303134 0x30383133 0x56323030
> > + * which brings: 320A20140813V200
> > + *
> > + * Note that memory version do invert MSB/LSB so printing
> > the char
> > + * buffer will show: A02341023180002V
> > + */
> > + printf("read chip version: ");
> > + for (i = 0; i < 16; i++) {
> > + int shift = (3 - (i % 4)) * 8;
> > +
> > + printf("%c", (char)((chip_info[i / 4] >> shift) &
> > 0xFF));
>
> A lot of magic numbers. Just to ask - isn't this the same type of
> conversion as we got with the network code?
>
> Cannot we have simple macro (or static inline) with byte swap called
> three times?
>

Will doublecheck but looks possible.

> > + }
> > + printf("\n");
> > + memcpy((char *)cbw, req->buf, USB_BULK_CB_WRAP_LEN);
> > + rockusb_tx_write((char *)chip_info, sizeof(chip_info));
> ^---- [1]
>
> > + rockusb_tx_write_csw_on_complete(cbw->tag,
> ^-----[2]
>
> > cbw->data_transfer_length,
> > + CSW_GOOD);
>
> Just to be sure if I understand the protocol -> you write the data in
> [1]
> And then immediately you prepare next block (structure) [2] to be
> written back after receiving reply data from host?

No, Host just waits for a second message as CSW.
What I did with first patch was to serialize sending of both USB
messages, so that sending the second would not voiding the first.

>
> Is this behaviour in sync with README in ./doc/README.rockusb ?
>

You are right, I completely missed to update doc/README.rockusb

Thanks,

Best Regards,
Alberto Panizzo

--
Presidente CDA
Amarula Solutions SRL Via le Canevare 30 31100 Treviso Italy
CTO - Co-Founder
Amarula Solutions BV Cruquiuskade 47 Amsterdam 1018 AM The Netherlands
Phone. +31(0)851119171 Fax. +31(0)204106211 www.amarulasolutions.com

> > +}
> > +
> > static void cb_write_lba(struct usb_ep *ep, struct usb_request *req)
> > {
> > ALLOC_CACHE_ALIGN_BUFFER(struct fsg_bulk_cb_wrap, cbw,
> > @@ -670,7 +706,7 @@ static const struct cmd_dispatch_info
> > cmd_dispatch_info[] = { },
> > {
> > .cmd = K_FW_GET_CHIP_VER,
> > - .cb = cb_not_support,
> > + .cb = cb_get_chip_version,
> > },
> > {
> > .cmd = K_FW_LOW_FORMAT,
>
>
>
>
> Best regards,
>
> Lukasz Majewski
>
> --
>
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de