Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230120184500.1899814-1-martin.botka@somainline.org>
 <20230120184500.1899814-3-martin.botka@somainline.org> <CAGb2v6607ErP=Jr_-LC_iE2yVLgW0PF+8mpv=AQNSsVreZ42iA@mail.gmail.com>
 <20230323135904.05174c13@donnerap.cambridge.arm.com>
In-Reply-To: <20230323135904.05174c13@donnerap.cambridge.arm.com>
Reply-To: wens@csie.org
From:   Chen-Yu Tsai <wens@csie.org>
Date:   Thu, 23 Mar 2023 22:24:04 +0800
Message-ID: <CAGb2v65WXtnm8O1ZL-0RT7vXew833=hzTDfFvEPH8j-wA8egGQ@mail.gmail.com>
Subject: Re: [PATCH v8 2/3] mfd: axp20x: Add support for AXP313a PMIC
To:     Andre Przywara <andre.przywara@arm.com>
Cc:     Martin Botka <martin.botka@somainline.org>,
        martin.botka1@gmail.com,
        Konrad Dybcio <konrad.dybcio@somainline.org>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@somainline.org>,
        Marijn Suijten <marijn.suijten@somainline.org>,
        Jami Kettunen <jamipkettunen@somainline.org>,
        Paul Bouchara <paul.bouchara@somainline.org>,
        Jan Trmal <jtrmal@gmail.com>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        Samuel Holland <samuel@sholland.org>,
        Lee Jones <lee@kernel.org>, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Thu, Mar 23, 2023 at 9:59 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Sat, 28 Jan 2023 01:40:34 +0800
