[PATCH 2/2] MFD: mc13xxx workaround SPI hardware bug on i.Mx

[PATCH 2/2] MFD: mc13xxx workaround SPI hardware bug on i.Mx

[Mark Brown]

On Tue, May 29, 2012 at 11:06:29AM +0200, Philippe R?tornaz wrote:
> The MC13xxx PMIC is mainly used on i.Mx SoC. On thoses SoC the SPI
> hardware will deassert CS line as soon as the SPI FIFO is empty.
> The MC13xxx hardware is very sensitive to CS line change as it
> corrupts the transfert if CS is deasserted in the middle of a register
> read or write.
> It is not possible to use the CS line as a GPIO on some SoC, so we
> need to workaround this by implementing a single SPI transfer to
> access the PMIC.

Reviwed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

though it's really sad this can't be done in the SPI controller where
the bug is.  You should also set use_single_rw in the regmap_config,
though this is less critical as currently the core won't automatically
generate any bulk I/O.

Obviously this will affect a much wider range of devices when used with
i.MX.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120530/0b925a82/attachment.sig>

[PATCH 2/2] MFD: mc13xxx workaround SPI hardware bug on i.Mx

[Philippe Rétornaz]

Le mercredi 30 mai 2012 18:08:38 Mark Brown a ?crit :
> On Tue, May 29, 2012 at 11:06:29AM +0200, Philippe R?tornaz wrote:
> > The MC13xxx PMIC is mainly used on i.Mx SoC. On thoses SoC the SPI
> > hardware will deassert CS line as soon as the SPI FIFO is empty.
> > The MC13xxx hardware is very sensitive to CS line change as it
> > corrupts the transfert if CS is deasserted in the middle of a register
> > read or write.
> > It is not possible to use the CS line as a GPIO on some SoC, so we
> > need to workaround this by implementing a single SPI transfer to
> > access the PMIC.
> 
> Reviwed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> 
> though it's really sad this can't be done in the SPI controller where
> the bug is.  You should also set use_single_rw in the regmap_config,
> though this is less critical as currently the core won't automatically
> generate any bulk I/O.

I already put it in struct regmap_config, should I put it elsewhere ? 

@@ -54,6 +54,67 @@ static struct regmap_config mc13xxx_regmap_spi_config = {
        .max_register = MC13XXX_NUMREGS,
 
        .cache_type = REGCACHE_NONE,
+       .use_single_rw = 1,
+};

BTW, who will merge this patchset ? 

Thanks ! 

Philippe

[PATCH 2/2] MFD: mc13xxx workaround SPI hardware bug on i.Mx

[Samuel Ortiz]

Hi Philippe,

On Thu, May 31, 2012 at 09:08:39AM +0200, Philippe R?tornaz wrote:
> Le mercredi 30 mai 2012 18:08:38 Mark Brown a ?crit :
> > On Tue, May 29, 2012 at 11:06:29AM +0200, Philippe R?tornaz wrote:
> > > The MC13xxx PMIC is mainly used on i.Mx SoC. On thoses SoC the SPI
> > > hardware will deassert CS line as soon as the SPI FIFO is empty.
> > > The MC13xxx hardware is very sensitive to CS line change as it
> > > corrupts the transfert if CS is deasserted in the middle of a register
> > > read or write.
> > > It is not possible to use the CS line as a GPIO on some SoC, so we
> > > need to workaround this by implementing a single SPI transfer to
> > > access the PMIC.
> > 
> > Reviwed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> > 
> > though it's really sad this can't be done in the SPI controller where
> > the bug is.  You should also set use_single_rw in the regmap_config,
> > though this is less critical as currently the core won't automatically
> > generate any bulk I/O.
> 
> I already put it in struct regmap_config, should I put it elsewhere ? 
> 
> @@ -54,6 +54,67 @@ static struct regmap_config mc13xxx_regmap_spi_config = {
>         .max_register = MC13XXX_NUMREGS,
>  
>         .cache_type = REGCACHE_NONE,
> +       .use_single_rw = 1,
> +};
> 
> BTW, who will merge this patchset ? 
I will.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH 2/2] MFD: mc13xxx workaround SPI hardware bug on i.Mx

