[RFC PATCH 5/5] ipq40xx: add target for Google WiFi (Gale)

[Brian Norris]

Hi Adrian,

On Sat, Jul 18, 2020 at 2:13 PM <mail at adrianschmutzler.de> wrote:
> just some formal stuff below.

Thanks! It's useful, especially where I'm still learning the OpenWRT
Makefile structures.

I'll spin up a new revision sooner or later, but I'm hoping I'll get
some opinion on the RFC portion (cover letter / first ~3 patches)
before doing that.

> > -----Original Message-----
> > From: openwrt-devel [mailto:openwrt-devel-bounces at lists.openwrt.org]
> > On Behalf Of Brian Norris
> > Sent: Samstag, 18. Juli 2020 22:52
> > To: openwrt-devel at lists.openwrt.org
> > Cc: Brian Norris <computersforpeace at gmail.com>
> > Subject: [RFC PATCH 5/5] ipq40xx: add target for Google WiFi (Gale)
...
> > I include "factory.bin" support, where we generate a GPT-based disk image
> > with 2 partitions -- a kernel partition (using the custom "Chrome OS kernel"
> > GUID type) and a root filesystem partition. If the AP is in Developer Mode,
> > the stock bootloader can boot it via a USB disk or (after gaining access via USB
> > boot) flashed to the eMMC.
>
> I'd like a bit more detailed flashing instructions here. What you have right now only works if the reader knows what to do anyway.

Well, it's not exactly trivial, but I guess not that difficult,
relative to the average device OpenWRT supports. As luck would have
it, somebody has already documented it:

https://github.com/marcosscriven/galeforce#how-to-apply-an-image

Would it be best to summarize, link, or both?

> > +++ b/target/linux/ipq40xx/files/arch/arm/boot/dts/qcom-ipq4019-gale-v2.
> > +++ dts
> > @@ -0,0 +1,402 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2016, 2018 The Linux Foundation. All rights reserved.
> > + * Copyright (c) 2016 Google, Inc
> > + */
> > +
> > +#include "qcom-ipq4019.dtsi"
> > +#include <dt-bindings/input/input.h>
> > +#include <dt-bindings/gpio/gpio.h>
> > +
> > +&tlmm {
>
> I'd have expected the root node first.

I've learned muscle memory to put things like pinctrl first, because
they're often referenced by other nodes (possibly including in the
root node) via phandle, and at least in the past, DTC would not
support out-of-order references for phandles.

But that doesn't apply here (no phandles from the root section), so
that doesn't apply. Will change.

> > +/ {
> > +     model = "Google IPQ4019/Gale";
>
> This should be generally consistent with what you choose for DEVICE_MODEL. See my others comments there below.
>
> > +     compatible = "google,gale-v2", "qcom,ipq4019";
...
> > --- a/target/linux/ipq40xx/image/Makefile
> > +++ b/target/linux/ipq40xx/image/Makefile
> > @@ -218,6 +218,20 @@ define Device/avm_fritzbox-4040  endef
> > TARGET_DEVICES += avm_fritzbox-4040
> >
> > +define Device/google_gale-v2
>
> Alphabetic sorting.
>
> > +     DEVICE_VENDOR := Google
> > +     DEVICE_MODEL := WiFi (Gale)
>
> This uses yet another name compared to device definition/compatible and model in DTS.
> Please be more consistent. Despite, if the device node (and thus the image) has a v2 in it, please also add
>
> DEVICE_VARIANT := v2
>
> here.

Thanks for the scrutiny! I do have some questions here: is the
VENDOR/MODEL supposed to match closer to a
marketing-friendly/user-friendly name, or a developer/low-level name?
Or just some balance of both? Because there's several constraints
here:
* The bootloader recognizes compatible="google,gale-v2" -- I don't
believe I can reliably drop the "v2" there, but I suppose that doesn't
require the file names, etc., to include it
* There really is no v1 publicly-available; as noted in the commit
message, I believe that was pre-release hardware, and the revisions
just stuck around through development
* the "v2" here does *not* mean second generation, as in
https://en.wikipedia.org/wiki/Google_Nest_Wifi#Second_generation
* "WiFi" doesn't really make for a good MODEL on its own, although
it's OK when paired with the VENDOR. But I still preferred sticking
the codename (Gale) around, since that's the unambiguous way hackers
can recognize the model.

What do you think? Should I try to keep the keywords "google", "wifi",
and "gale" in all of the config, image, and DTS name? And I'll avoid
the "v2" labeling (and DEVICE_VARIANT) outside of "compatible",
because I think that would be misleading.

Anyway, I'll try to figure out a better balance of the above on the
next version.

Thanks,
Brian

> > +     SOC := qcom-ipq4019
> > +     DEVICE_DTS := qcom-ipq4019-gale-v2
>
> This line can be dropped, it will be calculated from SOC and device node name automatically.