mc13xxx-core: kernel hangs after 'regmap_read'

mc13xxx-core: kernel hangs after 'regmap_read'

[Fabio Estevam]

Hi Marc,

I am running linux-next 3.4.0-next-20120521 on a mx31pdk board that
has a mc13783 PMIC connected to SPI.

I am getting a kernel hang after 'regmap_read' inside mc13xxx_reg_read function.

Does it work fine for you on your mx35 board? Am I missing any recent patch?

My understanding is that you have only tested i2c connection with this
PMIC. Is this correct?

Any ideas on how to solve this issue is appreciated.

Regards,

Fabio Estevam

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Marc Reilly]

Hi Fabio,

On Tuesday, May 22, 2012 02:06:55 AM Fabio Estevam wrote:
> Hi Marc,
> 
> I am running linux-next 3.4.0-next-20120521 on a mx31pdk board that
> has a mc13783 PMIC connected to SPI.
> 
> I am getting a kernel hang after 'regmap_read' inside mc13xxx_reg_read
> function.
> 
> Does it work fine for you on your mx35 board? Am I missing any recent
> patch?

I tested linux-next/next-20120521, no extra patches. I got build errors for 
something to do with usb udc, and start-up panic for the nand driver. 
Disabling both of these got me to a login. Then:

mount -t debugfs none /sys/kernel/debug
cat /sys/kernel/debug/regmap/0-0008/registers

00: 004cc0
01: ffffff
02: 185cc0
03: 000005
04: ffffff
05: 00401c
06: 000200
07: 0045d0
08: 000000
09: 000005
0a: 00147a
0b: 000000
0c: 000000
0d: 0b0040
0e: 000000
0f: 400032
10: 000000
11: 000000
12: 000001
13: 000000
14: 013326
15: 01ffff
...

Which looks correct.


> 
> My understanding is that you have only tested i2c connection with this
> PMIC. Is this correct?
That's correct. I don't have any SPI connected hardware to test with. Shawn 
tested SPI operation for an earlier version of the patch series, but that was 
before the regmap changes.


> Any ideas on how to solve this issue is appreciated.
My first suspicion is with the regmap register format of the mc13xxx for SPI. 
It was a new format which introduced a padding bit. I wonder if Mark Brown can 
comment on whether any other devices are using it successfully. (Sorry I can't 
offer more insights there).

Does your crash occur during mc13xxx_spi_probe? does mc13xxx_common_init() get 
called?


Cheers,
Marc

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Mark Brown]

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

On Tue, May 22, 2012 at 10:53:21AM +1000, Marc Reilly wrote:

> It was a new format which introduced a padding bit. I wonder if Mark Brown can 
> comment on whether any other devices are using it successfully. (Sorry I can't 
> offer more insights there).

It's only this chip that's doing so.  I'd suggest putting a scope on it
and having a look at the formatted data.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Fabio Estevam]

Hi Philippe,

On Tue, May 22, 2012 at 6:25 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:

> It's only this chip that's doing so.  I'd suggest putting a scope on it
> and having a look at the formatted data.

Does mx31-moboard allow you to put the scope on the SPI lines
connected to the mc13783?

On mx31pdk board these lines are connected directly from the mx31 to
mc13783 without any test points for easy access.

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Philippe Rétornaz]

Le mardi 22 mai 2012 08:40:30 Fabio Estevam a écrit :
> Hi Philippe,
> 
> On Tue, May 22, 2012 at 6:25 AM, Mark Brown
> 
> <broonie@opensource.wolfsonmicro.com> wrote:
> > It's only this chip that's doing so.  I'd suggest putting a scope on it
> > and having a look at the formatted data.
> 
> Does mx31-moboard allow you to put the scope on the SPI lines
> connected to the mc13783?

Yes, I have access to MISO/MOSI/CLK, but not CS on mx31moboard.
BTW, mx31-ads give access to the full SPI bus, but I never booted it.

I cannot debug this right now, but I should be able to do it on friday.

Regards,

Philippe

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Fabio Estevam]

On Tue, May 22, 2012 at 6:25 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, May 22, 2012 at 10:53:21AM +1000, Marc Reilly wrote:
>
>> It was a new format which introduced a padding bit. I wonder if Mark Brown can
>> comment on whether any other devices are using it successfully. (Sorry I can't
>> offer more insights there).
>
> It's only this chip that's doing so.  I'd suggest putting a scope on it
> and having a look at the formatted data.

As I don't have a way to access the SPI pins via scope I did a

git checkout  91b5e7411, which corresponds to the following commit:

commit 91b5e741184ea9836cd7d7509e4f9b6eefa27df2
Author: Marc Reilly <marc@cpdesign.com.au>
Date:   Sun Apr 1 16:41:37 2012 +1000

    mfd: Use regmap for the mc13xxx-core register access

,and now at least it does not silently hang and gives me a regmap failure:

spi_imx imx31-cspi.1: master is unqueued, this is deprecated
mc13xxx spi1.1: Failed to initialize register map: -22
mc13xxx: probe of spi1.1 failed with error -22

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Fabio Estevam]

Hi Marc,

On Mon, May 21, 2012 at 9:53 PM, Marc Reilly <marc@cpdesign.com.au> wrote:

> Does your crash occur during mc13xxx_spi_probe?

Yes

> does mc13xxx_common_init() get  called?

Yes

The sequence that shows the problem is:

mc13xxx_spi_probe
mc13xxx_common_init
mc13xxx_identify
mc13xxx_reg_read
regmap_read (kernel hangs)

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Fabio Estevam]

On Tue, May 22, 2012 at 10:12 PM, Fabio Estevam <festevam@gmail.com> wrote:

