[GIT PULL] Allwinner drivers changes for 4.2

[GIT PULL] Allwinner drivers changes for 4.2

[Maxime Ripard]

Hi Arnd, Kevin, Olof,

Here is the first batch of drivers changes for the 4.2 merge window.

Thanks!
Maxime

The following changes since commit b787f68c36d49bb1d9236f403813641efa74a031:

  Linux 4.1-rc1 (2015-04-26 17:59:10 -0700)

are available in the git repository at:

  https://git.kernel.org/pub/scm/linux/kernel/git/mripard/linux.git tags/sunxi-drivers-for-4.2

for you to fetch changes up to 44bb362ff9f2e6f9ab285e66ce92f55aee71808a:

  drivers: soc: sunxi: Introduce SoC driver to map SRAMs (2015-05-05 20:47:08 +0200)

----------------------------------------------------------------
Allwinner drivers patches for 4.2

This pull request contain a single driver to handle the SRAM mapping
between the CPU and devices.

----------------------------------------------------------------
Maxime Ripard (1):
      drivers: soc: sunxi: Introduce SoC driver to map SRAMs

 .../devicetree/bindings/soc/sunxi/sram.txt         |  64 ++++++
 drivers/soc/Kconfig                                |   1 +
 drivers/soc/Makefile                               |   1 +
 drivers/soc/sunxi/Kconfig                          |  10 +
 drivers/soc/sunxi/Makefile                         |   1 +
 drivers/soc/sunxi/sunxi_sram.c                     | 236 +++++++++++++++++++++
 include/linux/soc/sunxi/sunxi_sram.h               |  24 +++
 7 files changed, 337 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/sunxi/sram.txt
 create mode 100644 drivers/soc/sunxi/Kconfig
 create mode 100644 drivers/soc/sunxi/Makefile
 create mode 100644 drivers/soc/sunxi/sunxi_sram.c
 create mode 100644 include/linux/soc/sunxi/sunxi_sram.h

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150511/b3a3607a/attachment.sig>

[GIT PULL] Allwinner drivers changes for 4.2

[Arnd Bergmann]

On Monday 11 May 2015 21:35:27 Maxime Ripard wrote:
> Hi Arnd, Kevin, Olof,
> 
> Here is the first batch of drivers changes for the 4.2 merge window.
> 
> Thanks!
> Maxime
> 
> The following changes since commit b787f68c36d49bb1d9236f403813641efa74a031:
> 
>   Linux 4.1-rc1 (2015-04-26 17:59:10 -0700)
> 
> are available in the git repository at:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/mripard/linux.git tags/sunxi-drivers-for-4.2
> 
> for you to fetch changes up to 44bb362ff9f2e6f9ab285e66ce92f55aee71808a:
> 
>   drivers: soc: sunxi: Introduce SoC driver to map SRAMs (2015-05-05 20:47:08 +0200)
> 
> ----------------------------------------------------------------
> Allwinner drivers patches for 4.2
> 
> This pull request contain a single driver to handle the SRAM mapping
> between the CPU and devices.
> 

Hi Maxime,

Sorry I hadn't looked at the new driver before, but I did now and need a little
clarification. It seems to me that the device should be compatible with the
generic DT binding we have in Documentation/devicetree/bindings/misc/sram.txt,
and use more generic code. At least I can't see much in here that is really sunxi
specific.

Were you not aware of that generic binding, or did you have a good reason
not to use it? In the latter case, please document that in the patch description
(after replying here).

One small bug I found in the DT binding: the main DT node is lacking
a "ranges" property.

	Arnd

[GIT PULL] Allwinner drivers changes for 4.2

[Maxime Ripard]

Hi Arnd,

On Tue, May 12, 2015 at 10:15:20PM +0200, Arnd Bergmann wrote:
> On Monday 11 May 2015 21:35:27 Maxime Ripard wrote:
> > Hi Arnd, Kevin, Olof,
> > 
> > Here is the first batch of drivers changes for the 4.2 merge window.
> > 
> > Thanks!
> > Maxime
> > 
> > The following changes since commit b787f68c36d49bb1d9236f403813641efa74a031:
> > 
> >   Linux 4.1-rc1 (2015-04-26 17:59:10 -0700)
> > 
> > are available in the git repository at:
> > 
> >   https://git.kernel.org/pub/scm/linux/kernel/git/mripard/linux.git tags/sunxi-drivers-for-4.2
> > 
> > for you to fetch changes up to 44bb362ff9f2e6f9ab285e66ce92f55aee71808a:
> > 
> >   drivers: soc: sunxi: Introduce SoC driver to map SRAMs (2015-05-05 20:47:08 +0200)
> > 
> > ----------------------------------------------------------------
> > Allwinner drivers patches for 4.2
> > 
> > This pull request contain a single driver to handle the SRAM mapping
> > between the CPU and devices.
> > 
> 
> Hi Maxime,
> 
> Sorry I hadn't looked at the new driver before, but I did now and need a little
> clarification. It seems to me that the device should be compatible with the
> generic DT binding we have in Documentation/devicetree/bindings/misc/sram.txt,
> and use more generic code. At least I can't see much in here that is really sunxi
> specific.
>
> Were you not aware of that generic binding, or did you have a good reason
> not to use it?

