On Wed, May 13, 2020 at 12:41:27PM +0200, Mian Yousaf Kaukab wrote: > On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote: > > On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote: > > > Add documentation for the new optional flag added for SRAM driver. > > > > > diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml > > > > > + reserved-only: > > > + description: > > > + The flag indicating, that only SRAM reserved regions have to be remapped. > > > + remapping type is selected depending upon no-memory-wc as usual. > > > + type: boolean > > > > This feels a bit like a SW flag rather than a HW description, so I'm not > > sure it's appropriate to put it into DT. > > Reserved regions themselves are software descriptions, no? Then we have 'pool' > flag which is again a software flag and so on. This flag falls into same > category and nothing out of ordinary. > > > > Are there any cases where the SW should map all of the SRAM, i.e. where > > we wouldn't expect to set reserved-only? [...] > > Yes, here are a few examples: > arch/arm/boot/dts/aspeed-g*.dtsi Looking at the implementation of the sole user of this, which is in drivers/fsi/fsi-master-ast-cf.c, it looks like this really should've specified a partition because the driver basically goes on to allocate a fixed 4 KiB region of memory anyway. > arch/arm/boot/dts/at91*.dtsi While these define SRAM nodes, I don't see them referenced anywhere. > arch/arm/boot/dts/bcm7445.dtsi > Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything > except the reserved region. The driver currently maps everything, so if this relies on this particular reserved region not being mapped then that's already broken anyway. > > [...] I'd expect reserved-only to be > > the default, and perhaps only, mode of operation for the SRAM driver. > > It will break compatibility with existing dtbs. Yes, that's a bit unfortunate. I think this driver may suffer from a slightly ambiguous device tree binding and then people just trying to fit it to their use-cases. However, I think we could preserve DTB backwards-compatibility while at the same time correcting course and establish some sort of consistency. Looking at the examples that you've provided and others, there are two classes of users: users that don't specify any partitions either use all of the available SRAM exclusively or manually allocate some part of it, whereas users that have specified partitions all seem to use only the defined partitions. Given that, I think what we could do is check if there are any child nodes and if not, keep the existing behaviour of mapping the whole SRAM area. For cases where child nodes exist we could decide to go with the default that Stephen suggested and only map regions for which a child node has been defined. This should allow both categories of users to work the way that they were probably expected to work. Any thoughts? Thierry
Attachment:
signature.asc
Description: PGP signature