> The sequence that shows the problem is:
>
> mc13xxx_spi_probe
> mc13xxx_common_init
> mc13xxx_identify
> mc13xxx_reg_read
> regmap_read (kernel hangs)

If I do the following:

--- a/drivers/mfd/mc13xxx-spi.c
+++ b/drivers/mfd/mc13xxx-spi.c
@@ -72,8 +72,6 @@ static int mc13xxx_spi_probe(struct spi_device *spi)
                return -ENOMEM;

        dev_set_drvdata(&spi->dev, mc13xxx);
-       spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
-       spi->bits_per_word = 32;

        mc13xxx->dev = &spi->dev;
        mutex_init(&mc13xxx->lock);

Then this allows the kernel to boot at least (of course the mc13xxx
probe will fail in this case).

Mark,

Should the spi->mode and spi->bits_per_word be passed differently?

Maybe via "struct regmap_config" ?

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Mark Brown]

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

On Tue, May 22, 2012 at 11:05:35PM -0300, Fabio Estevam wrote:

> Should the spi->mode and spi->bits_per_word be passed differently?

> Maybe via "struct regmap_config" ?

You shouldn't be setting bits_per_word at all, that'll corrupt the data
that regmap has formatted.  Does removing that alone resolve the issue?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Fabio Estevam]

Mark,

On Wed, May 23, 2012 at 5:49 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:

> You shouldn't be setting bits_per_word at all, that'll corrupt the data
> that regmap has formatted.  Does removing that alone resolve the issue?

Removing only the line that sets bits_per_word:

--- a/drivers/mfd/mc13xxx-spi.c
+++ b/drivers/mfd/mc13xxx-spi.c
@@ -73,7 +73,6 @@ static int mc13xxx_spi_probe(struct spi_device *spi)

        dev_set_drvdata(&spi->dev, mc13xxx);
        spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
-       spi->bits_per_word = 32;

        mc13xxx->dev = &spi->dev;
        mutex_init(&mc13xxx->lock);


, does allow the kernel to boot, but the mx13xxx driver is not probed anymore:

spi_imx imx31-cspi.1: master is unqueued, this is deprecated
spi_imx imx31-cspi.1: probed
spi_imx imx31-cspi.0: master is unqueued, this is deprecated
l4f00242t03 spi0.0: l4f00242t03_probe: Unable to get the IO regulator
spi spi0.0: Driver l4f00242t03 requests probe deferral
spi_imx imx31-cspi.0: probed

Also, on the previous raw spi access version we had:

#define MC13XXX_REGOFFSET_SHIFT 25
int mc13xxx_reg_read(struct mc13xxx *mc13xxx, unsigned int offset, u32 *val)
{
	struct spi_transfer t;
	struct spi_message m;
	int ret;

	BUG_ON(!mutex_is_locked(&mc13xxx->lock));

	if (offset > MC13XXX_NUMREGS)
		return -EINVAL;

	*val = offset << MC13XXX_REGOFFSET_SHIFT;
....

,would the spi regmap access take into account this
MC13XXX_REGOFFSET_SHIFT operation?

I haven't been able to see it in the regmap spi access.

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Fabio Estevam]

On Wed, May 23, 2012 at 11:18 AM, Fabio Estevam <festevam@gmail.com> wrote:

> Removing only the line that sets bits_per_word:
>
> --- a/drivers/mfd/mc13xxx-spi.c
> +++ b/drivers/mfd/mc13xxx-spi.c
> @@ -73,7 +73,6 @@ static int mc13xxx_spi_probe(struct spi_device *spi)
>
>        dev_set_drvdata(&spi->dev, mc13xxx);
>        spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
> -       spi->bits_per_word = 32;
>
>        mc13xxx->dev = &spi->dev;
>        mutex_init(&mc13xxx->lock);
>
>
> , does allow the kernel to boot, but the mx13xxx driver is not probed anymore:

The reason for the mc13xxx not probing is because it tries to read the
mc13xxx version register and does not find a valid PMIC ID.

I did a dump of all the mc13xxx registers and all of them return the
same value of 0x810, which means we are not reading SPI correctly via
regmap.

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Fabio Estevam]

On Wed, May 23, 2012 at 1:42 PM, Shawn Guo <shawn.guo@freescale.com> wrote:

> Removing that alone does not resolve the issue, but with additional
> .write_flag_mask setting it does for me.

This is still not working for me on mx31pdk.

Still getting wrong SPI read data from the pmic.

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Shawn Guo]

On Wed, May 23, 2012 at 09:49:46AM +0100, Mark Brown wrote:
> On Tue, May 22, 2012 at 11:05:35PM -0300, Fabio Estevam wrote:
> 
> > Should the spi->mode and spi->bits_per_word be passed differently?
> 
> > Maybe via "struct regmap_config" ?
> 
> You shouldn't be setting bits_per_word at all, that'll corrupt the data
> that regmap has formatted.  Does removing that alone resolve the issue?

Removing that alone does not resolve the issue, but with additional
.write_flag_mask setting it does for me.

Regards,
Shawn

diff --git a/drivers/mfd/mc13xxx-spi.c b/drivers/mfd/mc13xxx-spi.c
index 3fcdab3..5d1969f 100644
--- a/drivers/mfd/mc13xxx-spi.c
+++ b/drivers/mfd/mc13xxx-spi.c
@@ -49,6 +49,7 @@ static struct regmap_config mc13xxx_regmap_spi_config = {
        .reg_bits = 7,
        .pad_bits = 1,
        .val_bits = 24,
+       .write_flag_mask = 0x80,

        .max_register = MC13XXX_NUMREGS,

@@ -73,7 +74,6 @@ static int mc13xxx_spi_probe(struct spi_device *spi)

        dev_set_drvdata(&spi->dev, mc13xxx);
        spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
-       spi->bits_per_word = 32;

        mc13xxx->dev = &spi->dev;
        mutex_init(&mc13xxx->lock);

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Mark Brown]

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

