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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 7 Sep 2022, at 14:37, Michal Orzel <michal.orzel@xxxxxxx> wrote:
> 
> 
> 
> 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.

Right I forgot that some parts are already in.
So we will need an extra patch to handle those.

Bertrand

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