[1/2] dt-bindings: misc: qcom,fastrpc: add qcom,assign-all-memory property

  Document the added qcom,assign-all-memory in devicetree bindings.

Signed-off-by: Dylan Van Assche <me@dylanvanassche.be>
---
 Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6 ++++++
 1 file changed, 6 insertions(+)
  

On 25/03/2023 14:44, Dylan Van Assche wrote:
> Document the added qcom,assign-all-memory in devicetree bindings.
> 
> Signed-off-by: Dylan Van Assche <me@dylanvanassche.be>
> ---
>  Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> index 1ab9588cdd89..fa5b00534b30 100644
> --- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> +++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> @@ -57,6 +57,12 @@ properties:
>        Virtual machine IDs for remote processor.
>      $ref: "/schemas/types.yaml#/definitions/uint32-array"
>  
> +  qcom,assign-all-mem:
> +    description:
> +      Assign memory to all Virtual machines defined by qcom,vmids.

This (neither commit msg) does not explain why this is needed and
actually does not sound like hardware-related property.

> +    type: boolean
> +
> +

Do not add double blank lines.

>    "#address-cells":
>      const: 1
>  

Best regards,
Krzysztof
  

Hi Krzysztof,

On Sun, 2023-03-26 at 10:55 +0200, Krzysztof Kozlowski wrote:
> On 25/03/2023 14:44, Dylan Van Assche wrote:
> > Document the added qcom,assign-all-memory in devicetree bindings.
> > 
> > Signed-off-by: Dylan Van Assche <me@dylanvanassche.be>
> > ---
> >  Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6
> > ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > index 1ab9588cdd89..fa5b00534b30 100644
> > --- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > +++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > @@ -57,6 +57,12 @@ properties:
> >        Virtual machine IDs for remote processor.
> >      $ref: "/schemas/types.yaml#/definitions/uint32-array"
> >  
> > +  qcom,assign-all-mem:
> > +    description:
> > +      Assign memory to all Virtual machines defined by qcom,vmids.
> 
> This (neither commit msg) does not explain why this is needed and
> actually does not sound like hardware-related property.

This is made a separate property to toggle different behavior in the
driver if it is needed for some FastRPC nodes. Downstream does guard
this with a property 'restrict-access' as well, see [1] for a random
SDM845 downstream kernel. On SDM845, this property is not present, thus
the IF block runs. On SDM670, this property is present, then the IF
block is skipped. That's why I opt for this property to have this
behaviour conditionally. I'm not sure how to explain it better though.

Any feedback is appreciated, thanks!

Kind regards,
Dylan Van Assche

[1]
https://github.com/SHIFTPHONES/android_kernel_shift_sdm845/blob/sos-3.x/drivers/char/adsprpc.c#L4615-L4631

> 
> > +    type: boolean
> > +
> > +
> 
> Do not add double blank lines.
> 
> >    "#address-cells":
> >      const: 1
> >  
> 
> Best regards,
> Krzysztof
>
  

On 27/03/2023 13:37, Dylan Van Assche wrote:
> Hi Krzysztof,
> 
> On Sun, 2023-03-26 at 10:55 +0200, Krzysztof Kozlowski wrote:
>> On 25/03/2023 14:44, Dylan Van Assche wrote:
>>> Document the added qcom,assign-all-memory in devicetree bindings.
>>>
>>> Signed-off-by: Dylan Van Assche <me@dylanvanassche.be>
>>> ---
>>>  Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6
>>> ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>>> b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>>> index 1ab9588cdd89..fa5b00534b30 100644
>>> --- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>>> +++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>>> @@ -57,6 +57,12 @@ properties:
>>>        Virtual machine IDs for remote processor.
>>>      $ref: "/schemas/types.yaml#/definitions/uint32-array"
>>>  
>>> +  qcom,assign-all-mem:
>>> +    description:
>>> +      Assign memory to all Virtual machines defined by qcom,vmids.
>>
>> This (neither commit msg) does not explain why this is needed and
>> actually does not sound like hardware-related property.
> 
> This is made a separate property to toggle different behavior in the
> driver if it is needed for some FastRPC nodes. 

Bindings are not for driver behavior.

> Downstream does guard
> this with a property 'restrict-access' as well, see [1] for a random
> SDM845 downstream kernel. On SDM845, this property is not present, thus
> the IF block runs. On SDM670, this property is present, then the IF
> block is skipped. That's why I opt for this property to have this
> behaviour conditionally. I'm not sure how to explain it better though.

