Re: [<PATCH v1> 1/1] scsi: qcom-ufs: Add support for bus voting using ICB framework

[Georgi Djakov]

Hi Asutosh,

On 1/25/19 12:27, Asutosh Das (asd) wrote:
> On 1/24/2019 5:16 PM, Georgi Djakov wrote:
[..]>>> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>>> b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>>> index a99ed55..94249ef 100644
>>> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>>> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>>> @@ -45,6 +45,18 @@ Optional properties:
>>> Â Note: If above properties are not defined it can be assumed that
>>> the supply
>>> Â regulators or clocks are always on.
>>> Â +* Following bus parameters are required:
>>> +interconnects
>>> +interconnect-names
>>
>> Is the above really required? Are the interconnect bandwidth requests
>> required to enable something critical to UFS functionality?
>> Would UFS still work without any bandwidth scaling, although for example
>> slower? Could you please clarify.
> Yes - UFS will still work without any bandwidth scaling - but the
> performance would be terrible.

Ok, thanks for clarifying! Then the properties should be optional. Maybe
we can also mention in the commit text how much the performance would
improve with this patch.

>>
>>> +- Please refer to Documentation/devicetree/bindings/interconnect/
>>> +Â for more details on the above.
>>> +qcom,msm-bus,name - string describing the bus path
>>> +qcom,msm-bus,num-cases - number of configurations in which ufs can
>>> operate in
>>> +qcom,msm-bus,num-paths - number of paths to vote for
>>> +qcom,msm-bus,vectors-KBps - Takes a tuple <ib ab>, <ib ab> (2 tuples
>>> for 2 num-paths)
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ The number of these entries *must* be same as
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ num-cases.
>>
>> DT bindings should be submitted as a separate patch. Anyway, people
>> frown upon putting configuration data in DT. Could we put this data into
>> the driver as a static table instead of DT? Also maybe use ab/pb for
>> average/peak bandwidth.
> The ab/ib value change depending on the target - that's the reasoning
> for putting it in dts file. However, I'm open to ideas as to how else to
> handle this.

As Evan already suggested, it would be best if we can calculate the
bandwidth. Can we do that based on the number of lanes, clock rate and
ufs standard version?
If calculating is really not possible and we have strong arguments for
that, we could add a more specific compatible DT string - for example
qcom,sdm845-ufshc and use per SoC bandwidth tables.

Thanks,
Georgi