Re: [tegrarcm PATCH 2/2] tegrarcm: Add support for loading MTS

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3773 bytes --]

On Tue, Jan 13, 2015 at 04:20:02PM -0800, Jimmy Zhang wrote:
> From: Allen Martin <amartin-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> The Denver CPU in Tegra132 requires microcode to be loaded before CPU
> initialization.  There are two microcode files required, "preboot" MTS
> and MTS proper.  Add support for loading MTS from either the binary
> versions checked in or from the command line.
> 
> Signed-off-by: Allen Martin <amartin-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  src/Makefile.am                       |  14 +-
>  src/bin2h.sh                          |   9 ++
>  src/main.c                            | 241 ++++++++++++++++++++++++++++++++--
>  src/miniloader/tegra132-mts.h         |  13 ++
>  src/miniloader/tegra132-preboot-mts.h |  13 ++
>  src/nv3p.c                            |  19 +++
>  src/nv3p.h                            |   9 ++
>  src/rcm.h                             |   1 +
>  src/tegrarcm.1.in                     |   7 +
>  9 files changed, 314 insertions(+), 12 deletions(-)
>  create mode 100755 src/bin2h.sh
>  create mode 100644 src/miniloader/tegra132-mts.h
>  create mode 100644 src/miniloader/tegra132-preboot-mts.h
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index d0d45cad4fee..0e9ff1bbd7f3 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -1,6 +1,14 @@
>  AM_CFLAGS = -Wall -std=c99
>  AM_CPPFLAGS = -isystem /usr/include/$(CRYPTOLIB) $(LIBUSB_CFLAGS)
>  
> +MTS_FIRMWARE_DIR ?= ../../mts-firmware
> +
> +miniloader/mts_si.h: $(MTS_FIRMWARE_DIR)/mts_si bin2h.sh
> +	./bin2h.sh $< $@
> +
> +miniloader/mts_preboot_si.h: $(MTS_FIRMWARE_DIR)/mts_preboot_si bin2h.sh
> +	./bin2h.sh $< $@
> +
>  bin_PROGRAMS = tegrarcm
>  tegrarcm_SOURCES = \
>  	main.c \

Given that these binaries now live in an external repository I think it
makes sense not to generate them in this fashion. The problem is that a
future chip might also require MTS files, in which case it becomes very
unclear where to put which, what version of the binaries to use, etc.

To avoid all that I'd just go with the command-line options and rely on
them exclusively for specifying the MTS binaries to use. In fact I think
we should make that the default for the miniloader as well going forward
to avoid having to further bloat the tegrarcm binary with every new SoC
generation.

> @@ -117,6 +131,16 @@ static void usage(char *progname)
>  	fprintf(stderr, "\t\tminiloader\n");
>  	fprintf(stderr, "\t--miniloader_entry=<mlentry>\n");
>  	fprintf(stderr, "\t\tSpecify the entry point for the miniloader\n");
> +	fprintf(stderr, "\t--preboot=pbfile\n");
> +	fprintf(stderr, "\t\tRead the preboot mts ucode from file instead of using built-in\n");
> +	fprintf(stderr, "\t\tpreboot mts\n");
> +	fprintf(stderr, "\t--preboot_entry=<pbentry>\n");

Can we please change this to --preboot-entry...

> +	fprintf(stderr, "\t\tSpecify the entry point for the preboot mts ucode\n");
> +	fprintf(stderr, "\t--mts=mtsfile\n");
> +	fprintf(stderr, "\t\tRead the mts ucode from file instead of using built-in\n");
> +	fprintf(stderr, "\t\tmts\n");
> +	fprintf(stderr, "\t--mts_entry=<mtsentry>\n");

and this to --mts-entry? I know we've used underscores in the past, but
they are very unconventional in command-line options and I mistype them
every time.

I have local patches to convert the existing command-line options from
underscores to dashes as well. I'll send those out after these patches
have been applied.

There are a couple of other nitpicks, but since I have changes in my
tree (among other things Tegra210 support) that I need to rebase on this
anyway, how about I take over these two patches and resend them as part
of another series?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]