Still you described driver... Please come with something more hardware
related.

Best regards,
Krzysztof
  

Hi Krzysztof,

On Mon, 2023-03-27 at 14:22 +0200, Krzysztof Kozlowski wrote:
> On 27/03/2023 13:37, Dylan Van Assche wrote:
> > Hi Krzysztof,
> > 
> > On Sun, 2023-03-26 at 10:55 +0200, Krzysztof Kozlowski wrote:
> > > On 25/03/2023 14:44, Dylan Van Assche wrote:
> > > > Document the added qcom,assign-all-memory in devicetree
> > > > bindings.
> > > > 
> > > > Signed-off-by: Dylan Van Assche <me@dylanvanassche.be>
> > > > ---
> > > >  Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6
> > > > ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > > > b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > > > index 1ab9588cdd89..fa5b00534b30 100644
> > > > --- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > > > +++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > > > @@ -57,6 +57,12 @@ properties:
> > > >        Virtual machine IDs for remote processor.
> > > >      $ref: "/schemas/types.yaml#/definitions/uint32-array"
> > > >  
> > > > +  qcom,assign-all-mem:
> > > > +    description:
> > > > +      Assign memory to all Virtual machines defined by
> > > > qcom,vmids.
> > > 
> > > This (neither commit msg) does not explain why this is needed and
> > > actually does not sound like hardware-related property.
> > 
> > This is made a separate property to toggle different behavior in
> > the
> > driver if it is needed for some FastRPC nodes. 
> 
> Bindings are not for driver behavior.
> 
> > Downstream does guard
> > this with a property 'restrict-access' as well, see [1] for a
> > random
> > SDM845 downstream kernel. On SDM845, this property is not present,
> > thus
> > the IF block runs. On SDM670, this property is present, then the IF
> > block is skipped. That's why I opt for this property to have this
> > behaviour conditionally. I'm not sure how to explain it better
> > though.
> 
> Still you described driver... Please come with something more
> hardware
> related.

So just updating the description is enough then?

As this is all reverse engineered, I have no access to the
documentation of FastRPC, so best effort:

"""
Mark allocated memory region accessible to remote processor.
This memory region is used by remote processor to communicate
when no dedicated Fastrpc context bank hardware is available 
for remote processor.
"""

Is this the description that is 'more hardware related'?

Kind regards,
Dylan Van Assche

> 
> Best regards,
> Krzysztof
>
  

On Mon, 27 Mar 2023 at 17:27, Dylan Van Assche <me@dylanvanassche.be> wrote:
>
> Hi Krzysztof,
>
> On Mon, 2023-03-27 at 14:22 +0200, Krzysztof Kozlowski wrote:
> > On 27/03/2023 13:37, Dylan Van Assche wrote:
> > > Hi Krzysztof,
> > >
> > > On Sun, 2023-03-26 at 10:55 +0200, Krzysztof Kozlowski wrote:
> > > > On 25/03/2023 14:44, Dylan Van Assche wrote:
> > > > > Document the added qcom,assign-all-memory in devicetree
> > > > > bindings.
> > > > >
> > > > > Signed-off-by: Dylan Van Assche <me@dylanvanassche.be>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6
> > > > > ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > > > > b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > > > > index 1ab9588cdd89..fa5b00534b30 100644
> > > > > --- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > > > > +++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> > > > > @@ -57,6 +57,12 @@ properties:
> > > > >        Virtual machine IDs for remote processor.
> > > > >      $ref: "/schemas/types.yaml#/definitions/uint32-array"
> > > > >
> > > > > +  qcom,assign-all-mem:
> > > > > +    description:
> > > > > +      Assign memory to all Virtual machines defined by
> > > > > qcom,vmids.
> > > >
> > > > This (neither commit msg) does not explain why this is needed and
> > > > actually does not sound like hardware-related property.
> > >
> > > This is made a separate property to toggle different behavior in
> > > the
> > > driver if it is needed for some FastRPC nodes.
> >
> > Bindings are not for driver behavior.
> >
> > > Downstream does guard
> > > this with a property 'restrict-access' as well, see [1] for a
> > > random
> > > SDM845 downstream kernel. On SDM845, this property is not present,
> > > thus
> > > the IF block runs. On SDM670, this property is present, then the IF
> > > block is skipped. That's why I opt for this property to have this
> > > behaviour conditionally. I'm not sure how to explain it better
> > > though.
> >
> > Still you described driver... Please come with something more
> > hardware
> > related.
>
> So just updating the description is enough then?
>
> As this is all reverse engineered, I have no access to the
> documentation of FastRPC, so best effort:

