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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 07/09/2022 15:33, Bertrand Marquis wrote:
> 
>> 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%7Cc677a7983cd94e48620708da90d5a15f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637981544487489692%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=1uwtf%2F6shf2PiKu0XZPFNQ%2B6iyhLrMsYb1XEru3IGlg%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.
> 
Well, not only docs. If we decide to use a single pair of #address-cells and 
#size-cells, then
we need to modify the code that expects different properties e.g. 
xen,static-mem-{address/size}-cells.

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