Rockchip RK3188 I2C driver

Rockchip RK3188 I2C driver

[Max Schwarz]

Hello Heiko,

I just wanted to advertise that I'm currently working on a driver for the 
native I2C adapters in the RK3188. I want to avoid duplicating work, and I 
just saw that you also started on one in your github repository (nice job on 
the clock driver and ethernet support, by the way!).

If you want to peek, my status quo is here:
https://github.com/xqms/linux.git topic/i2c-rk3x

The driver is almost finished, it's just missing support for long transfers 
(>32 bytes) and a bit of cleanup. Communication with the ACT8846 works without 
problems. It depends on your clock driver though, so I'll wait with a patch 
until that is finished, right?

By the way, I got my arc-emac working correctly only after re-setting the mac 
address from userspace (ifconfig eth0 hw ether ...). A simple fix for that is 
also in my github repo (topic/emac-addr-fix), if you are interested.

Cheers,
  Max

Rockchip RK3188 I2C driver

[Heiko Stübner]

Hi Max,

Am Dienstag, 15. April 2014, 02:19:31 schrieb Max Schwarz:
> Hello Heiko,
> 
> I just wanted to advertise that I'm currently working on a driver for the
> native I2C adapters in the RK3188. I want to avoid duplicating work, and I
> just saw that you also started on one in your github repository (nice job on
> the clock driver and ethernet support, by the way!).
> 
> If you want to peek, my status quo is here:
> https://github.com/xqms/linux.git topic/i2c-rk3x

no worries ... the i2c stub in my tree is very old, so I'm very much looking 
forward to your driver :-)


> The driver is almost finished, it's just missing support for long transfers
> (>32 bytes) and a bit of cleanup. Communication with the ACT8846 works
> without problems. It depends on your clock driver though, so I'll wait with
> a patch until that is finished, right?

Not necessarily. Normally the drivers go through different trees anyway (here 
clock tree vs. i2c tree) and you might get comments and might need a v2 
anyway. Also as the driver will simply use the standard clock API, you have no 
dependies on my series - so in my mind you should simply go ahead when you're 
finished with it.


> By the way, I got my arc-emac working correctly only after re-setting the
> mac address from userspace (ifconfig eth0 hw ether ...). A simple fix for
> that is also in my github repo (topic/emac-addr-fix), if you are
> interested.

THANKS, that was exactly the tip/fix needed. We've been working the emac for 
some days now (getting the correct clock rate, phy working etc) but didn't see 
the data getting from the phy to the emac.


Thanks again
Heko

Rockchip RK3188 I2C driver

[Heiko Stübner]

Am Dienstag, 15. April 2014, 10:42:18 schrieb Heiko St?bner:
> > The driver is almost finished, it's just missing support for long
> > transfers
> > (>32 bytes) and a bit of cleanup. Communication with the ACT8846 works
> > without problems. It depends on your clock driver though, so I'll wait
> > with
> > a patch until that is finished, right?
> 
> Not necessarily. Normally the drivers go through different trees anyway
> (here clock tree vs. i2c tree) and you might get comments and might need a
> v2 anyway. Also as the driver will simply use the standard clock API, you
> have no dependies on my series - so in my mind you should simply go ahead
> when you're finished with it.

Looking at the grf-handling of your i2c-driver [0] reminded me, that I'm 
generally not sure how we should handle these registers. All of them use what 
recently always got called a hiword-mask, with the upper 16 bit indicating 
which lower 16 bit get changed.

So while the

	regmap_write(grf, 4, BIT(11 + bus_idx) | BIT(27 + bus_idx));

will most likely get the desired result at least once, I'm not sure how this 
interacts with the caching regmap implements [and regmap in general], as the 
bit(27 + bus_idx) is not a real value bit and will always read 0.

It may be sensible to teach regmap to handle such hiword-mask registers itself 
like in the clock case [1], so that it can automatically select the 
appropriate mask bits when value-bits are changed.


I've added Mark Brown, the regmap maintainer, in Cc because I'm not 100% sure 
if this is the correct way to go.


Heiko


[0] 
https://github.com/xqms/linux/commit/531bcb12a2ac1975f61d16d05e4608800c054c0d#diff-375822b3a417ed4faea8f6ae3e5c1766R621
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/174687.html

