| Message ID | 20171020085259.8093-3-liwei213@huawei.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | scsi: ufs: add ufs driver code for Hisilicon Hi3660 SoC | expand |
On Fri, Oct 20, 2017 at 10:52 AM, Li Wei <liwei213@huawei.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. > +- reg : should contain UFS register address space & UFS SYS CTRL register address, > +- interrupt-parent : interrupt device > +- interrupts : interrupt number > +- clocks : List of phandle and clock specifier pairs > +- clock-names : List of clock input name strings sorted in the same > + order as the clocks property. "clk_ref", "clk_phy" is optional > +- resets : reset node register, one reset the clk and the other reset the controller > +- reset-names : describe reset node register I think I've asked about this before, but I think this should be done more consistently with the other UFS bindings. 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. 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. 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. 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. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Arnd Sorry to bother you, what's your opinion about my explanation and revision method? I am looking forward to your reply, thanks! -----邮件原件----- 发件人: liwei (CM) 发送时间: 2017年10月21日 17:59 收件人: 'Arnd Bergmann' 抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; Vinayak Holikatti; James E.J. Bottomley; Martin K. Petersen; Kevin Hilman; Gregory CLEMENT; Thomas Petazzoni; Masahiro Yamada; Riku Voipio; Thierry Reding; Krzysztof Kozlowski; Eric Anholt; DTML; Linux Kernel Mailing List; Linux ARM; linux-scsi; Guodong Xu; Fengbaopeng (kevin, Kirin Solution Dept); lihuan (Z); wangyupeng (A) 主题: 答复: [PATCH v5 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs Hi, Bergmann Sorry late for the reply,and thank you very much for your patience. My reply is as follows. I look forward to your further reply. -----邮件原件----- 发件人: arndbergmann@gmail.com [mailto:arndbergmann@gmail.com] 代表 Arnd Bergmann 发送时间: 2017年10月20日 17:16 收件人: liwei (CM) 抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; Vinayak Holikatti; James E.J. Bottomley; Martin K. Petersen; Kevin Hilman; Gregory CLEMENT; Thomas Petazzoni; Masahiro Yamada; Riku Voipio; Thierry Reding; Krzysztof Kozlowski; Eric Anholt; DTML; Linux Kernel Mailing List; Linux ARM; linux-scsi; Guodong Xu; Fengbaopeng (kevin, Kirin Solution Dept); lihuan (Z); wangyupeng (A) 主题: Re: [PATCH v5 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs On Fri, Oct 20, 2017 at 10:52 AM, Li Wei <liwei213@huawei.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. > +- reg : should contain UFS register address space & UFS SYS CTRL register address, > +- interrupt-parent : interrupt device > +- interrupts : interrupt number > +- clocks : List of phandle and clock specifier pairs > +- clock-names : List of clock input name strings sorted in the same > + order as the clocks property. "clk_ref", "clk_phy" is optional > +- resets : reset node register, one reset the clk and the other reset the controller > +- reset-names : describe reset node register I think I've asked about this before, but I think this should be done more consistently with the other UFS bindings. 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. 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; 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; 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; 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; Thanks! Arnd
On Tue, Oct 24, 2017 at 11:06 AM, liwei (CM) <liwei213@huawei.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@gmail.com [mailto:arndbergmann@gmail.com] 代表 Arnd Bergmann > On Fri, Oct 20, 2017 at 10:52 AM, Li Wei <liwei213@huawei.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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/ufs/ufs-hisi.txt b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt new file mode 100644 index 000000000000..17da633c9ed5 --- /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. +- reg : should contain UFS register address space & UFS SYS CTRL register address, +- interrupt-parent : interrupt device +- interrupts : interrupt number +- clocks : List of phandle and clock specifier pairs +- clock-names : List of clock input name strings sorted in the same + order as the clocks property. "clk_ref", "clk_phy" is optional +- resets : reset node register, one reset the clk and the other reset the controller +- reset-names : describe reset node register + +Optional properties for board device: +- reset-gpios : specifies to reset devices + +Example: + + ufs: ufs@ff3b0000 { + compatible = "hisilicon,hi3660-ufs", "jedec,ufs-1.1"; + /* 0: HCI standard */ + /* 1: UFS SYS CTRL */ + reg = <0x0 0xff3b0000 0x0 0x1000>, + <0x0 0xff3b1000 0x0 0x1000>; + interrupt-parent = <&gic>; + interrupts = <GIC_SPI 278 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&crg_ctrl HI3660_CLK_GATE_UFSIO_REF>, + <&crg_ctrl HI3660_CLK_GATE_UFSPHY_CFG>; + clock-names = "clk_ref", "clk_phy"; + freq-table-hz = <0 0>, <0 0>; + /* offset: 0x84; bit: 12 */ + /* offset: 0x84; bit: 7 */ + resets = <&crg_rst 0x84 12>, + <&crg_rst 0x84 7>; + reset-names = "rst", "assert"; + }; + + &ufs { + reset-gpios = <&gpio18 1 0>; + }; +
add ufs node document for Hisilicon. Signed-off-by: Li Wei <liwei213@huawei.com> --- Documentation/devicetree/bindings/ufs/ufs-hisi.txt | 46 ++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 Documentation/devicetree/bindings/ufs/ufs-hisi.txt