| Message ID | 20150511193527.GA29690@lukather |
|---|---|
| State | New |
| Headers | show |
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
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
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@01c00000 { + compatible = "simple-bus"; + #address-cells = <1>; + #size-cells = <1>; + ranges; + + sram@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
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@01c00000 { > + compatible = "simple-bus"; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + sram@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
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
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
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
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
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