Rockchip RK3188 I2C driver

[Mark Brown]

On Tue, Apr 15, 2014 at 07:25:28PM +0200, Heiko St?bner wrote:

> Looking at the grf-handling of your i2c-driver [0] reminded me, that I'm 
> generally not sure how we should handle these registers. All of them use what 
> recently always got called a hiword-mask, with the upper 16 bit indicating 
> which lower 16 bit get changed.

> So while the

> 	regmap_write(grf, 4, BIT(11 + bus_idx) | BIT(27 + bus_idx));

> will most likely get the desired result at least once, I'm not sure how this 
> interacts with the caching regmap implements [and regmap in general], as the 
> bit(27 + bus_idx) is not a real value bit and will always read 0.

Can you be more specific what the wire format is here please?  The above
sounds like you're saying that the register value contains 32 bits, the
top 16 being essentially an extension of the register field and the last
16 bits being a value but that just sounds like 16+n bit registers and
16 bit data so presumably isn't what you mean.  Is there a datasheet
somewhere?
-------------- 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/20140415/7cfbc363/attachment.sig>

Rockchip RK3188 I2C driver

[Heiko Stübner]

Am Dienstag, 15. April 2014, 18:55:04 schrieb Mark Brown:
> On Tue, Apr 15, 2014 at 07:25:28PM +0200, Heiko St?bner wrote:
> > Looking at the grf-handling of your i2c-driver [0] reminded me, that I'm
> > generally not sure how we should handle these registers. All of them use
> > what recently always got called a hiword-mask, with the upper 16 bit
> > indicating which lower 16 bit get changed.
> > 
> > So while the
> > 
> > 	regmap_write(grf, 4, BIT(11 + bus_idx) | BIT(27 + bus_idx));
> > 
> > will most likely get the desired result at least once, I'm not sure how
> > this interacts with the caching regmap implements [and regmap in
> > general], as the bit(27 + bus_idx) is not a real value bit and will
> > always read 0.
> 
> Can you be more specific what the wire format is here please?  The above
> sounds like you're saying that the register value contains 32 bits, the
> top 16 being essentially an extension of the register field and the last
> 16 bits being a value but that just sounds like 16+n bit registers and
> 16 bit data so presumably isn't what you mean.  Is there a datasheet
> somewhere?

ok, I'll try to explain better.

The register has 32 bit. The upper 16 bit [31:16] are a write enable mask, so 
to change bit x, you also have to set (x+16) to 1.

There is a manual floating around the net, for example linked on [0] (the first 
link to TRM v2.0) and in it for example on page 226 the GRF_SOC_CON2 register.

To cite the relevant bits 31:16:
	bit0~bit15 write enable
	When bit 16=1, bit 0 can be written by software .
	When bit 16=0, bit 0 cannot be written by software;
	When bit 17=1, bit 1 can be written by software .
	When bit 17=0, bit 1 cannot be written by software;
	...

And these "write enable" bits are reset to 0 after the write, so if you write 
0x30001 you will get something like 0x1 on the next read, without the mask.


Thanks
Heiko


[0] http://hwswbits.blogspot.nl/2013/06/full-rk3066-technical-reference-manual.html

Rockchip RK3188 I2C driver

[Mark Brown]

On Tue, Apr 15, 2014 at 08:39:24PM +0200, Heiko St?bner wrote:

> The register has 32 bit. The upper 16 bit [31:16] are a write enable mask, so 
> to change bit x, you also have to set (x+16) to 1.

Perhaps I'm missing something about why this is helpful but it does seem
like the hardware designers have good drugs here.

> To cite the relevant bits 31:16:
> 	bit0~bit15 write enable
> 	When bit 16=1, bit 0 can be written by software .
> 	When bit 16=0, bit 0 cannot be written by software;
> 	When bit 17=1, bit 1 can be written by software .
> 	When bit 17=0, bit 1 cannot be written by software;
> 	...

> And these "write enable" bits are reset to 0 after the write, so if you write 
> 0x30001 you will get something like 0x1 on the next read, without the mask.

