Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754018AbdIGGix (ORCPT ); Thu, 7 Sep 2017 02:38:53 -0400 Received: from szxga01-in.huawei.com ([45.249.212.187]:11762 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752775AbdIGGiv (ORCPT ); Thu, 7 Sep 2017 02:38:51 -0400 From: "liwei (CM)" To: Arnd Bergmann CC: 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 , "devicetree@vger.kernel.org" , Linux Kernel Mailing List , Linux ARM , linux-scsi , Guodong Xu , "Fengbaopeng (kevin, Kirin Solution Dept)" Subject: =?utf-8?B?562U5aSNOiBbUEFUQ0ggdjMgMS81XSBzY3NpOiB1ZnM6IGFkZCBIaXNpbGlj?= =?utf-8?Q?on_ufs_driver_code?= Thread-Topic: [PATCH v3 1/5] scsi: ufs: add Hisilicon ufs driver code Thread-Index: AQHTJ2ID2Yza+aZfm06QCvj03UdQWKKop71A Date: Thu, 7 Sep 2017 06:34:19 +0000 Message-ID: <1699CE87DE933F49876AD744B5DC140FA18C49@DGGEMM506-MBX.china.huawei.com> References: <20170829084116.106695-1-liwei213@huawei.com> <20170829084116.106695-2-liwei213@huawei.com> In-Reply-To: Accept-Language: zh-CN, en-US Content-Language: zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.189.155.72] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A010201.59B0E877.0039,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=169.254.3.138, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 8a1364aa1d6e9c4380e7c4214edef206 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id v876cvGs001815 Content-Length: 3565 Lines: 90 Hi, Arnd Thanks for your suggestions, I hope you'll reply again: -----邮件原件----- 发件人: arndbergmann@gmail.com [mailto:arndbergmann@gmail.com] 代表 Arnd Bergmann 发送时间: 2017年9月7日 6:47 收件人: 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; devicetree@vger.kernel.org; Linux Kernel Mailing List; Linux ARM; linux-scsi; Guodong Xu; Fengbaopeng (kevin, Kirin Solution Dept) 主题: Re: [PATCH v3 1/5] scsi: ufs: add Hisilicon ufs driver code On Tue, Aug 29, 2017 at 10:41 AM, Li Wei wrote: itel(host, UFS_ARESET, PERRSTDIS3_OFFSET); > + > + /* disable lp_reset_n */ > + ufs_sys_ctrl_set_bits(host, BIT_SYSCTRL_LP_RESET_N, RESET_CTRL_EN); > + mdelay(1); > + > + if (gpio_is_valid(host->reset_gpio)) > + gpio_direction_output(host->reset_gpio, 1); > + > + ufs_sys_ctrl_writel(host, MASK_UFS_DEVICE_RESET | BIT_UFS_DEVICE_RESET, > + UFS_DEVICE_RESET_CTRL); > + > + mdelay(20); Could those mdelay() be turned into msleep() functions? I will fix it in patch v4. > +static int ufs_hisi_get_resource(struct ufs_hisi_host *host) { > + struct resource *mem_res; > + struct device_node *np = NULL; > + struct device *dev = host->hba->dev; > + struct platform_device *pdev = to_platform_device(dev); > + > + /* get resource of ufs sys ctrl */ > + mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + host->ufs_sys_ctrl = devm_ioremap_resource(dev, mem_res); > + if (IS_ERR(host->ufs_sys_ctrl)) > + return PTR_ERR(host->ufs_sys_ctrl); > + > + np = of_find_compatible_node(NULL, NULL, > + "hisilicon,hi3660-crgctrl"); It's generally not a good idea to look up one device by its "compatible" string. What is the "crgctrl"? Does it have a proper DT binding? Maybe there should be a driver for it, or you could make it a "syscon" device and look it up by phandle instead. ok, crgctrl is our common register, if look up device by its "compatible" is not appropriate, I will add a properties in ufs node, like this: ufs: ufs@ff3b0000 { compatible = "jedec,ufs-1.1", "hisilicon,hi3660-ufs"; /* 0: HCI standard */ /* 1: UFS SYS CTRL */ reg = <0x0 0xff3b0000 0x0 0x1000>, <0x0 0xff3b1000 0x0 0x1000>; interrupt-parent = <&gic>; interrupts = ; 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"; } And find that by, is it OK? + host->rst = devm_reset_control_get(dev, "rst"); + host->assert = devm_reset_control_get(dev, "assert"); > diff --git a/drivers/scsi/ufs/ufs-hisi.h b/drivers/scsi/ufs/ufs-hisi.h > new file mode 100644 index 000000000000..52430a2aca90 > --- /dev/null > +++ b/drivers/scsi/ufs/ufs-hisi.h If the header is only used in one file, you don't need it, just move the definitions into the other file. Currently only one file use ufs-hisi.h, but I think so many definitions are defined in a .h file is more clearer, like ufs-qcom.h. If you think it isn't necessary, I will move it into ufs-hisi.c? Arnd