Vendor kernels put a lot of controls into DT, despite some of these
controls being related to software or being a platform constant.
Upstream tends to push some of the constraints into the driver data,
leaving only variadic parts in DT.
Could you please check if the property you are proposing is constant
among the devices on a platform or not. If it is a platform
peculiarity, a usual way is to add it to driver data rather than the
DT.

>
> """
> Mark allocated memory region accessible to remote processor.
> This memory region is used by remote processor to communicate
> when no dedicated Fastrpc context bank hardware is available
> for remote processor.
> """
>
> Is this the description that is 'more hardware related'?
  

On 27/03/2023 16:26, Dylan Van Assche wrote:
>> Bindings are not for driver behavior.
>>
>>> Downstream does guard
>>> this with a property 'restrict-access' as well, see [1] for a
>>> random
>>> SDM845 downstream kernel. On SDM845, this property is not present,
>>> thus
>>> the IF block runs. On SDM670, this property is present, then the IF
>>> block is skipped. That's why I opt for this property to have this
>>> behaviour conditionally. I'm not sure how to explain it better
>>> though.
>>
>> Still you described driver... Please come with something more
>> hardware
>> related.
> 
> So just updating the description is enough then?
> 
> As this is all reverse engineered, I have no access to the
> documentation of FastRPC, so best effort:
> 
> """
> Mark allocated memory region accessible to remote processor.
> This memory region is used by remote processor to communicate
> when no dedicated Fastrpc context bank hardware is available 
> for remote processor.

This description does not explain here anything. The memory region is
already accessible without this property.

You described the desired Linux feature or behavior, not the actual
hardware. The bindings are about the latter, so instead you need to
rephrase the property and its description to match actual hardware
capabilities/features/configuration etc.

Remember that any arguments to downstream are not really good arguments.
Their design choices and bindings are usually totally not acceptable.
They simply embed whatever driver needs in DT - policies, system
configuration, driver behavior...

Also, Dmitry made here good point.



Best regards,
Krzysztof
  

Hi Dmitry & Krzysztof,

On Mon, 2023-03-27 at 16:36 +0200, Krzysztof Kozlowski wrote:
> On 27/03/2023 16:26, Dylan Van Assche wrote:
> > > Bindings are not for driver behavior.
> > > 
> > > > Downstream does guard
> > > > this with a property 'restrict-access' as well, see [1] for a
> > > > random
> > > > SDM845 downstream kernel. On SDM845, this property is not
> > > > present,
> > > > thus
> > > > the IF block runs. On SDM670, this property is present, then
> > > > the IF
> > > > block is skipped. That's why I opt for this property to have
> > > > this
> > > > behaviour conditionally. I'm not sure how to explain it better
> > > > though.
> > > 
> > > Still you described driver... Please come with something more
> > > hardware
> > > related.
> > 
> > So just updating the description is enough then?
> > 
> > As this is all reverse engineered, I have no access to the
> > documentation of FastRPC, so best effort:
> > 
> > """
> > Mark allocated memory region accessible to remote processor.
> > This memory region is used by remote processor to communicate
> > when no dedicated Fastrpc context bank hardware is available 
> > for remote processor.
> 
> This description does not explain here anything. The memory region is
> already accessible without this property.
> 
> You described the desired Linux feature or behavior, not the actual
> hardware. The bindings are about the latter, so instead you need to
> rephrase the property and its description to match actual hardware
> capabilities/features/configuration etc.
> 
> Remember that any arguments to downstream are not really good
> arguments.
> Their design choices and bindings are usually totally not acceptable.
> They simply embed whatever driver needs in DT - policies, system
> configuration, driver behavior...
> 
> Also, Dmitry made here good point.
> 
> 

I agree, downstream is not doing great on being upstreamable.
Thanks Dmitry, your explanation makes it pretty clear what I should
figure out. This helps a lot! As far as I know, this assignment is only
skipped when the sensors are not on the SLPI but on the ADSP e.g.
SDM670, thus mid range SoCs. So reading these comments, this looks more
like 'driver behavior' which should not end up in bindings as mentioned
above. As I now understand the problem with this property, I will
rework it for v2 and drop it. This is only done for the SLPI so by
guarding it with a domain ID check we should be able to avoid the
property in the bindings.

Thanks for the feedback & patience! I really learned a lot!

Kind regards,
Dylan Van Assche

> 
> Best regards,
> Krzysztof
>