[PATCH v2 0/2] mfd: axp20x: Fix AXP806 access errors on cold boot

[PATCH v2 0/2] mfd: axp20x: Fix AXP806 access errors on cold boot

[Chen-Yu Tsai]

Hi Lee,

This is v2 of my AXP806 cold boot access fix series.

Changes since v1:

  - Added define for value 0x10 written to AXP806_REG_ADDR_EXT
    register.

Cover letter from v1:

Recently we've added full SPL support for A80 to mainline U-boot. This
means we no longer depend on Allwinner's bootloader. It also means that
some of system configuration the bootloader set up no longer applies.

The bootloader was correctly configuring the multi-device addressing
support in the AXP806 PMIC. If the PMIC is not correctly addressed, it
just ignores all reads and writes to the other registers. As mainline
U-boot does not support the AXP806, and since we can't always count on
a good bootloader, we should re-configure this in the kernel regardless.

Patch 1 adds the registers for the multi-device addressing scheme to
the AXP806 regmap.

Patch 2 configures the register at probe time, and reinitializes the
regmap cache.

Since support for this PMIC was just added in 4.9-rc1, I hope you can
merge these 2 patches as fixes for 4.9.

Thank you!


Regards
ChenYu

Chen-Yu Tsai (2):
  mfd: axp20x: Add address extension registers for AXP806 regmap
  mfd: axp20x: Fix AXP806 access errors on cold boot

 drivers/mfd/axp20x.c       | 41 ++++++++++++++++++++++++++++++++++++++++-
 include/linux/mfd/axp20x.h |  2 ++
 2 files changed, 42 insertions(+), 1 deletion(-)

-- 
2.10.2

[PATCH v2 1/2] mfd: axp20x: Add address extension registers for AXP806 regmap

[Chen-Yu Tsai]

The AXP806 supports either master/standalone or slave mode.
Slave mode allows sharing the serial bus, even with multiple
AXP806 which all have the same hardware address.

This is done with extra "serial interface address extension",
or AXP806_BUS_ADDR_EXT, and "register address extension", or
AXP806_REG_ADDR_EXT, registers. The former is read-only, with
1 bit customizable at the factory, and 1 bit depending on the
state of an external pin. The latter is writable. Only when
the these device addressing bits (in the upper 4 bits of the
registers) match, will the device respond to operations on
its other registers.

