Message-ID: <CAK8P3a37yYDQZpZxmunCdSO83qkqywTME5Xo0k56nWEs2k+=dg@mail.gmail.com>
Date:   Mon, 30 Oct 2017 16:21:55 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     "liwei (CM)" <liwei213@...wei.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        "xuwei (O)" <xuwei5@...ilicon.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Vinayak Holikatti <vinholikatti@...il.com>,
        "James E.J. Bottomley" <jejb@...ux.vnet.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Kevin Hilman <khilman@...libre.com>,
        Gregory CLEMENT <gregory.clement@...e-electrons.com>,
        Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Riku Voipio <riku.voipio@...aro.org>,
        Thierry Reding <treding@...dia.com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Eric Anholt <eric@...olt.net>,
        DTML <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        linux-scsi <linux-scsi@...r.kernel.org>,
        Guodong Xu <guodong.xu@...aro.org>,
        "Fengbaopeng (kevin, Kirin Solution Dept)" 
        <fengbaopeng@...ilicon.com>, "lihuan (Z)" <lihuan41@...ilicon.com>,
        "wangyupeng (A)" <wangyupeng4@...ilicon.com>
Subject: Re: 答复: [PATCH v5 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs

On Tue, Oct 24, 2017 at 11:06 AM, liwei (CM) <liwei213@...wei.com> 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@...il.com [mailto:arndbergmann@...il.com] 代表 Arnd Bergmann
> On Fri, Oct 20, 2017 at 10:52 AM, Li Wei <liwei213@...wei.com> 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

Powered by blists - more mailing lists