Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Stephen Warren]

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.