[PATCH v2 05/16] power: rk8xx: add support for RK806
Quentin Schulz
quentin.schulz at theobroma-systems.com
Thu Feb 29 12:58:36 CET 2024
Hi Jonas,
On 2/29/24 12:47, Jonas Karlman wrote:
> Hi Quentin,
>
> On 2024-02-23 10:05, Quentin Schulz wrote:
>> From: Quentin Schulz <quentin.schulz at theobroma-systems.com>
>>
>> This adds support for RK806, only the SPI variant has been tested.
>>
>> The communication "protocol" over SPI is the following:
>> - write three bytes:
>> - 1 byte: [0:3] length of the payload, [6] Enable CRC, [7] Write
>> - 1 byte: LSB register address
>> - 1 byte: MSB register address
>> - write/read length of payload
>>
>> The CRC is always disabled for now.
>>
>> The RK806 technically supports I2C as well, and this should be able to
>> support it without any change, but it wasn't tested.
>>
>> The DT node name prefix for the buck converters has changed in the
>> Device Tree and is now dcdc-reg. The logic for buck converters is
>> however manageable within the current logic inside the rk8xx regulator
>> driver. The same cannot be said for the NLDO and PLDO.
>>
>> Because pmic_bind_children() parses the DT nodes and extracts the LDO
>> index from the DT node name, NLDO and PLDO will have overlapping
>> indices. Therefore, we need a separate logic from the already-existing
>> ldo callbacks. Let's reuse as much as possible though.
>>
>> Cc: Quentin Schulz <foss+uboot at 0leil.net>
>> Signed-off-by: Quentin Schulz <quentin.schulz at theobroma-systems.com>
>> ---
>> drivers/power/pmic/rk8xx.c | 85 +++++++
>> drivers/power/regulator/rk8xx.c | 514 +++++++++++++++++++++++++++++++++++++++-
>> include/power/rk8xx_pmic.h | 19 ++
>> 3 files changed, 616 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
>> index 4e3a17337ee..15cb82029cc 100644
>> --- a/drivers/power/pmic/rk8xx.c
>> +++ b/drivers/power/pmic/rk8xx.c
>> @@ -9,8 +9,10 @@
>> #include <dm/lists.h>
>> #include <errno.h>
>> #include <log.h>
>> +#include <linux/bitfield.h>
>> #include <power/rk8xx_pmic.h>
>> #include <power/pmic.h>
>> +#include <spi.h>
>> #include <sysreset.h>
>>
>> static int rk8xx_sysreset_request(struct udevice *dev, enum sysreset_t type)
>> @@ -32,6 +34,10 @@ static int rk8xx_sysreset_request(struct udevice *dev, enum sysreset_t type)
>> pmic_clrsetbits(dev->parent, RK817_REG_SYS_CFG3, 0,
>> BIT(0));
>> break;
>> + case RK806_ID:
>> + pmic_clrsetbits(dev->parent, RK806_REG_SYS_CFG3, 0,
>> + BIT(0));
>> + break;
>> default:
>> printf("Unknown PMIC RK%x: Cannot shutdown\n",
>> priv->variant);
>> @@ -83,6 +89,11 @@ void rk8xx_off_for_plugin(struct udevice *dev)
>> }
>> }
>>
>> +static struct reg_data rk806_init_reg[] = {
>> + /* RST_FUN */
>> + { RK806_REG_SYS_CFG3, GENMASK(7, 6), BIT(7)},
>> +};
>> +
>> static struct reg_data rk817_init_reg[] = {
>> /* enable the under-voltage protection,
>> * the under-voltage protection will shutdown the LDO3 and reset the PMIC
>> @@ -92,7 +103,10 @@ static struct reg_data rk817_init_reg[] = {
>>
>> static const struct pmic_child_info pmic_children_info[] = {
>> { .prefix = "DCDC_REG", .driver = "rk8xx_buck"},
>> + { .prefix = "dcdc-reg", .driver = "rk8xx_buck"},
>> { .prefix = "LDO_REG", .driver = "rk8xx_ldo"},
>> + { .prefix = "nldo-reg", .driver = "rk8xx_nldo"},
>> + { .prefix = "pldo-reg", .driver = "rk8xx_pldo"},
>> { .prefix = "SWITCH_REG", .driver = "rk8xx_switch"},
>> { },
>> };
>> @@ -102,11 +116,47 @@ static int rk8xx_reg_count(struct udevice *dev)
>> return RK808_NUM_OF_REGS;
>> }
>>
>> +struct rk806_cmd {
>> + char len: 4; /* Payload size in bytes - 1 */
>> + char reserved: 2;
>> + char crc_en: 1;
>> + char op: 1; /* READ=0; WRITE=1; */
>> + char reg_l;
>> +#define REG_L_MASK GENMASK(7, 0)
>> + char reg_h;
>> +#define REG_H_MASK GENMASK(15, 8)
>> +};
>> +
>> static int rk8xx_write(struct udevice *dev, uint reg, const uint8_t *buff,
>> int len)
>> {
>> int ret;
>>
>> + if (device_get_uclass_id(dev->parent) == UCLASS_SPI) {
>> + struct spi_slave *spi = dev_get_parent_priv(dev);
>> + struct rk806_cmd cmd = {
>> + .op = 1,
>> + .len = len - 1,
>> + .reg_l = FIELD_GET(REG_L_MASK, reg),
>> + .reg_h = FIELD_GET(REG_H_MASK, reg),
>> + };
>> +
>> + ret = dm_spi_claim_bus(dev);
>> + if (ret) {
>> + debug("Couldn't claim bus for device: %p!\n", dev);
>> + return ret;
>> + }
>> +
>> + ret = spi_write_then_read(spi, (u8 *)&cmd, sizeof(cmd), buff, NULL, len);
>> + if (ret)
>> + debug("write error to device: %p register: %#x!\n",
>> + dev, reg);
>> +
>> + dm_spi_release_bus(dev);
>
> I am getting following build error on PineTab2 [1] with this:
>
> aarch64-linux-gnu-ld.bfd: drivers/power/pmic/rk8xx.o: in function `rk8xx_write':
> drivers/power/pmic/rk8xx.c:144:(.text.rk8xx_write+0x60): undefined reference to `dm_spi_claim_bus'
> aarch64-linux-gnu-ld.bfd: drivers/power/pmic/rk8xx.c:150:(.text.rk8xx_write+0x84): undefined reference to `spi_write_then_read'
> aarch64-linux-gnu-ld.bfd: drivers/power/pmic/rk8xx.c:155:(.text.rk8xx_write+0x90): undefined reference to `dm_spi_release_bus'
> aarch64-linux-gnu-ld.bfd: drivers/power/pmic/rk8xx.o: in function `rk8xx_read':
> drivers/power/pmic/rk8xx.c:182:(.text.rk8xx_read+0x60): undefined reference to `dm_spi_claim_bus'
> aarch64-linux-gnu-ld.bfd: drivers/power/pmic/rk8xx.c:188:(.text.rk8xx_read+0x84): undefined reference to `spi_write_then_read'
> aarch64-linux-gnu-ld.bfd: drivers/power/pmic/rk8xx.c:193:(.text.rk8xx_read+0x90): undefined reference to `dm_spi_release_bus'
> make[2]: *** [scripts/Makefile.spl:527: spl/u-boot-spl] Error 1
> make[1]: *** [Makefile:2055: spl/u-boot-spl] Error 2
>
> On PineTab2 this driver is enabled in SPL and this build error happens
> because SPL_SPI and/or SPL_DM_SPI is not enabled.
>
> The RK8XX driver is enabled in SPL to be able to quickly power down the
> tablet if it was powered on because a power cable was connected.
>
> CONFIG_ROCKCHIP_RK8XX_DISABLE_BOOT_ON_POWERON=y
> CONFIG_SPL_I2C=y
> CONFIG_SPL_PMIC_RK8XX=y
>
> [1] https://patchwork.ozlabs.org/patch/1895031/
>
I was investigating this yesterday but this turned out to be a perfect
match to Hal fixing a light bulb in Malcom in the Middle
(https://www.youtube.com/watch?v=AbSehcT19u0).
I will settle on adding some ifdef that should have been unnecessary and
fix the actual issue in a later patch series because it'll be involving
likely a lot of people and I wouldn't really want to block this patch
series for fixing missing dependencies in Kconfig symbols :)
In short, SPL_DM_SPI is not enough for the ifdef because while the
symbol is enabled, nothing in SPI will be compiled for the SPL unless
SPL_SPI is selected as well... And we're missing a bunch of those
(SPL_DM_I2C as well, SPL_PMIC on SPL_POWER, etc...). The issue is the
moment I add a hard dependency on SPL_SPI for SPL_DM_SPI a bunch of
defconfigs/devices will have SPL_DM_SPI dropped because of an unmatched
dependency. While this is nothing more than cleaning stuff up, I have a
feeling people who enabled those symbols expected them to work. So we
have two schools here: it's been broken for years (for SPL_I2C
dependency missing, July 2021) so if nobody figured it was missing until
then, it's probably not required for the current usecases where SPL_I2C
is missing or, they wanted it enabled and somehow it went through
maintainers' nets but we should fix it now to add it back for the
devices that are requiring it.
Cheers,
Quentin
More information about the U-Boot
mailing list