> Chen-Yu Tsai <wens@csie.org> wrote:
>
> Hi Chen-Yu,
>
> thanks for the review!
>
> > On Sat, Jan 21, 2023 at 2:45 AM Martin Botka
> > <martin.botka@somainline.org> wrote:
> > >
> > > The AXP313a is a PMIC chip produced by X-Powers, it can be connected via
> > > an I2C bus.
> > > The name AXP1530 seems to appear as well, and this is what is used in
> > > the BSP driver. From all we know it's the same chip, just a different
> > > name. However we have only seen AXP313a chips in the wild, so go with
> > > this name.
> > >
> > > Compared to the other AXP PMICs it's a rather simple affair: just three
> > > DCDC converters, three LDOs, and no battery charging support.
> > >
> > > Describe the regmap and the MFD bits, along with the registers exposed
> > > via I2C. Eventually advertise the device using the new compatible
> > > string.
> > >
> > > Signed-off-by: Martin Botka <martin.botka@somainline.org>
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > ---
> > >  drivers/mfd/axp20x-i2c.c   |  2 ++
> > >  drivers/mfd/axp20x.c       | 61 ++++++++++++++++++++++++++++++++++++++
> > >  include/linux/mfd/axp20x.h | 32 ++++++++++++++++++++
> > >  3 files changed, 95 insertions(+)
> > >
> > > diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
> > > index f49fbd307958..f061177cb18e 100644
> > > --- a/drivers/mfd/axp20x-i2c.c
> > > +++ b/drivers/mfd/axp20x-i2c.c
> > > @@ -63,6 +63,7 @@ static const struct of_device_id axp20x_i2c_of_match[] = {
> > >         { .compatible = "x-powers,axp209", .data = (void *)AXP209_ID },
> > >         { .compatible = "x-powers,axp221", .data = (void *)AXP221_ID },
> > >         { .compatible = "x-powers,axp223", .data = (void *)AXP223_ID },
> > > +       { .compatible = "x-powers,axp313a", .data = (void *)AXP313A_ID},
> > >         { .compatible = "x-powers,axp803", .data = (void *)AXP803_ID },
> > >         { .compatible = "x-powers,axp806", .data = (void *)AXP806_ID },
> > >         { },
> > > @@ -76,6 +77,7 @@ static const struct i2c_device_id axp20x_i2c_id[] = {
> > >         { "axp209", 0 },
> > >         { "axp221", 0 },
> > >         { "axp223", 0 },
> > > +       { "axp313a", 0 },
> > >         { "axp803", 0 },
> > >         { "axp806", 0 },
> > >         { },
> > > diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> > > index 01a6bbb6d266..ff15775f3c27 100644
> > > --- a/drivers/mfd/axp20x.c
> > > +++ b/drivers/mfd/axp20x.c
> > > @@ -39,6 +39,7 @@ static const char * const axp20x_model_names[] = {
> > >         "AXP221",
> > >         "AXP223",
> > >         "AXP288",
> > > +       "AXP313a",
> > >         "AXP803",
> > >         "AXP806",
> > >         "AXP809",
> > > @@ -154,6 +155,24 @@ static const struct regmap_range axp806_writeable_ranges[] = {
> > >         regmap_reg_range(AXP806_REG_ADDR_EXT, AXP806_REG_ADDR_EXT),
> > >  };
> > >
> > > +static const struct regmap_range axp313a_writeable_ranges[] = {
> > > +       regmap_reg_range(AXP313A_ON_INDICATE, AXP313A_IRQ_STATE),
> > > +};
> > > +
> > > +static const struct regmap_range axp313a_volatile_ranges[] = {
> > > +       regmap_reg_range(AXP313A_ON_INDICATE, AXP313A_IRQ_STATE),
> >
> > Why set the whole range as volatile? Why bother with a cache then?
>
> Fixed.
>
> >
> > > +};
> > > +
> > > +static const struct regmap_access_table axp313a_writeable_table = {
> > > +       .yes_ranges = axp313a_writeable_ranges,
> > > +       .n_yes_ranges = ARRAY_SIZE(axp313a_writeable_ranges),
> > > +};
> > > +
> > > +static const struct regmap_access_table axp313a_volatile_table = {
> > > +       .yes_ranges = axp313a_volatile_ranges,
> > > +       .n_yes_ranges = ARRAY_SIZE(axp313a_volatile_ranges),
> > > +};
> > > +
> > >  static const struct regmap_range axp806_volatile_ranges[] = {
> > >         regmap_reg_range(AXP20X_IRQ1_STATE, AXP20X_IRQ2_STATE),
> > >  };
> > > @@ -272,6 +291,15 @@ static const struct regmap_config axp288_regmap_config = {
> > >         .cache_type     = REGCACHE_RBTREE,
> > >  };
> > >
> > > +static const struct regmap_config axp313a_regmap_config = {
> > > +       .reg_bits = 8,
> > > +       .val_bits = 8,
> > > +       .wr_table = &axp313a_writeable_table,
> > > +       .volatile_table = &axp313a_volatile_table,
> > > +       .max_register = AXP313A_IRQ_STATE,
> > > +       .cache_type = REGCACHE_RBTREE,
> > > +};
> > > +
> > >  static const struct regmap_config axp806_regmap_config = {
> > >         .reg_bits       = 8,
> > >         .val_bits       = 8,
> > > @@ -415,6 +443,16 @@ static const struct regmap_irq axp288_regmap_irqs[] = {
> > >         INIT_REGMAP_IRQ(AXP288, BC_USB_CHNG,            5, 1),
> > >  };
> > >
> > > +static const struct regmap_irq axp313a_regmap_irqs[] = {
> > > +       INIT_REGMAP_IRQ(AXP313A, PEK_RIS_EDGE,          0, 7),
> > > +       INIT_REGMAP_IRQ(AXP313A, PEK_FAL_EDGE,          0, 6),
> > > +       INIT_REGMAP_IRQ(AXP313A, PEK_SHORT,             0, 5),
> > > +       INIT_REGMAP_IRQ(AXP313A, PEK_LONG,              0, 4),
> > > +       INIT_REGMAP_IRQ(AXP313A, DCDC3_V_LOW,           0, 3),
> > > +       INIT_REGMAP_IRQ(AXP313A, DCDC2_V_LOW,           0, 2),
> > > +       INIT_REGMAP_IRQ(AXP313A, DIE_TEMP_HIGH,         0, 0),
> > > +};
> > > +
> > >  static const struct regmap_irq axp803_regmap_irqs[] = {
> > >         INIT_REGMAP_IRQ(AXP803, ACIN_OVER_V,            0, 7),
> > >         INIT_REGMAP_IRQ(AXP803, ACIN_PLUGIN,            0, 6),
> > > @@ -548,6 +586,17 @@ static const struct regmap_irq_chip axp288_regmap_irq_chip = {
> > >
> > >  };
> > >
> > > +static const struct regmap_irq_chip axp313a_regmap_irq_chip = {
> > > +       .name                   = "axp313a_irq_chip",
> > > +       .status_base            = AXP313A_IRQ_STATE,
> > > +       .ack_base               = AXP313A_IRQ_STATE,
> > > +       .unmask_base            = AXP313A_IRQ_EN,
> > > +       .init_ack_masked        = true,
> > > +       .irqs                   = axp313a_regmap_irqs,
> > > +       .num_irqs               = ARRAY_SIZE(axp313a_regmap_irqs),
> > > +       .num_regs               = 1,
> > > +};
> > > +
> > >  static const struct regmap_irq_chip axp803_regmap_irq_chip = {
> > >         .name                   = "axp803",
> > >         .status_base            = AXP20X_IRQ1_STATE,
> > > @@ -676,6 +725,12 @@ static const struct mfd_cell axp152_cells[] = {
> > >         },
> > >  };
> > >
> > > +static struct mfd_cell axp313a_cells[] = {
> > > +       {
> > > +               .name = "axp20x-regulator",
> >
> > Lee asked for MFD_CELL_NAME() in v7 here.
>
> Fixed.
>
> > Could you also add the power button cell? This would make it an actual
> > MFD, and also complete, since that is the only other function this
> > PMIC has. Or at least add a note mentioning it. Implementing it will
>
> Done.
>
> > require a device that actually routes that pin out. AFAICT the MangoPi
> > doesn't.
> >
> > > +       },
> > > +};
> > > +
> > >  static const struct resource axp288_adc_resources[] = {
> > >         DEFINE_RES_IRQ_NAMED(AXP288_IRQ_GPADC, "GPADC"),
> > >  };
> > > @@ -892,6 +947,12 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
> > >                 axp20x->regmap_irq_chip = &axp288_regmap_irq_chip;
> > >                 axp20x->irq_flags = IRQF_TRIGGER_LOW;
> > >                 break;
> > > +       case AXP313A_ID:
> > > +               axp20x->nr_cells = ARRAY_SIZE(axp313a_cells);
> > > +               axp20x->cells = axp313a_cells;
> > > +               axp20x->regmap_cfg = &axp313a_regmap_config;
> > > +               axp20x->regmap_irq_chip = &axp313a_regmap_irq_chip;
> > > +               break;
> > >         case AXP803_ID:
> > >                 axp20x->nr_cells = ARRAY_SIZE(axp803_cells);
> > >                 axp20x->cells = axp803_cells;
> > > diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> > > index 2058194807bd..12e4fc3e8391 100644
> > > --- a/include/linux/mfd/axp20x.h
> > > +++ b/include/linux/mfd/axp20x.h
> > > @@ -17,6 +17,7 @@ enum axp20x_variants {
> > >         AXP221_ID,
> > >         AXP223_ID,
> > >         AXP288_ID,
> > > +       AXP313A_ID,
> > >         AXP803_ID,
> > >         AXP806_ID,
> > >         AXP809_ID,
> > > @@ -91,6 +92,17 @@ enum axp20x_variants {
> > >  #define AXP22X_ALDO3_V_OUT             0x2a
> > >  #define AXP22X_CHRG_CTRL3              0x35
> > >
> > > +#define AXP313A_ON_INDICATE            0x00
> > > +#define AXP313A_OUTPUT_CONTROL         0x10
> > > +#define AXP313A_DCDC1_CONRTOL          0x13
> > > +#define AXP313A_DCDC2_CONRTOL          0x14
> > > +#define AXP313A_DCDC3_CONRTOL          0x15
> > > +#define AXP313A_ALDO1_CONRTOL          0x16
> > > +#define AXP313A_DLDO1_CONRTOL          0x17
> >
> > Please also add register 0x1a (note, some bits of this are volatile)
> > and implement power off with bit 7. The current axp_power_off()
> > function will not work for this PMIC.
>
> Ah, good catch, thanks. Though I guess this will be unused (on 64-bit
> SoCs), since PSCI poweroff takes precedence due to its higher firmware
> priority.
> Fixed anyway, need to test this with a hacked priority value.
>
> Though the volatility of this register is a bit questionable, isn't it? It
> just seems to apply to the poweroff and reset bits, that are self-reset.
> Nobody cares for the former, and we don't use the reset (yet). I added it
> to the volatile range anyway.
>
> > This PMIC also supports software-triggered reset with bit 6 in the same
> > register. This function would be nice to have, however there's no related
> > code in the mfd driver right now, since IIRC none of the other ones had
> > this.
>
> What would be the use case? Who would trigger that reset? I guess it might
> be more useful for management firmware like crust?

That, or implementing system reset if PSCI/crust isn't available? PMIC reset
would be more thorough than the SoC watchdog reset, where you could have
regulators left in a state incorrect for BROM execution (such as storage
power disabled).


ChenYu

> > > +#define AXP313A_OUTPUT_MONITOR         0x1d
> >
> > Not sure why you need this?
>
> Removed.
>
> > > +#define AXP313A_IRQ_EN                 0x20
> > > +#define AXP313A_IRQ_STATE              0x21
> > > +
> > >  #define AXP806_STARTUP_SRC             0x00
> > >  #define AXP806_CHIP_ID                 0x03
> > >  #define AXP806_PWR_OUT_CTRL1           0x10
> > > @@ -322,6 +334,16 @@ enum {
> > >         AXP22X_REG_ID_MAX,
> > >  };
> > >
> > > +enum {
> > > +       AXP313A_DCDC1 = 0,
> > > +       AXP313A_DCDC2,
> > > +       AXP313A_DCDC3,
> > > +       AXP313A_LDO1,
> >
> > This is called ALDO1 in the datasheet ...
> >
> > > +       AXP313A_LDO2,
> >
> > ... and this one DLDO1.
> >
> > You already have the registers named that way, so you might as well
> > fix the names here as well.
>
> Fixed.
>
> Thanks,
> Andre
>
> >
> >
> > Thanks
> > ChenYu
> >
> > > +       AXP313A_RTC_LDO,
> > > +       AXP313A_REG_ID_MAX,
> > > +};
> > > +
> > >  enum {
> > >         AXP806_DCDCA = 0,
> > >         AXP806_DCDCB,
> > > @@ -548,6 +570,16 @@ enum axp288_irqs {
> > >         AXP288_IRQ_BC_USB_CHNG,
> > >  };
> > >
> > > +enum axp313a_irqs {
> > > +       AXP313A_IRQ_DIE_TEMP_HIGH,
> > > +       AXP313A_IRQ_DCDC2_V_LOW = 2,
> > > +       AXP313A_IRQ_DCDC3_V_LOW,
> > > +       AXP313A_IRQ_PEK_LONG,
> > > +       AXP313A_IRQ_PEK_SHORT,
> > > +       AXP313A_IRQ_PEK_FAL_EDGE,
> > > +       AXP313A_IRQ_PEK_RIS_EDGE,
> > > +};
> > > +
> > >  enum axp803_irqs {
> > >         AXP803_IRQ_ACIN_OVER_V = 1,
> > >         AXP803_IRQ_ACIN_PLUGIN,
> > > --
> > > 2.39.0
> > >
>