On 5/13/20 4:41 AM, 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. I suppose that's true to some extent. This is indeed a description of the system environment presented to the SW that consumes the DT, which is a bit more than pure HW description but still a description of something imposed externally rather than describing something that's up to the discretion of the consuming SW. So, go ahead. >> 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 > arch/arm/boot/dts/at91*.dtsi > 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. > >> [...] 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. > >> If we can't do that because some SW currently expects to be able to map >> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the >> SRAM driver which parts it's using, hence still allowing the driver to >> only map in-use portions? > > User doesn’t need sram driver in that case. It can use genalloc api directly. This sounds a bit odd. Without a driver for the reserved region, nothing should be touching it, since otherwise there's no code that owns an manages the region. If any code needs to consume the region, it should obtain info about the region from some form of provider code that can handle both the allocation and mapping. Anything else sounds like some consumer code directly making use of DT nodes it doesn't own. But since I'm not familiar enough with the SRAM driver and genalloc code that you mention to fully understand the allocation paths I guess I won't object for now, although it does still sound fishy.