On Wed, May 23, 2012 at 11:18:10AM -0300, Fabio Estevam wrote:

> #define MC13XXX_REGOFFSET_SHIFT 25
> int mc13xxx_reg_read(struct mc13xxx *mc13xxx, unsigned int offset, u32 *val)
> {
> 	struct spi_transfer t;
> 	struct spi_message m;
> 	int ret;
> 
> 	BUG_ON(!mutex_is_locked(&mc13xxx->lock));
> 
> 	if (offset > MC13XXX_NUMREGS)
> 		return -EINVAL;
> 
> 	*val = offset << MC13XXX_REGOFFSET_SHIFT;

> ,would the spi regmap access take into account this
> MC13XXX_REGOFFSET_SHIFT operation?

The data sent should be however many bits of register address are
specified, followed by however many padding bits are specified, followed
by the data.  The shift there will be some combination of the padding
and register address.

Though if the device is looking for everything byte swapped for some
insane reason...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Fabio Estevam]

On Wed, May 23, 2012 at 2:36 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:

> The data sent should be however many bits of register address are
> specified, followed by however many padding bits are specified, followed
> by the data.  The shift there will be some combination of the padding
> and register address.

Ok, from the mc13783 datasheet:

"Both SPI ports are configured to utilize 32-bit serial data words,
using 1 read/write bit, 6 address bits, 1
null bit, and 24 data bits. The SPI ports’ 64 registers correspond to
the 6 address bits."

,and we have the struct regmap_config as:

	.reg_bits = 7,
	.pad_bits = 1,
	.val_bits = 24,

>From the bootloader I read PMIC register 0 as 0x00081000.

In the kernel all the PMIC registers accesses are read as: 0x00000810

So it looks when I read a register via SPI through regmap the register
address field is not being updated correctly (like as it was always
reading from register 0) and there is also a shift by 8 in there.

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Shawn Guo]

On Wed, May 23, 2012 at 01:34:32PM -0300, Fabio Estevam wrote:
> On Wed, May 23, 2012 at 1:42 PM, Shawn Guo <shawn.guo@freescale.com> wrote:
> 
> > Removing that alone does not resolve the issue, but with additional
> > .write_flag_mask setting it does for me.
> 
> This is still not working for me on mx31pdk.
> 
> Still getting wrong SPI read data from the pmic.
> 
Interesting.

I'm booting my imx51 babbage with MC13892 with no problem now.  And
reading the registers give me the same values as I do with u-boot.

Regards,
Shawn

root@linaro-developer:~# cat /sys/kernel/debug/regmap/spi32766.0/registers
00: 005c80
01: ffffff
02: 195448
03: 000081
04: ffffff
05: 00401c
06: 000218
07: 0045d0
08: 000000
09: 000000
0a: 000001
0b: 000000
0c: 000000
0d: 000040
0e: 000000
0f: 400000
10: 000000
11: 000000
12: 000000
13: 000000
14: 0003af
15: 01ffff
16: 000000
17: 007fff
18: 454a54
19: 45673a
1a: 00631a
1b: 80739c
1c: 212048
1d: 000808
1e: 010fe0
1f: 0001f4
20: 049208
21: 049249
22: 200000
23: 000000
24: 000000
25: 000000
26: 000000
27: 000000
28: 000000
29: 000000
2a: 000000
2b: 008000
2c: 000000
2d: 574574
2e: 0001c0
2f: 055055
30: 60007b
31: 000040
32: 000008
33: 000000
34: 000000
35: 000000
36: 000000
37: 000000
38: 000000
39: 000000
3a: 000000
3b: 000000
3c: 000000
3d: 000000
3e: 000000
3f: 000000

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Fabio Estevam]

Shawn,

On Wed, May 23, 2012 at 9:48 PM, Shawn Guo <shawn.guo@freescale.com> wrote:

> I'm booting my imx51 babbage with MC13892 with no problem now.  And
> reading the registers give me the same values as I do with u-boot.

Ok, just to confirm: is the patch below that you are using?

diff --git a/drivers/mfd/mc13xxx-spi.c b/drivers/mfd/mc13xxx-spi.c
index 3fcdab3..5d1969f 100644
--- a/drivers/mfd/mc13xxx-spi.c
+++ b/drivers/mfd/mc13xxx-spi.c
@@ -49,6 +49,7 @@ static struct regmap_config mc13xxx_regmap_spi_config = {
 	.reg_bits = 7,
 	.pad_bits = 1,
 	.val_bits = 24,
+	.write_flag_mask = 0x80,

 	.max_register = MC13XXX_NUMREGS,

@@ -73,7 +74,6 @@ static int mc13xxx_spi_probe(struct spi_device *spi)

 	dev_set_drvdata(&spi->dev, mc13xxx);
 	spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
-	spi->bits_per_word = 32;

 	mc13xxx->dev = &spi->dev;
 	mutex_init(&mc13xxx->lock);


I am trying to understand why mx31pdk still fails.

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Shawn Guo]

On Thu, May 24, 2012 at 01:07:43AM -0300, Fabio Estevam wrote:
> Shawn,
> 
> On Wed, May 23, 2012 at 9:48 PM, Shawn Guo <shawn.guo@freescale.com> wrote:
> 
> > I'm booting my imx51 babbage with MC13892 with no problem now.  And
> > reading the registers give me the same values as I do with u-boot.
> 
> Ok, just to confirm: is the patch below that you are using?
> 
Yes.

