On Tue, 6 Nov 2018 at 17:20, Nicolas Dechesne <nicolas.dechesne@linaro.org> wrote:

On Tue, Nov 6, 2018 at 12:39 PM laxman siripuram
<itsmelaxman91@gmail.com> wrote:
>
> Hi nico,
> when i checked on sd410c based board before i found qdl not started with out giving "--s emmc". I tried several times before writing you the previous mail but found the same behaviour.
>
> But now i tried doing qdl again, now its working fine always and also working fine always with specifying "--s <any string>".
>
> fyi,
> i've tried passing "--s ufs" "--s xxx" "--s abc" still worked fine. it should not work right? so this arg has no impact for sd410 based boards?

right, i've tested it as well, and it works fine even with your
patch.. Looking at the log, the firmware is telling us that it expects
MemoryName to be eMMC, see the response value below..

FIREHOSE WRITE: <?xml version="1.0"?>
<data><configure MemoryName="ufs"
MaxPayloadSizeToTargetInBytes="1048576" verbose="0" ZLPAwareHost="0"
SkipStorageInit="0"/></data>

FIREHOSE READ: <?xml version="1.0" encoding="UTF-8" ?><data><log
value="Host's payload to target size is too large" /></data>
LOG: Host's payload to target size is too large

FIREHOSE READ: <?xml version="1.0" encoding="UTF-8" ?><data><response
value="NAK" MinVersionSupported="1" MemoryName="eMMC"
MaxPayloadSizeFromTargetInBytes="4096"
MaxPayloadSizeToTargetInBytes="16384"
MaxPayloadSizeToTargetInBytesSupported="16384"
MaxXMLSizeInBytes="4096" Version="1" TargetName="8916" /></data>

FIREHOSE WRITE: <?xml version="1.0"?>
<data><configure MemoryName="ufs"
MaxPayloadSizeToTargetInBytes="16384" verbose="0" ZLPAwareHost="0"
SkipStorageInit="0"/></data>

FIREHOSE READ: <?xml version="1.0" encoding="UTF-8" ?><data><response
value="ACK" MinVersionSupported="1" MemoryName="eMMC"
MaxPayloadSizeFromTargetInBytes="4096"
MaxPayloadSizeToTargetInBytes="16384"
MaxPayloadSizeToTargetInBytesSupported="16384"
MaxXMLSizeInBytes="4096" Version="1" TargetName="8916" /></data>

So somehow the firmware ignores our value for MemoryName and keeps it
set to emmc.. Not sure if it's a lucky firmware bug, or if it is
specified on devices which only support eMMC, then MemoryName is being
ignored..

Anyways, your patch is still good to go.

