Re: çå: [PATCH v5 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs

[Arnd Bergmann]

On Tue, Oct 24, 2017 at 11:06 AM, liwei (CM) <liwei213@xxxxxxxxxx> wrote:
> what's your opinion about my explanation and revision method?
> I am looking forward to your reply, thanks!

Sorry for the delay, I was travelling last week.
> åää: arndbergmann@xxxxxxxxx [mailto:arndbergmann@xxxxxxxxx] äè Arnd Bergmann
> On Fri, Oct 20, 2017 at 10:52 AM, Li Wei <liwei213@xxxxxxxxxx> wrote:
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
>> @@ -0,0 +1,46 @@
>> +* Hisilicon Universal Flash Storage (UFS) Host Controller
>> +
>> +UFS nodes are defined to describe on-chip UFS hardware macro.
>> +Each UFS Host Controller should have its own node.
>> +
>> +Required properties:
>> +- compatible        : compatible list, contains one of the following -
>> +                       "hisilicon,hi3660-ufs" for hisi ufs host controller
>> +                        present on Hi3660 chipset.

One more thing I just noticed: you don't describe the device as compatible with
the ufshcd spec as defined in the generic binding. Shouldn't we have
both compatible
strings listed here?

> In particular, I wonder if what you describe as the "UFS SYS CTRL"
> area corresponds to what Qualcomm have described as their PHY implementation. It certainly seems to driver some of the properties that would normally be associated with a PHY.
>
> Liwei:Yes, a part of "UFS SYS CTRL" is associated with a PHY, but from our chip colleague that we assure "UFS SYS CTRL" is associated with clk/reset/power on/power off and so on.
> In fact, in addition to the controller itself, the controller related periphery are all in this area. So it's not appropriate to put this into a separate phy node.

I'm not sure I understand here. Do you mean the reset handle is for
resetting the PHY rather than the UFS HCD?

> > For the "clock-names" property, you specify "clk_ref", which I assume is the same as what Qualcomm call "ref_clk". I'd suggest you use the existing name and add that as the default name in the ufshcd-pltfrm.txt binding document.
>
> Liwei:" ref_clk " is already in the ufshcd-pltfrm.txt binding document, and parse in ufshcd.c, so we will replace "clk_ref" with "ref_clk". I will fix it in patch v6;

ok

> > The "clk_phy" property appears to be related to the PHY, so it might be better to have a separate phy node with either just the clk, or with the clk plus the "UFS SYS CTRL" register area, whichever matches your hardware better, and then use teh "phys/phy-names" property to refer to that.
>
> Liwei: OK, I will add a separate phy node and fix it in patch v6;

Thanks.

>> The reset handling you describe here (both resets and reset-gpios) appears to be completely generic, so I'd suggest adding those to ufshcd-pltfrm.txt instead of your own binding, to ensure that future drivers use the same identifiers.
>
> Liwei: From our soc chip colleague, reset include "rst", "assert" is not generic and related with our soc
> implementation, you can see ufs_hisi_soc_init() in drivers/scsi/ufs/ufs-hisi.c, the position of rst and assert
> is very special, it's hard to put it in a generic process;

It seems odd that the ability to reset a device is specific to your
implementation. What I meant is
that other implementations may also require a reset, so describing the
way you refer to this
into the generic binding would be helpful, to prevent others from
adding another incompatible
way to do the same thing.

A lot of generic bindings have a reference to a reset controller that
if present needs to
be used before first accessing the device itself.

> reset-gpios is used to solve a defect of the SOC chip reset function and it is not generic , but our chip has been updated, so this is no longer needed, and I will remove it in the patch v6;

Ok.

      Arnd