For non-volatile registers this should work fine if the write enable
bits are just latched on at probe time, we'll never actually look at the
what the hardware reads back once things are in cache.  For ones that
are volatile we'll need to teach the framework about it...  a write
enable mask setting that we splat into the value perhaps?
-------------- 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/20140415/57db1d0a/attachment.sig>

Rockchip RK3188 I2C driver

[Max Schwarz]

On Tuesday 15 April 2014 at 19:50:25, Mark Brown wrote:

> Perhaps I'm missing something about why this is helpful but it does seem
> like the hardware designers have good drugs here.

I assume that the point is to avoid read/write cycles for bit-level set/clear.
You can do a "change bit X, leave everything else intact" in a single write 
without reading the previous value, be it from the register itself or from 
some cache.

> For non-volatile registers this should work fine if the write enable
> bits are just latched on at probe time, we'll never actually look at the
> what the hardware reads back once things are in cache.  For ones that
> are volatile we'll need to teach the framework about it...  a write
> enable mask setting that we splat into the value perhaps?

The regmap is not volatile at the moment, but it should probably be. We don't 
have documentation on the registers (or do you know more, Heiko?), so we can't 
be sure how the bits we don't know about behave.

I see three possible solutions to our problem:

1) Ideally, regmap_update_bits(map, reg, mask, val) & co would directly boil 
   down to a single register write of (mask << 16) | val, even for volatile  
   registers. That's a rather big change to regmap though, isn't it?

2) A smaller change would be to always use 0xFFFF as the write enable mask 
   while writing. That would mean that write accesses to volatile registers 
   would always need a read access before to determine the previous value, 
   though.

3) Make the GRF/syscon device give direct register access to the drivers (i.e. 
   passing a pointer to the mapped registers) and bypass regmap entirely.

   That would make sense to me because we are AFAIK not using any of regmap's   
   features here:

   * Caching is not that useful because a) we are only doing a few init-time
     accesses and b) the write-mask provides similar functionality.
   * The bus for register access is always going to be MMIO.
   * All the drivers using the GRF are rockchip-specific and can know about 
     the write-mask thing.

>From the outcome, I would prefer (1). From the complexity, (3). Thoughts?

Cheers,
  Max

Rockchip RK3188 I2C driver

[Heiko Stübner]

Am Donnerstag, 17. April 2014, 02:04:20 schrieb Max Schwarz:
> On Tuesday 15 April 2014 at 19:50:25, Mark Brown wrote:
> > Perhaps I'm missing something about why this is helpful but it does seem
> > like the hardware designers have good drugs here.
> 
> I assume that the point is to avoid read/write cycles for bit-level
> set/clear. You can do a "change bit X, leave everything else intact" in a
> single write without reading the previous value, be it from the register
> itself or from some cache.
> 
> > For non-volatile registers this should work fine if the write enable
> > bits are just latched on at probe time, we'll never actually look at the
> > what the hardware reads back once things are in cache.  For ones that
> > are volatile we'll need to teach the framework about it...  a write
> > enable mask setting that we splat into the value perhaps?
> 
> The regmap is not volatile at the moment, but it should probably be. We
> don't have documentation on the registers (or do you know more, Heiko?), so
> we can't be sure how the bits we don't know about behave.

>From a cursory glance through the register docs, only GRF_SOC_STATUS0, 
GRF_SOC_STATUS1 and GRF_DDRC_STAT seem to be volatile, but they are also not 
writeable at all. All others look like they only receive settings, but do not 
contain volatile information.

Apart from GRF_SOC_STATUS1 (whose only entry gpu-idle-state is simply 
contained in DDRC_STAT on the rk3066), the shared GRF registers are very much 
alike between rk3188 and the leaked rk3066 trm.


> I see three possible solutions to our problem:
> 
> 1) Ideally, regmap_update_bits(map, reg, mask, val) & co would directly boil
> down to a single register write of (mask << 16) | val, even for volatile
> registers. That's a rather big change to regmap though, isn't it?
> 
> 2) A smaller change would be to always use 0xFFFF as the write enable mask
>    while writing. That would mean that write accesses to volatile registers
>    would always need a read access before to determine the previous value,
>    though.
> 
> 3) Make the GRF/syscon device give direct register access to the drivers
> (i.e. passing a pointer to the mapped registers) and bypass regmap
> entirely.
> 
>    That would make sense to me because we are AFAIK not using any of
> regmap's features here:
> 
>    * Caching is not that useful because a) we are only doing a few init-time
> accesses and b) the write-mask provides similar functionality. * The bus
> for register access is always going to be MMIO.
>    * All the drivers using the GRF are rockchip-specific and can know about
>      the write-mask thing.
> 
> From the outcome, I would prefer (1). From the complexity, (3). Thoughts?