I asked myself the same question, and I don't really think that this
would be wise, since that in order to be accessible by the CPU it has
to be mapped to it through this driver.

I felt like this alone justify a new compatible, even though we might
end up using the same driver.

> In the latter case, please document that in the patch description
> (after replying here).

Ok.

> One small bug I found in the DT binding: the main DT node is lacking
> a "ranges" property.

Which DT node are you talking about ?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150513/1826bc44/attachment.sig>

[GIT PULL] Allwinner drivers changes for 4.2

[Arnd Bergmann]

On Wednesday 13 May 2015 11:43:48 Maxime Ripard wrote:
> Hi Arnd,
> 
> On Tue, May 12, 2015 at 10:15:20PM +0200, Arnd Bergmann wrote:
> > On Monday 11 May 2015 21:35:27 Maxime Ripard wrote:
> > > Hi Arnd, Kevin, Olof,
> > > 
> > > Here is the first batch of drivers changes for the 4.2 merge window.
> > > 
> > > Thanks!
> > > Maxime
> > > 
> > > The following changes since commit b787f68c36d49bb1d9236f403813641efa74a031:
> > > 
> > >   Linux 4.1-rc1 (2015-04-26 17:59:10 -0700)
> > > 
> > > are available in the git repository at:
> > > 
> > >   https://git.kernel.org/pub/scm/linux/kernel/git/mripard/linux.git tags/sunxi-drivers-for-4.2
> > > 
> > > for you to fetch changes up to 44bb362ff9f2e6f9ab285e66ce92f55aee71808a:
> > > 
> > >   drivers: soc: sunxi: Introduce SoC driver to map SRAMs (2015-05-05 20:47:08 +0200)
> > > 
> > > ----------------------------------------------------------------
> > > Allwinner drivers patches for 4.2
> > > 
> > > This pull request contain a single driver to handle the SRAM mapping
> > > between the CPU and devices.
> > > 
> > 
> > Hi Maxime,
> > 
> > Sorry I hadn't looked at the new driver before, but I did now and need a little
> > clarification. It seems to me that the device should be compatible with the
> > generic DT binding we have in Documentation/devicetree/bindings/misc/sram.txt,
> > and use more generic code. At least I can't see much in here that is really sunxi
> > specific.
> >
> > Were you not aware of that generic binding, or did you have a good reason
> > not to use it?
> 
> I asked myself the same question, and I don't really think that this
> would be wise, since that in order to be accessible by the CPU it has
> to be mapped to it through this driver.
> 
> I felt like this alone justify a new compatible, even though we might
> end up using the same driver.

Have you discussed this with Heiko?

> > In the latter case, please document that in the patch description
> > (after replying here).
> 
> Ok.
> 
> > One small bug I found in the DT binding: the main DT node is lacking
> > a "ranges" property.
> 
> Which DT node are you talking about ?

I was referring to the ranges in this:

