Using regmap_update_bits to update a write only register

Using regmap_update_bits to update a write only register

[Daniel Baluta]

Hi Mark,

Is it possible to use regmap_update_bits to update the values
of some bits in a write only register?

I was hoping that by filling the .reg_defaults field of regmap_config
the regmap_update_bits function will not try to read the register
from hardware.

Instead I think first call of regmap_update_bits will try to read the register
from hardware, but this fails with -EIO because the register is marked as
write only.

Am I doing something wrong?

thanks,
Daniel.

Re: Using regmap_update_bits to update a write only register

[Mark Brown]

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

On Thu, Mar 05, 2015 at 07:35:32PM +0200, Daniel Baluta wrote:

> Is it possible to use regmap_update_bits to update the values
> of some bits in a write only register?

That should work.

> I was hoping that by filling the .reg_defaults field of regmap_config
> the regmap_update_bits function will not try to read the register
> from hardware.

> Instead I think first call of regmap_update_bits will try to read the register
> from hardware, but this fails with -EIO because the register is marked as
> write only.

> Am I doing something wrong?

Probably, or there's a bug.  What should happen is that if the register
default appeared successfully then the read will get statisfied from the
cache in the manner you describe - presumably that's gone wrong somehow.
Have you set num_reg_defaults?  That's the obvious thing...

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

Re: Using regmap_update_bits to update a write only register

[Mark Brown]

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

On Thu, Mar 05, 2015 at 08:14:14PM +0200, Daniel Baluta wrote:
> On Mar 5, 2015 7:54 PM, "Mark Brown" <broonie@kernel.org> wrote:

> > Probably, or there's a bug.  What should happen is that if the register
> > default appeared successfully then the read will get statisfied from the
> > cache in the manner you describe - presumably that's gone wrong somehow.
> > Have you set num_reg_defaults?  That's the obvious thing...

> Did that. I will have a closer look. Thanks for the answer.

OK, the other thing that springs to mind to check is that the register
didn't somehow get marked as volatile.

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

Re: Using regmap_update_bits to update a write only register

[Lars-Peter Clausen]

On 03/06/2015 12:21 PM, Mark Brown wrote:
> On Thu, Mar 05, 2015 at 08:14:14PM +0200, Daniel Baluta wrote:
>> On Mar 5, 2015 7:54 PM, "Mark Brown" <broonie@kernel.org> wrote:
>
>>> Probably, or there's a bug.  What should happen is that if the register
>>> default appeared successfully then the read will get statisfied from the
>>> cache in the manner you describe - presumably that's gone wrong somehow.
>>> Have you set num_reg_defaults?  That's the obvious thing...
>
>> Did that. I will have a closer look. Thanks for the answer.
>
> OK, the other thing that springs to mind to check is that the register
> didn't somehow get marked as volatile.
>

There were some bugs in the past were non-readable register automatically 
got marked as volatile, this has been fixed though a few months ago. Try to 
make sure you use the latest upstream version of regmap.

- Lars

Re: Using regmap_update_bits to update a write only register

[Daniel Baluta]

On Fri, Mar 6, 2015 at 2:45 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 03/06/2015 12:21 PM, Mark Brown wrote:
>>
>> On Thu, Mar 05, 2015 at 08:14:14PM +0200, Daniel Baluta wrote:
>>>
>>> On Mar 5, 2015 7:54 PM, "Mark Brown" <broonie@kernel.org> wrote:
>>
>>
>>>> Probably, or there's a bug.  What should happen is that if the register
>>>> default appeared successfully then the read will get statisfied from the
>>>> cache in the manner you describe - presumably that's gone wrong somehow.
>>>> Have you set num_reg_defaults?  That's the obvious thing...
>>
>>
>>> Did that. I will have a closer look. Thanks for the answer.
>>
>>
>> OK, the other thing that springs to mind to check is that the register
>> didn't somehow get marked as volatile.

Checked that! is_volatile_reg returns false for that register.

>>
>
> There were some bugs in the past were non-readable register automatically
> got marked as volatile, this has been fixed though a few months ago. Try to
> make sure you use the latest upstream version of regmap.

Thanks for pointing this out. I am using 3.19. Changing from
REGCACHE_RBTREE to REGCACHE_FLAT fixed the problem for me.

I will update my sources and try again with REGCACHE_RBTREE when
I'll have some time.

thanks,
Daniel.

Re: Using regmap_update_bits to update a write only register

[Daniel Baluta]