Regards,
Shawn

> diff --git a/drivers/mfd/mc13xxx-spi.c b/drivers/mfd/mc13xxx-spi.c
> index 3fcdab3..5d1969f 100644
> --- a/drivers/mfd/mc13xxx-spi.c
> +++ b/drivers/mfd/mc13xxx-spi.c
> @@ -49,6 +49,7 @@ static struct regmap_config mc13xxx_regmap_spi_config = {
>  	.reg_bits = 7,
>  	.pad_bits = 1,
>  	.val_bits = 24,
> +	.write_flag_mask = 0x80,
> 
>  	.max_register = MC13XXX_NUMREGS,
> 
> @@ -73,7 +74,6 @@ static int mc13xxx_spi_probe(struct spi_device *spi)
> 
>  	dev_set_drvdata(&spi->dev, mc13xxx);
>  	spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
> -	spi->bits_per_word = 32;
> 
>  	mc13xxx->dev = &spi->dev;
>  	mutex_init(&mc13xxx->lock);
> 
> 
> I am trying to understand why mx31pdk still fails.
> 

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Shawn Guo]

On Thu, May 24, 2012 at 01:07:43AM -0300, Fabio Estevam wrote:
> I am trying to understand why mx31pdk still fails.
> 
So different from imx51-babbage which uses MC13892, mx31pdk uses
MC13783?  But both chips should have the same regmap, right?

-- 
Regards,
Shawn

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Uwe Kleine-König]

Hello,

On Thu, May 24, 2012 at 02:39:00PM +0800, Shawn Guo wrote:
> On Thu, May 24, 2012 at 01:07:43AM -0300, Fabio Estevam wrote:
> > I am trying to understand why mx31pdk still fails.
> > 
> So different from imx51-babbage which uses MC13892, mx31pdk uses
> MC13783?  But both chips should have the same regmap, right?
They are similar. One difference is the protocol used. MC13783 only
speaks spi, MC13892 can do both, spi and i2c. Does someone has a working
MC13892 that uses spi?

For debugging I'd instrument the spi driver to log and compare what is
read and written to the data registers before and after the introduction
of the regmap stuff.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Shawn Guo]

On Thu, May 24, 2012 at 08:46:35AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, May 24, 2012 at 02:39:00PM +0800, Shawn Guo wrote:
> > On Thu, May 24, 2012 at 01:07:43AM -0300, Fabio Estevam wrote:
> > > I am trying to understand why mx31pdk still fails.
> > > 
> > So different from imx51-babbage which uses MC13892, mx31pdk uses
> > MC13783?  But both chips should have the same regmap, right?
> They are similar. One difference is the protocol used. MC13783 only
> speaks spi, MC13892 can do both, spi and i2c. Does someone has a working
> MC13892 that uses spi?
> 
I do.  With the following patch applied on top of linux-next, it works
on my imx51-babbage board.

Regards,
Shawn

diff --git a/drivers/mfd/mc13xxx-spi.c b/drivers/mfd/mc13xxx-spi.c
index 3fcdab3..5d1969f 100644
--- a/drivers/mfd/mc13xxx-spi.c
+++ b/drivers/mfd/mc13xxx-spi.c
@@ -49,6 +49,7 @@ static struct regmap_config mc13xxx_regmap_spi_config = {
        .reg_bits = 7,
        .pad_bits = 1,
        .val_bits = 24,
+       .write_flag_mask = 0x80,

        .max_register = MC13XXX_NUMREGS,

@@ -73,7 +74,6 @@ static int mc13xxx_spi_probe(struct spi_device *spi)

        dev_set_drvdata(&spi->dev, mc13xxx);
        spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
-       spi->bits_per_word = 32;

        mc13xxx->dev = &spi->dev;
        mutex_init(&mc13xxx->lock);

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Marc Reilly]

Hi,

On Thursday, May 24, 2012 05:33:16 PM Shawn Guo wrote:
> On Thu, May 24, 2012 at 08:46:35AM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Thu, May 24, 2012 at 02:39:00PM +0800, Shawn Guo wrote:
> > > On Thu, May 24, 2012 at 01:07:43AM -0300, Fabio Estevam wrote:
> > > > I am trying to understand why mx31pdk still fails.
> > > 
> > > So different from imx51-babbage which uses MC13892, mx31pdk uses
> > > MC13783?  But both chips should have the same regmap, right?
> > 
> > They are similar. One difference is the protocol used. MC13783 only
> > speaks spi, MC13892 can do both, spi and i2c. Does someone has a working
> > MC13892 that uses spi?
> 
> I do.  With the following patch applied on top of linux-next, it works
> on my imx51-babbage board.
> 
> Regards,
> Shawn
> 
> diff --git a/drivers/mfd/mc13xxx-spi.c b/drivers/mfd/mc13xxx-spi.c
> index 3fcdab3..5d1969f 100644
> --- a/drivers/mfd/mc13xxx-spi.c
> +++ b/drivers/mfd/mc13xxx-spi.c
> @@ -49,6 +49,7 @@ static struct regmap_config mc13xxx_regmap_spi_config = {
>         .reg_bits = 7,
>         .pad_bits = 1,
>         .val_bits = 24,
> +       .write_flag_mask = 0x80,

Should probably have .read_flag_mask = 0x00, here too. If either are non zero, 
both are set.

I guess this is the problem, regmap's default read_flag_mask for the spi bus 
is 0x80, and the write mask defaults to 0. The mc13xxx works the opposite way 
though! aarg. My bad for not noticing. So setting the write_flag_mask to 0x80 
also sets the read mask to 0 correctly also.

 
>         .max_register = MC13XXX_NUMREGS,
> 
> @@ -73,7 +74,6 @@ static int mc13xxx_spi_probe(struct spi_device *spi)
> 
>         dev_set_drvdata(&spi->dev, mc13xxx);
>         spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
> -       spi->bits_per_word = 32;

My bad here too. Because I had no hardware to test with I tried to preserve as 
much of the old code as I could.

Sorry about the stuff ups. I owe each of you a beer (unfortunately I'll have 
to exclude the list in general from that offer :) ).

