| 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
| ||
|
Date: Tue, 18 Apr 2023 00:21:25 +0800
From: Shengyu Qu <wiagn233@...look.com>
To: Andre Przywara <andre.przywara@....com>
Cc: lee@...nel.org, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, wens@...e.org,
lgirdwood@...il.com, broonie@...nel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Martin Botka <martin.botka@...ainline.org>,
martin.botka1@...il.com
Subject: Re: [PATCH v1 2/4] mfd: axp20x: Add support for AXP15060 PMIC
Hi Andre,
> On Fri, 7 Apr 2023 22:18:11 +0800
> Shengyu Qu <wiagn233@...look.com> wrote:
>
> Hi Shengyu,
>
> thanks for doing the tedious work and sending this!
>
>> The AXP15060 is a PMIC chip produced by X-Powers, and could be connected
>> via an I2C bus.
>>
>> Describe the regmap and the MFD bits, along with the registers exposed
>> via I2C. Eventually advertise the device using a new compatible string
>> and add support for power off the system.
>>
>> The driver would disable PEK function if IRQ is not configured in device
>> tree, since some boards (For example, Starfive Visionfive 2) didn't
>> connect IRQ line of PMIC to SOC.
>>
>> GPIO function isn't enabled in this commit, since its configuration
>> operation is different from any existing AXP PMICs and needs
>> logic modification on existing driver. GPIO support might come in later
>> patches.
>>
>> Signed-off-by: Shengyu Qu <wiagn233@...look.com>
>> ---
>> drivers/mfd/axp20x-i2c.c | 2 +
>> drivers/mfd/axp20x.c | 90 ++++++++++++++++++++++++++++++++++++++
>> include/linux/mfd/axp20x.h | 85 +++++++++++++++++++++++++++++++++++
>> 3 files changed, 177 insertions(+)
>>
>> diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
>> index f49fbd307958..b4f5cb457117 100644
>> --- a/drivers/mfd/axp20x-i2c.c
>> +++ b/drivers/mfd/axp20x-i2c.c
>> @@ -65,6 +65,7 @@ static const struct of_device_id axp20x_i2c_of_match[] = {
>> { .compatible = "x-powers,axp223", .data = (void *)AXP223_ID },
>> { .compatible = "x-powers,axp803", .data = (void *)AXP803_ID },
>> { .compatible = "x-powers,axp806", .data = (void *)AXP806_ID },
>> + { .compatible = "x-powers,axp15060", .data = (void *)AXP15060_ID },
>> { },
>> };
>> MODULE_DEVICE_TABLE(of, axp20x_i2c_of_match);
>> @@ -78,6 +79,7 @@ static const struct i2c_device_id axp20x_i2c_id[] = {
>> { "axp223", 0 },
>> { "axp803", 0 },
>> { "axp806", 0 },
>> + { "axp15060", 0 },
>> { },
>> };
>> MODULE_DEVICE_TABLE(i2c, axp20x_i2c_id);
>> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>> index 01a6bbb6d266..42ec27a4dfc4 100644
>> --- a/drivers/mfd/axp20x.c
>> +++ b/drivers/mfd/axp20x.c
>> @@ -43,6 +43,7 @@ static const char * const axp20x_model_names[] = {
>> "AXP806",
>> "AXP809",
>> "AXP813",
>> + "AXP15060",
>> };
>>
>> static const struct regmap_range axp152_writeable_ranges[] = {
>> @@ -168,6 +169,31 @@ static const struct regmap_access_table axp806_volatile_table = {
>> .n_yes_ranges = ARRAY_SIZE(axp806_volatile_ranges),
>> };
>>
>> +static const struct regmap_range axp15060_writeable_ranges[] = {
>> + regmap_reg_range(AXP15060_PWR_OUT_CTRL1, AXP15060_DCDC_MODE_CTRL2),
>> + regmap_reg_range(AXP15060_OUTPUT_MONITOR_DISCHARGE, AXP15060_CPUSLDO_V_CTRL),
>> + regmap_reg_range(AXP15060_PWR_WAKEUP_CTRL, AXP15060_PWR_DISABLE_DOWN_SEQ),
>> + regmap_reg_range(AXP15060_PEK_KEY, AXP15060_PEK_KEY),
>> + regmap_reg_range(AXP15060_IRQ1_EN, AXP15060_IRQ2_EN),
>> + regmap_reg_range(AXP15060_IRQ1_STATE, AXP15060_IRQ2_STATE),
>> +};
>> +
>> +static const struct regmap_range axp15060_volatile_ranges[] = {
>> + regmap_reg_range(AXP15060_STARTUP_SRC, AXP15060_STARTUP_SRC),
>> + regmap_reg_range(AXP15060_PWR_WAKEUP_CTRL, AXP15060_PWR_DISABLE_DOWN_SEQ),
>> + regmap_reg_range(AXP15060_IRQ1_STATE, AXP15060_IRQ2_STATE),
>> +};
>> +
>> +static const struct regmap_access_table axp15060_writeable_table = {
>> + .yes_ranges = axp15060_writeable_ranges,
>> + .n_yes_ranges = ARRAY_SIZE(axp15060_writeable_ranges),
>> +};
>> +
>> +static const struct regmap_access_table axp15060_volatile_table = {
>> + .yes_ranges = axp15060_volatile_ranges,
>> + .n_yes_ranges = ARRAY_SIZE(axp15060_volatile_ranges),
>> +};
>> +
>> static const struct resource axp152_pek_resources[] = {
>> DEFINE_RES_IRQ_NAMED(AXP152_IRQ_PEK_RIS_EDGE, "PEK_DBR"),
>> DEFINE_RES_IRQ_NAMED(AXP152_IRQ_PEK_FAL_EDGE, "PEK_DBF"),
>> @@ -236,6 +262,11 @@ static const struct resource axp809_pek_resources[] = {
>> DEFINE_RES_IRQ_NAMED(AXP809_IRQ_PEK_FAL_EDGE, "PEK_DBF"),
>> };
>>
>> +static const struct resource axp15060_pek_resources[] = {
>> + DEFINE_RES_IRQ_NAMED(AXP15060_IRQ_PEK_RIS_EDGE, "PEK_DBR"),
>> + DEFINE_RES_IRQ_NAMED(AXP15060_IRQ_PEK_FAL_EDGE, "PEK_DBF"),
>> +};
>> +
>> static const struct regmap_config axp152_regmap_config = {
>> .reg_bits = 8,
>> .val_bits = 8,
>> @@ -281,6 +312,15 @@ static const struct regmap_config axp806_regmap_config = {
>> .cache_type = REGCACHE_RBTREE,
>> };
>>
>> +static const struct regmap_config axp15060_regmap_config = {
>> + .reg_bits = 8,
>> + .val_bits = 8,
>> + .wr_table = &axp15060_writeable_table,
>> + .volatile_table = &axp15060_volatile_table,
>> + .max_register = AXP15060_IRQ2_STATE,
>> + .cache_type = REGCACHE_RBTREE,
>> +};
>> +
>> #define INIT_REGMAP_IRQ(_variant, _irq, _off, _mask) \
>> [_variant##_IRQ_##_irq] = { .reg_offset = (_off), .mask = BIT(_mask) }
>>
>> @@ -502,6 +542,23 @@ static const struct regmap_irq axp809_regmap_irqs[] = {
>> INIT_REGMAP_IRQ(AXP809, GPIO0_INPUT, 4, 0),
>> };
>>
>> +static const struct regmap_irq axp15060_regmap_irqs[] = {
>> + INIT_REGMAP_IRQ(AXP15060, DIE_TEMP_HIGH_LV1, 0, 0),
>> + INIT_REGMAP_IRQ(AXP15060, DIE_TEMP_HIGH_LV2, 0, 1),
>> + INIT_REGMAP_IRQ(AXP15060, DCDC1_V_LOW, 0, 2),
>> + INIT_REGMAP_IRQ(AXP15060, DCDC2_V_LOW, 0, 3),
>> + INIT_REGMAP_IRQ(AXP15060, DCDC3_V_LOW, 0, 4),
>> + INIT_REGMAP_IRQ(AXP15060, DCDC4_V_LOW, 0, 5),
>> + INIT_REGMAP_IRQ(AXP15060, DCDC5_V_LOW, 0, 6),
>> + INIT_REGMAP_IRQ(AXP15060, DCDC6_V_LOW, 0, 7),
>> + INIT_REGMAP_IRQ(AXP15060, PEK_LONG, 1, 0),
>> + INIT_REGMAP_IRQ(AXP15060, PEK_SHORT, 1, 1),
>> + INIT_REGMAP_IRQ(AXP15060, GPIO1_INPUT, 1, 2),
>> + INIT_REGMAP_IRQ(AXP15060, PEK_FAL_EDGE, 1, 3),
>> + INIT_REGMAP_IRQ(AXP15060, PEK_RIS_EDGE, 1, 4),
>> + INIT_REGMAP_IRQ(AXP15060, GPIO2_INPUT, 1, 5),
>> +};
>> +
>> static const struct regmap_irq_chip axp152_regmap_irq_chip = {
>> .name = "axp152_irq_chip",
>> .status_base = AXP152_IRQ1_STATE,
>> @@ -581,6 +638,17 @@ static const struct regmap_irq_chip axp809_regmap_irq_chip = {
>> .num_regs = 5,
>> };
>>
>> +static const struct regmap_irq_chip axp15060_regmap_irq_chip = {
>> + .name = "axp15060",
>> + .status_base = AXP15060_IRQ1_STATE,
>> + .ack_base = AXP15060_IRQ1_STATE,
>> + .unmask_base = AXP15060_IRQ1_EN,
>> + .init_ack_masked = true,
>> + .irqs = axp15060_regmap_irqs,
>> + .num_irqs = ARRAY_SIZE(axp15060_regmap_irqs),
>> + .num_regs = 2,
>> +};
>> +
>> static const struct mfd_cell axp20x_cells[] = {
>> {
>> .name = "axp20x-gpio",
>> @@ -825,6 +893,16 @@ static const struct mfd_cell axp813_cells[] = {
>> },
>> };
>>
>> +static const struct mfd_cell axp15060_cells[] = {
>> + {
>> + .name = "axp221-pek",
>> + .num_resources = ARRAY_SIZE(axp15060_pek_resources),
>> + .resources = axp15060_pek_resources,
>> + }, {
>> + .name = "axp20x-regulator",
>> + },
> As Lee mentioned the other day, there are the MFD_CELL_NAME and
> MFD_CELL_RES macro to wrap those now.
I think a whole conversion commit would be much better to keep code
readable rather than multiple description method.
> I compared all the register offsets and bit names against the (AXP853T)
> manual, they match.
Thanks.
>
>> +};
>> +
>> static int axp20x_power_off(struct sys_off_data *data)
>> {
>> struct axp20x_dev *axp20x = data->cb_data;
>> @@ -934,6 +1012,18 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
>> */
>> axp20x->regmap_irq_chip = &axp803_regmap_irq_chip;
>> break;
>> + case AXP15060_ID:
>> + /* Don't register the power key part if there is no interrupt line. */
> Mmh, I don't know if this is really needed. If the IRQ line is not
> connected, it isn't really critical, it just wouldn't work, would it? I
> wonder if the same problem applies to all the other PMICs?
> I see that we handle it like this for the AXP806, but I wonder if this is
> more a by-product of the slave mode requirement there.
There would be a OOPS about pek driver in dmesg output if this
part does not exist. Since most use cases of AXP PMICs are Allwinner
SOCs and board designers would follow AW's reference design, this
shouldn't be a problem for other variants.
>> + if (axp20x->irq > 0) {
>> + axp20x->nr_cells = ARRAY_SIZE(axp15060_cells);
>> + axp20x->cells = axp15060_cells;
>> + } else {
>> + axp20x->nr_cells = ARRAY_SIZE(axp806_cells);
>> + axp20x->cells = axp806_cells;
> That looks a bit odd. Either we rename this alternative axp806_cells
> definition somewhere above to something generic (axp_regulator_cells?), or
> there should be a comment here explaining why all of a sudden an AXP15060
> without an IRQ line is the same as the AXP806.
Agree, I'm thinking about creating a new cell named axp_regulator_only_cells
or something with no .id configured, and add comment for it.
>
>> + }
>> + axp20x->regmap_cfg = &axp15060_regmap_config;
>> + axp20x->regmap_irq_chip = &axp15060_regmap_irq_chip;
>> + break;
>> default:
>> dev_err(dev, "unsupported AXP20X ID %lu\n", axp20x->variant);
>> return -EINVAL;
>> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
>> index 2058194807bd..abc2bdc54bf5 100644
>> --- a/include/linux/mfd/axp20x.h
>> +++ b/include/linux/mfd/axp20x.h
>> @@ -21,6 +21,7 @@ enum axp20x_variants {
>> AXP806_ID,
>> AXP809_ID,
>> AXP813_ID,
>> + AXP15060_ID,
>> NR_AXP20X_VARIANTS,
>> };
>>
>> @@ -131,6 +132,39 @@ enum axp20x_variants {
>> /* Other DCDC regulator control registers are the same as AXP803 */
>> #define AXP813_DCDC7_V_OUT 0x26
>>
>> +#define AXP15060_STARTUP_SRC 0x00
>> +#define AXP15060_PWR_OUT_CTRL1 0x10
>> +#define AXP15060_PWR_OUT_CTRL2 0x11
>> +#define AXP15060_PWR_OUT_CTRL3 0x12
>> +#define AXP15060_DCDC1_V_CTRL 0x13
>> +#define AXP15060_DCDC2_V_CTRL 0x14
>> +#define AXP15060_DCDC3_V_CTRL 0x15
>> +#define AXP15060_DCDC4_V_CTRL 0x16
>> +#define AXP15060_DCDC5_V_CTRL 0x17
>> +#define AXP15060_DCDC6_V_CTRL 0x18
>> +#define AXP15060_ALDO1_V_CTRL 0x19
>> +#define AXP15060_DCDC_MODE_CTRL1 0x1a
>> +#define AXP15060_DCDC_MODE_CTRL2 0x1b
>> +#define AXP15060_OUTPUT_MONITOR_DISCHARGE 0x1e
>> +#define AXP15060_IRQ_PWROK_VOFF 0x1f
>> +#define AXP15060_ALDO2_V_CTRL 0x20
>> +#define AXP15060_ALDO3_V_CTRL 0x21
>> +#define AXP15060_ALDO4_V_CTRL 0x22
>> +#define AXP15060_ALDO5_V_CTRL 0x23
>> +#define AXP15060_BLDO1_V_CTRL 0x24
>> +#define AXP15060_BLDO2_V_CTRL 0x25
>> +#define AXP15060_BLDO3_V_CTRL 0x26
>> +#define AXP15060_BLDO4_V_CTRL 0x27
>> +#define AXP15060_BLDO5_V_CTRL 0x28
>> +#define AXP15060_CLDO1_V_CTRL 0x29
>> +#define AXP15060_CLDO2_V_CTRL 0x2a
>> +#define AXP15060_CLDO3_V_CTRL 0x2b
>> +#define AXP15060_CLDO4_V_CTRL 0x2d
>> +#define AXP15060_CPUSLDO_V_CTRL 0x2e
>> +#define AXP15060_PWR_WAKEUP_CTRL 0x31
>> +#define AXP15060_PWR_DISABLE_DOWN_SEQ 0x32
>> +#define AXP15060_PEK_KEY 0x36
>> +
>> /* Interrupt */
>> #define AXP152_IRQ1_EN 0x40
>> #define AXP152_IRQ2_EN 0x41
>> @@ -139,6 +173,11 @@ enum axp20x_variants {
>> #define AXP152_IRQ2_STATE 0x49
>> #define AXP152_IRQ3_STATE 0x4a
>>
>> +#define AXP15060_IRQ1_EN 0x40
>> +#define AXP15060_IRQ2_EN 0x41
>> +#define AXP15060_IRQ1_STATE 0x48
>> +#define AXP15060_IRQ2_STATE 0x49
>> +
>> #define AXP20X_IRQ1_EN 0x40
>> #define AXP20X_IRQ2_EN 0x41
>> #define AXP20X_IRQ3_EN 0x42
>> @@ -222,6 +261,8 @@ enum axp20x_variants {
>> #define AXP22X_GPIO_STATE 0x94
>> #define AXP22X_GPIO_PULL_DOWN 0x95
>>
>> +#define AXP15060_CLDO4_GPIO2_MODESET 0x2c
>> +
> Compared the register offsets against the manual, they match.
>
> Cheers,
> Andre
Thanks,
Shengyu
>> /* Battery */
>> #define AXP20X_CHRG_CC_31_24 0xb0
>> #define AXP20X_CHRG_CC_23_16 0xb1
>> @@ -419,6 +460,33 @@ enum {
>> AXP813_REG_ID_MAX,
>> };
>>
>> +enum {
>> + AXP15060_DCDC1 = 0,
>> + AXP15060_DCDC2,
>> + AXP15060_DCDC3,
>> + AXP15060_DCDC4,
>> + AXP15060_DCDC5,
>> + AXP15060_DCDC6,
>> + AXP15060_ALDO1,
>> + AXP15060_ALDO2,
>> + AXP15060_ALDO3,
>> + AXP15060_ALDO4,
>> + AXP15060_ALDO5,
>> + AXP15060_BLDO1,
>> + AXP15060_BLDO2,
>> + AXP15060_BLDO3,
>> + AXP15060_BLDO4,
>> + AXP15060_BLDO5,
>> + AXP15060_CLDO1,
>> + AXP15060_CLDO2,
>> + AXP15060_CLDO3,
>> + AXP15060_CLDO4,
>> + AXP15060_CPUSLDO,
>> + AXP15060_SW,
>> + AXP15060_RTC_LDO,
>> + AXP15060_REG_ID_MAX,
>> +};
>> +
>> /* IRQs */
>> enum {
>> AXP152_IRQ_LDO0IN_CONNECT = 1,
>> @@ -637,6 +705,23 @@ enum axp809_irqs {
>> AXP809_IRQ_GPIO0_INPUT,
>> };
>>
>> +enum axp15060_irqs {
>> + AXP15060_IRQ_DIE_TEMP_HIGH_LV1 = 1,
>> + AXP15060_IRQ_DIE_TEMP_HIGH_LV2,
>> + AXP15060_IRQ_DCDC1_V_LOW,
>> + AXP15060_IRQ_DCDC2_V_LOW,
>> + AXP15060_IRQ_DCDC3_V_LOW,
>> + AXP15060_IRQ_DCDC4_V_LOW,
>> + AXP15060_IRQ_DCDC5_V_LOW,
>> + AXP15060_IRQ_DCDC6_V_LOW,
>> + AXP15060_IRQ_PEK_LONG,
>> + AXP15060_IRQ_PEK_SHORT,
>> + AXP15060_IRQ_GPIO1_INPUT,
>> + AXP15060_IRQ_PEK_FAL_EDGE,
>> + AXP15060_IRQ_PEK_RIS_EDGE,
>> + AXP15060_IRQ_GPIO2_INPUT,
>> +};
>> +
>> struct axp20x_dev {
>> struct device *dev;
>> int irq;
Download attachment "OpenPGP_0xE3520CC91929C8E7.asc" of type "application/pgp-keys" (6869 bytes)
Download attachment "OpenPGP_signature" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists