Re: [PATCH v3 2/4] docs, xen/arm: Introduce static heap memory

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

> On 7 Sep 2022, at 14:31, Michal Orzel <michal.orzel@xxxxxxx> wrote:
> 
> 
> 
> On 07/09/2022 15:28, Bertrand Marquis wrote:
>> 
>> Hi Michal,
>> 
>>> On 7 Sep 2022, at 14:09, Michal Orzel <michal.orzel@xxxxxxx> wrote:
>>> 
>>> 
>>> On 07/09/2022 14:45, Julien Grall wrote:
>>>> 
>>>> On 07/09/2022 13:41, Michal Orzel wrote:
>>>>> 
>>>>> 
>>>>> On 07/09/2022 14:32, Julien Grall wrote:
>>>>>> [CAUTION: External Email]
>>>>>> 
>>>>>> On 07/09/2022 13:12, Michal Orzel wrote:
>>>>>>> Hi Julien,
>>>>>> 
>>>>>> Hi Michal,
>>>>>> 
>>>>>>> On 07/09/2022 13:36, Julien Grall wrote:
>>>>>>>> 
>>>>>>>> Hi Henry,
>>>>>>>> 
>>>>>>>> While reviewing the binding sent by Penny I noticed some inconsistency
>>>>>>>> with the one you introduced. See below.
>>>>>>>> 
>>>>>>>> On 07/09/2022 09:36, Henry Wang wrote:
>>>>>>>>> +- xen,static-heap
>>>>>>>>> +
>>>>>>>>> +    Property under the top-level "chosen" node. It specifies the 
>>>>>>>>> address
>>>>>>>>> +    and size of Xen static heap memory. Note that at least a 64KB
>>>>>>>>> +    alignment is required.
>>>>>>>>> +
>>>>>>>>> +- #xen,static-heap-address-cells and #xen,static-heap-size-cells
>>>>>>>>> +
>>>>>>>>> +    Specify the number of cells used for the address and size of the
>>>>>>>>> +    "xen,static-heap" property under "chosen".
>>>>>>>>> +
>>>>>>>>> +Below is an example on how to specify the static heap in device tree:
>>>>>>>>> +
>>>>>>>>> +    / {
>>>>>>>>> +        chosen {
>>>>>>>>> +            #xen,static-heap-address-cells = <0x2>;
>>>>>>>>> +            #xen,static-heap-size-cells = <0x2>;
>>>>>>>> 
>>>>>>>> Your binding, is introduce #xen,static-heap-{address, size}-cells
>>>>>>>> whereas Penny's one is using #{address, size}-cells even if the 
>>>>>>>> property
>>>>>>>> is not "reg".
>>>>>>>> 
>>>>>>>> I would like some consistency in the way we define bindings. Looking at
>>>>>>>> the tree, we already seem to have introduced
>>>>>>>> #xen-static-mem-address-cells. So maybe we should follow your approach?
>>>>>>>> 
>>>>>>>> That said, I am wondering whether we should just use one set of 
>>>>>>>> property
>>>>>>>> name.
>>>>>>>> 
>>>>>>>> I am open to suggestion here. My only request is we are consistent 
>>>>>>>> (i.e.
>>>>>>>> this doesn't depend on who wrote the bindings).
>>>>>>>> 
>>>>>>> In my opinion we should follow the device tree specification which 
>>>>>>> states
>>>>>>> that the #address-cells and #size-cells correspond to the reg property.
>>>>>> 
>>>>>> Hmmm.... Looking at [1], the two properties are not exclusive to 'reg'
>>>>>> Furthermore, I am not aware of any restriction for us to re-use them. Do
>>>>>> you have a pointer?
>>>>> 
>>>>> As we are discussing re-usage of #address-cells and #size-cells for 
>>>>> custom properties that are not "reg",
>>>>> I took this info from the latest device tree specs found under 
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.devicetree.org%2Fspecifications%2F&amp;data=05%7C01%7Cmichal.orzel%40amd.com%7C83da1eb9d32441cb9e8108da90d4f2d6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637981541539851438%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=3M9aT3LjCEOhZUHWSbgSSmKppY1Wion4TT3BeKLnWSo%3D&amp;reserved=0:
>>>>> "The #address-cells property defines the number of <u32> cells used to 
>>>>> encode the address field in a child node's reg property"
>>>>> and
>>>>> "The #size-cells property defines the number of <u32> cells used to 
>>>>> encode the size field in a child node’s reg property"
>>>> 
>>>> Right. But there is nothing in the wording suggesting that
>>>> #address-cells and #size-cells can't be re-used. From [1], it is clear
>>>> that the meaning has changed.
>>>> 
>>>> So why can't we do the same?
>>> I think this is a matter of how someone reads these sentences.
>>> I do not think that such documents need to state:
>>> "This property is for the reg. Do not use it for other purposes."
>>> The first part of the sentence is enough to inform what is supported.
>>> 
>>> On the other hand, looking at [1] these properties got new purposes
>>> so I think we could do the same. Now the question is whether we want that.
>>> I think it is doable to just have a single pair of #address/#size 
>>> properties.
>>> For instance xen,shared-mem requiring just 0x1 for address/size
>>> and reg requiring 0x2. This would just imply putting additional 0x00.
>> 
>> I think we want in general to reduce complexity when possible.
>> Here we are adding a lot of entries in the device tree where we know that
>> in all cases having only 2 will work all the time.
>> 
>> I am not convinced by the arguments on not using #address-cells and will
>> leave that one to Stefano
>> 
>> But in any case we should only add one pair here for sure, as you say the
>> only implication is to add a couple of 0 in the worst case.
> I agree. The only drawback is the need to modify the already introduced 
> properties
> to be coherent.

Agree, someone will need to do a pass on the whole doc which might be easier 
with all things in.

Cheers
Bertrand

> 
>> 
>> Cheers
>> Bertrand
>> 
>>> 
>>>> 
>>>> Cheers,
>>>> 
>>>> --
>>>> Julien Grall