+soc at 01c00000 {
+       compatible = "simple-bus";
+       #address-cells = <1>;
+       #size-cells = <1>;
+       ranges;
+
+       sram at 00000000 {
+               compatible = "allwinner,sun4i-a10-sram";
+               reg = <0x00000000 0x4000>;
+               allwinner,sram-name = "A1";
+       };

but I now think I was misreading it, and the problem is different:
Rather than having separate devices for parts of the SRAM, you
are actually missing a node for the SRAM physical window. I think
the individual SRAM pieces should be nodes below one that describes
all of the SRAM, as we do in 
Documentation/devicetree/bindings/misc/sram.txt

	Arnd

[GIT PULL] Allwinner drivers changes for 4.2

[Maxime Ripard]

On Wed, May 13, 2015 at 12:30:39PM +0200, Arnd Bergmann wrote:
> > > Sorry I hadn't looked at the new driver before, but I did now and need a little
> > > clarification. It seems to me that the device should be compatible with the
> > > generic DT binding we have in Documentation/devicetree/bindings/misc/sram.txt,
> > > and use more generic code. At least I can't see much in here that is really sunxi
> > > specific.
> > >
> > > Were you not aware of that generic binding, or did you have a good reason
> > > not to use it?
> > 
> > I asked myself the same question, and I don't really think that this
> > would be wise, since that in order to be accessible by the CPU it has
> > to be mapped to it through this driver.
> > 
> > I felt like this alone justify a new compatible, even though we might
> > end up using the same driver.
> 
> Have you discussed this with Heiko?

No, I didn't.

We don't need to use his driver, there was no point about discussing
with him about anything.

> > > In the latter case, please document that in the patch description
> > > (after replying here).
> > 
> > Ok.
> > 
> > > One small bug I found in the DT binding: the main DT node is lacking
> > > a "ranges" property.
> > 
> > Which DT node are you talking about ?
> 
> I was referring to the ranges in this:
> 
> +soc at 01c00000 {
> +       compatible = "simple-bus";
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +       ranges;
> +
> +       sram at 00000000 {
> +               compatible = "allwinner,sun4i-a10-sram";
> +               reg = <0x00000000 0x4000>;
> +               allwinner,sram-name = "A1";
> +       };
> 
> but I now think I was misreading it, and the problem is different:
> Rather than having separate devices for parts of the SRAM, you
> are actually missing a node for the SRAM physical window. I think
> the individual SRAM pieces should be nodes below one that describes
> all of the SRAM, as we do in 
> Documentation/devicetree/bindings/misc/sram.txt

These are physically separate SRAM, used for different purposes, by
different devices.

Since when in the DT different instances of the same IP should be
represented in a single node?

And again, this patch is really not about "Simple IO memory regions to
be managed by the genalloc API". We have no use for these SRAMs, and
just want them to be mapped to the device, so the CPU won't even have
access to them for most of them.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150513/b6bb98f8/attachment.sig>

[GIT PULL] Allwinner drivers changes for 4.2

[Arnd Bergmann]

On Wednesday 13 May 2015 13:54:55 Maxime Ripard wrote:
> On Wed, May 13, 2015 at 12:30:39PM +0200, Arnd Bergmann wrote:
> > > > Sorry I hadn't looked at the new driver before, but I did now and need a little
> > > > clarification. It seems to me that the device should be compatible with the
> > > > generic DT binding we have in Documentation/devicetree/bindings/misc/sram.txt,
> > > > and use more generic code. At least I can't see much in here that is really sunxi
> > > > specific.
> > > >
> > > > Were you not aware of that generic binding, or did you have a good reason
> > > > not to use it?
> > > 
> > > I asked myself the same question, and I don't really think that this
> > > would be wise, since that in order to be accessible by the CPU it has
> > > to be mapped to it through this driver.
> > > 
> > > I felt like this alone justify a new compatible, even though we might
> > > end up using the same driver.
> > 
> > Have you discussed this with Heiko?
> 
> No, I didn't.
> 
> We don't need to use his driver, there was no point about discussing
> with him about anything.

The point is that you add a new binding for an SRAM. We already have
a binding for that, so you should try to make your device fit in with
the existing design.

> > but I now think I was misreading it, and the problem is different:
> > Rather than having separate devices for parts of the SRAM, you
> > are actually missing a node for the SRAM physical window. I think
> > the individual SRAM pieces should be nodes below one that describes
> > all of the SRAM, as we do in 
> > Documentation/devicetree/bindings/misc/sram.txt
> 
> These are physically separate SRAM, used for different purposes, by
> different devices.
> 
> Since when in the DT different instances of the same IP should be
> represented in a single node?
> 
> And again, this patch is really not about "Simple IO memory regions to
> be managed by the genalloc API". We have no use for these SRAMs, and
> just want them to be mapped to the device, so the CPU won't even have
> access to them for most of them.

I believe that is the case for a lot of the other SRAMs as well,
and that is why I think we need Heiko to review your binding and
say if it makes sense separately from the generic driver, or if
we should extend the existing binding.

We can still have separate drivers if necessary, but I really don't
want to see multiple incompatible ways of describing something this
fundamental in DT.

	Arnd

[GIT PULL] Allwinner drivers changes for 4.2

[Heiko Stuebner]

Am Mittwoch, 13. Mai 2015, 15:03:04 schrieb Arnd Bergmann:
> On Wednesday 13 May 2015 13:54:55 Maxime Ripard wrote:
> > On Wed, May 13, 2015 at 12:30:39PM +0200, Arnd Bergmann wrote:
> > > > > Sorry I hadn't looked at the new driver before, but I did now and
> > > > > need a little clarification. It seems to me that the device should
> > > > > be compatible with the generic DT binding we have in
> > > > > Documentation/devicetree/bindings/misc/sram.txt, and use more
> > > > > generic code. At least I can't see much in here that is really
> > > > > sunxi specific.
> > > > > 
> > > > > Were you not aware of that generic binding, or did you have a good
> > > > > reason
> > > > > not to use it?
> > > > 
> > > > I asked myself the same question, and I don't really think that this
> > > > would be wise, since that in order to be accessible by the CPU it has
> > > > to be mapped to it through this driver.
> > > > 
> > > > I felt like this alone justify a new compatible, even though we might
> > > > end up using the same driver.
> > > 
> > > Have you discussed this with Heiko?
> > 
> > No, I didn't.
> > 
> > We don't need to use his driver, there was no point about discussing
> > with him about anything.
> 
> The point is that you add a new binding for an SRAM. We already have
> a binding for that, so you should try to make your device fit in with
> the existing design.
> 
> > > but I now think I was misreading it, and the problem is different:
> > > Rather than having separate devices for parts of the SRAM, you
> > > are actually missing a node for the SRAM physical window. I think
> > > the individual SRAM pieces should be nodes below one that describes
> > > all of the SRAM, as we do in
> > > Documentation/devicetree/bindings/misc/sram.txt
> > 
> > These are physically separate SRAM, used for different purposes, by
> > different devices.
> > 
> > Since when in the DT different instances of the same IP should be
> > represented in a single node?
> > 
> > And again, this patch is really not about "Simple IO memory regions to
> > be managed by the genalloc API". We have no use for these SRAMs, and
> > just want them to be mapped to the device, so the CPU won't even have
> > access to them for most of them.
> 
> I believe that is the case for a lot of the other SRAMs as well,
> and that is why I think we need Heiko to review your binding and
> say if it makes sense separately from the generic driver, or if
> we should extend the existing binding.
> 
> We can still have separate drivers if necessary, but I really don't
> want to see multiple incompatible ways of describing something this
> fundamental in DT.

Preface: I only did the reserved sections so I could claim parts of my 
Rockchip sram for the smp code that needed to reside at a specific place in it, 
so I guess I don't necessarily feel qualified to judge one against the other 
:-), but anyway


The commit message for the driver contains

"We could also imagine changing this at runtime for example to change the
mapping of these SRAMs to use them for suspend/resume or runtime memory rate
change, if that ever happens."

which sounds to me a lot like the generic use case for the current sram driver 
- for example in conjuction with the PIE stuff if it ever resurfaces.


But from my short glance I also don't see how this quite custom mapping thing 
(device vs. cpu) would be able to fit into the generic description.


Heiko

[GIT PULL] Allwinner drivers changes for 4.2

[Maxime Ripard]

Hi Arnd,

On Wed, May 13, 2015 at 04:42:51PM +0200, Heiko Stuebner wrote:
> Am Mittwoch, 13. Mai 2015, 15:03:04 schrieb Arnd Bergmann:
> > On Wednesday 13 May 2015 13:54:55 Maxime Ripard wrote:
> > > On Wed, May 13, 2015 at 12:30:39PM +0200, Arnd Bergmann wrote:
> > > > > > Sorry I hadn't looked at the new driver before, but I did now and
> > > > > > need a little clarification. It seems to me that the device should
> > > > > > be compatible with the generic DT binding we have in
> > > > > > Documentation/devicetree/bindings/misc/sram.txt, and use more
> > > > > > generic code. At least I can't see much in here that is really
> > > > > > sunxi specific.
> > > > > > 
> > > > > > Were you not aware of that generic binding, or did you have a good
> > > > > > reason
> > > > > > not to use it?
> > > > > 
> > > > > I asked myself the same question, and I don't really think that this
> > > > > would be wise, since that in order to be accessible by the CPU it has
> > > > > to be mapped to it through this driver.
> > > > > 
> > > > > I felt like this alone justify a new compatible, even though we might
> > > > > end up using the same driver.
> > > > 
> > > > Have you discussed this with Heiko?
> > > 
> > > No, I didn't.
> > > 
> > > We don't need to use his driver, there was no point about discussing
> > > with him about anything.
> > 
> > The point is that you add a new binding for an SRAM. We already have
> > a binding for that, so you should try to make your device fit in with
> > the existing design.
> > 
> > > > but I now think I was misreading it, and the problem is different:
> > > > Rather than having separate devices for parts of the SRAM, you
> > > > are actually missing a node for the SRAM physical window. I think
> > > > the individual SRAM pieces should be nodes below one that describes
> > > > all of the SRAM, as we do in
> > > > Documentation/devicetree/bindings/misc/sram.txt
> > > 
> > > These are physically separate SRAM, used for different purposes, by
> > > different devices.
> > > 
> > > Since when in the DT different instances of the same IP should be
> > > represented in a single node?
> > > 
> > > And again, this patch is really not about "Simple IO memory regions to
> > > be managed by the genalloc API". We have no use for these SRAMs, and
> > > just want them to be mapped to the device, so the CPU won't even have
> > > access to them for most of them.
> > 
> > I believe that is the case for a lot of the other SRAMs as well,
> > and that is why I think we need Heiko to review your binding and
> > say if it makes sense separately from the generic driver, or if
> > we should extend the existing binding.
> > 
> > We can still have separate drivers if necessary, but I really don't
> > want to see multiple incompatible ways of describing something this
> > fundamental in DT.
> 
> Preface: I only did the reserved sections so I could claim parts of my 
> Rockchip sram for the smp code that needed to reside at a specific place in it, 
> so I guess I don't necessarily feel qualified to judge one against the other 
> :-), but anyway
> 
> 
> The commit message for the driver contains
> 
> "We could also imagine changing this at runtime for example to change the
> mapping of these SRAMs to use them for suspend/resume or runtime memory rate
> change, if that ever happens."
> 
> which sounds to me a lot like the generic use case for the current sram driver 
> - for example in conjuction with the PIE stuff if it ever resurfaces.
> 
> 
> But from my short glance I also don't see how this quite custom mapping thing 
> (device vs. cpu) would be able to fit into the generic description.

