Comment 6 for bug 1846329
Revision history for this message
| Dave Jones (waveform) wrote Re: [Bug 1846329] Re: [FFe] 2019.07 to support Pi4 boot | #6 |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
0e1f616
(Get the code!)
On Thu, 3 Oct 2019 at 19:30, Robie Basak <email address hidden> wrote:
[snip]
> debian/changelog:
>
> Please squash all changelog entries into a single one for 2019.07+dfsg-
> 1ubuntu1. That also means that the changelog should describe only the
> differences from 2019.04+
> as visible from the archive and skipping any churn that took place
> during development in your branch.
Done.
> Looking at your PPA, rather than using the orig tarball as published by
> upstream, that orig tarball is supposed to have been repacked to remove
> non-DFSG code, according to Files-Excluded in debian/copyright. uscan(1)
> can help with this. But in this case an upload of this upstream version
> had already been made by Debian. Therefore, please use the repacked orig
> tarball as published by Debian (eg. as archived at
> https:/
> potential mismatches. There's no point in changing anything in your PPA
> now; your sponsor (presumably me) should be able to correct this when
> uploading.
Ah, that explains one of the lintian warnings I was confused about!
Useful to learn this is the implication of the +dfsg tag when
maintaining such a package.
> Only debian/
> updating, and it was more than a refresh. I suggest avoiding using the
> word "refresh" to describe the change, and please drop the unnecessary
> refresh of the other patches.
Done.
> Bump to v2019.07:
>
> This is not practical to review. I understand it is necessary for rpi4
> support. I'll leave it to the release team to make a call on the
> regression risk.
The cert team have now had a chance to test the new version on a
variety of hardware. Rather ironically the only failure we encountered
was on the Pi Compute Module 3/3+ which turned out to be because our
u-boot builds ignore the device-tree provided by the board. That's now
fixed too and the result has been re-tested on every supported pi
platform and architecture (the required change was only to pi u-boot
configurations).
> python-pyelftools dependency: note that this binary package is in
> universe even though its source is in main. Are you expecting to put u
> -boot-rpi in main? I only see it in universe currently.
Not a question I'd thought about, but it would seem sensible to
propose it for main at some point given our reliance upon it. I assume
this is all stuff that can be sorted out at some arbitrary point in
the future?
> debian/
>
> Typo "Descrpition"
Doh! Fixed.
> Consider doing this with multiple patches, one per commit, depending on
> how often you expect you need to update it against Andrei Gherzan's
> work. Maybe easier to leave it now it's done. Your choice.
Andrei's branch hasn't been touched since July, so I'd guess he's
happy with where it is for now. I'd vaguely hope the next time I touch
this, it'll be to pull the changes from upstream so I'm sanguine about
leaving this as a single blob for now.
> Makefile: DTB addition duplicates existing bcm2837-rpi-3-b.dtb entry
>
> rpi_4_defconfig drops:
> -CONFIG_USB_DWC2=y
> -CONFIG_
> -CONFIG_
> -CONFIG_
> -CONFIG_
> ...are all of these intentional? CONFIG_
No, that's a mistake. Or rather, Andrei's patches didn't include those
and I added them back into the rpi_4_32b_defconfig in the vague hopes
of producing a u-boot that might work across the various platforms.
The result didn't (entirely) work, but it didn't harm Pi4
compatibility either so I left them there for consistency with the
other Pi configs ... and then forgot to add them to the 64-bit config
too. I've updated the 64-bit config to include them for consistency
and will re-test the result after it's built (I don't anticipate any
issue given the 32-bit variant worked happily, but I'm doubly cautious
at this point after the CM3/+ failures).
Incidentally, ENV_IS_IN_FAT is something I want to disable at some
point but for now we need it for Core support; I'm not convinced
SUPPORT_RAW_INITRD is actually required (its special syntax is
currently used in our boot scripts, but it's telling the 64-bit u-boot
worked happily without it); and the two USB_ETHER_* options, and the
DWC2 are to support the ethernet and USB controllers on the Pi 2, 3,
and 3+ (in other words irrelevant for the Pi 4).
> bcm2835_sdhci.c: dev_get_driver_data call: no error checking now this is
> a dynamic lookup and not a constant?
The dev_get_
returning a value from the udevice struct. However, if the MMC device
has no matching "compatible" string then the result will be 0. This is
an invalid eMMC clock identifier (clock IDs start at 1), so the
subsequent bcm2835_
appropriate error message. For the sake of completeness, a test could
be placed prior to this ensuring clock_id isn't 0, but I'm happy that
the current code operates correctly in both valid and invalid
circumstances.
> Quite a bit of RPi code is touched - for example the introduction of
> bcm2835-common.dtsi and status -> status_r/status_w. Are all rpi
> hardware variants tested against this branch?
Yup, as of the weekend the Pi2, 3, CM3, 3+, 3A+, CM3+, and 4 have all
been tested on both armhf and arm64 variants.
> debian/
>
> As we discussed, I'm not sure if we should drop this change (as it's not
> strictly necessary for the rpi4) or keep it (as it cleans things up and
> will make future backports easier). The trade-off is regression risk due
> to the late upload for thi cycle versus increased work in maintaining a
> branch just for Eoan for the same of this change. I'll leave the
> decision between you and the release team.
In investigating this, I discovered another point which I could use to
justify this as "safe":
Our current procedure for building images grabs u-boot.bin from the
u-boot package. However, the image is simply *copied* from the
extracted package without using mkknlimg
(https:/
other words, if this wasn't safe, our current daily images would
*already* be broken (and for the sake of consistency it'd be nice to
have both fresh images and upgraded images use "unadulterated"
binaries).
> debian/
>
> Style: please quote all shell variables where it's trivial to do so.
> Saves a reviewer having to verify that there definitely cannot be any
> whitespace in all cases. $platform_path in particular please.
>
> I would consider:
>
> for uboot_binary in "$UBOOT_
> <something using dirname and basename on $uboot_binary to get the parent directory name>
> cp "$uboot_binary" "$BOOT/
> done
Good point; done.
> This would protect a little better against directories containing other
> things appearing in "$UBOOT_DIR" in the future, although it's still not
> perfect. The reason I noticed this is because rpi-config-
> now being added to /usr/lib/u-boot so the directory no longer contains
> _only_ bootloaders, though your -d check is still perfectly functional
> currently. I suppose what I'm saying is that what /usr/lib/u-boot/
> contains exactly isn't clearly defined, though you are relying on such a
> definition here, which might make it error-prone in the future. However
> in mitigation the packaging is in full control of this and there isn't
> very much of it. Your choice.
Indeed - this also ties into your comment below about the placement of
rpi-config-
> > if dpkg --compare-versions "$2" lt "2019.07+dfsg-1~" && is_old_config;
> then
>
> I find this surprising as 2019.07+dfsg-1 has been published by Debian,
> but this test doesn't involve that version. How about 2019.07+dfsg-
> 1ubuntu1~ instead?
Done. I wasn't sure what the version should be in the case where
Debian *hadn't* released 2019.07+dfsg-1 (which was the case when I
first started hacking on this!), hence why I opted for the "lower"
version.
> debian/
>
> I would put rpi-config-
> the standard does allow /usr/lib as an exception in this case. Is this a
> conscious decision? I would use /usr/share anyway here, as would provide
> a convenient separation between packaging and payload in this case.
> However I leave this choice up to you. Revelant policy:
> https:/
> structure
Good point. It was a conscious decision, but the thought process was
little more than "there's already a /usr/lib/u-boot dir, and ... it's
a lib ... sort of". Moving it to /usr/share also keeps the role of
/usr/lib/u-boot plain and simplifies the postinst, so I've implemented
this.
> Style: please quote all shell variables where it's trivial to do so.
> Saves a reviewer having to verify that there definitely cannot be any
> whitespace in all cases.
Done.
> Perhaps check that the expected-to-be-new files being unconditionally
> written to in migrate_config() do not exist before writing them, and
> consider that another case of "should not migrate"? I guess those tests
> would need to be in is_old_config(), and presumably some files are
> expected to be overwritten so this would only apply to those that
> aren't. Your choice.
In this case I've left it as is largely because, while I'm certain
about keeping usercfg.txt and syscfg.txt (the two tested for) around,
I'm less sure about the other files. They're currently structured for
potential ease of manipulation by a future tool, but as that tool is
still unfinished I'm not 100% certain they'll be like this when
everything's done.