Add these 2 registers to the regmap so we can access them.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/mfd/axp20x.c       | 3 ++-
 include/linux/mfd/axp20x.h | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index ba130be32e61..cdaeb34a9a38 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -135,6 +135,7 @@ static const struct regmap_range axp806_writeable_ranges[] = {
 	regmap_reg_range(AXP806_PWR_OUT_CTRL1, AXP806_CLDO3_V_CTRL),
 	regmap_reg_range(AXP20X_IRQ1_EN, AXP20X_IRQ2_EN),
 	regmap_reg_range(AXP20X_IRQ1_STATE, AXP20X_IRQ2_STATE),
+	regmap_reg_range(AXP806_REG_ADDR_EXT, AXP806_REG_ADDR_EXT),
 };
 
 static const struct regmap_range axp806_volatile_ranges[] = {
@@ -305,7 +306,7 @@ static const struct regmap_config axp806_regmap_config = {
 	.val_bits	= 8,
 	.wr_table	= &axp806_writeable_table,
 	.volatile_table	= &axp806_volatile_table,
-	.max_register	= AXP806_VREF_TEMP_WARN_L,
+	.max_register	= AXP806_REG_ADDR_EXT,
 	.cache_type	= REGCACHE_RBTREE,
 };
 
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index fec597fb34cb..7e85ececcedf 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -115,6 +115,8 @@ enum {
 #define AXP806_CLDO2_V_CTRL		0x25
 #define AXP806_CLDO3_V_CTRL		0x26
 #define AXP806_VREF_TEMP_WARN_L		0xf3
+#define AXP806_BUS_ADDR_EXT		0xfe
+#define AXP806_REG_ADDR_EXT		0xff
 
 /* Interrupt */
 #define AXP152_IRQ1_EN			0x40
-- 
2.10.2

Re: [PATCH v2 1/2] mfd: axp20x: Add address extension registers for AXP806 regmap

[Lee Jones]

On Wed, 23 Nov 2016, Chen-Yu Tsai wrote:

> The AXP806 supports either master/standalone or slave mode.
> Slave mode allows sharing the serial bus, even with multiple
> AXP806 which all have the same hardware address.
> 
> This is done with extra "serial interface address extension",
> or AXP806_BUS_ADDR_EXT, and "register address extension", or
> AXP806_REG_ADDR_EXT, registers. The former is read-only, with
> 1 bit customizable at the factory, and 1 bit depending on the
> state of an external pin. The latter is writable. Only when
> the these device addressing bits (in the upper 4 bits of the
> registers) match, will the device respond to operations on
> its other registers.
> 
> Add these 2 registers to the regmap so we can access them.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  drivers/mfd/axp20x.c       | 3 ++-
>  include/linux/mfd/axp20x.h | 2 ++
>  2 files changed, 4 insertions(+), 1 deletion(-)

Applied for v4.11, thanks.

> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index ba130be32e61..cdaeb34a9a38 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -135,6 +135,7 @@ static const struct regmap_range axp806_writeable_ranges[] = {
>  	regmap_reg_range(AXP806_PWR_OUT_CTRL1, AXP806_CLDO3_V_CTRL),
>  	regmap_reg_range(AXP20X_IRQ1_EN, AXP20X_IRQ2_EN),
>  	regmap_reg_range(AXP20X_IRQ1_STATE, AXP20X_IRQ2_STATE),
> +	regmap_reg_range(AXP806_REG_ADDR_EXT, AXP806_REG_ADDR_EXT),
>  };
>  
>  static const struct regmap_range axp806_volatile_ranges[] = {
> @@ -305,7 +306,7 @@ static const struct regmap_config axp806_regmap_config = {
>  	.val_bits	= 8,
>  	.wr_table	= &axp806_writeable_table,
>  	.volatile_table	= &axp806_volatile_table,
> -	.max_register	= AXP806_VREF_TEMP_WARN_L,
> +	.max_register	= AXP806_REG_ADDR_EXT,
>  	.cache_type	= REGCACHE_RBTREE,
>  };
>  
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index fec597fb34cb..7e85ececcedf 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -115,6 +115,8 @@ enum {
>  #define AXP806_CLDO2_V_CTRL		0x25
>  #define AXP806_CLDO3_V_CTRL		0x26
>  #define AXP806_VREF_TEMP_WARN_L		0xf3
> +#define AXP806_BUS_ADDR_EXT		0xfe
> +#define AXP806_REG_ADDR_EXT		0xff
>  
>  /* Interrupt */
>  #define AXP152_IRQ1_EN			0x40

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 2/2] mfd: axp20x: Fix AXP806 access errors on cold boot

[Chen-Yu Tsai]

The AXP806 supports either master/standalone or slave mode.
Slave mode allows sharing the serial bus, even with multiple
AXP806 which all have the same hardware address.

This is done with extra "serial interface address extension",
or AXP806_BUS_ADDR_EXT, and "register address extension", or
AXP806_REG_ADDR_EXT, registers. The former is read-only, with
1 bit customizable at the factory, and 1 bit depending on the
state of an external pin. The latter is writable. Only when
the these device addressing bits (in the upper 4 bits of the
registers) match, will the device respond to operations on
its other registers.

The AXP806_REG_ADDR_EXT was previously configured by Allwinner's
bootloader. Work on U-boot SPL support now allows us to switch
to mainline U-boot, which doesn't do this for us. There might
be other bare minimum bootloaders out there which don't to this
either. It's best to handle this in the kernel.

This patch sets AXP806_REG_ADDR_EXT to 0x10, which is what we
know to be the proper value for a standard AXP806 in slave mode.
Afterwards it will reinitialize the regmap cache, to purge any
invalid stale values.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/mfd/axp20x.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index cdaeb34a9a38..a0166c667656 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -31,6 +31,8 @@
 
 #define AXP20X_OFF	0x80
 
+#define AXP806_REG_ADDR_EXT_ADDR_SLAVE_MODE	BIT(4)
+
 static const char * const axp20x_model_names[] = {
 	"AXP152",
 	"AXP202",
@@ -829,6 +831,42 @@ int axp20x_device_probe(struct axp20x_dev *axp20x)
 {
 	int ret;
 
+	/*
+	 * The AXP806 supports either master/standalone or slave mode.
+	 * Slave mode allows sharing the serial bus, even with multiple
+	 * AXP806 which all have the same hardware address.
+	 *
+	 * This is done with extra "serial interface address extension",
+	 * or AXP806_BUS_ADDR_EXT, and "register address extension", or
+	 * AXP806_REG_ADDR_EXT, registers. The former is read-only, with
+	 * 1 bit customizable at the factory, and 1 bit depending on the
+	 * state of an external pin. The latter is writable. Only when
+	 * the these device addressing bits (in the upper 4 bits of the
+	 * registers) match, will the device respond to operations on its
+	 * other registers.
+	 *
+	 * Since we only support an AXP806 chained to an AXP809 in slave
+	 * mode, and there isn't any existing hardware which uses AXP806
+	 * in master mode, or has 2 AXP806s in the same system, we can
+	 * just program the register address extension to the slave mode
+	 * address.
+	 */
+	if (axp20x->variant == AXP806_ID) {
+		/* Write to the register address extension register */
+		regmap_write(axp20x->regmap, AXP806_REG_ADDR_EXT,
+			     AXP806_REG_ADDR_EXT_ADDR_SLAVE_MODE);
+
+		/* Make sure the write hits the device */
+		regcache_sync_region(axp20x->regmap, AXP806_REG_ADDR_EXT,
+				     AXP806_REG_ADDR_EXT);
+
+		/*
+		 * Reinitialize the regmap cache in case the device didn't
+		 * properly respond to our reads before.
+		 */
+		regmap_reinit_cache(axp20x->regmap, axp20x->regmap_cfg);
+	}
+
 	ret = regmap_add_irq_chip(axp20x->regmap, axp20x->irq,
 				  IRQF_ONESHOT | IRQF_SHARED, -1,
 				  axp20x->regmap_irq_chip,
-- 
2.10.2

Re: [PATCH v2 2/2] mfd: axp20x: Fix AXP806 access errors on cold boot

[Lee Jones]

Mark,

Is the following valid/necessary?

On Wed, 23 Nov 2016, Chen-Yu Tsai wrote:
> The AXP806 supports either master/standalone or slave mode.
> Slave mode allows sharing the serial bus, even with multiple
> AXP806 which all have the same hardware address.
> 
> This is done with extra "serial interface address extension",
> or AXP806_BUS_ADDR_EXT, and "register address extension", or
> AXP806_REG_ADDR_EXT, registers. The former is read-only, with
> 1 bit customizable at the factory, and 1 bit depending on the
> state of an external pin. The latter is writable. Only when
> the these device addressing bits (in the upper 4 bits of the
> registers) match, will the device respond to operations on
> its other registers.
> 
> The AXP806_REG_ADDR_EXT was previously configured by Allwinner's
> bootloader. Work on U-boot SPL support now allows us to switch
> to mainline U-boot, which doesn't do this for us. There might
> be other bare minimum bootloaders out there which don't to this
> either. It's best to handle this in the kernel.
> 
> This patch sets AXP806_REG_ADDR_EXT to 0x10, which is what we
> know to be the proper value for a standard AXP806 in slave mode.
> Afterwards it will reinitialize the regmap cache, to purge any
> invalid stale values.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  drivers/mfd/axp20x.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index cdaeb34a9a38..a0166c667656 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -31,6 +31,8 @@
>  
>  #define AXP20X_OFF	0x80
>  
> +#define AXP806_REG_ADDR_EXT_ADDR_SLAVE_MODE	BIT(4)
> +
>  static const char * const axp20x_model_names[] = {
>  	"AXP152",
>  	"AXP202",
> @@ -829,6 +831,42 @@ int axp20x_device_probe(struct axp20x_dev *axp20x)
>  {
>  	int ret;
>  
> +	/*
> +	 * The AXP806 supports either master/standalone or slave mode.
> +	 * Slave mode allows sharing the serial bus, even with multiple
> +	 * AXP806 which all have the same hardware address.
> +	 *
> +	 * This is done with extra "serial interface address extension",
> +	 * or AXP806_BUS_ADDR_EXT, and "register address extension", or
> +	 * AXP806_REG_ADDR_EXT, registers. The former is read-only, with
> +	 * 1 bit customizable at the factory, and 1 bit depending on the
> +	 * state of an external pin. The latter is writable. Only when
> +	 * the these device addressing bits (in the upper 4 bits of the
> +	 * registers) match, will the device respond to operations on its
> +	 * other registers.
> +	 *
> +	 * Since we only support an AXP806 chained to an AXP809 in slave
> +	 * mode, and there isn't any existing hardware which uses AXP806
> +	 * in master mode, or has 2 AXP806s in the same system, we can
> +	 * just program the register address extension to the slave mode
> +	 * address.
> +	 */
> +	if (axp20x->variant == AXP806_ID) {
> +		/* Write to the register address extension register */
> +		regmap_write(axp20x->regmap, AXP806_REG_ADDR_EXT,
> +			     AXP806_REG_ADDR_EXT_ADDR_SLAVE_MODE);
> +
> +		/* Make sure the write hits the device */
> +		regcache_sync_region(axp20x->regmap, AXP806_REG_ADDR_EXT,
> +				     AXP806_REG_ADDR_EXT);
> +
> +		/*
> +		 * Reinitialize the regmap cache in case the device didn't
> +		 * properly respond to our reads before.
> +		 */
> +		regmap_reinit_cache(axp20x->regmap, axp20x->regmap_cfg);
> +	}
> +
>  	ret = regmap_add_irq_chip(axp20x->regmap, axp20x->irq,
>  				  IRQF_ONESHOT | IRQF_SHARED, -1,
>  				  axp20x->regmap_irq_chip,

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 2/2] mfd: axp20x: Fix AXP806 access errors on cold boot

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]

On Fri, Dec 09, 2016 at 11:20:18AM +0000, Lee Jones wrote:

> Is the following valid/necessary?

> On Wed, 23 Nov 2016, Chen-Yu Tsai wrote:
> > The AXP806 supports either master/standalone or slave mode.
> > Slave mode allows sharing the serial bus, even with multiple
> > AXP806 which all have the same hardware address.

> > This is done with extra "serial interface address extension",
> > or AXP806_BUS_ADDR_EXT, and "register address extension", or
> > AXP806_REG_ADDR_EXT, registers. The former is read-only, with
> > 1 bit customizable at the factory, and 1 bit depending on the

I don't really know anything about the details of this chip, sorry.

> > This patch sets AXP806_REG_ADDR_EXT to 0x10, which is what we
> > know to be the proper value for a standard AXP806 in slave mode.
> > Afterwards it will reinitialize the regmap cache, to purge any
> > invalid stale values.

If the chip has been reset then you'd want to reset the cache too.  I've
no idea if that's needed here or not though, it depends what happens to
the global state of the chip when this reconfiguration happens.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]

Re: [PATCH v2 2/2] mfd: axp20x: Fix AXP806 access errors on cold boot

[Chen-Yu Tsai]

On Wed, Dec 14, 2016 at 12:47 AM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Dec 09, 2016 at 11:20:18AM +0000, Lee Jones wrote:
>
>> Is the following valid/necessary?
>
>> On Wed, 23 Nov 2016, Chen-Yu Tsai wrote:
>> > The AXP806 supports either master/standalone or slave mode.
>> > Slave mode allows sharing the serial bus, even with multiple
>> > AXP806 which all have the same hardware address.
>
>> > This is done with extra "serial interface address extension",
>> > or AXP806_BUS_ADDR_EXT, and "register address extension", or
>> > AXP806_REG_ADDR_EXT, registers. The former is read-only, with
>> > 1 bit customizable at the factory, and 1 bit depending on the
>
> I don't really know anything about the details of this chip, sorry.

If these 2 registers don't match, any access to the other registers
is ignored, so the kernel either read bogus data, or the read fails.

What this patch does is make sure the registers match, to guarantee
access, and then reinitialize the regmap cache to get rid of any
stale data.

>> > This patch sets AXP806_REG_ADDR_EXT to 0x10, which is what we
>> > know to be the proper value for a standard AXP806 in slave mode.
>> > Afterwards it will reinitialize the regmap cache, to purge any
>> > invalid stale values.
>
> If the chip has been reset then you'd want to reset the cache too.  I've
> no idea if that's needed here or not though, it depends what happens to
> the global state of the chip when this reconfiguration happens.

It is not a reset in the general sense. I suppose a better way would
be to do an explicit write to the register first, then initialize
the regmap. I'd have to export the write function from the RSB bus
driver first though.

Regards
ChenYu

Re: [PATCH v2 2/2] mfd: axp20x: Fix AXP806 access errors on cold boot

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 965 bytes --]

On Wed, Dec 14, 2016 at 09:52:31PM +0800, Chen-Yu Tsai wrote:

> What this patch does is make sure the registers match, to guarantee
> access, and then reinitialize the regmap cache to get rid of any
> stale data.

So what you're saying is that previous writes may have been ignored?

> > If the chip has been reset then you'd want to reset the cache too.  I've
> > no idea if that's needed here or not though, it depends what happens to
> > the global state of the chip when this reconfiguration happens.

> It is not a reset in the general sense. I suppose a better way would
> be to do an explicit write to the register first, then initialize
> the regmap. I'd have to export the write function from the RSB bus
> driver first though.

Surely just doing a write immediately after initializing the regmap
would have the same effect?  That'd ensure that the hardware has the
desired value before there are any other writes.  But I might be missing
something here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] mfd: axp20x: Fix AXP806 access errors on cold boot

[Chen-Yu Tsai]

On Thu, Dec 15, 2016 at 1:40 AM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Dec 14, 2016 at 09:52:31PM +0800, Chen-Yu Tsai wrote:
>
>> What this patch does is make sure the registers match, to guarantee
>> access, and then reinitialize the regmap cache to get rid of any
>> stale data.
>
> So what you're saying is that previous writes may have been ignored?

Sorry for the late reply.

Yes, any previous reads and writes may have been ignored. I'm not sure
if regmap cache prefetches any registers. Last I checked it didn't.

>> > If the chip has been reset then you'd want to reset the cache too.  I've
>> > no idea if that's needed here or not though, it depends what happens to
>> > the global state of the chip when this reconfiguration happens.
>
>> It is not a reset in the general sense. I suppose a better way would
>> be to do an explicit write to the register first, then initialize
>> the regmap. I'd have to export the write function from the RSB bus
>> driver first though.
>
> Surely just doing a write immediately after initializing the regmap
> would have the same effect?  That'd ensure that the hardware has the
> desired value before there are any other writes.  But I might be missing
> something here.

So I just tested this. Dropping both the regcache_sync_region() and
regmap_reinit_cache() is ok.

I'll send a revised patch.

ChenYu