So, what's the conclusion on this?

This driver has been properly sent, without any kind of review from
you. I then sent a pull request with it for 4.1, which has only been
silently ignored.

And now, it seems like this is going to be the same for 4.2. I'd
*really* like to have some kind of a discussion here, and not let it
fall into oblivion. It fixes some real issues we have.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150521/1b0d06d8/attachment.sig>

[GIT PULL] Allwinner drivers changes for 4.2

[Arnd Bergmann]

On Thursday 21 May 2015 14:20:19 Maxime Ripard wrote:
> > Preface: I only did the reserved sections so I could claim parts of my 
> > Rockchip sram for the smp code that needed to reside at a specific place in it, 
> > so I guess I don't necessarily feel qualified to judge one against the other 
> > :-), but anyway
> > 
> > 
> > The commit message for the driver contains
> > 
> > "We could also imagine changing this at runtime for example to change the
> > mapping of these SRAMs to use them for suspend/resume or runtime memory rate
> > change, if that ever happens."
> > 
> > which sounds to me a lot like the generic use case for the current sram driver 
> > - for example in conjuction with the PIE stuff if it ever resurfaces.
> > 
> > 
> > But from my short glance I also don't see how this quite custom mapping thing 
> > (device vs. cpu) would be able to fit into the generic description.
> 
> So, what's the conclusion on this?
> 
> This driver has been properly sent, without any kind of review from
> you. I then sent a pull request with it for 4.1, which has only been
> silently ignored.
> 
> And now, it seems like this is going to be the same for 4.2. I'd
> *really* like to have some kind of a discussion here, and not let it
> fall into oblivion. It fixes some real issues we have.