I'm wondering about regmap_init where the buf_size is set up. I think it will 
only end up being 3 bytes. I think line 249 should be something like:

	map->format.buf_size = (config->reg_bits 
                          + config->val_bits 
					         + (config->pad_bits % 8)) / 8;

or perhaps better:
	map->reg_shift = config->pad_bits % 8;
	map->format.buf_size = (config->reg_bits 
                          + config->val_bits 
					         + map->reg_shift) / 8;

Am I right here or just confused?

Cheers,
Marc

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Mark Brown]

On Thu, May 24, 2012 at 07:08:37PM +1000, Marc Reilly wrote:

> I'm wondering about regmap_init where the buf_size is set up. I think it will 
> only end up being 3 bytes. I think line 249 should be something like:

> 	map->format.buf_size = (config->reg_bits 
>                           + config->val_bits 
> 					         + (config->pad_bits % 8)) / 8;

> or perhaps better:
> 	map->reg_shift = config->pad_bits % 8;
> 	map->format.buf_size = (config->reg_bits 
>                           + config->val_bits 
> 					         + map->reg_shift) / 8;

Yes, that's been missed in the addition of padding.  We should also be
using DIV_ROUND_UP() which we aren't at the minute.  We could also do
reg_bytes + pad_bytes + val_bytes which should cover everything I think?

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Marc Reilly]

Hi,

On Thursday, May 24, 2012 08:37:49 PM Mark Brown wrote:
> On Thu, May 24, 2012 at 07:08:37PM +1000, Marc Reilly wrote:
> > I'm wondering about regmap_init where the buf_size is set up. I think it
> > will
> > 
> > only end up being 3 bytes. I think line 249 should be something like:
> > 	map->format.buf_size = (config->reg_bits
> > 	
> >                           + config->val_bits
> > 					         
> > 					         + (config->pad_bits % 8)) / 8;
> > 
> > or perhaps better:
> > 	map->reg_shift = config->pad_bits % 8;
> > 	map->format.buf_size = (config->reg_bits
> > 	
> >                           + config->val_bits
> > 					         
> > 					         + map->reg_shift) / 8;
> 
> Yes, that's been missed in the addition of padding.  We should also be
> using DIV_ROUND_UP() which we aren't at the minute.  

That would break, in _regmap_read_raw():

	ret = map->bus->read(map->bus_context, map->work_buf,
			     map->format.reg_bytes + map->format.pad_bytes,
			     val, val_len);

If pad_bytes was 1 here, then the register size would end up being 2 bytes.

The way I understood the pad_bytes field was it was the number of _complete_ 
padding bytes (ie, full 8 bits) between the register address and value. Any 
remainder of padding bits is incorporated into the register and shifted by 
shift bits.

> We could also do
> reg_bytes + pad_bytes + val_bytes which should cover everything I think?

If we want to cover everything, we could do reg_bytes + pad_bytes + val_bytes 
+ 3 ... :) (I'm not seriously suggesting that).

Cheers,
Marc

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Mark Brown]

On Thu, May 24, 2012 at 09:22:29PM +1000, Marc Reilly wrote:

> > > 	map->reg_shift = config->pad_bits % 8;
> > > 	map->format.buf_size = (config->reg_bits
> > > 	
> > >                           + config->val_bits
> > > 					         
> > > 					         + map->reg_shift) / 8;

> > Yes, that's been missed in the addition of padding.  We should also be
> > using DIV_ROUND_UP() which we aren't at the minute.  

> That would break, in _regmap_read_raw():

> 	ret = map->bus->read(map->bus_context, map->work_buf,
> 			     map->format.reg_bytes + map->format.pad_bytes,
> 			     val, val_len);

> If pad_bytes was 1 here, then the register size would end up being 2 bytes.

The above is about buf_size...  pad_bytes isn't in the quoted text which
is the issue.

> The way I understood the pad_bytes field was it was the number of _complete_ 
> padding bytes (ie, full 8 bits) between the register address and value. Any 
> remainder of padding bits is incorporated into the register and shifted by 
> shift bits.

Yes.

> > We could also do
> > reg_bytes + pad_bytes + val_bytes which should cover everything I think?

> If we want to cover everything, we could do reg_bytes + pad_bytes + val_bytes 
> + 3 ... :) (I'm not seriously suggesting that).

Yes, but the above should be the total that goes onto the wire.

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Fabio Estevam]

On Thu, May 24, 2012 at 6:08 AM, Marc Reilly <marc@cpdesign.com.au> wrote:

> I'm wondering about regmap_init where the buf_size is set up. I think it will
> only end up being 3 bytes. I think line 249 should be something like:
>
>        map->format.buf_size = (config->reg_bits
>                          + config->val_bits
>                                                 + (config->pad_bits % 8)) / 8;

Yes, you are right. buf_size should be changed as you suggests so that
it can be 4 bytes instead of 3.

This is the patch I am using now:

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 0bcda48..6beef98 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -246,11 +246,12 @@ struct regmap *regmap_init(struct device *dev,
 		map->lock = regmap_lock_mutex;
 		map->unlock = regmap_unlock_mutex;
 	}
