| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230110000025.221430b6@slackpad.lan>
Date: Tue, 10 Jan 2023 00:00:25 +0000
From: Andre Przywara <andre.przywara@....com>
To: Martin Botka <martin.botka@...ainline.org>
Cc: martin.botka1@...il.com,
Konrad Dybcio <konrad.dybcio@...ainline.org>,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@...ainline.org>,
Marijn Suijten <marijn.suijten@...ainline.org>,
Jami Kettunen <jamipkettunen@...ainline.org>,
Paul Bouchara <paul.bouchara@...ainline.org>,
Jan Trmal <jtrmal@...il.com>, Lee Jones <lee@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Chen-Yu Tsai <wens@...e.org>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/3] mfd: ax20x: Add suppport for AXP1530 PMIC
On Sat, 17 Dec 2022 01:13:01 +0100
Martin Botka <martin.botka@...ainline.org> wrote:
Hi Martin,
hope you had a good break! Did you have any chance to come back to this
again? Now would be a good time to send a new version, otherwise it's
getting pretty tight for v6.3 already.
On Friday, "junari" in the #linux-sunxi IRC channel, made some
interesting discovery: he is playing around with an AXP313a on some
H616 device and figured that DCDC3 is not behaving like the datasheet:
https://oftc.irclog.whitequark.org/linux-sunxi/2023-01-06#31784528;
He later confirmed the voltage:
https://oftc.irclog.whitequark.org/linux-sunxi/2023-01-08#31788373;
Basically it looks like the DCDC3 parameters you harvested from the BSP
code seem to be correct after all. Do you have any chance to measure
the voltage?
If not, can we try to deduce what the right settings are? The voltage
difference seems to be significant (860mV vs 1200mV), I wonder if any
device connected there (DRAM?) would work with the wrong setting?
Cheers,
Andre
> On December 16, 2022 7:17:52 PM GMT+01:00, Andre Przywara <andre.przywara@....com> wrote:
> >On Wed, 14 Dec 2022 20:03:04 +0100
> >Martin Botka <martin.botka@...ainline.org> wrote:
> >
> >Hi Martin,
> >
> >> AXP1530 is a PMIC chip produced by X-Powers and an be connected via
> >> I2C bus.
> >> Where AXP313A seems to be closely related so the same driver can be used and
> >> seen it only paired with H616 SoC.
> >
> >So as mentioned, I am pretending this is for the AXP313A now, looking at
> >its datasheet.
> >Of course the elephant in the room is s/AXP1530/AXP313A/, but other than
> >that:
> >
> >>
> >> Signed-off-by: Martin Botka <martin.botka@...ainline.org>
> >> ---
> >> drivers/mfd/axp20x-i2c.c | 2 ++
> >> drivers/mfd/axp20x.c | 62 ++++++++++++++++++++++++++++++++++++++
> >> include/linux/mfd/axp20x.h | 32 ++++++++++++++++++++
> >> 3 files changed, 96 insertions(+)
> >>
> >> diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
> >> index 8fd6727dc30a..6bfb931a580e 100644
> >> --- a/drivers/mfd/axp20x-i2c.c
> >> +++ b/drivers/mfd/axp20x-i2c.c
> >> @@ -60,6 +60,7 @@ static void axp20x_i2c_remove(struct i2c_client *i2c)
> >> #ifdef CONFIG_OF
> >> static const struct of_device_id axp20x_i2c_of_match[] = {
> >> { .compatible = "x-powers,axp152", .data = (void *)AXP152_ID },
> >> + { .compatible = "x-powers,axp1530", .data = (void *)AXP1530_ID},
> >> { .compatible = "x-powers,axp202", .data = (void *)AXP202_ID },
> >> { .compatible = "x-powers,axp209", .data = (void *)AXP209_ID },
> >> { .compatible = "x-powers,axp221", .data = (void *)AXP221_ID },
> >> @@ -73,6 +74,7 @@ MODULE_DEVICE_TABLE(of, axp20x_i2c_of_match);
> >>
> >> static const struct i2c_device_id axp20x_i2c_id[] = {
> >> { "axp152", 0 },
> >> + { "axp1530", 0 },
> >> { "axp202", 0 },
> >> { "axp209", 0 },
> >> { "axp221", 0 },
> >> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> >> index 880c41fa7021..6caa7e87ad80 100644
> >> --- a/drivers/mfd/axp20x.c
> >> +++ b/drivers/mfd/axp20x.c
> >> @@ -34,6 +34,7 @@
> >>
> >> static const char * const axp20x_model_names[] = {
> >> "AXP152",
> >> + "AXP1530",
> >> "AXP202",
> >> "AXP209",
> >> "AXP221",
> >> @@ -66,6 +67,24 @@ static const struct regmap_access_table axp152_volatile_table = {
> >> .n_yes_ranges = ARRAY_SIZE(axp152_volatile_ranges),
> >> };
> >>
> >> +static const struct regmap_range axp1530_writeable_ranges[] = {
> >> + regmap_reg_range(AXP1530_ON_INDICATE, AXP1530_FREQUENCY),
> >
> >Where does this FREQUENCY register come from? BSP source? Is that the
> >lost register to set the PWM frequency?
> >The 313 datasheet doesn't mention it, and since we deny programming the
> >frequency, I would just leave it out.
> >If people find it existing (and useful!) later on, we should be able to
> >add it without breaking anything.
>
> BSP. Ack.
> >
> >> +};
> >> +
> >> +static const struct regmap_range axp1530_volatile_ranges[] = {
> >> + regmap_reg_range(AXP1530_ON_INDICATE, AXP1530_FREQUENCY),
> >> +};
> >> +
> >> +static const struct regmap_access_table axp1530_writeable_table = {
> >> + .yes_ranges = axp1530_writeable_ranges,
> >> + .n_yes_ranges = ARRAY_SIZE(axp1530_writeable_ranges),
> >> +};
> >> +
> >> +static const struct regmap_access_table axp1530_volatile_table = {
> >> + .yes_ranges = axp1530_volatile_ranges,
> >> + .n_yes_ranges = ARRAY_SIZE(axp1530_volatile_ranges),
> >> +};
> >> +
> >> static const struct regmap_range axp20x_writeable_ranges[] = {
> >> regmap_reg_range(AXP20X_DATACACHE(0), AXP20X_IRQ5_STATE),
> >> regmap_reg_range(AXP20X_CHRG_CTRL1, AXP20X_CHRG_CTRL2),
> >> @@ -245,6 +264,15 @@ static const struct regmap_config axp152_regmap_config = {
> >> .cache_type = REGCACHE_RBTREE,
> >> };
> >>
> >> +static const struct regmap_config axp1530_regmap_config = {
> >> + .reg_bits = 8,
> >> + .val_bits = 8,
> >> + .wr_table = &axp1530_writeable_table,
> >> + .volatile_table = &axp1530_volatile_table,
> >> + .max_register = AXP1530_FREQUENCY,
> >> + .cache_type = REGCACHE_RBTREE,
> >> +};
> >> +
> >> static const struct regmap_config axp20x_regmap_config = {
> >> .reg_bits = 8,
> >> .val_bits = 8,
> >> @@ -304,6 +332,16 @@ static const struct regmap_irq axp152_regmap_irqs[] = {
> >> INIT_REGMAP_IRQ(AXP152, GPIO0_INPUT, 2, 0),
> >> };
> >>
> >> +static const struct regmap_irq axp1530_regmap_irqs[] = {
> >> + INIT_REGMAP_IRQ(AXP1530, KEY_L2H_EN, 0, 7),
> >> + INIT_REGMAP_IRQ(AXP1530, KEY_H2L_EN, 0, 6),
> >> + INIT_REGMAP_IRQ(AXP1530, POKSIRQ_EN, 0, 5),
> >> + INIT_REGMAP_IRQ(AXP1530, POKLIRQ_EN, 0, 4),
> >
> >Are those identifiers from the BSP source? The (translated) manual gives
> >some explanation, namely:
> > PWRON key rising edge
> > PWRON key falling edge
> > Short press the PWRON button
> > Long press the PWRON button
> >
> >So I'd suggest we follow the existing naming:
> > PEK_RIS_EDGE, PEK_FAL_EDGE, PEK_SHORT, PEK_LONG (respectively)
> >
> >Or come up with names that people could actually decipher ;-)
> >
> >
> Ack.
> >> + INIT_REGMAP_IRQ(AXP1530, DCDC3_UNDER, 0, 3),
> >> + INIT_REGMAP_IRQ(AXP1530, DCDC2_UNDER, 0, 2),
> >> + INIT_REGMAP_IRQ(AXP1530, TEMP_OVER, 0, 0),
> >> +};
> >> +
> >> static const struct regmap_irq axp20x_regmap_irqs[] = {
> >> INIT_REGMAP_IRQ(AXP20X, ACIN_OVER_V, 0, 7),
> >> INIT_REGMAP_IRQ(AXP20X, ACIN_PLUGIN, 0, 6),
> >> @@ -514,6 +552,18 @@ static const struct regmap_irq_chip axp152_regmap_irq_chip = {
> >> .num_regs = 3,
> >> };
> >>
> >> +static const struct regmap_irq_chip axp1530_regmap_irq_chip = {
> >> + .name = "axp1530_irq_chip",
> >> + .status_base = AXP1530_IRQ_STATUS1,
> >> + .ack_base = AXP1530_IRQ_STATUS1,
> >> + .mask_base = AXP1530_IRQ_ENABLE1,
> >> + .mask_invert = true,
> >> + .init_ack_masked = true,
> >> + .irqs = axp1530_regmap_irqs,
> >> + .num_irqs = ARRAY_SIZE(axp1530_regmap_irqs),
> >> + .num_regs = 1,
> >> +};
> >> +
> >> static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
> >> .name = "axp20x_irq_chip",
> >> .status_base = AXP20X_IRQ1_STATE,
> >> @@ -683,6 +733,12 @@ static const struct mfd_cell axp152_cells[] = {
> >> },
> >> };
> >>
> >> +static struct mfd_cell axp1530_cells[] = {
> >> + {
> >> + .name = "axp20x-regulator",
> >> + },
> >> +};
> >> +
> >> static const struct resource axp288_adc_resources[] = {
> >> DEFINE_RES_IRQ_NAMED(AXP288_IRQ_GPADC, "GPADC"),
> >> };
> >> @@ -874,6 +930,12 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
> >> axp20x->regmap_cfg = &axp152_regmap_config;
> >> axp20x->regmap_irq_chip = &axp152_regmap_irq_chip;
> >> break;
> >> + case AXP1530_ID:
> >> + axp20x->nr_cells = ARRAY_SIZE(axp1530_cells);
> >> + axp20x->cells = axp1530_cells;
> >> + axp20x->regmap_cfg = &axp1530_regmap_config;
> >> + axp20x->regmap_irq_chip = &axp1530_regmap_irq_chip;
> >> + break;
> >> case AXP202_ID:
> >> case AXP209_ID:
> >> axp20x->nr_cells = ARRAY_SIZE(axp20x_cells);
> >> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> >> index 9ab0e2fca7ea..cad25754500f 100644
> >> --- a/include/linux/mfd/axp20x.h
> >> +++ b/include/linux/mfd/axp20x.h
> >> @@ -12,6 +12,7 @@
> >>
> >> enum axp20x_variants {
> >> AXP152_ID = 0,
> >> + AXP1530_ID,
> >> AXP202_ID,
> >> AXP209_ID,
> >> AXP221_ID,
> >> @@ -45,6 +46,18 @@ enum axp20x_variants {
> >> #define AXP152_DCDC_FREQ 0x37
> >> #define AXP152_DCDC_MODE 0x80
> >>
> >> +#define AXP1530_ON_INDICATE 0x00
> >> +#define AXP1530_OUTPUT_CONTROL 0x10
> >> +#define AXP1530_DCDC1_CONRTOL 0x13
> >> +#define AXP1530_DCDC2_CONRTOL 0x14
> >> +#define AXP1530_DCDC3_CONRTOL 0x15
> >> +#define AXP1530_ALDO1_CONRTOL 0x16
> >> +#define AXP1530_DLDO1_CONRTOL 0x17
> >> +#define AXP1530_OUTOUT_MONITOR 0x1D
> >
> >Shall this read AXP1530_OUTPUT_MONITOR?
> >
> >> +#define AXP1530_IRQ_ENABLE1 0x20
> >> +#define AXP1530_IRQ_STATUS1 0x21
> >
> >There is only one interrupt register, so can we drop the trailing number?
> >
> Yep.
> >> +#define AXP1530_FREQUENCY 0x87
> >
> >As mentioned, the manual does not mention it, and we don't use it anyway.
> >
> Ack.
> >> +
> >> #define AXP20X_PWR_INPUT_STATUS 0x00
> >> #define AXP20X_PWR_OP_MODE 0x01
> >> #define AXP20X_USB_OTG_STATUS 0x02
> >> @@ -287,6 +300,15 @@ enum axp20x_variants {
> >> #define AXP288_FG_TUNE5 0xed
> >>
> >> /* Regulators IDs */
> >> +enum {
> >> + AXP1530_DCDC1 = 0,
> >> + AXP1530_DCDC2,
> >> + AXP1530_DCDC3,
> >> + AXP1530_LDO1,
> >> + AXP1530_LDO2,
> >
> >I guess we should add the RTC LDO as LDO3 here.
> >
> >The rest of the numbers match with the datasheet.
> >
> Ack.
>
> I will take some time off due to Uni so v6 will be delayed peobably last holidays.
>
> Best regards and happy holidays,
> Martin
> >Cheers,
> >Andre
> >
> >> + AXP1530_REG_ID_MAX,
> >> +};
> >> +
> >> enum {
> >> AXP20X_LDO1 = 0,
> >> AXP20X_LDO2,
> >> @@ -440,6 +462,16 @@ enum {
> >> AXP152_IRQ_GPIO0_INPUT,
> >> };
> >>
> >> +enum axp1530_irqs {
> >> + AXP1530_IRQ_TEMP_OVER,
> >> + AXP1530_IRQ_DCDC2_UNDER = 2,
> >> + AXP1530_IRQ_DCDC3_UNDER,
> >> + AXP1530_IRQ_POKLIRQ_EN,
> >> + AXP1530_IRQ_POKSIRQ_EN,
> >> + AXP1530_IRQ_KEY_L2H_EN,
> >> + AXP1530_IRQ_KEY_H2L_EN,
> >> +};
> >> +
> >> enum {
> >> AXP20X_IRQ_ACIN_OVER_V = 1,
> >> AXP20X_IRQ_ACIN_PLUGIN,
> >
Powered by blists - more mailing lists