On Fri, Mar 6, 2015 at 3:27 PM, Daniel Baluta <daniel.baluta@gmail.com> wrote:
> On Fri, Mar 6, 2015 at 2:45 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 03/06/2015 12:21 PM, Mark Brown wrote:
>>>
>>> On Thu, Mar 05, 2015 at 08:14:14PM +0200, Daniel Baluta wrote:
>>>>
>>>> On Mar 5, 2015 7:54 PM, "Mark Brown" <broonie@kernel.org> wrote:
>>>
>>>
>>>>> Probably, or there's a bug.  What should happen is that if the register
>>>>> default appeared successfully then the read will get statisfied from the
>>>>> cache in the manner you describe - presumably that's gone wrong somehow.
>>>>> Have you set num_reg_defaults?  That's the obvious thing...
>>>
>>>
>>>> Did that. I will have a closer look. Thanks for the answer.
>>>
>>>
>>> OK, the other thing that springs to mind to check is that the register
>>> didn't somehow get marked as volatile.
>
> Checked that! is_volatile_reg returns false for that register.
>
>>>
>>
>> There were some bugs in the past were non-readable register automatically
>> got marked as volatile, this has been fixed though a few months ago. Try to
>> make sure you use the latest upstream version of regmap.
>
> Thanks for pointing this out. I am using 3.19. Changing from
> REGCACHE_RBTREE to REGCACHE_FLAT fixed the problem for me.
>
> I will update my sources and try again with REGCACHE_RBTREE when
> I'll have some time.

You can find the reference code snippet for this problem here:

http://pastebin.com/vxFKqqyV

As you can see at line 45 is the definition for reg_default.

As far as I noticed, this happens if the reg addresses in the array
are not sorted.

I can reproduce the problem with:

static struct reg_default xxx_reg_defaults[] = {
        { XXX_REG_CTRL0,  0x00 },
        { XXX_REG_CTRL1,  0x00 },
        { XXX_REG_STATUS, 0x00 },
};

but, not if the reg default definition is:

static struct reg_default xxx_reg_defaults[] = {
        { XXX_REG_STATUS, 0x00 },
        { XXX_REG_CTRL0,  0x00 },
        { XXX_REG_CTRL1,  0x00 },
  };

Is this normal?

thanks,
Daniel.

Re: Using regmap_update_bits to update a write only register

[Lars-Peter Clausen]

On 03/06/2015 06:26 PM, Daniel Baluta wrote:
[...]
> I can reproduce the problem with:
>
> static struct reg_default xxx_reg_defaults[] = {
>          { XXX_REG_CTRL0,  0x00 },
>          { XXX_REG_CTRL1,  0x00 },
>          { XXX_REG_STATUS, 0x00 },
> };
>
> but, not if the reg default definition is:
>
> static struct reg_default xxx_reg_defaults[] = {
>          { XXX_REG_STATUS, 0x00 },
>          { XXX_REG_CTRL0,  0x00 },
>          { XXX_REG_CTRL1,  0x00 },
>    };
>
> Is this normal?

That's a rhetorical question, right?

It might be that there is a bug when growing a rbblock to the left. It 
probably went unnoticed because everybody has their reg defaults ordered in 
ascending order.

Try to put a few debug printks into regcache_rbtree_write() and 
regcache_rbtree_insert_to_block() to see what exactly is going on when a new 
register is inserted into the block. How do base_reg and top_reg change.

- Lars

Re: Using regmap_update_bits to update a write only register

[Lars-Peter Clausen]

On 03/06/2015 06:36 PM, Lars-Peter Clausen wrote:
> On 03/06/2015 06:26 PM, Daniel Baluta wrote:
> [...]
>> I can reproduce the problem with:
>>
>> static struct reg_default xxx_reg_defaults[] = {
>>          { XXX_REG_CTRL0,  0x00 },
>>          { XXX_REG_CTRL1,  0x00 },
>>          { XXX_REG_STATUS, 0x00 },
>> };
>>
>> but, not if the reg default definition is:
>>
>> static struct reg_default xxx_reg_defaults[] = {
>>          { XXX_REG_STATUS, 0x00 },
>>          { XXX_REG_CTRL0,  0x00 },
>>          { XXX_REG_CTRL1,  0x00 },
>>    };
>>
>> Is this normal?
>
> That's a rhetorical question, right?
>
> It might be that there is a bug when growing a rbblock to the left. It
> probably went unnoticed because everybody has their reg defaults ordered in
> ascending order.
>
> Try to put a few debug printks into regcache_rbtree_write() and
> regcache_rbtree_insert_to_block() to see what exactly is going on when a new
> register is inserted into the block. How do base_reg and top_reg change.