-	map->format.buf_size = (config->reg_bits + config->val_bits) / 8;
 	map->format.reg_bytes = DIV_ROUND_UP(config->reg_bits, 8);
 	map->format.pad_bytes = config->pad_bits / 8;
 	map->format.val_bytes = DIV_ROUND_UP(config->val_bits, 8);
-	map->format.buf_size += map->format.pad_bytes;
+	map->format.buf_size = (config->reg_bits + config->val_bits +
+						(config->pad_bits % 8)) / 8;
+
 	map->reg_shift = config->pad_bits % 8;
 	if (config->reg_stride)
 		map->reg_stride = config->reg_stride;
diff --git a/drivers/mfd/mc13xxx-spi.c b/drivers/mfd/mc13xxx-spi.c
index 3fcdab3..4c14dfc 100644
--- a/drivers/mfd/mc13xxx-spi.c
+++ b/drivers/mfd/mc13xxx-spi.c
@@ -49,6 +49,8 @@ static struct regmap_config mc13xxx_regmap_spi_config = {
 	.reg_bits = 7,
 	.pad_bits = 1,
 	.val_bits = 24,
+	.write_flag_mask = 0x80,
+	.read_flag_mask = 0x00,

 	.max_register = MC13XXX_NUMREGS,

@@ -73,7 +75,6 @@ static int mc13xxx_spi_probe(struct spi_device *spi)

 	dev_set_drvdata(&spi->dev, mc13xxx);
 	spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
-	spi->bits_per_word = 32;

 	mc13xxx->dev = &spi->dev;
 	mutex_init(&mc13xxx->lock);

, which is still not allowing me to read the SPI registers correctly.

Have I missed anything?

Still reading 0x810 for all registers (0x810000 is the value of
register 0 , btw).

Thanks,

Fabio Estevam

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Marc Reilly]

Hi,


> > > Yes, that's been missed in the addition of padding.  We should also be
> > > using DIV_ROUND_UP() which we aren't at the minute.
> > 
> > That would break, in _regmap_read_raw():
> > 	ret = map->bus->read(map->bus_context, map->work_buf,
> > 	
> > 			     map->format.reg_bytes + map->format.pad_bytes,
> > 			     val, val_len);
> > 
> > If pad_bytes was 1 here, then the register size would end up being 2
> > bytes.
> 
> The above is about buf_size...  pad_bytes isn't in the quoted text which
> is the issue.

I misunderstood where you intended to put the DIV_ROUND_UP, sorry.

Something like this:

--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -246,7 +246,9 @@ struct regmap *regmap_init(struct device *dev,
 		map->lock = regmap_lock_mutex;
 		map->unlock = regmap_unlock_mutex;
 	}
-	map->format.buf_size = (config->reg_bits + config->val_bits) / 8;
+	map->format.buf_size = DIV_ROUND_UP(config->reg_bits +
+					    config->val_bits +
+					    config->pad_bits % 8, 8);
 	map->format.reg_bytes = DIV_ROUND_UP(config->reg_bits, 8);
 	map->format.pad_bytes = config->pad_bits / 8;
 	map->format.val_bytes = DIV_ROUND_UP(config->val_bits, 8);
-- 

Hope that fixes things.

Cheers,
Marc

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Marc Reilly]

Hi,

> Yes, you are right. buf_size should be changed as you suggests so that
> it can be 4 bytes instead of 3.
> 
> This is the patch I am using now:

I just sent something similar before I saw this.

> 
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index 0bcda48..6beef98 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -246,11 +246,12 @@ struct regmap *regmap_init(struct device *dev,
>  		map->lock = regmap_lock_mutex;
>  		map->unlock = regmap_unlock_mutex;
>  	}
> -	map->format.buf_size = (config->reg_bits + config->val_bits) / 8;
>  	map->format.reg_bytes = DIV_ROUND_UP(config->reg_bits, 8);
>  	map->format.pad_bytes = config->pad_bits / 8;
>  	map->format.val_bytes = DIV_ROUND_UP(config->val_bits, 8);
> -	map->format.buf_size += map->format.pad_bytes;
> +	map->format.buf_size = (config->reg_bits + config->val_bits +
> +						(config->pad_bits % 8)) / 8;
> +

This won't work when pad bits is > 8


>  	map->reg_shift = config->pad_bits % 8;
>  	if (config->reg_stride)
>  		map->reg_stride = config->reg_stride;
> diff --git a/drivers/mfd/mc13xxx-spi.c b/drivers/mfd/mc13xxx-spi.c
> index 3fcdab3..4c14dfc 100644
> --- a/drivers/mfd/mc13xxx-spi.c
> +++ b/drivers/mfd/mc13xxx-spi.c
> @@ -49,6 +49,8 @@ static struct regmap_config mc13xxx_regmap_spi_config = {
>  	.reg_bits = 7,
>  	.pad_bits = 1,
>  	.val_bits = 24,
> +	.write_flag_mask = 0x80,
> +	.read_flag_mask = 0x00,
> 
>  	.max_register = MC13XXX_NUMREGS,
> 
> @@ -73,7 +75,6 @@ static int mc13xxx_spi_probe(struct spi_device *spi)
> 
>  	dev_set_drvdata(&spi->dev, mc13xxx);
>  	spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
> -	spi->bits_per_word = 32;
> 
>  	mc13xxx->dev = &spi->dev;
>  	mutex_init(&mc13xxx->lock);
> 
> , which is still not allowing me to read the SPI registers correctly.
> 
> Have I missed anything?


If I had something to test with, I'd be looking at the parameters of 
"regmap_spi_read" in drivers/base/regmap/regmap-spi.c . Verify reg_size is 1 
(8bits) and val_size is 3. Check also what the reg address is. The first read 
when the mc13xxx is probed is register 46, so would expect reg to be 0x5C.

> 
> Still reading 0x810 for all registers (0x810000 is the value of
> register 0 , btw).


This could mean that all the registers are being sent as 0 and the value is 
shifted by 12 bits. (which is a bit weird). It's also a strange that Shawn's 
board seems to work.

Do you have any other devices on that SPI that you can verify are working 
correctly? (you said it worked from the bootloader, I'm running out of 
ideas... :| )


Cheers,
Marc

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Philippe Rétornaz]

