Handling device shared SFR on dt platform

Handling device shared SFR on dt platform

[Sylwester Nawrocki]

Hello,

I'm working on adding the device tree support for two devices that
share some IO registers. Exactly, there is one shared SFR that 
contains a common bit to enable two PHY devices, and another two
bits, each to separately reset each PHY.

Currently this shared register is statically memory mapped and it is
handled by a platform specific callback, that both drivers use to
control their corresponding (sub)devices in synchronised manner.

The two devices are a MIPI-CSI2 receiver and a MIPI-DSI transmitter, 
and the common register is there to control the camera and the display 
port PHY. The hardware design is a bit unfortunate, but there is not
much I could do about it.

The callback can be looked at at:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/plat-s5p/setup-mipiphy.c

I have been trying to find method to handle this on a dt platform. 
Has anyone faced similar issue before ? I would appreciate any ideas
or suggestions.

One of ideas was to create some sort of PHY framework, similar to
clock API, and perhaps handle such platform quirks outside DT.

--

Thanks,
Sylwester

Handling device shared SFR on dt platform

[Sylwester Nawrocki]

Hello,

I'm working on adding the device tree support for two devices that
share some IO registers. Exactly, there is one shared SFR that 
contains a common bit to enable two PHY devices, and another two
bits, each to separately reset each PHY.

Currently this shared register is statically memory mapped and it is
handled by a platform specific callback, that both drivers use to
control their corresponding (sub)devices in synchronised manner.

The two devices are a MIPI-CSI2 receiver and a MIPI-DSI transmitter, 
and the common register is there to control the camera and the display 
port PHY. The hardware design is a bit unfortunate, but there is not
much I could do about it.

The callback can be looked at at:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/plat-s5p/setup-mipiphy.c

I have been trying to find method to handle this on a dt platform. 
Has anyone faced similar issue before ? I would appreciate any ideas
or suggestions.

One of ideas was to create some sort of PHY framework, similar to
clock API, and perhaps handle such platform quirks outside DT.

--

Thanks,
Sylwester

Re: Handling device shared SFR on dt platform

[Grant Likely]

On Wed, 21 Mar 2012 23:38:11 +0100, Sylwester Nawrocki <snjw23@gmail.com> wrote:
> 
> Hello,
> 
> I'm working on adding the device tree support for two devices that
> share some IO registers. Exactly, there is one shared SFR that 
> contains a common bit to enable two PHY devices, and another two
> bits, each to separately reset each PHY.
> 
> Currently this shared register is statically memory mapped and it is
> handled by a platform specific callback, that both drivers use to
> control their corresponding (sub)devices in synchronised manner.
> 
> The two devices are a MIPI-CSI2 receiver and a MIPI-DSI transmitter, 
> and the common register is there to control the camera and the display 
> port PHY. The hardware design is a bit unfortunate, but there is not
> much I could do about it.

Generally, this is handled by having a third node for the shared
register block and both device nodes holding a phandle to it.

No, there isn't any common infrastructure for implementing this, but
it isn't very much code.

g.

Handling device shared SFR on dt platform

[Grant Likely]

On Wed, 21 Mar 2012 23:38:11 +0100, Sylwester Nawrocki <snjw23@gmail.com> wrote:
> 
> Hello,
> 
> I'm working on adding the device tree support for two devices that
> share some IO registers. Exactly, there is one shared SFR that 
> contains a common bit to enable two PHY devices, and another two
> bits, each to separately reset each PHY.
> 
> Currently this shared register is statically memory mapped and it is
> handled by a platform specific callback, that both drivers use to
> control their corresponding (sub)devices in synchronised manner.
> 
> The two devices are a MIPI-CSI2 receiver and a MIPI-DSI transmitter, 
> and the common register is there to control the camera and the display 
> port PHY. The hardware design is a bit unfortunate, but there is not
> much I could do about it.

Generally, this is handled by having a third node for the shared
register block and both device nodes holding a phandle to it.

No, there isn't any common infrastructure for implementing this, but
it isn't very much code.

g.

Re: Handling device shared SFR on dt platform

[Sylwester Nawrocki]

Hello Grant,

On 03/24/2012 08:32 PM, Grant Likely wrote:
>
> Generally, this is handled by having a third node for the shared
> register block and both device nodes holding a phandle to it.
> 
> No, there isn't any common infrastructure for implementing this, but
> it isn't very much code.