Try to replace the bitmap_shift_right() in regcache_rbtree_insert_to_block() 
with shift bitmap_shift_left(), I suspect that will do the trick.

- Lars

Re: Using regmap_update_bits to update a write only register

[Daniel Baluta]

On Fri, Mar 6, 2015 at 7:36 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 03/06/2015 06:26 PM, Daniel Baluta wrote:
> [...]
>>
>> I can reproduce the problem with:
>>
>> static struct reg_default xxx_reg_defaults[] = {
>>          { XXX_REG_CTRL0,  0x00 },
>>          { XXX_REG_CTRL1,  0x00 },
>>          { XXX_REG_STATUS, 0x00 },
>> };
>>
>> but, not if the reg default definition is:
>>
>> static struct reg_default xxx_reg_defaults[] = {
>>          { XXX_REG_STATUS, 0x00 },
>>          { XXX_REG_CTRL0,  0x00 },
>>          { XXX_REG_CTRL1,  0x00 },
>>    };
>>
>> Is this normal?
>
>
> That's a rhetorical question, right?
>
> It might be that there is a bug when growing a rbblock to the left. It
> probably went unnoticed because everybody has their reg defaults ordered in
> ascending order.
>
> Try to put a few debug printks into regcache_rbtree_write() and
> regcache_rbtree_insert_to_block() to see what exactly is going on when a new
> register is inserted into the block. How do base_reg and top_reg change.

I cannot test is right now because I don't have access to the physical device.
Is there a way to use to test the regmap API without an I2C/SPI device?

Daniel.

Re: Using regmap_update_bits to update a write only register

[Mark Brown]

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

On Fri, Mar 06, 2015 at 03:27:55PM +0200, Daniel Baluta wrote:

> > There were some bugs in the past were non-readable register automatically
> > got marked as volatile, this has been fixed though a few months ago. Try to
> > make sure you use the latest upstream version of regmap.

> Thanks for pointing this out. I am using 3.19. Changing from
> REGCACHE_RBTREE to REGCACHE_FLAT fixed the problem for me.

> I will update my sources and try again with REGCACHE_RBTREE when
> I'll have some time.

RBTREE should of course work.

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

Re: Using regmap_update_bits to update a write only register

[Mark Brown]

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

On Fri, Mar 06, 2015 at 09:48:16PM +0200, Daniel Baluta wrote:

> I cannot test is right now because I don't have access to the physical device.
> Is there a way to use to test the regmap API without an I2C/SPI device?

You should be able to create a MMIO regmap on a block of memory.

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

Re: Using regmap_update_bits to update a write only register

[Lars-Peter Clausen]

On 03/06/2015 08:48 PM, Daniel Baluta wrote:
> On Fri, Mar 6, 2015 at 7:36 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 03/06/2015 06:26 PM, Daniel Baluta wrote:
>> [...]
>>>
>>> I can reproduce the problem with:
>>>
>>> static struct reg_default xxx_reg_defaults[] = {
>>>           { XXX_REG_CTRL0,  0x00 },
>>>           { XXX_REG_CTRL1,  0x00 },
>>>           { XXX_REG_STATUS, 0x00 },
>>> };
>>>
>>> but, not if the reg default definition is:
>>>
>>> static struct reg_default xxx_reg_defaults[] = {
>>>           { XXX_REG_STATUS, 0x00 },
>>>           { XXX_REG_CTRL0,  0x00 },
>>>           { XXX_REG_CTRL1,  0x00 },
>>>     };
>>>
>>> Is this normal?
>>
>>
>> That's a rhetorical question, right?
>>
>> It might be that there is a bug when growing a rbblock to the left. It
>> probably went unnoticed because everybody has their reg defaults ordered in
>> ascending order.
>>
>> Try to put a few debug printks into regcache_rbtree_write() and
>> regcache_rbtree_insert_to_block() to see what exactly is going on when a new
>> register is inserted into the block. How do base_reg and top_reg change.
>
> I cannot test is right now because I don't have access to the physical device.
> Is there a way to use to test the regmap API without an I2C/SPI device?

I did some quick testing today, and the resizing the present bitmap 
definitely broken with the current code. Patch will follow shortly.

- Lars