> > Still reading 0x810 for all registers (0x810000 is the value of
> > register 0 , btw).
> 
> This could mean that all the registers are being sent as 0 and the value is
> shifted by 12 bits. (which is a bit weird). It's also a strange that Shawn's
> board seems to work.
> 
> Do you have any other devices on that SPI that you can verify are working
> correctly? (you said it worked from the bootloader, I'm running out of
> ideas... :| )


Well, I think I found out why it's not working on mc13783.
With regmap, each transfert is done with 8bits words. The SPI hardware assert 
the SS signal only during 8 bits "register" transfert then deassert the SS. 
Then the SS is asserted and 24bits (3 bytes) are transfered (datas).
This clearly violate the datasheet which say SS must be asserted for the 
*whole* transfert: register + data.

This is why the old code used a 32bits word transfert, it ensured that the SPI 
hardware was keeping SS asserted without interruptions.

Is there any way to tell regmap to use 32bits transfert with the following 
configuration (or doing it in a single shot 4x8bits):
To read register 0x42 we need to "write" to SPI:

0x42 << 24

and the mc13783 will answer immediatly in the low-24 bits of the _same_ spi 
exchange.

to write register 0x42 we need to "write" to SPI:

(0x80 | 0x42) << 24  | data

I have an oscilloscope screenshot of a transfert if needed.

Regards,

Philippe

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Mark Brown]

On Thu, May 24, 2012 at 06:16:50PM +0200, Philippe Rétornaz wrote:

> Well, I think I found out why it's not working on mc13783.
> With regmap, each transfert is done with 8bits words. The SPI hardware assert 
> the SS signal only during 8 bits "register" transfert then deassert the SS. 
> Then the SS is asserted and 24bits (3 bytes) are transfered (datas).
> This clearly violate the datasheet which say SS must be asserted for the 
> *whole* transfert: register + data.

> This is why the old code used a 32bits word transfert, it ensured that the SPI 
> hardware was keeping SS asserted without interruptions.

> Is there any way to tell regmap to use 32bits transfert with the following 
> configuration (or doing it in a single shot 4x8bits):

I think this is just a plain bug in the SPI controller driver.  I think
I have seen it before in some FSL BSPs (I do remember having to fall
back to the bitbanging driver), it's surprising that it's also present
in the mainline driver so perhaps it's something different.  The driver
is deasserting chip select between transfers but it should only do so at
the end of the message unless told otherwise by the caller setting
cs_change.  regmap-spi uses spi_write_then_read() which does a single
spi_message for the write and read parts of the transfer.

If this is actually the issue the usual fix for this if the SPI
controller hardware can't be persuaded to do the right thing is to put
the chip select pin in GPIO mode and manage it by hand, though looking
at the driver it appears it should be doingn that already.  If you
change to using the bitbanging SPI driver it should do the right thing
(but will obviously be hideously slow), that ought to be at least a good
reference for expected behaviour here.

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Mark Brown]

On Thu, May 24, 2012 at 11:06:08PM +1000, Marc Reilly wrote:

> -	map->format.buf_size = (config->reg_bits + config->val_bits) / 8;
> +	map->format.buf_size = DIV_ROUND_UP(config->reg_bits +
> +					    config->val_bits +
> +					    config->pad_bits % 8, 8);

Yup, though probably without the % 8 (I'd need to reread the context to
confirm) as if pad_bits isn't a multiple of 8 you ought to find that
reg_bits isn't either.

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Uwe Kleine-König]

On Thu, May 24, 2012 at 05:36:05PM +0100, Mark Brown wrote:
> On Thu, May 24, 2012 at 06:16:50PM +0200, Philippe Rétornaz wrote:
> 
> > Well, I think I found out why it's not working on mc13783.
> > With regmap, each transfert is done with 8bits words. The SPI hardware assert 
> > the SS signal only during 8 bits "register" transfert then deassert the SS. 
> > Then the SS is asserted and 24bits (3 bytes) are transfered (datas).
> > This clearly violate the datasheet which say SS must be asserted for the 
> > *whole* transfert: register + data.
> 
> > This is why the old code used a 32bits word transfert, it ensured that the SPI 
> > hardware was keeping SS asserted without interruptions.
> 
> > Is there any way to tell regmap to use 32bits transfert with the following 
> > configuration (or doing it in a single shot 4x8bits):
> 
> I think this is just a plain bug in the SPI controller driver.  I think
> I have seen it before in some FSL BSPs (I do remember having to fall
> back to the bitbanging driver), it's surprising that it's also present
> in the mainline driver so perhaps it's something different.  The driver
> is deasserting chip select between transfers but it should only do so at
> the end of the message unless told otherwise by the caller setting
> cs_change.  regmap-spi uses spi_write_then_read() which does a single
> spi_message for the write and read parts of the transfer.
> 
> If this is actually the issue the usual fix for this if the SPI
> controller hardware can't be persuaded to do the right thing is to put
> the chip select pin in GPIO mode and manage it by hand, though looking
> at the driver it appears it should be doingn that already.  If you
> change to using the bitbanging SPI driver it should do the right thing
> (but will obviously be hideously slow), that ought to be at least a good
> reference for expected behaviour here.
The imx spi driver can do both (GPIO and hardware CS) because not all
pins that can do hardware CS are available as GPIO.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Fabio Estevam]