[Fabio Estevam]

Hi Samuel,
On Thu, May 31, 2012 at 6:03 AM, Samuel Ortiz <sameo@linux.intel.com> wrote:

>> BTW, who will merge this patchset ?
> I will.

Have you had a chance to apply this?

[PATCH 2/2] MFD: mc13xxx workaround SPI hardware bug on i.Mx

[Philippe Rétornaz]

Le vendredi 22 juin 2012 12:48:19 Fabio Estevam a ?crit :
> Hi Samuel,
> 
> On Thu, May 31, 2012 at 6:03 AM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> >> BTW, who will merge this patchset ?
> > 
> > I will.
> 
> Have you had a chance to apply this?

It would be really nice if it could still be applied to the 3.5 since it 
prevent a crash at boot of all imx31 using mc13xxx.

Thanks ! 

Philippe

[PATCH 2/2] MFD: mc13xxx workaround SPI hardware bug on i.Mx

[Mark Brown]

On Thu, May 31, 2012 at 09:08:39AM +0200, Philippe R?tornaz wrote:

> I already put it in struct regmap_config, should I put it elsewhere ? 

> @@ -54,6 +54,67 @@ static struct regmap_config mc13xxx_regmap_spi_config = {
>         .max_register = MC13XXX_NUMREGS,
>  
>         .cache_type = REGCACHE_NONE,
> +       .use_single_rw = 1,
> +};

That's fine, I'd just expected to see it here.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120531/9f8a5d68/attachment.sig>

[PATCH 2/2] MFD: mc13xxx workaround SPI hardware bug on i.Mx

[Marc Reilly]

Hi Philipe,

Thanks for fixing this.

Acked-by: Marc Reilly <marc@cpdesign.com.au>

Cheers,
Marc

(Some real trivial spelling corrections inline below. I'd ignore them unless 
you do a V2)


On Tuesday, May 29, 2012 07:06:29 PM Philippe R?tornaz wrote:
> The MC13xxx PMIC is mainly used on i.Mx SoC. On thoses SoC the SPI

s/thoses/these

> hardware will deassert CS line as soon as the SPI FIFO is empty.
> The MC13xxx hardware is very sensitive to CS line change as it
> corrupts the transfert if CS is deasserted in the middle of a register

s/transfert/transfer