Thank you, that sounds good. I'm just wondering how to handle the shared
resource access synchronisation. There is an IORESOURCE_MUXED resource type 
flag which could be used to prevent drivers from stomping on each others 
feet when accessing the shared register, if I understand the software muxed
resource semantics correctly. That is, using something like 
request_muxed_(mem_)region()/release_mem_region() for the shared register
protection.

What concerns me, is an overhead from region request/ioremap(?)/release, just
to access a single 32-bit register. I'm going to see if those accesses could
be moved to only device driver's probe() and remove() callbacks and what might
be the resulting power consumption increase from that, if any.


--

Thanks,
Sylwester

Handling device shared SFR on dt platform

[Sylwester Nawrocki]

Hello Grant,

On 03/24/2012 08:32 PM, Grant Likely wrote:
>
> Generally, this is handled by having a third node for the shared
> register block and both device nodes holding a phandle to it.
> 
> No, there isn't any common infrastructure for implementing this, but
> it isn't very much code.

Thank you, that sounds good. I'm just wondering how to handle the shared
resource access synchronisation. There is an IORESOURCE_MUXED resource type 
flag which could be used to prevent drivers from stomping on each others 
feet when accessing the shared register, if I understand the software muxed
resource semantics correctly. That is, using something like 
request_muxed_(mem_)region()/release_mem_region() for the shared register
protection.

What concerns me, is an overhead from region request/ioremap(?)/release, just
to access a single 32-bit register. I'm going to see if those accesses could
be moved to only device driver's probe() and remove() callbacks and what might
be the resulting power consumption increase from that, if any.


--

Thanks,
Sylwester

Re: Handling device shared SFR on dt platform

[Grant Likely]

On Wed, 28 Mar 2012 21:45:14 +0200, Sylwester Nawrocki <snjw23@gmail.com> wrote:
> Hello Grant,
> 
> On 03/24/2012 08:32 PM, Grant Likely wrote:
> >
> > Generally, this is handled by having a third node for the shared
> > register block and both device nodes holding a phandle to it.
> > 
> > No, there isn't any common infrastructure for implementing this, but
> > it isn't very much code.
> 
> Thank you, that sounds good. I'm just wondering how to handle the shared
> resource access synchronisation. There is an IORESOURCE_MUXED resource type 
> flag which could be used to prevent drivers from stomping on each others 
> feet when accessing the shared register, if I understand the software muxed
> resource semantics correctly. That is, using something like 
> request_muxed_(mem_)region()/release_mem_region() for the shared register
> protection.
> 
> What concerns me, is an overhead from region request/ioremap(?)/release, just
> to access a single 32-bit register. I'm going to see if those accesses could
> be moved to only device driver's probe() and remove() callbacks and what might
> be the resulting power consumption increase from that, if any.

Personally, I'd just create a shared function used by both drivers to
access the register and protect it with a spinlock.  That way the
region only needs to be mapped once, and the overhead is as low as
possible.

g.

Handling device shared SFR on dt platform

[Grant Likely]

On Wed, 28 Mar 2012 21:45:14 +0200, Sylwester Nawrocki <snjw23@gmail.com> wrote:
> Hello Grant,
> 
> On 03/24/2012 08:32 PM, Grant Likely wrote:
> >
> > Generally, this is handled by having a third node for the shared
> > register block and both device nodes holding a phandle to it.
> > 
> > No, there isn't any common infrastructure for implementing this, but
> > it isn't very much code.
> 
> Thank you, that sounds good. I'm just wondering how to handle the shared
> resource access synchronisation. There is an IORESOURCE_MUXED resource type 
> flag which could be used to prevent drivers from stomping on each others 
> feet when accessing the shared register, if I understand the software muxed
> resource semantics correctly. That is, using something like 
> request_muxed_(mem_)region()/release_mem_region() for the shared register
> protection.
> 
> What concerns me, is an overhead from region request/ioremap(?)/release, just
> to access a single 32-bit register. I'm going to see if those accesses could
> be moved to only device driver's probe() and remove() callbacks and what might
> be the resulting power consumption increase from that, if any.

Personally, I'd just create a shared function used by both drivers to
access the register and protect it with a spinlock.  That way the
region only needs to be mapped once, and the overhead is as low as
possible.

g.

Re: Handling device shared SFR on dt platform

[Sylwester Nawrocki]

