On Tue, Sep 24, 2024 at 12:24:16PM +0200, Quentin Schulz wrote:
> Hi Chris,
>
> On 9/23/24 7:36 PM, Chris Morgan wrote:
> > On Mon, Sep 23, 2024 at 01:21:01PM +0200, Quentin Schulz wrote:
> > > Hi Chris,
> > >
> > > On 9/19/24 4:00 PM, Chris Morgan wrote:
> > > > From: Chris Morgan <[email protected]>
> > > >
> > > > Some of the Powkiddy devices switched to using a different vendor for
> > > > the vdd_cpu regulator. Unfortunately the device does not have a new
> > > > revision to denote this, so users have no way of knowing in advance.
> > > >
> > > > Add code to detect if a device is present at addresses 0x1c or 0x40 on
> > > > the i2c0 bus and update the devicetree if needed.
> > > >
> > > > Signed-off-by: Chris Morgan <[email protected]>
> > > > ---
> > > > board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c | 147
> > > > +++++++++++++++++++++
> > > > 1 file changed, 147 insertions(+)
> > > >
> > > > diff --git a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> > > > b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> > > > index 224019f9ba3..c1d1826fd14 100644
> > > > --- a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> > > > +++ b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> > > > @@ -43,6 +43,7 @@ struct rg3xx_model {
> > > > const char *board_name;
> > > > const char *fdtfile;
> > > > const bool detect_panel;
> > > > + const bool detect_regulator;
> > > > const bool uart_con;
> > > > };
> > > > @@ -69,6 +70,7 @@ static const struct rg3xx_model rg3xx_model_details[]
> > > > = {
> > > > /* Device is identical to RG353P. */
> > > > .fdtfile = DTB_DIR "rk3566-anbernic-rg353p.dtb",
> > > > .detect_panel = 1,
> > > > + .detect_regulator = 0,
> > > > .uart_con = 1,
> > > > },
> > > > [RG353P] = {
> > > > @@ -77,6 +79,7 @@ static const struct rg3xx_model rg3xx_model_details[]
> > > > = {
> > > > .board_name = "Anbernic RG353P",
> > > > .fdtfile = DTB_DIR "rk3566-anbernic-rg353p.dtb",
> > > > .detect_panel = 1,
> > > > + .detect_regulator = 0,
> > > > .uart_con = 1,
> > > > },
> > > > [RG353V] = {
> > > > @@ -85,6 +88,7 @@ static const struct rg3xx_model rg3xx_model_details[]
> > > > = {
> > > > .board_name = "Anbernic RG353V",
> > > > .fdtfile = DTB_DIR "rk3566-anbernic-rg353v.dtb",
> > > > .detect_panel = 1,
> > > > + .detect_regulator = 0,
> > > > .uart_con = 1,
> > > > },
> > > > [RG503] = {
> > > > @@ -93,6 +97,7 @@ static const struct rg3xx_model rg3xx_model_details[]
> > > > = {
> > > > .board_name = "Anbernic RG503",
> > > > .fdtfile = DTB_DIR "rk3566-anbernic-rg503.dtb",
> > > > .detect_panel = 0,
> > > > + .detect_regulator = 0,
> > > > .uart_con = 1,
> > > > },
> > > > [RGB30] = {
> > > > @@ -101,6 +106,7 @@ static const struct rg3xx_model
> > > > rg3xx_model_details[] = {
> > > > .board_name = "Powkiddy RGB30",
> > > > .fdtfile = DTB_DIR "rk3566-powkiddy-rgb30.dtb",
> > > > .detect_panel = 0,
> > > > + .detect_regulator = 1,
> > > > .uart_con = 0,
> > > > },
> > > > [RK2023] = {
> > > > @@ -109,6 +115,7 @@ static const struct rg3xx_model
> > > > rg3xx_model_details[] = {
> > > > .board_name = "Powkiddy RK2023",
> > > > .fdtfile = DTB_DIR "rk3566-powkiddy-rk2023.dtb",
> > > > .detect_panel = 0,
> > > > + .detect_regulator = 1,
> > > > .uart_con = 0,
> > > > },
> > > > [RGARCD] = {
> > > > @@ -117,6 +124,7 @@ static const struct rg3xx_model
> > > > rg3xx_model_details[] = {
> > > > .board_name = "Anbernic RG ARC-D",
> > > > .fdtfile = DTB_DIR "rk3566-anbernic-rg-arc-d.dtb",
> > > > .detect_panel = 0,
> > > > + .detect_regulator = 0,
> > > > .uart_con = 1,
> > > > },
> > > > [RGB10MAX3] = {
> > > > @@ -125,6 +133,7 @@ static const struct rg3xx_model
> > > > rg3xx_model_details[] = {
> > > > .board_name = "Powkiddy RGB10MAX3",
> > > > .fdtfile = DTB_DIR "rk3566-powkiddy-rgb10max3.dtb",
> > > > .detect_panel = 0,
> > > > + .detect_regulator = 1,
> > > > .uart_con = 0,
> > > > },
> > > > /* Devices with duplicate ADC value */
> > > > @@ -134,6 +143,7 @@ static const struct rg3xx_model
> > > > rg3xx_model_details[] = {
> > > > .board_name = "Anbernic RG353PS",
> > > > .fdtfile = DTB_DIR "rk3566-anbernic-rg353ps.dtb",
> > > > .detect_panel = 1,
> > > > + .detect_regulator = 0,
> > > > .uart_con = 1,
> > > > },
> > > > [RG353VS] = {
> > > > @@ -142,6 +152,7 @@ static const struct rg3xx_model
> > > > rg3xx_model_details[] = {
> > > > .board_name = "Anbernic RG353VS",
> > > > .fdtfile = DTB_DIR "rk3566-anbernic-rg353vs.dtb",
> > > > .detect_panel = 1,
> > > > + .detect_regulator = 0,
> > > > .uart_con = 1,
> > > > },
> > > > [RGARCS] = {
> > > > @@ -150,6 +161,7 @@ static const struct rg3xx_model
> > > > rg3xx_model_details[] = {
> > > > .board_name = "Anbernic RG ARC-S",
> > > > .fdtfile = DTB_DIR "rk3566-anbernic-rg-arc-s.dtb",
> > > > .detect_panel = 0,
> > > > + .detect_regulator = 0,
> > > > .uart_con = 1,
> > > > },
> > > > };
> > > > @@ -172,6 +184,22 @@ static const struct rg353_panel
> > > > rg353_panel_details[] = {
> > > > },
> > > > };
> > > > +struct powkiddy_regulators {
> > > > + const u8 addr;
> > > > + const char *regulator_compat;
> > > > +};
> > > > +
> > > > +static const struct powkiddy_regulators regulator_details[] = {
> > > > + {
> > > > + .addr = 0x1c,
> > > > + .regulator_compat = "tcs,tcs4525",
> > > > + },
> > > > + {
> > > > + .addr = 0x40,
> > > > + .regulator_compat = "fcs,fan53555",
> > > > + },
> > > > +};
> > > > +
> > > > /*
> > > > * Start LED very early so user knows device is on. Set color
> > > > * to red.
> > > > @@ -361,6 +389,44 @@ int rgxx3_detect_display(void)
> > > > return 0;
> > > > }
> > > > +/*
> > > > + * Some of the Powkiddy devices switched the CPU regulator, but users
> > > > + * are not able to determine this by looking at their hardware.
> > > > + * Attempt to auto-detect this situation and fixup the device-tree.
> > > > + */
> > > > +int rgxx3_detect_regulator(void)
> > > > +{
> > > > + struct udevice *bus;
> > > > + struct udevice *chip;
> > > > + u8 val;
> > > > + int ret;
> > > > +
> > > > + /* Get the correct i2c bus (i2c0). */
> > > > + ret = uclass_get_device_by_name(UCLASS_I2C,
> > > > + "i2c@fdd40000", &bus);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + /*
> > > > + * Check for all vdd_cpu regulators and read an arbitrary
> > > > + * register to confirm it's present.
> > > > + */
> > > > + for (int i = 0; i < ARRAY_SIZE(regulator_details); i++) {
> > > > + ret = i2c_get_chip(bus, regulator_details[i].addr,
> > > > + 1, &chip);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = dm_i2c_read(chip, 0, &val, 1);
> > > > + if (!ret) {
> > > > + env_set("vdd_cpu",
> > > > regulator_details[i].regulator_compat);
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > int rgxx3_read_board_id(void)
> > > > {
> > > > u32 adc_info;
> > > > @@ -485,6 +551,16 @@ int rk_board_late_init(void)
> > > > printf("Failed to detect panel type\n");
> > > > }
> > > > + /*
> > > > + * Skip vdd_cpu regulator detection if not needed. Warn but
> > > > + * don't fail for errors in auto-detection of regulator.
> > > > + */
> > > > + if (rg3xx_model_details[gd->board_type].detect_regulator) {
> > > > + ret = rgxx3_detect_regulator();
> > > > + if (ret)
> > > > + printf("Unable to detect vdd_cpu regulator\n");
> > > > + }
> > > > +
> > > > end:
> > > > /* Turn off red LED and turn on orange LED. */
> > > > writel(GPIO_WRITEMASK(GPIO_C7 | GPIO_C6 | GPIO_C5) | GPIO_C6,
> > > > @@ -547,6 +623,71 @@ int rgxx3_panel_fixup(void *blob)
> > > > return 0;
> > > > }
> > > > +int rgxx3_regulator_fixup(void *blob)
> > > > +{
> > > > + const struct powkiddy_regulators *vdd_cpu = NULL;
> > > > + int node, ret, i;
> > > > + char path[] = "/i2c@fdd40000/regulator@00";
> > > > + char name[] = "regulator@00";
> > > > + char *env;
> > > > +
> > > > + env = env_get("vdd_cpu");
> > > > + if (!env) {
> > > > + printf("Can't get vdd_cpu env\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + /*
> > > > + * Find the device we have in our tree, which may or may not
> > > > + * be present.
> > > > + */
> > > > + for (i = 0; i < ARRAY_SIZE(regulator_details); i++) {
> > > > + sprintf(path, "/i2c@fdd40000/regulator@%02x",
> > > > + regulator_details[i].addr);
> > > > + node = fdt_path_offset(blob, path);
> > > > + if (node > 0)
> > > > + break;
> > > > +
> > > > + printf("Unable to find vdd_cpu\n");
> > > > + return -ENODEV;
> > > > + }
> > > > +
> > > > + node = fdt_path_offset(blob, path);
> > > > + if (!(node > 0)) {
> > > > + printf("Can't find the vdd_cpu node\n");
> > > > + return -ENODEV;
> > > > + }
> > > > +
> > > > + ret = fdt_node_check_compatible(blob, node, env);
> > > > + if (ret < 0)
> > > > + return -ENODEV;
> > > > +
> > > > + /* vdd_cpu regulators match, return 0. */
> > > > + if (!ret)
> > > > + return 0;
> > > > +
> > > > + /* Regulators don't match, search by first compatible value. */
> > > > + for (i = 0; i < ARRAY_SIZE(regulator_details); i++) {
> > > > + if (!strcmp(env,
> > > > regulator_details[i].regulator_compat)) {
> > > > + vdd_cpu = ®ulator_details[i];
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + if (!vdd_cpu) {
> > > > + printf("Unable to identify vdd_cpu by compat string\n");
> > > > + return -ENODEV;
> > > > + }
> > > > +
> > > > + /* Set the compatible and reg with the auto-detected values */
> > > > + fdt_setprop_string(blob, node, "compatible",
> > > > vdd_cpu->regulator_compat);
> > > > + fdt_setprop_u32(blob, node, "reg", vdd_cpu->addr);
> > > > + sprintf(name, "regulator@%02x", vdd_cpu->addr);
> > > > + fdt_set_name(blob, node, name);
> > > > +
> > >
> > > I'm not sure this is viable in the long run... This relies on the
> > > properties
> > > for the two devices to be identical. Are we sure it is absolutely the
> > > case?
> >
> > Yes, for the moment at least the devices are entirely identical except
> > for the CPU regulator. Unfortunately the identical ADC IDs is why I
> > cannot simply have an additional entry for a new hardware revision.
> >
> > >
> > > An option could be to have a DTSO for the "new" regulator that removes the
> > > old regulator entirely and we would simply be applying that DTSO at
> > > runtime
> > > before the kernel starts. I assume you probably want this for SPL or
> > > U-Boot
> > > proper as well so applying the DTSO there would be required as well, not
> > > sure how to do this though.
> >
> > In truth I really don't need the regulator this way for U-Boot at all.
> > Near as I can tell it defaults to being on and the right voltage. I can
> > do a DTSO, I just figured a simple fixup would be easier (plus we're
> > already doing a fixup for a similar situation with panel revisions).
> >
>
> The benefit of using DTSO is that 1) it drastically lower the complexity of
> the C code (or I would very much hope so). 2) you can have this upstream in
> the kernel and it being very explicit something fishy is going on with the
> HW so people are aware of those things (which also means if someone has the
> idea of supporting another bootloader, e.g. Barebox, LittleKernel, ..., they
> would know about the different variants to support and similar "hacks" to
> implement).
>
> You're the maintainer of those boards and this doesn't impact any other
> product, so I would say this is up to you :)
The difference in code between "detect problem" and "detect problem and
fix" is about 3 lines, so I might as well just fix the problem when we
find it.
Long term if the device tree lets us specify multiple things [1] and
we can somehow probe for which one is present that would be ideal, but
for now this works so I say "ship it".
[1] This is kind of interesting and could solve this and other problems
I face. I'm definitely keeping my eye on it:
https://lore.kernel.org/linux-devicetree/[email protected]/
Thank you,
Chris
>
> > >
> > > Can you please tell the HW vendor to have some way of detecting some
> > > variants of the HW instead of letting us just guess stuff? How do they
> > > even
> > > support this in their downstream kernel/bootloader????
> > > Have they already ran out of channels on SARADC?
> > >
> >
> > I have told both Powkiddy and Anbernic this. Whether they will listen
> > is a different matter. They haven't run out of channels on the SARADC
> > (except for the Powkiddy X55, which is its own board and has no
> > detection logic). In theory if we allow for a ~10% variance in the
> > resistor values for a resistor on ADC channel 1, and the range is
> > between 0 and 1024 (20 and 1004), that gives us about 49 total devices
> > we could "detect" at one time, assuming no one collides.
> >
>
> The channel 1 is commonly used for the recovery button IIRC on recent
> Rockchip SoCs-based products, so another channel probably makes more sense.
>
> I assume one of the possible issue is that they didn't foresee such a change
> would be required and forgot about those ADC channels AND the new regulator
> is pin-to-pin compatible with the same footprint, meaning they didn't need
> to redo a new PCB. That's the only "excuse" i can give them :)
>
> > I have no idea how Powkiddy fixes up the CPU regulator, but I know
> > Anbernic does panel fixup with some hacky code in U-Boot and Linux
> > proper that detects for the panel like I do. In their case it's built
> > into the probe function of Rockchip's own "display-simple-dsi" driver.
> > In our case we detect the panel ID in U-Boot and then fixup the device
> > tree for Linux with the correct compatible.
> >
>
> I definitely never had to do that myself. Never! I don't know what you're
> talking about ;) Oh look, a squirrel.
>
> Cheers,
> Quentin
