On Thu, May 3, 2018 at 9:39 PM Joe Hershberger <joe.hershber...@ni.com>
wrote:
> On Mon, Apr 30, 2018 at 3:32 AM, Alex Kiernan <alex.kier...@gmail.com>
wrote:
<snip>
> > +/**
> > + * Constructs and sends a packet in response to received fastboot
packet
> > + *
> > + * @param fb_header Header for response packet
> > + * @param fastboot_data Pointer to received fastboot data
> > + * @param fastboot_data_len Length of received fastboot data
> > + * @param retransmit Nonzero if sending last sent packet
> > + */
> > +static void fastboot_send(struct fastboot_header fb_header, char
*fastboot_data,
> > + unsigned int fastboot_data_len, uchar
retransmit)
> > +{
> > + uchar *packet;
> > + uchar *packet_base;
> > + int len = 0;
> > + const char *error_msg = "An error occurred.";
> > + short tmp;
> > + struct fastboot_header fb_response_header = fb_header;
> > + char response[FASTBOOT_RESPONSE_LEN] = {0};
> > + /*
> > + * We will always be sending some sort of packet, so
> > + * cobble together the packet headers now.
> > + */
> > + packet = net_tx_packet + net_eth_hdr_size() + IP_UDP_HDR_SIZE;
> > + packet_base = packet;
> > +
> > + /* Resend last packet */
> > + if (retransmit) {
> > + memcpy(packet, last_packet, last_packet_len);
> > + net_send_udp_packet(net_server_ethaddr,
fastboot_remote_ip,
> > + fastboot_remote_port,
fastboot_our_port,
> > + last_packet_len);
> > + return;
> > + }
> > +
> > + fb_response_header.seq = htons(fb_response_header.seq);
> > + memcpy(packet, &fb_response_header, sizeof(fb_response_header));
> > + packet += sizeof(fb_response_header);
> > +
> > + switch (fb_header.id) {
> > + case FASTBOOT_QUERY:
> > + tmp = htons(fb_sequence_number);
> > + memcpy(packet, &tmp, sizeof(tmp));
> > + packet += sizeof(tmp);
> > + break;
> > + case FASTBOOT_INIT:
> > + tmp = htons(fb_udp_version);
> > + memcpy(packet, &tmp, sizeof(tmp));
> > + packet += sizeof(tmp);
> > + tmp = htons(fb_packet_size);
> > + memcpy(packet, &tmp, sizeof(tmp));
> > + packet += sizeof(tmp);
> > + break;
> > + case FASTBOOT_ERROR:
> > + memcpy(packet, error_msg, strlen(error_msg));
> > + packet += strlen(error_msg);
> > + break;
> > + case FASTBOOT_FASTBOOT:
> > + if (!cmd_string) {
> > + /* Parse command and send ack */
> > + cmd_parameter = fastboot_data;
> This seems unnecessary. There are only 4 cases handled, and of them
> only download seems to be a command that happens more than once. And
> in download, the first past through this parameter is saved internally
> as bytes_expected.
> > + cmd_string = strsep(&cmd_parameter, ":");
> > + cmd_string = strdup(cmd_string);
> > + if (cmd_parameter)
> > + cmd_parameter = strdup(cmd_parameter);
> > + } else if (!strcmp("getvar", cmd_string)) {
> > + fb_getvar(response);
> Seems like you should simply pass the "fastboot_data" as a parameter
> here rather than using a global.
I'm completely pulling this apart in a later patch. I wonder if I should
really be folding some of these back into this - I was trying to maintain a
clear line to the AOSP code which this came from.
> > +/**
> > + * Copies image data from fastboot_data to CONFIG_FASTBOOT_BUF_ADDR.
> > + * Writes to response.
> > + *
> > + * @param fastboot_data Pointer to received fastboot data
> > + * @param fastboot_data_len Length of received fastboot data
> > + * @param repsonse Pointer to fastboot response buffer
> > + */
> > +static void fb_download(char *fastboot_data, unsigned int
fastboot_data_len,
> > + char *response)
> > +{
> > + char *tmp;
> > +
> > + if (bytes_expected == 0) {
> > + if (!cmd_parameter) {
> > + write_fb_response("FAIL", "Expected command
parameter",
> > + response);
> > + return;
> > + }
> > + bytes_expected = simple_strtoul(cmd_parameter, &tmp,
16);
> > + if (bytes_expected == 0) {
> > + write_fb_response("FAIL", "Expected nonzero
image size",
> > + response);
> > + return;
> > + }
> > + }
> > + if (fastboot_data_len == 0 && bytes_received == 0) {
> > + /* Nothing to download yet. Response is of the form:
> > + * [DATA|FAIL]$cmd_parameter
> > + *
> > + * where cmd_parameter is an 8 digit hexadecimal number
> > + */
> > + if (bytes_expected > CONFIG_FASTBOOT_BUF_SIZE)
> > + write_fb_response("FAIL", cmd_parameter,
response);
> > + else
> > + write_fb_response("DATA", cmd_parameter,
response);
> > + } else if (fastboot_data_len == 0 &&
> > + (bytes_received >= bytes_expected)) {
> > + /* Download complete. Respond with "OKAY" */
> > + write_fb_response("OKAY", "", response);
> > + image_size = bytes_received;
> > + bytes_expected = 0;
> > + bytes_received = 0;
> > + } else {
> > + if (fastboot_data_len == 0 ||
> > + (bytes_received + fastboot_data_len) >
bytes_expected) {
> > + write_fb_response("FAIL",
> > + "Received invalid data
length",
> > + response);
> > + return;
> > + }
> > + /* Download data to CONFIG_FASTBOOT_BUF_ADDR */
> > + memcpy((void *)CONFIG_FASTBOOT_BUF_ADDR +
bytes_received,
> > + fastboot_data, fastboot_data_len);
> This is different than all the other approaches, which take the load
> address as a command parameter. Is there a good reason to have it
> baked in like this?
Feels odd to me too, but this is just following the USB code. I'm thinking
we should change the command so it's:
fastboot ... [<load-address> [<size>]]
And then default to the current values is they're not overridden?
> > + bytes_received += fastboot_data_len;
> > + }
> > +}
> > +
> > +/**
> > + * Writes the previously downloaded image to the partition indicated by
> > + * cmd_parameter. Writes to response.
> > + *
> > + * @param repsonse Pointer to fastboot response buffer
> > + */
> > +static void fb_flash(char *response)
> > +{
> > + fb_mmc_flash_write(cmd_parameter, (void
*)CONFIG_FASTBOOT_BUF_ADDR,
> > + image_size, response);
> It's peculiar that this hard-codes mmc.
Fixed up in a later patch.
> > +/**
> > + * Boots into downloaded image.
> > + */
> > +static void boot_downloaded_image(void)
> > +{
> > + char kernel_addr[12];
> > + char *fdt_addr = env_get("fdt_addr_r");
> > + char *const bootm_args[] = {
> > + "bootm", kernel_addr, "-", fdt_addr, NULL
> > + };
> It seems like this should be able to affected from the host side...
> for instance choosing a config in an ITB.
> How is the fdt supposed to be populated in the scenario at all?
I strip this bit out in a later patch and replace it with either a simple
bootm (which follows the USB code), or run fastbootcmd.
Again, this is part of trying to land this patch as close as possible to
the code which comes from AOSP.
--
Alex Kiernan
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