On 04/03/2012 06:04 PM, Grant Likely wrote:
>> What concerns me, is an overhead from region request/ioremap(?)/release, 
>> just to access a single 32-bit register. I'm going to see if those accesses
>> could be moved to only device driver's probe() and remove() callbacks and 
>> what might be the resulting power consumption increase from that, if any.
> 
> Personally, I'd just create a shared function used by both drivers to
> access the register and protect it with a spinlock.  That way the
> region only needs to be mapped once, and the overhead is as low as
> possible.

Hmm, that's what I originally started with. I've just removed function 
callback from the platform data struct in favour of having the drivers 
using directly the function exported by plat-s5p. Then I started devising
something alternative, in hope to get rid of the drivers compilation 
dependency on PLAT_S5P. There seems little point in that though, since 
the chance to have those IP blocks available on other archs or ARM sub-archs,
than Samsung S5P/Exynos, are extremely low. 

I'll post finally some patches for review. Thank you.

---
Regards,
Sylwester

Handling device shared SFR on dt platform

[Sylwester Nawrocki]

On 04/03/2012 06:04 PM, Grant Likely wrote:
>> What concerns me, is an overhead from region request/ioremap(?)/release, 
>> just to access a single 32-bit register. I'm going to see if those accesses
>> could be moved to only device driver's probe() and remove() callbacks and 
>> what might be the resulting power consumption increase from that, if any.
> 
> Personally, I'd just create a shared function used by both drivers to
> access the register and protect it with a spinlock.  That way the
> region only needs to be mapped once, and the overhead is as low as
> possible.

Hmm, that's what I originally started with. I've just removed function 
callback from the platform data struct in favour of having the drivers 
using directly the function exported by plat-s5p. Then I started devising
something alternative, in hope to get rid of the drivers compilation 
dependency on PLAT_S5P. There seems little point in that though, since 
the chance to have those IP blocks available on other archs or ARM sub-archs,
than Samsung S5P/Exynos, are extremely low. 

I'll post finally some patches for review. Thank you.

---
Regards,
Sylwester

Re: Handling device shared SFR on dt platform

[Grant Likely]

On Wed, 04 Apr 2012 22:35:50 +0200, Sylwester Nawrocki <snjw23-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 04/03/2012 06:04 PM, Grant Likely wrote:
> >> What concerns me, is an overhead from region request/ioremap(?)/release, 
> >> just to access a single 32-bit register. I'm going to see if those accesses
> >> could be moved to only device driver's probe() and remove() callbacks and 
> >> what might be the resulting power consumption increase from that, if any.
> > 
> > Personally, I'd just create a shared function used by both drivers to
> > access the register and protect it with a spinlock.  That way the
> > region only needs to be mapped once, and the overhead is as low as
> > possible.
> 
> Hmm, that's what I originally started with. I've just removed function 
> callback from the platform data struct in favour of having the drivers 
> using directly the function exported by plat-s5p. Then I started devising
> something alternative, in hope to get rid of the drivers compilation 
> dependency on PLAT_S5P. There seems little point in that though, since 
> the chance to have those IP blocks available on other archs or ARM sub-archs,
> than Samsung S5P/Exynos, are extremely low. 

As long as everything remains multiplatform friendly, I'm fine with
this.  It will need to change though if ever there needs to be
multiple definitions of that helper function because that means only
one platform can be enabled at a time.

g.

Handling device shared SFR on dt platform

[Grant Likely]

On Wed, 04 Apr 2012 22:35:50 +0200, Sylwester Nawrocki <snjw23@gmail.com> wrote:
> On 04/03/2012 06:04 PM, Grant Likely wrote:
> >> What concerns me, is an overhead from region request/ioremap(?)/release, 
> >> just to access a single 32-bit register. I'm going to see if those accesses
> >> could be moved to only device driver's probe() and remove() callbacks and 
> >> what might be the resulting power consumption increase from that, if any.
> > 
> > Personally, I'd just create a shared function used by both drivers to
> > access the register and protect it with a spinlock.  That way the
> > region only needs to be mapped once, and the overhead is as low as
> > possible.
> 
> Hmm, that's what I originally started with. I've just removed function 
> callback from the platform data struct in favour of having the drivers 
> using directly the function exported by plat-s5p. Then I started devising
> something alternative, in hope to get rid of the drivers compilation 
> dependency on PLAT_S5P. There seems little point in that though, since 
> the chance to have those IP blocks available on other archs or ARM sub-archs,
> than Samsung S5P/Exynos, are extremely low. 

As long as everything remains multiplatform friendly, I'm fine with
this.  It will need to change though if ever there needs to be
multiple definitions of that helper function because that means only
one platform can be enabled at a time.

g.