I've looked at the driver some more now, and tried to come up with
a binding that I think would fit better with the existing mmio-sram
one. Do you think you could get it to work like this?

sram at 00000000 {
	// we bind to the first one here
	compatible = "allwinner,sun4i-a10-sram-controller", "mmio-sram";
	// first the SRAM, second the controller regs
	reg = <0x10000000 0x11000>, <0x01c00000 0x30>;
	ranges = <0 0x10000000 0x11000>;
	#address-cells = <1>;
	#size-cells = <1>;

	otg-sram: otg-sram at 10000 {
		compatible = "allwinner,sun4i-a10-sram";
		reg = <0x10000 0x1000>;
	};
};

&usb-otg {
	// allow otg driver to find the sram by phandle and
	// not need a table in the driver
	sram = <&sram-D>;
};

One important advantage here would be that we later have the option
of turning it into a subsystem with multiple sram controller drivers
and have sram clients just ask for a node by referencing a phandle.

The part I'm unsure about is whether the connections between
a particular client, the physical address of the SRAM as seen
from the CPU, and the register to control it are all fixed, or
if you can have clients that work with one or another SRAM chunk
depending on configuration.

	Arnd

[GIT PULL] Allwinner drivers changes for 4.2

[Maxime Ripard]

Hi Arnd,

On Thu, May 28, 2015 at 07:16:52PM +0200, Arnd Bergmann wrote:
> On Thursday 21 May 2015 14:20:19 Maxime Ripard wrote:
> > > Preface: I only did the reserved sections so I could claim parts of my 
> > > Rockchip sram for the smp code that needed to reside at a specific place in it, 
> > > so I guess I don't necessarily feel qualified to judge one against the other 
> > > :-), but anyway
> > > 
> > > 
> > > The commit message for the driver contains
> > > 
> > > "We could also imagine changing this at runtime for example to change the
> > > mapping of these SRAMs to use them for suspend/resume or runtime memory rate
> > > change, if that ever happens."
> > > 
> > > which sounds to me a lot like the generic use case for the current sram driver 
> > > - for example in conjuction with the PIE stuff if it ever resurfaces.
> > > 
> > > 
> > > But from my short glance I also don't see how this quite custom mapping thing 
> > > (device vs. cpu) would be able to fit into the generic description.
> > 
> > So, what's the conclusion on this?
> > 
> > This driver has been properly sent, without any kind of review from
> > you. I then sent a pull request with it for 4.1, which has only been
> > silently ignored.
> > 
> > And now, it seems like this is going to be the same for 4.2. I'd
> > *really* like to have some kind of a discussion here, and not let it
> > fall into oblivion. It fixes some real issues we have.
> 
> I've looked at the driver some more now, and tried to come up with
> a binding that I think would fit better with the existing mmio-sram
> one. Do you think you could get it to work like this?
> 
> sram at 00000000 {
> 	// we bind to the first one here
> 	compatible = "allwinner,sun4i-a10-sram-controller", "mmio-sram";
> 	// first the SRAM, second the controller regs
> 	reg = <0x10000000 0x11000>, <0x01c00000 0x30>;

Even if we consider the various contiguous SRAMs as a single one with
sections, that would cause some issues, because we have some other
SRAMs too, that are mapped at different addresses than those and still
controlled by the SRAM controller (C and D).

Depending on the SoC, and the current state of the driver, that would
mean a various number of reg cells...

> 	ranges = <0 0x10000000 0x11000>;

With ranges being wrong for most of them.

> 	#address-cells = <1>;
> 	#size-cells = <1>;
> 
> 	otg-sram: otg-sram at 10000 {
> 		compatible = "allwinner,sun4i-a10-sram";
> 		reg = <0x10000 0x1000>;
> 	};

And that would reserve a section of the SRAM, even if the device is
not used at all, which would mean that we would lose the benefit of
the mmio-sram genpool stuff.

> };
> 
> &usb-otg {
> 	// allow otg driver to find the sram by phandle and
> 	// not need a table in the driver
> 	sram = <&sram-D>;

That would be the best solution to reference the link between a device
and its SRAM though.

What about some kind of a "hybrid" solution: have all the SRAM
declared as separate node to avoid the ranges and reg issues described
above, and use an allwinner,sram or whatever to reference the link,
without requiring the string used by the client you were finding odd?

And then, the SRAM controller driver would simply parse this property
using the struct device when the client driver claim the SRAM?

> };
> 
> One important advantage here would be that we later have the option
> of turning it into a subsystem with multiple sram controller drivers
> and have sram clients just ask for a node by referencing a phandle.