I don't think I like (3) :-) . So far we've seen Rockchip and Hisilicon use 
this "hiword-mask" scheme on at least some of their registers. Enabling syscon 
to also share the raw mapped registers might encourage abuse of it slipping 
in, on platforms which need the concurrency handling regmap provides.

For (2), regmap already does the necessary read in _update_bits ... either 
from its cache or from the register itself for volatile ones. So in our case, 
we'd simply fall back to use the register like a regular one.

Interestingly, when looking through the regmap_config struct, it already 
contains the "write_flag_mask", described as "Mask to be set in the top byte of 
the register when doing  a write.", which sounds a lot like what we need, only 
that it's two bytes for us instead of the current one.


So I guess the easiest way would be to make write_flag_mask somehow usable for 
us, because it looks like regmap will handle the rest on its own already.

And then teach syscon to set this flag and about volatile registers in general, 
because it doesn't seem to handle them yet.


Heiko

Rockchip RK3188 I2C driver

[Mark Brown]

On Thu, Apr 17, 2014 at 02:04:20AM +0200, Max Schwarz wrote:
> On Tuesday 15 April 2014 at 19:50:25, Mark Brown wrote:

> > Perhaps I'm missing something about why this is helpful but it does seem
> > like the hardware designers have good drugs here.

> I assume that the point is to avoid read/write cycles for bit-level set/clear.
> You can do a "change bit X, leave everything else intact" in a single write 
> without reading the previous value, be it from the register itself or from 
> some cache.

I suppose...

> 1) Ideally, regmap_update_bits(map, reg, mask, val) & co would directly boil 
>    down to a single register write of (mask << 16) | val, even for volatile  
>    registers. That's a rather big change to regmap though, isn't it?

Fairly, yes, and it's a very specialised case which would still need to
take care of the cache.

> 2) A smaller change would be to always use 0xFFFF as the write enable mask 
>    while writing. That would mean that write accesses to volatile registers 
>    would always need a read access before to determine the previous value, 
>    though.

That's no different to any other device, though.

> 3) Make the GRF/syscon device give direct register access to the drivers (i.e. 
>    passing a pointer to the mapped registers) and bypass regmap entirely.
>    That would make sense to me because we are AFAIK not using any of regmap's   
>    features here:
> 
>    * Caching is not that useful because a) we are only doing a few init-time
>      accesses and b) the write-mask provides similar functionality.
>    * The bus for register access is always going to be MMIO.
>    * All the drivers using the GRF are rockchip-specific and can know about 
>      the write-mask thing.

> From the outcome, I would prefer (1). From the complexity, (3). Thoughts?

If you're only doing a few accesses then surely there's no meaningful
overhead from just writing what you want?  So long as you don't cache
these registers regmap won't really get in the way.
-------------- 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/20140417/5c97c7b2/attachment.sig>

Rockchip RK3188 I2C driver

[Max Schwarz]

On Thursday 17 April 2014 at 19:38:35, Mark Brown wrote:

> If you're only doing a few accesses then surely there's no meaningful
> overhead from just writing what you want?  So long as you don't cache
> these registers regmap won't really get in the way.

I think Heiko and I have been operating under the misconception that caching 
is somehow enabled by default - which is not the case. Thanks for clearing 
that up ;-)

Heiko - I'm now sure that we are fine with the access pattern we employ right 
now (always set the mask bits for writes). We also don't need to mark 
registers as volatile (has no meaning without caching).

In the meantime I developed a patch for regmap, but I think the better route 
would be to leave things as they are. If you guys still think "proper" regmap 
support is the way to go, the patch is attached.

Cheers,
  Max
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-regmap-support-for-registers-with-write-mask-in-uppe.patch
Type: text/x-patch
Size: 5540 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140418/c7e0e05c/attachment-0001.bin>

