[PATCH V2 2/4] board: rockchip: Add vdd_cpu reg fixup for RGXX3 Series
Quentin Schulz
quentin.schulz at cherry.de
Tue Sep 24 12:24:16 CEST 2024
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 <macromorgan at hotmail.com>
>>>
>>> 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 <macromorgan at hotmail.com>
>>> ---
>>> 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 at 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 at fdd40000/regulator at 00";
>>> + char name[] = "regulator at 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 at 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 :)
>>
>> 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
More information about the U-Boot
mailing list