> read or write.
> It is not possible to use the CS line as a GPIO on some SoC, so we
> need to workaround this by implementing a single SPI transfer to
> access the PMIC.
> 
> Signed-off-by: Philippe R?tornaz <philippe.retornaz@epfl.ch>
> ---
>  drivers/mfd/mc13xxx-spi.c |   65
> ++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 64
> insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mfd/mc13xxx-spi.c b/drivers/mfd/mc13xxx-spi.c
> index 5d1969f..03df422 100644
> --- a/drivers/mfd/mc13xxx-spi.c
> +++ b/drivers/mfd/mc13xxx-spi.c
> @@ -54,6 +54,67 @@ static struct regmap_config mc13xxx_regmap_spi_config =
> { .max_register = MC13XXX_NUMREGS,
> 
>  	.cache_type = REGCACHE_NONE,
> +	.use_single_rw = 1,
> +};
> +
> +static int mc13xxx_spi_read(void *context, const void *reg, size_t
> reg_size, +				void *val, size_t val_size)
> +{
> +	unsigned char w[4] = { *((unsigned char *) reg), 0, 0, 0};
> +	unsigned char r[4];
> +	unsigned char *p = val;
> +	struct device *dev = context;
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct spi_transfer t = {
> +		.tx_buf = w,
> +		.rx_buf = r,
> +		.len = 4,
> +	};
> +
> +	struct spi_message m;
> +	int ret;
> +
> +	if (val_size != 3 || reg_size != 1)
> +		return -ENOTSUPP;
> +
> +	spi_message_init(&m);
> +	spi_message_add_tail(&t, &m);
> +	ret = spi_sync(spi, &m);
> +
> +	memcpy(p, &r[1], 3);
> +
> +	return ret;
> +}
> +
> +static int mc13xxx_spi_write(void *context, const void *data, size_t
> count) +{
> +	struct device *dev = context;
> +	struct spi_device *spi = to_spi_device(dev);
> +
> +	if (count != 4)
> +		return -ENOTSUPP;
> +
> +	return spi_write(spi, data, count);
> +}
> +
> +/*
> + * We cannot use regmap-spi generic bus implementation here.
> + * The MC13783 chip will get corrupted if CS signal is deasserted
> + * and on i.Mx31 SoC (the target SoC for MC13783 PMIC) the SPI controller
> + * has the following errata (DSPhl22960):
> + * "The CSPI negates SS when the FIFO becomes empty with
> + * SSCTL= 0. Software cannot guarantee that the FIFO will not
> + * drain because of higher priority interrupts and the
> + * non-realtime characteristics of the operating system. As a
> + * result, the SS will negate before all of the data has been
> + * transferred to/from the peripheral."
> + * We workaround this by accessing the SPI controller with a
> + * single transfert.

s/transfert/transfer

> + */
> +
> +static struct regmap_bus regmap_mc13xxx_bus = {
> +	.write = mc13xxx_spi_write,
> +	.read = mc13xxx_spi_read,
>  };
> 
>  static int mc13xxx_spi_probe(struct spi_device *spi)
> @@ -78,7 +139,9 @@ static int mc13xxx_spi_probe(struct spi_device *spi)
>  	mc13xxx->dev = &spi->dev;
>  	mutex_init(&mc13xxx->lock);
> 
> -	mc13xxx->regmap = regmap_init_spi(spi, &mc13xxx_regmap_spi_config);
> +	mc13xxx->regmap = regmap_init(&spi->dev, &regmap_mc13xxx_bus, &spi->dev,
> +					&mc13xxx_regmap_spi_config);
> +
>  	if (IS_ERR(mc13xxx->regmap)) {
>  		ret = PTR_ERR(mc13xxx->regmap);
>  		dev_err(mc13xxx->dev, "Failed to initialize register map: %d\n",

[PATCH 1/2] MFD: Fix mc13xxx SPI regmap

[Fabio Estevam]

Hi Philippe,

On Tue, May 29, 2012 at 6:06 AM, Philippe R?tornaz
<philippe.retornaz@epfl.ch> wrote:
> This fix the SPI regmap configuration, the wrong write flag was used.
> Also, bits_per_word should not be set as the regmap spi implementation
> uses a 8bits transfert granularity.
>
> Signed-off-by: Philippe R?tornaz <philippe.retornaz@epfl.ch>

Excellent!

I tested your patch series on a mx51evk, mx27pdk and mx31pdk:

Tested-by: Fabio Estevam <fabio.estevam@freescale.com>

Thanks,

Fabio Estevam

[PATCH 0/2] MC13xxx: fix access on i.mx31

[Samuel Ortiz]

Hi Philippe,


On Tue, May 29, 2012 at 11:06:27AM +0200, Philippe R?tornaz wrote:
> Hello
> 
> This patchset fixes the mc13xxx regression introduced by:
> "mfd: Use regmap for the mc13xxx-core register access".
> 
> The first patch correct the write_flag_mask to use the correct value
> and remove the invalid bits_per_word in the probe.
> 
> The second patch workaround an silicon errata on some imx SoC.
Both patches applied to my for-linus branch. They'll be part of the MFD fixes
pull request for 3.5.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH 0/2] MC13xxx: fix access on i.mx31

[Philippe Rétornaz]

Le vendredi 29 juin 2012 15:09:45 Samuel Ortiz a ?crit :
> Hi Philippe,
> 
> On Tue, May 29, 2012 at 11:06:27AM +0200, Philippe R?tornaz wrote:
> > Hello
> > 
> > This patchset fixes the mc13xxx regression introduced by:
> > "mfd: Use regmap for the mc13xxx-core register access".
> > 
> > The first patch correct the write_flag_mask to use the correct value
> > and remove the invalid bits_per_word in the probe.
> > 
> > The second patch workaround an silicon errata on some imx SoC.
> 
> Both patches applied to my for-linus branch. They'll be part of the MFD
> fixes pull request for 3.5.

Thank you very much ! 

Philippe