Rockchip RK3188 I2C driver

[Mark Brown]

On Thu, Apr 17, 2014 at 03:27:36PM +0200, Heiko St?bner wrote:

> Interestingly, when looking through the regmap_config struct, it already 
> contains the "write_flag_mask", described as "Mask to be set in the top byte of 
> the register when doing  a write.", which sounds a lot like what we need, only 
> that it's two bytes for us instead of the current one.

> So I guess the easiest way would be to make write_flag_mask somehow usable for 
> us, because it looks like regmap will handle the rest on its own already.

> And then teach syscon to set this flag and about volatile registers in general, 
> because it doesn't seem to handle them yet.

How about a val_set_bits_mask or something?  That's not a good name, I'm
short on inspiration right now though.

Another option is to do something with pad_bits to tell regmap to insert
a block of set bits.  Not sure if that's sensible for MMIO though, it's
more sensible for I2C or SPI style register maps.
-------------- 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/20140418/dc6a2150/attachment.sig>

Rockchip RK3188 I2C driver

[Heiko Stübner]

Am Freitag, 18. April 2014, 01:06:27 schrieb Max Schwarz:
> On Thursday 17 April 2014 at 19:38:35, Mark Brown wrote:
> > If you're only doing a few accesses then surely there's no meaningful
> > overhead from just writing what you want?  So long as you don't cache
> > these registers regmap won't really get in the way.
> 
> I think Heiko and I have been operating under the misconception that caching
> is somehow enabled by default - which is not the case. Thanks for clearing
> that up ;-)

Actually I think it's the other way around :-).

See regmap_read() calling _reagmap_read(), which in turn calls 
regcache_read(), except when map->cache_bypass is enabled, which then checks 
the volatile setting for the individual register.

So I guess we'd need to teach syscon to handle volatile registers (mark our 
grf ones as such) and then could leave the rest alone.


Heiko

Rockchip RK3188 I2C driver

[Max Schwarz]

On Friday 18 April 2014 at 11:06:56, Heiko St?bner wrote:
> > I think Heiko and I have been operating under the misconception that
> > caching is somehow enabled by default - which is not the case. Thanks for
> > clearing that up ;-)
> 
> Actually I think it's the other way around :-).
> 
> See regmap_read() calling _reagmap_read(), which in turn calls
> regcache_read(), except when map->cache_bypass is enabled, which then checks
> the volatile setting for the individual register.

But map->cache_bypass *is* enabled by regcache_init() in regcache.c when 
cache_type is REGCACHE_NONE, which is the default value:

if (map->cache_type == REGCACHE_NONE) {
	map->cache_bypass = true;
	return 0;
}

You can see the behaviour in detail if you compile with event tracing and boot 
with trace_event=regmap:* .

If you think about it, it would be quite insane to enable caching in syscon 
without knowing anything about the registers.

Cheers,
  Max

Rockchip RK3188 I2C driver

[Heiko Stübner]

Am Freitag, 18. April 2014, 11:30:21 schrieb Max Schwarz:
> On Friday 18 April 2014 at 11:06:56, Heiko St?bner wrote:
> > > I think Heiko and I have been operating under the misconception that
> > > caching is somehow enabled by default - which is not the case. Thanks
> > > for
> > > clearing that up ;-)
> > 
> > Actually I think it's the other way around :-).
> > 
> > See regmap_read() calling _reagmap_read(), which in turn calls
> > regcache_read(), except when map->cache_bypass is enabled, which then
> > checks the volatile setting for the individual register.
> 
> But map->cache_bypass *is* enabled by regcache_init() in regcache.c when
> cache_type is REGCACHE_NONE, which is the default value:
> 
> if (map->cache_type == REGCACHE_NONE) {
> 	map->cache_bypass = true;
> 	return 0;
> }
> 
> You can see the behaviour in detail if you compile with event tracing and
> boot with trace_event=regmap:* .
> 
> If you think about it, it would be quite insane to enable caching in syscon
> without knowing anything about the registers.

yep, you're right of course ... now I see it too :-)

So we really can leave everything as it is now - and I can stop worrying about 
creating future issues with the soc_con registers :-)


Heiko