Message ID | 20250117014537.22513-3-andre.przywara@arm.com |
---|---|
State | New |
Delegated to: | Andre Przywara |
Headers | show |
Series | sunxi: (early) Allwinner A133 SoC support | expand |
Dne petek, 17. januar 2025 ob 02:45:31 Srednjeevropski standardni čas je Andre Przywara napisal(a): > Some of the X-Power AXP PMICs can be ordered with an alternative I2C > address, for instance an AXP717 could be shipped with address 0x34 or > with address 0x35. > The datasheets for the AXP717 and AXP803 list two possible addresses, > and they are always consecutive. For DM (DT) based drivers this is no > problem, but the Allwinner SPL code relies on a hardcoded address. > > Add a Kconfig variable that will add "1" to the existing address if it > is set. > This enables to use the AXP717 as used on boards with the new Allwinner > A523 chip. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> This works until some board vendor start mixing one or another address PMIC. Note that BSP boot0 does address autodetection, so it's not entirely out of the question. Anyway, let's hope we don't see anything like that. Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com> Best regards, Jernej
On Sat, 18 Jan 2025 08:21:31 +0100 Jernej Škrabec <jernej.skrabec@gmail.com> wrote: Hi Jernej, many thanks for the review and your opinion. > Dne petek, 17. januar 2025 ob 02:45:31 Srednjeevropski standardni čas je Andre Przywara napisal(a): > > Some of the X-Power AXP PMICs can be ordered with an alternative I2C > > address, for instance an AXP717 could be shipped with address 0x34 or > > with address 0x35. > > The datasheets for the AXP717 and AXP803 list two possible addresses, > > and they are always consecutive. For DM (DT) based drivers this is no > > problem, but the Allwinner SPL code relies on a hardcoded address. > > > > Add a Kconfig variable that will add "1" to the existing address if it > > is set. > > This enables to use the AXP717 as used on boards with the new Allwinner > > A523 chip. > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > This works until some board vendor start mixing one or another address > PMIC. Note that BSP boot0 does address autodetection, so it's not entirely > out of the question. Anyway, let's hope we don't see anything like that. I looked at the datasheets for all supported PMICs, and they either state one or two supported addresses, in the latter case both consecutive. Autodetection sounds nice, but also unnecessary: we surely know what address it is for a certain board? And with those A523 boards having two PMICs, autodetection might become sketchy, as we don't know for sure which PMIC we got? But that got me thinking: what about putting the I2C address in Kconfig directly, with defaults depending on the PMIC type? config AXP_I2C_ADDR hex "AXP PMIC I2C address" depends on ARCH_SUNXI && !SUNXI_NO_PMIC default 0x36 if AXP305_POWER .... That's should work seamlessly for all supported PMICs, and we just need one line for the Avaota, same as with this patch here. What do you think? Cheers, Andre > > Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com> > > Best regards, > Jernej > > >
Dne nedelja, 19. januar 2025 ob 23:25:30 Srednjeevropski standardni čas je Andre Przywara napisal(a): > On Sat, 18 Jan 2025 08:21:31 +0100 > Jernej Škrabec <jernej.skrabec@gmail.com> wrote: > > Hi Jernej, > > many thanks for the review and your opinion. > > > Dne petek, 17. januar 2025 ob 02:45:31 Srednjeevropski standardni čas je Andre Przywara napisal(a): > > > Some of the X-Power AXP PMICs can be ordered with an alternative I2C > > > address, for instance an AXP717 could be shipped with address 0x34 or > > > with address 0x35. > > > The datasheets for the AXP717 and AXP803 list two possible addresses, > > > and they are always consecutive. For DM (DT) based drivers this is no > > > problem, but the Allwinner SPL code relies on a hardcoded address. > > > > > > Add a Kconfig variable that will add "1" to the existing address if it > > > is set. > > > This enables to use the AXP717 as used on boards with the new Allwinner > > > A523 chip. > > > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > > This works until some board vendor start mixing one or another address > > PMIC. Note that BSP boot0 does address autodetection, so it's not entirely > > out of the question. Anyway, let's hope we don't see anything like that. > > I looked at the datasheets for all supported PMICs, and they either > state one or two supported addresses, in the latter case both > consecutive. Autodetection sounds nice, but also unnecessary: we surely > know what address it is for a certain board? And with those A523 boards > having two PMICs, autodetection might become sketchy, as we don't know > for sure which PMIC we got? Speaking for T527 BSP boot0 - they check version register to make sure that correct PMIC is installed. > > But that got me thinking: what about putting the I2C address in Kconfig > directly, with defaults depending on the PMIC type? > > config AXP_I2C_ADDR > hex "AXP PMIC I2C address" > depends on ARCH_SUNXI && !SUNXI_NO_PMIC > default 0x36 if AXP305_POWER > .... > > That's should work seamlessly for all supported PMICs, and we just need > one line for the Avaota, same as with this patch here. > > What do you think? Yeah, looks more universal and avoids code changes in pmic_bus.c when adding support for new AXP PMIC, which is very nice indeed. Best regards, Jernej > > Cheers, > Andre > > > > > > > Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com> > > > > Best regards, > > Jernej > > > > > > > >
Hi, On Mon, 20 Jan 2025 at 09:42, Jernej Škrabec <jernej.skrabec@gmail.com> wrote: > > Dne nedelja, 19. januar 2025 ob 23:25:30 Srednjeevropski standardni čas je Andre Przywara napisal(a): > > On Sat, 18 Jan 2025 08:21:31 +0100 > > Jernej Škrabec <jernej.skrabec@gmail.com> wrote: > > > > Hi Jernej, > > > > many thanks for the review and your opinion. > > > > > Dne petek, 17. januar 2025 ob 02:45:31 Srednjeevropski standardni čas je Andre Przywara napisal(a): > > > > Some of the X-Power AXP PMICs can be ordered with an alternative I2C > > > > address, for instance an AXP717 could be shipped with address 0x34 or > > > > with address 0x35. > > > > The datasheets for the AXP717 and AXP803 list two possible addresses, > > > > and they are always consecutive. For DM (DT) based drivers this is no > > > > problem, but the Allwinner SPL code relies on a hardcoded address. > > > > > > > > Add a Kconfig variable that will add "1" to the existing address if it > > > > is set. > > > > This enables to use the AXP717 as used on boards with the new Allwinner > > > > A523 chip. > > > > > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > > > > This works until some board vendor start mixing one or another address > > > PMIC. Note that BSP boot0 does address autodetection, so it's not entirely > > > out of the question. Anyway, let's hope we don't see anything like that. > > > > I looked at the datasheets for all supported PMICs, and they either > > state one or two supported addresses, in the latter case both > > consecutive. Autodetection sounds nice, but also unnecessary: we surely > > know what address it is for a certain board? And with those A523 boards > > having two PMICs, autodetection might become sketchy, as we don't know > > for sure which PMIC we got? > > Speaking for T527 BSP boot0 - they check version register to make sure that > correct PMIC is installed. > > > > > But that got me thinking: what about putting the I2C address in Kconfig > > directly, with defaults depending on the PMIC type? > > > > config AXP_I2C_ADDR > > hex "AXP PMIC I2C address" > > depends on ARCH_SUNXI && !SUNXI_NO_PMIC > > default 0x36 if AXP305_POWER > > .... > > > > That's should work seamlessly for all supported PMICs, and we just need > > one line for the Avaota, same as with this patch here. > > > > What do you think? > > Yeah, looks more universal and avoids code changes in pmic_bus.c when > adding support for new AXP PMIC, which is very nice indeed. Shouldn't this be in the devicetree? [..] Regards, Simon
On Mon, 20 Jan 2025 12:21:28 -0700 Simon Glass <sjg@chromium.org> wrote: Hi Simon, > On Mon, 20 Jan 2025 at 09:42, Jernej Škrabec <jernej.skrabec@gmail.com> wrote: > > > > Dne nedelja, 19. januar 2025 ob 23:25:30 Srednjeevropski standardni čas je Andre Przywara napisal(a): > > > On Sat, 18 Jan 2025 08:21:31 +0100 > > > Jernej Škrabec <jernej.skrabec@gmail.com> wrote: > > > > > > Hi Jernej, > > > > > > many thanks for the review and your opinion. > > > > > > > Dne petek, 17. januar 2025 ob 02:45:31 Srednjeevropski standardni čas je Andre Przywara napisal(a): > > > > > Some of the X-Power AXP PMICs can be ordered with an alternative I2C > > > > > address, for instance an AXP717 could be shipped with address 0x34 or > > > > > with address 0x35. > > > > > The datasheets for the AXP717 and AXP803 list two possible addresses, > > > > > and they are always consecutive. For DM (DT) based drivers this is no > > > > > problem, but the Allwinner SPL code relies on a hardcoded address. > > > > > > > > > > Add a Kconfig variable that will add "1" to the existing address if it > > > > > is set. > > > > > This enables to use the AXP717 as used on boards with the new Allwinner > > > > > A523 chip. > > > > > > > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > > > > > > This works until some board vendor start mixing one or another address > > > > PMIC. Note that BSP boot0 does address autodetection, so it's not entirely > > > > out of the question. Anyway, let's hope we don't see anything like that. > > > > > > I looked at the datasheets for all supported PMICs, and they either > > > state one or two supported addresses, in the latter case both > > > consecutive. Autodetection sounds nice, but also unnecessary: we surely > > > know what address it is for a certain board? And with those A523 boards > > > having two PMICs, autodetection might become sketchy, as we don't know > > > for sure which PMIC we got? > > > > Speaking for T527 BSP boot0 - they check version register to make sure that > > correct PMIC is installed. > > > > > > > > But that got me thinking: what about putting the I2C address in Kconfig > > > directly, with defaults depending on the PMIC type? > > > > > > config AXP_I2C_ADDR > > > hex "AXP PMIC I2C address" > > > depends on ARCH_SUNXI && !SUNXI_NO_PMIC > > > default 0x36 if AXP305_POWER > > > .... > > > > > > That's should work seamlessly for all supported PMICs, and we just need > > > one line for the Avaota, same as with this patch here. > > > > > > What do you think? > > > > Yeah, looks more universal and avoids code changes in pmic_bus.c when > > adding support for new AXP PMIC, which is very nice indeed. > > Shouldn't this be in the devicetree? It is, and the DM based I2C driver used in U-Boot proper does this properly, and works fine. But this here is for the SPL, where we don't have DT support. We need just minimal support to adjust the regulator for the DRAMs. So far there is one fixed address used by each PMIC, so this is simply hardcoded, based on which PMIC is selected. The patch I am now proposing (snippet above) just moves that hardcoding into Kconfig. Cheers, Andre > [..] > Regards, > Simon >
Hi Andre, On Mon, 20 Jan 2025 at 17:06, Andre Przywara <andre.przywara@arm.com> wrote: > > On Mon, 20 Jan 2025 12:21:28 -0700 > Simon Glass <sjg@chromium.org> wrote: > > Hi Simon, > > > On Mon, 20 Jan 2025 at 09:42, Jernej Škrabec <jernej.skrabec@gmail.com> wrote: > > > > > > Dne nedelja, 19. januar 2025 ob 23:25:30 Srednjeevropski standardni čas je Andre Przywara napisal(a): > > > > On Sat, 18 Jan 2025 08:21:31 +0100 > > > > Jernej Škrabec <jernej.skrabec@gmail.com> wrote: > > > > > > > > Hi Jernej, > > > > > > > > many thanks for the review and your opinion. > > > > > > > > > Dne petek, 17. januar 2025 ob 02:45:31 Srednjeevropski standardni čas je Andre Przywara napisal(a): > > > > > > Some of the X-Power AXP PMICs can be ordered with an alternative I2C > > > > > > address, for instance an AXP717 could be shipped with address 0x34 or > > > > > > with address 0x35. > > > > > > The datasheets for the AXP717 and AXP803 list two possible addresses, > > > > > > and they are always consecutive. For DM (DT) based drivers this is no > > > > > > problem, but the Allwinner SPL code relies on a hardcoded address. > > > > > > > > > > > > Add a Kconfig variable that will add "1" to the existing address if it > > > > > > is set. > > > > > > This enables to use the AXP717 as used on boards with the new Allwinner > > > > > > A523 chip. > > > > > > > > > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > > > > > > > > This works until some board vendor start mixing one or another address > > > > > PMIC. Note that BSP boot0 does address autodetection, so it's not entirely > > > > > out of the question. Anyway, let's hope we don't see anything like that. > > > > > > > > I looked at the datasheets for all supported PMICs, and they either > > > > state one or two supported addresses, in the latter case both > > > > consecutive. Autodetection sounds nice, but also unnecessary: we surely > > > > know what address it is for a certain board? And with those A523 boards > > > > having two PMICs, autodetection might become sketchy, as we don't know > > > > for sure which PMIC we got? > > > > > > Speaking for T527 BSP boot0 - they check version register to make sure that > > > correct PMIC is installed. > > > > > > > > > > > But that got me thinking: what about putting the I2C address in Kconfig > > > > directly, with defaults depending on the PMIC type? > > > > > > > > config AXP_I2C_ADDR > > > > hex "AXP PMIC I2C address" > > > > depends on ARCH_SUNXI && !SUNXI_NO_PMIC > > > > default 0x36 if AXP305_POWER > > > > .... > > > > > > > > That's should work seamlessly for all supported PMICs, and we just need > > > > one line for the Avaota, same as with this patch here. > > > > > > > > What do you think? > > > > > > Yeah, looks more universal and avoids code changes in pmic_bus.c when > > > adding support for new AXP PMIC, which is very nice indeed. > > > > Shouldn't this be in the devicetree? > > It is, and the DM based I2C driver used in U-Boot proper does this > properly, and works fine. But this here is for the SPL, where we don't > have DT support. We need just minimal support to adjust the regulator > for the DRAMs. So far there is one fixed address used by each PMIC, so > this is simply hardcoded, based on which PMIC is selected. The patch I > am now proposing (snippet above) just moves that hardcoding into > Kconfig. I suppose you could use of-platdata? Regards, SImon
diff --git a/arch/arm/mach-sunxi/pmic_bus.c b/arch/arm/mach-sunxi/pmic_bus.c index 8e19324c8ac..f0201f76f42 100644 --- a/arch/arm/mach-sunxi/pmic_bus.c +++ b/arch/arm/mach-sunxi/pmic_bus.c @@ -31,6 +31,11 @@ static struct udevice *pmic; #else static int pmic_i2c_address(void) { + int i2c_offset = 0; + + if (IS_ENABLED(CONFIG_AXP_ALT_I2C_ADDR)) + i2c_offset = 1; + if (IS_ENABLED(CONFIG_AXP152_POWER)) return AXP152_I2C_ADDR; if (IS_ENABLED(CONFIG_AXP305_POWER)) @@ -38,7 +43,7 @@ static int pmic_i2c_address(void) if (IS_ENABLED(CONFIG_AXP313_POWER)) return AXP313_I2C_ADDR; if (IS_ENABLED(CONFIG_AXP717_POWER)) - return AXP717_I2C_ADDR; + return AXP717_I2C_ADDR + i2c_offset; /* Other AXP2xx and AXP8xx variants */ return AXP209_I2C_ADDR; diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig index 4b81aeb7497..4c2211097fd 100644 --- a/drivers/power/Kconfig +++ b/drivers/power/Kconfig @@ -142,6 +142,14 @@ config SY8106A_POWER endchoice +config AXP_ALT_I2C_ADDR + bool "use alternative I2C address" + depends on AXP717_POWER + ---help--- + Some PMICs can be ordered with a different I2C address (+1), to avoid + address clashes or accommodate two PMICs on the same bus. + Select this to use the secondary I2C address for the AXP PMICs. + config AXP_DCDC1_VOLT int "axp pmic dcdc1 voltage" depends on AXP221_POWER || AXP809_POWER || AXP818_POWER
Some of the X-Power AXP PMICs can be ordered with an alternative I2C address, for instance an AXP717 could be shipped with address 0x34 or with address 0x35. The datasheets for the AXP717 and AXP803 list two possible addresses, and they are always consecutive. For DM (DT) based drivers this is no problem, but the Allwinner SPL code relies on a hardcoded address. Add a Kconfig variable that will add "1" to the existing address if it is set. This enables to use the AXP717 as used on boards with the new Allwinner A523 chip. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- arch/arm/mach-sunxi/pmic_bus.c | 7 ++++++- drivers/power/Kconfig | 8 ++++++++ 2 files changed, 14 insertions(+), 1 deletion(-)