From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756995AbcBCPjs (ORCPT ); Wed, 3 Feb 2016 10:39:48 -0500 Received: from mout.kundenserver.de ([212.227.126.133]:50209 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756394AbcBCPjp (ORCPT ); Wed, 3 Feb 2016 10:39:45 -0500 From: Arnd Bergmann To: Joao Pinto Cc: santosh.sy@samsung.com, h.vinayak@samsung.com, julian.calaby@gmail.com, akinobu.mita@gmail.com, hch@infradead.org, gbroner@codeaurora.org, subhashj@codeaurora.org, CARLOS.PALMINHA@synopsys.com, ijc+devicetree@hellion.org.uk, linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org, devicetree@vger.kernel.org Subject: Re: [PATCH 3/3] add support for DWC UFS Host Controller Date: Wed, 03 Feb 2016 16:39:10 +0100 Message-ID: <2087303.0ORDEBS3hn@wuerfel> User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; ) In-Reply-To: <56B2164E.5060007@synopsys.com> References: <2072510.CA47OHQsUN@wuerfel> <56B2164E.5060007@synopsys.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:P08xEPzfkEj4KqmbCmdVtHwnQeSis2XvUvjwMk5O6MYkcsq85BZ IIVk4cCr8fd3T83dl2Nq9v1NdjB0EKddl7tzYWkSZfHwwkrYgZNwsT5e2sg99T5e1D136UQ EKtr6QoL4smkaCd3GuI8PDYpPnW3TqpBfPwkFgR9IaOJzCxZpi1zUIz25AguxUb5osy/m30 6nr/MYcwExXAmLq6sJ//g== X-UI-Out-Filterresults: notjunk:1;V01:K0:CJJnomM3n4A=:ruYm9h+lW0SVL/QVfYrCQl o9Oiah7OdJDCgtPDn0D2arM2BWkq0hngWUaiBwirSM0P8aavHJU+7+TMbokA3ky2CGKQVxmLO /Y6zwEZwdmj4/FkHNvfowK0QZBDwqAVAwyHCx/YnLOfFaUHG7BI+fHyKPzepEXBLBX0m7yGCg pQZWdHFN7g24fc19MRyqdhuktkIJuF9B4e79kwZ3CazY3YBXMYeuBThE77udn0FHzTFHJITCm CraCcV50Q0euNxLrWBl9Lu8QJ2mo4/+u9xc9CyS8v0C+Xn0kQONpzjsMMmcFe4QZLjP+mrH+D KP2e1SWUA1zcmzPmipZ5V1f7tjZeassmdjRVq209rq3MLLMBJ9K8tKJKFw7BhgM/mkT37XXET QGNLx979RXIGMcEdDEysdwP9HtP9kcPnTh167ElmQ7BlOlkTHCFSYO2cnbg00EKROhulDIB/+ pGNGQC6ziECHiQ5aIDcQtTfA0N9sdTeNhywFNKBq03vjOg9lSCrOqC7Lr251TSjzQRpTvnOkV 3UA4qu2vfyfBTZtIC6xzVPhu70JCsrV9zyfAv1jkehlTh70xm/8Qfu6Acnrq22q6q5qDOfTZh TLYEw3qRzQjPZWffm9DoW6bXJ2IM/NwJfXQ2538pvJXSbREtVaLWIMm/hyzp75McaejAPSThP XrJKIUP9DK/BZTR5NxOknc1lO+sej47YYf3Ums1ZxwJxJSgdwPRVPiKLY19MVcfp/IQyiM3hq NtcbYHiWq+Dw7ykx Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wednesday 03 February 2016 15:01:34 Joao Pinto wrote: > > Hi Arnd, > > On 2/3/2016 12:54 PM, Arnd Bergmann wrote: > > On Wednesday 03 February 2016 11:28:26 Joao Pinto wrote: > >> > >> Signed-off-by: Joao Pinto > > > > This needs a changelog comment, like every patch. > > > >> @@ -0,0 +1,16 @@ > >> +* Universal Flash Storage (UFS) DesignWare Host Controller > >> + > >> +DWC_UFSHC nodes are defined to describe on-chip UFS host controllers. > >> +Each UFS controller instance should have its own node. > >> + > >> +Required properties: > >> +- compatible : compatible list, contains "snps,ufshcd" > > > > Are there multiple versions of this controller? Usually for designware > > parts the version is known, so we should document which versions exist > > This controller recent releases was 2.0, but we released last year 1.1. The > driver works with both. The driver must work with all DWC UFS versions. Ok, then make the driver match on the "snps,ufshcd-1.1" compatible string, but document both strings in the binding document, and make it mandatory to specify the 1.1 version as a compatible fallback. If we ever need to handle a quirk for the 2.0 version then, it can easily be done. > >> +config SCSI_UFS_DWC_HOOKS > >> + bool "DesignWare hooks to UFS controller" > >> + depends on SCSI_UFSHCD > >> + ---help--- > >> + This selects the DesignWare hooks for the UFS host controller. > >> + > >> + Select this if you have a DesignWare UFS controller. > >> + If unsure, say N. > > > > This could be a silent symbol that gets selected by SCSI_UFS_DWC_PLAT > > We could do that, but imagine that we select SCSI_UFS_QCOM, then the synopsys > hooks would be selected also which in my opinion is not very accurate. > In my opinion we should have a selectable DWC_HOOKS. I don't understand. At the moment, you can enable SCSI_UFS_DWC_HOOKS even if nothing uses it and you only have SCSI_UFS_QCOM enabled. With my suggestion, the hooks would disappear unless they are actually used. Then again, with my later comments, we no longer need the hooks. > >> +/** > >> + * ufshcd_dwc_setup_mphy() > >> + * This function configures Local (host) Synopsys MPHY specific attributes > >> + * > >> + * @hba: Pointer to drivers structure > >> + * > >> + * Returns 0 on success non-zero value on failure > >> + */ > >> +int ufshcd_dwc_setup_mphy(struct ufs_hba *hba) > >> +{ > >> + int ret = 0; > >> + > >> +#ifdef CONFIG_SCSI_UFS_DWC_40BIT_RMMI > >> + dev_info(hba->dev, "Configuring MPHY 40-bit RMMI"); > >> + ret = ufshcd_dwc_setup_40bit_rmmi(hba); > >> + if (ret) { > >> + dev_err(hba->dev, "40-bit RMMI configuration failed"); > >> + goto out; > >> + } > >> +#else > >> +#ifdef CONFIG_SCSI_UFS_DWC_20BIT_RMMI > >> + dev_info(hba->dev, "Configuring MPHY 20-bit RMMI"); > >> + ret = ufshcd_dwc_setup_20bit_rmmi(hba); > >> + if (ret) { > >> + dev_err(hba->dev, "20-bit RMMI configuration failed"); > >> + goto out; > >> + } > >> +#endif > >> +#endif > >> + /* To write Shadow register bank to effective configuration block */ > >> + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_MPHYCFGUPDT), 0x01); > >> + if (ret) > >> + goto out; > >> + > >> + /* To configure Debug OMC */ > >> + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_DEBUGOMC), 0x01); > >> + > >> +out: > >> + return ret; > >> +} > > > > Try to use the generic PHY abstraction here and remove all the #ifdef etc. > > Could you please point an example for me to check? drivers/phy/phy-qcom-ufs-qmp-14nm.c is a phy driver, and it gets used through the generic devm_phy_get()/phy_power_on()/phy_power_off()/... interfaces. This should probably be moved into the generic UFS platform driver so the PHY handling can be shared between all backends. > >> }; > > > > I think you're better off with a separate PCI driver for this. Remove > > all the #ifdef mess here, put whatever is dwc specific into a new file, > > and perhaps move the common parts into a shared file that can be used > > by both the samsung and designware drivers. > > I have a branch with that approach, but honestly it would be a big change in the > UFS arch for the pci and I decided to make it simple. I sent that suggestion for > the scsi mailing list and the comments showed me that. Does anyone have anything > against putting ufshcd-pci.c as a pci common code and then have a ufs-dwc-pci.c > and a ufs-samsung-pci.c that uses that common code? Another approach would be to just rename the existing file to ufs-samsung-pci.c and start the ufs-dwc-pci.c as a copy of that. The file is not really all that large anyway. Arnd