>
> Regards,
> Laxman
>
> On Tue, 6 Nov 2018 at 16:26, Nicolas Dechesne <nicolas.dechesne@linaro.org> wrote:
>>
>> On Tue, Nov 6, 2018 at 11:51 AM laxman siripuram
>> <itsmelaxman91@gmail.com> wrote:
>> >
>> > Hi nico,
>> > Thanks for confirmation and for DB410c recovery(with this patch) it is mandatory to use "--s emmc" for qdl to work.
>>
>> hmm. I was not expecting that actually. I need to test that as well.
>> Before that patch we were already setting MemoryNode to ufs , even on
>> DB410c and it worked. let me have a look.
>> >
>> > Regards,
>> > Laxman
>> >
>> > On Tue, 6 Nov 2018 at 16:06, Nicolas Dechesne <nicolas.dechesne@linaro.org> wrote:
>> >>
>> >> hi Laxman,
>> >>
>> >> thanks for your patch and improvements. I have tested it on my
>> >> DB820c, and it works fine. Also tested that I get a failure when using
>> >> --storage emmc, as expected.
>> >>
>> >> On Tue, Nov 6, 2018 at 9:54 AM Laxman <itsmelaxman91@gmail.com> wrote:
>> >> >
>> >> > added qdl support for emmc storage
>> >> > use option --s emmc or ufs as a argument to qdl command
>> >> > if not specified any option the default storage would be ufs
>> >> >
>> >> > Signed-off-by: Laxman <itsmelaxman91@gmail.com>
>> >>
>> >> Tested-by: Nicolas Dechesne <nicolas.dechesne@linaro.org>
>> >>
>> >> > ---
>> >> > firehose.c | 16 ++++++++--------
>> >> > qdl.c | 10 +++++++---
>> >> > qdl.h | 2 +-
>> >> > 3 files changed, 16 insertions(+), 12 deletions(-)
>> >> >
>> >> > diff --git a/firehose.c b/firehose.c
>> >> > index c4e9500..cae90bd 100644
>> >> > --- a/firehose.c
>> >> > +++ b/firehose.c
>> >> > @@ -265,7 +265,7 @@ static int firehose_configure_response_parser(xmlNode *node)
>> >> > return max_size;
>> >> > }
>> >> >
>> >> > -static int firehose_send_configure(int fd, size_t payload_size, bool skip_storage_init)
>> >> > +static int firehose_send_configure(int fd, size_t payload_size, bool skip_storage_init, const char *storage)
>> >> > {
>> >> > xmlNode *root;
>> >> > xmlNode *node;
>> >> > @@ -277,7 +277,7 @@ static int firehose_send_configure(int fd, size_t payload_size, bool skip_storag
>> >> > xmlDocSetRootElement(doc, root);
>> >> >
>> >> > node = xmlNewChild(root, NULL, (xmlChar*)"configure", NULL);
>> >> > - xml_setpropf(node, "MemoryName", "ufs");
>> >> > + xml_setpropf(node, "MemoryName", storage);
>> >> > xml_setpropf(node, "MaxPayloadSizeToTargetInBytes", "%d", payload_size);
>> >> > xml_setpropf(node, "verbose", "%d", 0);
>> >> > xml_setpropf(node, "ZLPAwareHost", "%d", 0);
>> >> > @@ -291,17 +291,17 @@ static int firehose_send_configure(int fd, size_t payload_size, bool skip_storag
>> >> > return firehose_read(fd, -1, firehose_configure_response_parser);
>> >> > }
>> >> >
>> >> > -static int firehose_configure(int fd, bool skip_storage_init)
>> >> > +static int firehose_configure(int fd, bool skip_storage_init, const char *storage)
>> >> > {
>> >> > int ret;
>> >> >
>> >> > - ret = firehose_send_configure(fd, max_payload_size, skip_storage_init);
>> >> > + ret = firehose_send_configure(fd, max_payload_size, skip_storage_init, storage);
>> >> > if (ret < 0)
>> >> > return ret;
>> >> >
>> >> > /* Retry if remote proposed different size */
>> >> > if (ret != max_payload_size) {
>> >> > - ret = firehose_send_configure(fd, ret, skip_storage_init);
>> >> > + ret = firehose_send_configure(fd, ret, skip_storage_init, storage);
>> >> > if (ret < 0)
>> >> > return ret;
>> >> >
>> >> > @@ -601,7 +601,7 @@ static int firehose_reset(int fd)
>> >> > return firehose_read(fd, -1, firehose_nop_parser);
>> >> > }
>> >> >
>> >> > -int firehose_run(int fd, const char *incdir)
>> >> > +int firehose_run(int fd, const char *incdir, const char *storage)
>> >> > {
>> >> > int bootable;
>> >> > int ret;
>> >> > @@ -618,7 +618,7 @@ int firehose_run(int fd, const char *incdir)
>> >> > return ret;
>> >> >
>> >> > if(ufs_need_provisioning()) {
>> >> > - ret = firehose_configure(fd, true);
>> >> > + ret = firehose_configure(fd, true, storage);
>> >> > if (ret)
>> >> > return ret;
>> >> > ret = ufs_provisioning_execute(fd, firehose_apply_ufs_common,
>> >> > @@ -630,7 +630,7 @@ int firehose_run(int fd, const char *incdir)
>> >> > return ret;
>> >> > }
>> >> >
>> >> > - ret = firehose_configure(fd, false);
>> >> > + ret = firehose_configure(fd, false, storage);
>> >> > if (ret)
>> >> > return ret;
>> >> >
>> >> > diff --git a/qdl.c b/qdl.c
>> >> > index 8cf1011..98801cc 100644
>> >> > --- a/qdl.c
>> >> > +++ b/qdl.c
>> >> > @@ -220,14 +220,14 @@ static void print_usage(void)
>> >> > {
>> >> > extern const char *__progname;
>> >> > fprintf(stderr,
>> >> > - "%s [--debug] [--finalize-provisioning] [--include <PATH>] <prog.mbn> [<program> <patch> ...]\n",
>> >> > + "%s [--debug] [--storage <emmc|ufs>] [--finalize-provisioning] [--include <PATH>] <prog.mbn> [<program> <patch> ...]\n",
>> >> > __progname);
>> >> > }
>> >> >
>> >> > int main(int argc, char **argv)
>> >> > {
>> >> > struct termios tios;
>> >> > - char *prog_mbn;
>> >> > + char *prog_mbn, *storage="ufs";
>> >> > char *incdir = NULL;
>> >> > int type;
>> >> > int ret;
>> >> > @@ -240,6 +240,7 @@ int main(int argc, char **argv)
>> >> > {"debug", no_argument, 0, 'd'},
>> >> > {"include", required_argument, 0, 'i'},
>> >> > {"finalize-provisioning", no_argument, 0, 'l'},
>> >> > + {"storage", required_argument, 0, 's'},
>> >> > {0, 0, 0, 0}
>> >> > };
>> >> >
>> >> > @@ -254,6 +255,9 @@ int main(int argc, char **argv)
>> >> > case 'l':
>> >> > qdl_finalize_provisioning = true;
>> >> > break;
>> >> > + case 's':
>> >> > + storage = optarg;
>> >> > + break;
>> >> > default:
>> >> > print_usage();
>> >> > return 1;
>> >> > @@ -303,7 +307,7 @@ int main(int argc, char **argv)
>> >> > if (ret < 0)
>> >> > goto out;
>> >> >
>> >> > - ret = firehose_run(fd, incdir);
>> >> > + ret = firehose_run(fd, incdir, storage);
>> >> >
>> >> > out:
>> >> > ret = tcsetattr(fd, TCSANOW, &tios);
>> >> > diff --git a/qdl.h b/qdl.h
>> >> > index 73ea84c..f835442 100644
>> >> > --- a/qdl.h
>> >> > +++ b/qdl.h
>> >> > @@ -7,7 +7,7 @@
>> >> > #include "program.h"
>> >> > #include <libxml/tree.h>
>> >> >
>> >> > -int firehose_run(int fd, const char *incdir);
>> >> > +int firehose_run(int fd, const char *incdir, const char *storage);
>> >> > int sahara_run(int fd, char *prog_mbn);
>> >> > void print_hex_dump(const char *prefix, const void *buf, size_t len);
>> >> > unsigned attr_as_unsigned(xmlNode *node, const char *attr, int *errors);
>> >> > --
>> >> > 2.7.4
>> >> >