I'd be all in favor of a subsystem, but most likely, when such a
subsystem will be introduced, we will not have considered all possible
cases, and would end up with different bindings anyway...

> The part I'm unsure about is whether the connections between
> a particular client, the physical address of the SRAM as seen
> from the CPU, and the register to control it are all fixed, or
> if you can have clients that work with one or another SRAM chunk
> depending on configuration.

As far as we know, it's all fixed.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150528/17ce0612/attachment.sig>

[GIT PULL] Allwinner drivers changes for 4.2

[Arnd Bergmann]

On Thursday 28 May 2015 21:08:13 Maxime Ripard wrote:
> Hi Arnd,
> 
> On Thu, May 28, 2015 at 07:16:52PM +0200, Arnd Bergmann wrote:
> > On Thursday 21 May 2015 14:20:19 Maxime Ripard wrote:
> > > > Preface: I only did the reserved sections so I could claim parts of my 
> > > > Rockchip sram for the smp code that needed to reside at a specific place in it, 
> > > > so I guess I don't necessarily feel qualified to judge one against the other 
> > > > :-), but anyway
> > > > 
> > > > 
> > > > The commit message for the driver contains
> > > > 
> > > > "We could also imagine changing this at runtime for example to change the
> > > > mapping of these SRAMs to use them for suspend/resume or runtime memory rate
> > > > change, if that ever happens."
> > > > 
> > > > which sounds to me a lot like the generic use case for the current sram driver 
> > > > - for example in conjuction with the PIE stuff if it ever resurfaces.
> > > > 
> > > > 
> > > > But from my short glance I also don't see how this quite custom mapping thing 
> > > > (device vs. cpu) would be able to fit into the generic description.
> > > 
> > > So, what's the conclusion on this?
> > > 
> > > This driver has been properly sent, without any kind of review from
> > > you. I then sent a pull request with it for 4.1, which has only been
> > > silently ignored.
> > > 
> > > And now, it seems like this is going to be the same for 4.2. I'd
> > > *really* like to have some kind of a discussion here, and not let it
> > > fall into oblivion. It fixes some real issues we have.
> > 
> > I've looked at the driver some more now, and tried to come up with
> > a binding that I think would fit better with the existing mmio-sram
> > one. Do you think you could get it to work like this?
> > 
> > sram at 00000000 {
> > 	// we bind to the first one here
> > 	compatible = "allwinner,sun4i-a10-sram-controller", "mmio-sram";
> > 	// first the SRAM, second the controller regs
> > 	reg = <0x10000000 0x11000>, <0x01c00000 0x30>;
> 
> Even if we consider the various contiguous SRAMs as a single one with
> sections, that would cause some issues, because we have some other
> SRAMs too, that are mapped at different addresses than those and still
> controlled by the SRAM controller (C and D).
> 
> Depending on the SoC, and the current state of the driver, that would
> mean a various number of reg cells...

Yes, good point.

> > 	ranges = <0 0x10000000 0x11000>;
> 
> With ranges being wrong for most of them.
> 
> > 	#address-cells = <1>;
> > 	#size-cells = <1>;
> > 
> > 	otg-sram: otg-sram at 10000 {
> > 		compatible = "allwinner,sun4i-a10-sram";
> > 		reg = <0x10000 0x1000>;
> > 	};
> 
> And that would reserve a section of the SRAM, even if the device is
> not used at all, which would mean that we would lose the benefit of
> the mmio-sram genpool stuff.

What it essentially means it that we would not list the section
in the DT if it's not used. Removing it from DT would be a bit
impractical, but if we use 'status="disabled"' (and ensure
that actually works), it should be ok.

> > };
> > 
> > &usb-otg {
> > 	// allow otg driver to find the sram by phandle and
> > 	// not need a table in the driver
> > 	sram = <&sram-D>;
> 
> That would be the best solution to reference the link between a device
> and its SRAM though.
> 
> What about some kind of a "hybrid" solution: have all the SRAM
> declared as separate node to avoid the ranges and reg issues described
> above, and use an allwinner,sram or whatever to reference the link,
> without requiring the string used by the client you were finding odd?

yes, sounds reasonable.
 
> And then, the SRAM controller driver would simply parse this property
> using the struct device when the client driver claim the SRAM?

Ok. I'm a bit torn between using "allwinner,sram" and making it
a standard "sram" property that could be used for a generic subsystem
if other platforms need the same thing in the future.

> > One important advantage here would be that we later have the option
> > of turning it into a subsystem with multiple sram controller drivers
> > and have sram clients just ask for a node by referencing a phandle.
> 
> I'd be all in favor of a subsystem, but most likely, when such a
> subsystem will be introduced, we will not have considered all possible
> cases, and would end up with different bindings anyway...

We can to some degree accomodate earlier bindings that are "mostly"
compatible by special-casing them. E.g. a generic subsystem could
look for both "srams" and "allwinner,sram" if the first is not found.

A related question is whether we want to pass arguments here, and how
to link the controller to the sram nodes.

	Arnd

[GIT PULL] Allwinner drivers changes for 4.2

[Maxime Ripard]

On Thu, May 28, 2015 at 09:17:03PM +0200, Arnd Bergmann wrote:
> On Thursday 28 May 2015 21:08:13 Maxime Ripard wrote:
> > Hi Arnd,
> > 
> > On Thu, May 28, 2015 at 07:16:52PM +0200, Arnd Bergmann wrote:
> > > On Thursday 21 May 2015 14:20:19 Maxime Ripard wrote:
> > > > > Preface: I only did the reserved sections so I could claim parts of my 
> > > > > Rockchip sram for the smp code that needed to reside at a specific place in it, 
> > > > > so I guess I don't necessarily feel qualified to judge one against the other 
> > > > > :-), but anyway
> > > > > 
> > > > > 
> > > > > The commit message for the driver contains
> > > > > 
> > > > > "We could also imagine changing this at runtime for example to change the
> > > > > mapping of these SRAMs to use them for suspend/resume or runtime memory rate
> > > > > change, if that ever happens."
> > > > > 
> > > > > which sounds to me a lot like the generic use case for the current sram driver 
> > > > > - for example in conjuction with the PIE stuff if it ever resurfaces.
> > > > > 
> > > > > 
> > > > > But from my short glance I also don't see how this quite custom mapping thing 
> > > > > (device vs. cpu) would be able to fit into the generic description.
> > > > 
> > > > So, what's the conclusion on this?
> > > > 
> > > > This driver has been properly sent, without any kind of review from
> > > > you. I then sent a pull request with it for 4.1, which has only been
> > > > silently ignored.
> > > > 
> > > > And now, it seems like this is going to be the same for 4.2. I'd
> > > > *really* like to have some kind of a discussion here, and not let it
> > > > fall into oblivion. It fixes some real issues we have.
> > > 
> > > I've looked at the driver some more now, and tried to come up with
> > > a binding that I think would fit better with the existing mmio-sram
> > > one. Do you think you could get it to work like this?
> > > 
> > > sram at 00000000 {
> > > 	// we bind to the first one here
> > > 	compatible = "allwinner,sun4i-a10-sram-controller", "mmio-sram";
> > > 	// first the SRAM, second the controller regs
> > > 	reg = <0x10000000 0x11000>, <0x01c00000 0x30>;
> > 
> > Even if we consider the various contiguous SRAMs as a single one with
> > sections, that would cause some issues, because we have some other
> > SRAMs too, that are mapped at different addresses than those and still
> > controlled by the SRAM controller (C and D).
> > 
> > Depending on the SoC, and the current state of the driver, that would
> > mean a various number of reg cells...
> 
> Yes, good point.
> 
> > > 	ranges = <0 0x10000000 0x11000>;
> > 
> > With ranges being wrong for most of them.
> > 
> > > 	#address-cells = <1>;
> > > 	#size-cells = <1>;
> > > 
> > > 	otg-sram: otg-sram at 10000 {
> > > 		compatible = "allwinner,sun4i-a10-sram";
> > > 		reg = <0x10000 0x1000>;
> > > 	};
> > 
> > And that would reserve a section of the SRAM, even if the device is
> > not used at all, which would mean that we would lose the benefit of
> > the mmio-sram genpool stuff.
> 
> What it essentially means it that we would not list the section
> in the DT if it's not used. Removing it from DT would be a bit
> impractical, but if we use 'status="disabled"' (and ensure
> that actually works), it should be ok.

Ok.

> > > };
> > > 
> > > &usb-otg {
> > > 	// allow otg driver to find the sram by phandle and
> > > 	// not need a table in the driver
> > > 	sram = <&sram-D>;
> > 
> > That would be the best solution to reference the link between a device
> > and its SRAM though.
> > 
> > What about some kind of a "hybrid" solution: have all the SRAM
> > declared as separate node to avoid the ranges and reg issues described
> > above, and use an allwinner,sram or whatever to reference the link,
> > without requiring the string used by the client you were finding odd?
> 
> yes, sounds reasonable.
>  
> > And then, the SRAM controller driver would simply parse this property
> > using the struct device when the client driver claim the SRAM?
> 
> Ok. I'm a bit torn between using "allwinner,sram" and making it
> a standard "sram" property that could be used for a generic subsystem
> if other platforms need the same thing in the future.

I wouldn't be very comfortable adding a generic property when we have
only considered a single case, hence why I suggested allwinner,sram.

But if you prefer to simply use sram, I'm fine with it.

> > > One important advantage here would be that we later have the option
> > > of turning it into a subsystem with multiple sram controller drivers
> > > and have sram clients just ask for a node by referencing a phandle.
> > 
> > I'd be all in favor of a subsystem, but most likely, when such a
> > subsystem will be introduced, we will not have considered all possible
> > cases, and would end up with different bindings anyway...
> 
> We can to some degree accomodate earlier bindings that are "mostly"
> compatible by special-casing them. E.g. a generic subsystem could
> look for both "srams" and "allwinner,sram" if the first is not found.

That would work too.

> A related question is whether we want to pass arguments here, and how
> to link the controller to the sram nodes.

Do we need to?

When a client would try to claim an SRAM, we can parse the phandle,
find the SRAM name, and look into its own table for the matching name.

And since it's pretty much how pinctrl (can) work, we can extend that
mechanism in the future if we want to support other drivers with teh
same bindings.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150528/360b555a/attachment.sig>

[GIT PULL] Allwinner drivers changes for 4.2

[Arnd Bergmann]

On Thursday 28 May 2015 22:18:17 Maxime Ripard wrote:
> 
> > A related question is whether we want to pass arguments here, and how
> > to link the controller to the sram nodes.
> 
> Do we need to?
> 
> When a client would try to claim an SRAM, we can parse the phandle,
> find the SRAM name, and look into its own table for the matching name.
> 
> And since it's pretty much how pinctrl (can) work, we can extend that
> mechanism in the future if we want to support other drivers with teh
> same bindings.

The only reason for passing an argument would be the mux ID of the user,
in particular for SRAM_C2, which has three possible users other
than CPU. Passing these as an argument would avoid having to put
the configuration into the inter-driver interface.

	Arnd

[GIT PULL] Allwinner drivers changes for 4.2

[Maxime Ripard]

On Wed, May 13, 2015 at 03:03:04PM +0200, Arnd Bergmann wrote:
> On Wednesday 13 May 2015 13:54:55 Maxime Ripard wrote:
> > On Wed, May 13, 2015 at 12:30:39PM +0200, Arnd Bergmann wrote:
> > > > > Sorry I hadn't looked at the new driver before, but I did now and need a little
> > > > > clarification. It seems to me that the device should be compatible with the
> > > > > generic DT binding we have in Documentation/devicetree/bindings/misc/sram.txt,
> > > > > and use more generic code. At least I can't see much in here that is really sunxi
> > > > > specific.
> > > > >
> > > > > Were you not aware of that generic binding, or did you have a good reason
> > > > > not to use it?
> > > > 
> > > > I asked myself the same question, and I don't really think that this
> > > > would be wise, since that in order to be accessible by the CPU it has
> > > > to be mapped to it through this driver.
> > > > 
> > > > I felt like this alone justify a new compatible, even though we might
> > > > end up using the same driver.
> > > 
> > > Have you discussed this with Heiko?
> > 
> > No, I didn't.
> > 
> > We don't need to use his driver, there was no point about discussing
> > with him about anything.
> 
> The point is that you add a new binding for an SRAM. We already have
> a binding for that, so you should try to make your device fit in with
> the existing design.

Again, we have a binding for, according to the documentation, "Generic
on-chip SRAM. Simple IO memory regions to be managed by the genalloc
API." whatever the genalloc API means from a DT point of view.

These SRAM are in no way generic. They are not always IO mapped, and
are not to be managed by the genalloc API.

I'm not sure how that qualifies.

Plus, if we ever need to use genalloc, nothing actually prevents us
from using that binding, both are not incompatible.

> > > but I now think I was misreading it, and the problem is different:
> > > Rather than having separate devices for parts of the SRAM, you
> > > are actually missing a node for the SRAM physical window. I think
> > > the individual SRAM pieces should be nodes below one that describes
> > > all of the SRAM, as we do in 
> > > Documentation/devicetree/bindings/misc/sram.txt
> > 
> > These are physically separate SRAM, used for different purposes, by
> > different devices.
> > 
> > Since when in the DT different instances of the same IP should be
> > represented in a single node?
> > 
> > And again, this patch is really not about "Simple IO memory regions to
> > be managed by the genalloc API". We have no use for these SRAMs, and
> > just want them to be mapped to the device, so the CPU won't even have
> > access to them for most of them.
> 
> I believe that is the case for a lot of the other SRAMs as well,
> and that is why I think we need Heiko to review your binding and
> say if it makes sense separately from the generic driver, or if
> we should extend the existing binding.
> 
> We can still have separate drivers if necessary, but I really don't
> want to see multiple incompatible ways of describing something this
> fundamental in DT.

And this is not incompatible. We can very well add sub-regions later
on if needed.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150513/f883b2ab/attachment.sig>