On Thu, May 24, 2012 at 1:41 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:

>> If this is actually the issue the usual fix for this if the SPI
>> controller hardware can't be persuaded to do the right thing is to put
>> the chip select pin in GPIO mode and manage it by hand, though looking
>> at the driver it appears it should be doingn that already.  If you
>> change to using the bitbanging SPI driver it should do the right thing
>> (but will obviously be hideously slow), that ought to be at least a good
>> reference for expected behaviour here.
> The imx spi driver can do both (GPIO and hardware CS) because not all
> pins that can do hardware CS are available as GPIO.

Right, unfortunately on mx31 the SPI CS pins cannot be used as GPIOs.

On mx51evk the SPI CS are used as GPIOs and that probably explains why
it worked on mx51evk and fails on mx31pdk.

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Mark Brown]

On Thu, May 24, 2012 at 02:39:02PM -0300, Fabio Estevam wrote:
> On Thu, May 24, 2012 at 1:41 PM, Uwe Kleine-König

> > The imx spi driver can do both (GPIO and hardware CS) because not all
> > pins that can do hardware CS are available as GPIO.

> Right, unfortunately on mx31 the SPI CS pins cannot be used as GPIOs.

> On mx51evk the SPI CS are used as GPIOs and that probably explains why
> it worked on mx51evk and fails on mx31pdk.

Oh dear, this affects regmap but it'll probably also affect other things
- how plausible is it that we'll be able to fix in the driver (assuming
it's not just hardware misprogramming)?  We can probably manage to come
up with something for regmap but it's the wrong level to fix things.

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[philippe.retornaz]

Mark Brown <broonie@opensource.wolfsonmicro.com> a écrit :

> On Thu, May 24, 2012 at 02:39:02PM -0300, Fabio Estevam wrote:
>> On Thu, May 24, 2012 at 1:41 PM, Uwe Kleine-König
>
>> > The imx spi driver can do both (GPIO and hardware CS) because not all
>> > pins that can do hardware CS are available as GPIO.
>
>> Right, unfortunately on mx31 the SPI CS pins cannot be used as GPIOs.
>
>> On mx51evk the SPI CS are used as GPIOs and that probably explains why
>> it worked on mx51evk and fails on mx31pdk.
>
> Oh dear, this affects regmap but it'll probably also affect other things
> - how plausible is it that we'll be able to fix in the driver (assuming
> it's not just hardware misprogramming)?  We can probably manage to come
> up with something for regmap but it's the wrong level to fix things.
>

Sadly, after looking at the imx31 datasheet it seems it's a hardware  
limitation. We could maybe workaround it by using DMA to access the  
CSPI but even with dma, this would need a single transfer in order to  
keep the CS signal asserted.

Thus, we need to workaround this in the regmap-spi or mc13783-spi driver.
Either we find a way to have regmap-spi to use 32bits transfert or we  
implement a custom bus inside mc13783-spi.

Thanks,

Philippe

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Mark Brown]

On Thu, May 24, 2012 at 09:42:29PM +0200, philippe.retornaz@epfl.ch wrote:

> Sadly, after looking at the imx31 datasheet it seems it's a hardware
> limitation. We could maybe workaround it by using DMA to access the
> CSPI but even with dma, this would need a single transfer in order
> to keep the CS signal asserted.

Oh dear, though the DMA approach sounds like it might be doable...

> Thus, we need to workaround this in the regmap-spi or mc13783-spi driver.
> Either we find a way to have regmap-spi to use 32bits transfert or
> we implement a custom bus inside mc13783-spi.

Like I said in my previous message the ideal thing would be that the SPI
driver would handle this, that'll ensure that the fix gets propagated to
the maximum possible set of users and is nicer from an abstraction point
of view.

A regmap internal workaround can possibly use Stephen Warren's stuff for
allowing buses to specify endianness stuff that was posted earlier today
though there's a few more tricks needed since this combines reads and
writes.  We may also need to extend the SPI bus to make the capabilities
of the controller discoverable, right now I'm not sure that regmap can
discover when it could activate any more complex stuff.  If we can make
it cheap enough to decide it'd probably be a small performance win even
where the controllers don't have issues.

Re: mc13xxx-core: kernel hangs after 'regmap_read'

[Shawn Guo]

On Thu, May 24, 2012 at 07:08:37PM +1000, Marc Reilly wrote:
> > diff --git a/drivers/mfd/mc13xxx-spi.c b/drivers/mfd/mc13xxx-spi.c
> > index 3fcdab3..5d1969f 100644
> > --- a/drivers/mfd/mc13xxx-spi.c
> > +++ b/drivers/mfd/mc13xxx-spi.c
> > @@ -49,6 +49,7 @@ static struct regmap_config mc13xxx_regmap_spi_config = {
> >         .reg_bits = 7,
> >         .pad_bits = 1,
> >         .val_bits = 24,
> > +       .write_flag_mask = 0x80,
> 
> Should probably have .read_flag_mask = 0x00, here too. If either are non zero, 
> both are set.
> 
Since mc13xxx_regmap_spi_config is a static variable, .read_flag_mask
is initialized as 0 already.

> I guess this is the problem, regmap's default read_flag_mask for the spi bus 
> is 0x80, and the write mask defaults to 0. The mc13xxx works the opposite way 
> though! aarg. My bad for not noticing. So setting the write_flag_mask to 0x80 
> also sets the read mask to 0 correctly also.

-- 
Regards,
Shawn