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
