From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751770AbdB1IYa (ORCPT ); Tue, 28 Feb 2017 03:24:30 -0500 Received: from mailout4.samsung.com ([203.254.224.34]:50496 "EHLO mailout4.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751332AbdB1IY0 (ORCPT ); Tue, 28 Feb 2017 03:24:26 -0500 X-AuditID: b6c32a2c-f79b56d0000012f0-16-58b533b1b36e Subject: Re: [PATCH v5 02/11] phy: exynos-ufs: add UFS PHY driver for EXYNOS SoC To: Kishon Vijay Abraham I , Alim Akhtar Cc: linux-scsi@vger.kernel.org, "linux-kernel@vger.kernel.org" , "James E.J. Bottomley" , vinayak holikatti , Jaehoon Chung , Seungwon Jeon , "devicetree@vger.kernel.org" , Pankaj Dubey , vivek.gautam@codeaurora.org From: Alim Akhtar Message-id: <12decf59-0486-9efd-63de-12fee95a8cea@samsung.com> Date: Tue, 28 Feb 2017 13:51:04 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-version: 1.0 In-reply-to: <58B4EFC8.2060300@ti.com> Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA01Se0gTcRznd3e73aaTcz76aq1gYaHmixIuUoswmPVPGdY0okZeavlqU1GD EiU1naaQr2W0xKVOzRQf04jyXVAyUnFZk0gLsmaZUmqBuZ2W/31e3xd8KVzcyXOn4hJTWGWi Il5KConOfk8vn9a9HXL/sSY+o5u8ytwbGOExdcZajFlteUswppVcHmP8qCaZ0Z5qkimcMJBM TccinzHcz2SeLxvwQ3ay0eIiTNatMfNl/UtLuKy4XY9kw6YuTLbQtv04GSUMimbj49JYpV/I eWFs7bQWSy6MSH/wvgbPQuVHCpCAAnof1Be28TjsCsapFrIACSkxrUOgNX1fJz8QNLwxY/9I c+Mob6O8av4djzO6EVTqc3GOWBBM333Nt6ac6HAw9y6SVuxMn4D8+QZbCKfVOIzr2zCrQdLe YK5ot2ERHQLNFUZbMUF7gLnwJbJiF1oOBpOa5DKO8KJqhrBiAb0LstUrNh2nd0CXpdo2AOhf JDyse7VGqDUSCk/LErm1nWB2uJ3PYXf4fCuXz+XzEEyVzPE4UongZrMF41IH4dlYNWFthNOe 0NLjx8nB0D34leD6O4DJ4sjt4ABFv2fWKz0gZ258PSKC/FwxJ8tg9ImWKEFSzaZrNJsu0Pyf pUW4HrmyyaqEGFYVmBzgq1IkqFITY3wvJCW0Ids3ee0xoCntsT5EU0hqL6ova5eLeYo0VUZC HwIKlzqL8pfXJFG0IiOTVSadU6bGs6o+tJUipFtEUf6lcjEdo0hhL7NsMqvccDFK4J6FTrua SgdNYRfVt8OChFPegkhdmuQO5bxzv0+vQRKcQunTv6Tbx43EtLqd0ekmoiNFea1DPX8+9Tms Pp6vPDZwwO7a0W2PwroJydClxiuBNcJOycJJMmJ3fWas4sapcErtdlaAhRp/ztISO6fg8g8u XcOT35qW7DXZ/MrrVfWHc6SEKlYR4IUrVYq/Yc1v9EkDAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFuplkeLIzCtJLcpLzFFi42LZdlhJXneq8dYIg9b9lhZLb1VbzD9yjtVi +YUlTBb/199msbjxq43V4sLTHjaLy7vmsFl0X9/BZrFo6xd2ix0LqyxO/NzB7MDtcbmvl8lj 56y77B6Hf/xg9ujbsorR4/iN7UwenzfJBbBFcdmkpOZklqUW6dslcGUsebyAqaA7tGLZg0XM DYzTXLsYOTkkBEwkZn68wwphi0lcuLeerYuRi0NIYDujxNH+TkaIhLrE3Ouv2CBsYYmV/56z QxS9ZpQ40jmDBSQhLBAkcffgF7AiEYFAiVt370EV9bNILP/2mBHEYRboY5Y4+GY/E0gVm4C2 xN3pW8BsXgE7ibXTL7CD2CwCqhJ3u8+ArRYViJCY/3QVVI2gxMmZT8C2cQqoSTT1/ALbxixg JjFv80NmCFteYvvbOcwTGIVmIWmZhaRsFpKyBYzMqxhFUwuKc9NziwsM9YoTc4tL89L1kvNz NzGCI0krcQfjuhnhhxgFOBiVeHgPrNoSIcSaWFZcmXuIUYKDWUmEt+MnUIg3JbGyKrUoP76o NCe1+BCjNAeLkjjvreoNEUIC6YklqdmpqQWpRTBZJg5OqQZGiwWajye7uF7O3FQ9Y5Vuw8XX oj1xnafOf/Mx7ruipBsgnPVuHbvRN27O2NtNdRWMO4sa8w9Z10beFs/f/eqL7L6j4n0FUSrS a6aZenYcr/T/+VPm/92AJ+qNPMsDt1/tVWzbu8WS7/fNJYeUboV5HJC+PveI00rBOw+KF0by zH7DdM66mmWxEktxRqKhFnNRcSIAA6fPjaACAAA= X-CMS-MailID: 20170228082416epcas5p3751abda15e487d0e2b6154cd4127ab13 X-Msg-Generator: CA X-Sender-IP: 182.195.40.13 X-Local-Sender: =?UTF-8?B?7JWM66a8G1NTSVItVHVybiBLZXkgU29sdXRpb25zG+yCvA==?= =?UTF-8?B?7ISx7KCE7J6QGy4vU2VuaW9yIENoaWVmIEVuZ2luZWVy?= X-Global-Sender: =?UTF-8?B?QUxJTSBBS0hUQVIbU1NJUi1UdXJuIEtleSBTb2x1dGlvbnMb?= =?UTF-8?B?U2Ftc3VuZyBFbGVjdHJvbmljcxsuL1NlbmlvciBDaGllZiBFbmdpbmVlcg==?= X-Sender-Code: =?UTF-8?B?QzEwG1NXQUhRG0MxMElEMDdJRDAxMDk5Nw==?= Content-type: text/plain; charset=utf-8 X-MTR: 20170228082416epcas5p3751abda15e487d0e2b6154cd4127ab13 X-EPHeader: CA CMS-TYPE: 105P X-Auth-Email: alim.akhtar@samsung.com X-HopCount: 7 X-CMS-RootMailID: 20170203092123epcas4p20b1d56d7a6fdec46903c41fc65718795 X-RootMTR: 20170203092123epcas4p20b1d56d7a6fdec46903c41fc65718795 References: <1447046787-480-1-git-send-email-alim.akhtar@samsung.com> <1447046787-480-3-git-send-email-alim.akhtar@samsung.com> <564AC63F.1000508@ti.com> <564AE11F.50908@samsung.com> <564DD11D.2060101@ti.com> <58B3B87B.7010302@ti.com> <58B4EFC8.2060300@ti.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Kishon, On 02/28/2017 09:04 AM, Kishon Vijay Abraham I wrote: > Hi, > > On Monday 27 February 2017 07:40 PM, Alim Akhtar wrote: >> Hi Kishon, >> >> On 02/27/2017 10:56 AM, Kishon Vijay Abraham I wrote: >>> Hi, >>> >>> On Thursday 23 February 2017 12:20 AM, Alim Akhtar wrote: >>>> On Fri, Feb 3, 2017 at 2:49 PM, Alim Akhtar wrote: >>>>> Hi Kishon, >>>>> >>>>> >>>>> On 11/19/2015 07:09 PM, Kishon Vijay Abraham I wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> On Tuesday 17 November 2015 01:41 PM, Alim Akhtar wrote: >>>>>>> >>>>>>> Hi >>>>>>> Thanks again for looking into this. >>>>>>> >>>>>>> On 11/17/2015 11:46 AM, Kishon Vijay Abraham I wrote: >>>>>>>> >>>>>>>> Hi, >>>>>>>> >>>>>>>> On Monday 09 November 2015 10:56 AM, Alim Akhtar wrote: >>>>>>>>> >>>>>>>>> From: Seungwon Jeon >>>>>>>>> >>>>>>>>> This patch introduces Exynos UFS PHY driver. This driver >>>>>>>>> supports to deal with phy calibration and power control >>>>>>>>> according to UFS host driver's behavior. >>>>>>>>> >>>>>>>>> Signed-off-by: Seungwon Jeon >>>>>>>>> Signed-off-by: Alim Akhtar >>>>>>>>> Cc: Kishon Vijay Abraham I >>>>>>>>> --- >>>>>>>>> drivers/phy/Kconfig | 7 ++ >>>>>>>>> drivers/phy/Makefile | 1 + >>>>>>>>> drivers/phy/phy-exynos-ufs.c | 241 >>>>>>>>> ++++++++++++++++++++++++++++++++++++ >>>>>>>>> drivers/phy/phy-exynos-ufs.h | 85 +++++++++++++ >>>>>>>>> drivers/phy/phy-exynos7-ufs.h | 89 +++++++++++++ >>>>>>>>> include/linux/phy/phy-exynos-ufs.h | 85 +++++++++++++ >>>>>>>>> 6 files changed, 508 insertions(+) >>>>>>>>> create mode 100644 drivers/phy/phy-exynos-ufs.c >>>>>>>>> create mode 100644 drivers/phy/phy-exynos-ufs.h >>>>>>>>> create mode 100644 drivers/phy/phy-exynos7-ufs.h >>>>>>>>> create mode 100644 include/linux/phy/phy-exynos-ufs.h >>>>>>>>> >>>>>>>>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig >>>>>>>>> index 7eb5859dd035..7d38a92e0297 100644 >>>>>>>>> --- a/drivers/phy/Kconfig >>>>>>>>> +++ b/drivers/phy/Kconfig >>>>>>>>> @@ -389,4 +389,11 @@ config PHY_CYGNUS_PCIE >>>>>>>>> Enable this to support the Broadcom Cygnus PCIe PHY. >>>>>>>>> If unsure, say N. >>>>>>>>> >>>>>>>>> +config PHY_EXYNOS_UFS >>>>>>>>> + tristate "EXYNOS SoC series UFS PHY driver" >>>>>>>>> + depends on OF && ARCH_EXYNOS || COMPILE_TEST >>>>>>>>> + select GENERIC_PHY >>>>>>>>> + help >>>>>>>>> + Support for UFS PHY on Samsung EXYNOS chipsets. >>>>>>>>> + >>>>>>>>> endmenu >>>>>>>>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile >>>>>>>>> index 075db1a81aa5..9bec4d1a89e1 100644 >>>>>>>>> --- a/drivers/phy/Makefile >>>>>>>>> +++ b/drivers/phy/Makefile >>>>>>>>> @@ -10,6 +10,7 @@ obj-$(CONFIG_ARMADA375_USBCLUSTER_PHY) += >>>>>>>>> phy-armada375-usb2.o >>>>>>>>> obj-$(CONFIG_BCM_KONA_USB2_PHY) += phy-bcm-kona-usb2.o >>>>>>>>> obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO) += phy-exynos-dp-video.o >>>>>>>>> obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO) += phy-exynos-mipi-video.o >>>>>>>>> +obj-$(CONFIG_PHY_EXYNOS_UFS) += phy-exynos-ufs.o >>>>>>>>> obj-$(CONFIG_PHY_LPC18XX_USB_OTG) += phy-lpc18xx-usb-otg.o >>>>>>>>> obj-$(CONFIG_PHY_PXA_28NM_USB2) += phy-pxa-28nm-usb2.o >>>>>>>>> obj-$(CONFIG_PHY_PXA_28NM_HSIC) += phy-pxa-28nm-hsic.o >>>>>>>>> diff --git a/drivers/phy/phy-exynos-ufs.c >>>>>>>>> b/drivers/phy/phy-exynos-ufs.c >>>>>>>>> new file mode 100644 >>>>>>>>> index 000000000000..cb1aeaa3d4eb >>>>>>>>> --- /dev/null >>>>>>>>> +++ b/drivers/phy/phy-exynos-ufs.c >>>>>>>>> @@ -0,0 +1,241 @@ >>>>>>>>> +/* >>>>>>>>> + * UFS PHY driver for Samsung EXYNOS SoC >>>>>>>>> + * >>>>>>>>> + * Copyright (C) 2015 Samsung Electronics Co., Ltd. >>>>>>>>> + * Author: Seungwon Jeon >>>>>>>>> + * >>>>>>>>> + * This program is free software; you can redistribute it and/or >>>>>>>>> modify >>>>>>>>> + * it under the terms of the GNU General Public License as published >>>>>>>>> by >>>>>>>>> + * the Free Software Foundation; either version 2 of the License, or >>>>>>>>> + * (at your option) any later version. >>>>>>>>> + */ >>>>>>>>> +#include >>>>>>>>> +#include >>>>>>>>> +#include >>>>>>>>> +#include >>>>>>>>> +#include >>>>>>>>> +#include >>>>>>>>> +#include >>>>>>>>> +#include >>>>>>>>> +#include >>>>>>>>> +#include >>>>>>>>> +#include >>>>>>>>> +#include >>>>>>>>> + >>>>>>>>> +#include "phy-exynos-ufs.h" >>>>>>>>> + >>>>>>>>> +#define for_each_phy_lane(phy, i) \ >>>>>>>>> + for (i = 0; i < (phy)->lane_cnt; i++) >>>>>>>>> +#define for_each_phy_cfg(cfg) \ >>>>>>>>> + for (; (cfg)->id; (cfg)++) >>>>>>>>> + >>>>>>>>> +#define PHY_DEF_LANE_CNT 1 >>>>>>>>> + >>>>>>>>> +static void exynos_ufs_phy_config(struct exynos_ufs_phy *phy, >>>>>>>>> + const struct exynos_ufs_phy_cfg *cfg, u8 lane) >>>>>>>>> +{ >>>>>>>>> + enum {LANE_0, LANE_1}; /* lane index */ >>>>>>>>> + >>>>>>>>> + switch (lane) { >>>>>>>>> + case LANE_0: >>>>>>>>> + writel(cfg->val, (phy)->reg_pma + cfg->off_0); >>>>>>>>> + break; >>>>>>>>> + case LANE_1: >>>>>>>>> + if (cfg->id == PHY_TRSV_BLK) >>>>>>>>> + writel(cfg->val, (phy)->reg_pma + cfg->off_1); >>>>>>>>> + break; >>>>>>>>> + } >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +static bool match_cfg_to_pwr_mode(u8 desc, u8 required_pwr) >>>>>>>>> +{ >>>>>>>>> + if (IS_PWR_MODE_ANY(desc)) >>>>>>>>> + return true; >>>>>>>>> + >>>>>>>>> + if (IS_PWR_MODE_HS(required_pwr) && IS_PWR_MODE_HS_ANY(desc)) >>>>>>>>> + return true; >>>>>>>>> + >>>>>>>>> + if (COMP_PWR_MODE(required_pwr, desc)) >>>>>>>>> + return true; >>>>>>>>> + >>>>>>>>> + if (COMP_PWR_MODE_MD(required_pwr, desc) && >>>>>>>>> + COMP_PWR_MODE_GEAR(required_pwr, desc) && >>>>>>>>> + COMP_PWR_MODE_SER(required_pwr, desc)) >>>>>>>>> + return true; >>>>>>>>> + >>>>>>>>> + return false; >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +int exynos_ufs_phy_calibrate(struct phy *phy, >>>>>>>>> + enum phy_cfg_tag tag, u8 pwr) >>>>>>>> >>>>>>>> >>>>>>>> This is similar to the first version of your patch without >>>>>>>> EXPORT_SYMBOL. >>>>>>>> >>>>>>>> I think you have to create a new generic PHY_OPS for calibrate PHY while >>>>>>>> making >>>>>>>> sure that it is as generic as possible (which means calibrate_phy >>>>>>>> shouldn't >>>>>>>> have tag and pwr arguments or a strong justification as to why those >>>>>>>> arguments >>>>>>>> are required in a generic API). >>>>>>> >>>>>>> I don't see the advantage to making this a generic phy_ops, this is >>>>>>> exynos >>>>>>> specific ufs-phy, please have a look at other implementations >>>>>> >>>>>> >>>>>> only the implementation is specific to exynos. I've seen lot of other >>>>>> vendors >>>>>> want to do something like calibrate phy. >>>>>> >>>>>> So if we add something like (*calibrate)(struct phy *phy), then it can be >>>>>> used >>>>>> by others as well. Russell King also want to minimize the code to program >>>>>> calibration settings. So it would be good to come up with a set of >>>>>> standard >>>>>> bindings like 'phy,tx-swing', 'phy,emphasis', 'phy,amplitude' etc.. to >>>>>> program >>>>>> these settings. >>>>>>> >>>>>>> drivers/phy/phy-qcom-ufs.c (which I belive mereged recently) >>>>>> >>>>>> >>>>>> Thats why I hate when someone else merge PHY drivers :-( That driver can >>>>>> as >>>>>> well be in drivers/misc as it doesn't use PHY framework as it is supposed >>>>>> to be >>>>>> used. It just exports a dozen of API's to be used by controller drivers. >>>>>> ick.. >>>>>> >>>>>>> may be other vendors might come with there own implementation of phy. >>>>>> >>>>>> >>>>>> right, it's all about providing the correct callback functions. >>>>>>> >>>>>>> I am using what is currently provided by the generic phy framework. >>>>>> >>>>>> >>>>>> I think for your use case, what is currently provided in the PHY framework >>>>>> is >>>>>> not sufficient. >>>>>> >>>>> Its little over a year since last time we discuss about adding a generic >>>>> calibration API. I can see in the past people tried adding *calibration* API >>>>> [1] but not sure why [1] was not landed in mainline. >>>>> Anyway now we have many users of phy_calibration API, like UFS, USB and may >>>>> be PCIe, there is a real need to add this functionality. So, here is my >>>>> approach: >>> >>> Agree, there are quite a few users that require calibration of phy parameters. >>> I think previously it was accommodated in phy_init, hence it was not merged. >> Ok, thanks for this information. >> >>>>> * Along with [1], we can add a void *priv for handling device specific phy >>>>> private data, and before calling phy_calibration() from phy consumer, >>>>> phy->priv is populated with private data. >>> >>> Not sure how you plan to use priv here? >>> >> From ufs driver I am populating PHY _priv_ data and calling phy_calibrate() >> >> e.g >> ----------------------- from ufs-exynos.c >> Instead of using below code earlier >> - exynos_ufs_phy_calibrate(generic_phy,CFG_PRE_INIT,PWR_MODE_ANY); >> >> Now I am using below from ufs-exynos driver >> >> + generic_phy->priv =(void*)CFG_PRE_INIT; >> + phy_calibrate(generic_phy); >> >> and in drivers/phy/phy-exynos-ufs.c >> using phy->priv in calibration function. > > Don't prefer passing of such private pointers between drivers. Why is this needed? > As already explained before, this is needed to pass the calibration point (when you want to do the calibration?, like before init, after init or before/after _mode_ change etc). I Don't think we have much option here, if we want to make phy_calibration generic enough. We have few options: 1> One way is to have phy_calibration takes some argument like calibration point (which was part of my v3~v5), but looking at the current implementation of phy-qcom-ufs.c, this approach might not be generic enough. 2> And using EXPORT_SYMBOL way is not encourage (as in my V1, even though others in phy drivers uses it). 3> the current proposal of using _priv_ data. Out of the above 3 option, I feel using _priv_ data is more generic way (and most of the major sub-system in Linux uses it). lets see what other people think about __priv__ approach. Please suggest your prefer way to handle this. > Thanks > Kishon > > >