Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751254AbcL1I6Z (ORCPT ); Wed, 28 Dec 2016 03:58:25 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:37410 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751105AbcL1I6W (ORCPT ); Wed, 28 Dec 2016 03:58:22 -0500 DMARC-Filter: OpenDMARC Filter v1.3.1 smtp.codeaurora.org C2291612C1 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=pass smtp.mailfrom=vivek.gautam@codeaurora.org MIME-Version: 1.0 In-Reply-To: <21de9ca7-e586-e5c8-ecda-d30c18bb6e40@samsung.com> References: <20161226052029.10552-1-jh80.chung@samsung.com> <20161226052029.10552-2-jh80.chung@samsung.com> <21de9ca7-e586-e5c8-ecda-d30c18bb6e40@samsung.com> From: Vivek Gautam Date: Wed, 28 Dec 2016 14:28:19 +0530 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC PATCH 1/6] phy: exynos-pcie: Add support for Exynos PCIe phy To: Jaehoon Chung Cc: linux-pci@vger.kernel.org, "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-samsung-soc@vger.kernel.org" , Bjorn Helgaas , "robh+dt" , Mark Rutland , Kukjin Kim , krzk@kernel.org, javier@osg.samsung.com, kishon , Will Deacon , catalin.marinas@arm.com, CPGS Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3728 Lines: 107 Hi Jaehoon, On Wed, Dec 28, 2016 at 8:19 AM, Jaehoon Chung wrote: > Hi Vivek, > > On 12/27/2016 02:53 PM, Vivek Gautam wrote: >> Hi Jaehoon, >> >> >> On Mon, Dec 26, 2016 at 10:50 AM, Jaehoon Chung wrote: >>> This patch supports to use Generic Phy framework for Exynos PCIe phy. >>> When Exynos that supported the pcie want to use the PCIe, >>> it needs to control the phy resgister. >>> But it should be more complex to control in their own PCIe device drivers. >>> >>> Signed-off-by: Jaehoon Chung >>> --- >>> drivers/phy/Kconfig | 9 ++ >>> drivers/phy/Makefile | 1 + >>> drivers/phy/phy-exynos-pcie.c | 227 ++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 237 insertions(+) >>> create mode 100644 drivers/phy/phy-exynos-pcie.c >>> >>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig >>> index fe00f91..94b0433 100644 >>> --- a/drivers/phy/Kconfig >>> +++ b/drivers/phy/Kconfig >>> @@ -341,6 +341,15 @@ config PHY_EXYNOS5_USBDRD >>> This driver provides PHY interface for USB 3.0 DRD controller >>> present on Exynos5 SoC series. >>> >>> +config PHY_EXYNOS_PCIE >>> + bool "Exynos PCIe PHY driver" >> >> Is there a reason for this not being 'tristate' ? > > Will change. I notice that PCI_EXYNOS5433 is bool as well. If the host has to be 'bool' then it makes sense to have phy also bool as well. But if PCI_EXYNOS5433 can be made tristate, then this also changes to tristate. > >> >>> + depends on ARCH_EXYNOS && OF >>> + depends on PCI_EXYNOS5433 >>> + select GENERIC_PHY >>> + help >>> + Enable PCIe PHY support for Exynos SoC series. >> >> If this driver is for Exynos5433, then same should come in this help >> text as well. > > will support the other exynos series. > I'm working on refactoring exynos5440 with PHY generic Framework. > Then this drive is not for only Exnyos5433. how about? Ok, it's good then. My only concern is 'depends on PCI_EXYNOS5433' makes it look like it is for EXYNOS5433. I am fine if that changes as well. [...] >>> + >>> +#define PCIE_EXYNOS5433_PMU_PHY_OFFSET 0x730 >>> +#define PCIE_PHY_OFFSET(x) ((x) * 0x4) >>> + >>> +/* Sysreg Fsys register offset and bit for Exynos5433 */ >>> +#define PCIE_PHY_MAC_RESET 0x208 >>> +#define PCIE_MAC_RESET_MASK 0xFF >>> +#define PCIE_MAC_RESET BIT(4) >>> +#define PCIE_L1SUB_CM_CON 0x1010 >>> +#define PCIE_REFCLK_GATING_EN BIT(0) >>> +#define PCIE_PHY_COMMON_RESET 0x1020 >>> +#define PCIE_PHY_RESET BIT(0) >>> +#define PCIE_PHY_GLOBAL_RESET 0x1040 >>> +#define PCIE_GLOBAL_RESET BIT(0) >>> +#define PCIE_REFCLK BIT(1) >>> +#define PCIE_REFCLK_MASK 0x16 >>> +#define PCIE_APP_REQ_EXIT_L1_MODE BIT(5) >>> + >>> +enum exynos_pcie_phy_data_type { >>> + PCIE_PHY_TYPE_EXYNOS5433, >>> +}; >>> + >>> +struct exynos_pcie_phy_data { >>> + enum exynos_pcie_phy_data_type ctrl_type; >> >> Why do we need this controller type ? >> If there are changes in the IP between different version, >> then you can as well use different compatibles. > > Do you mean is the using "of_device_is_compatible()"? I meant that multiple compatible strings can be added based on the IP versions. And any IP specific data can be put in the .data field of 'of_device_id' structure. If there's more to differentiate between the IP versions at runtime, you can use of_device_is_compatible(). [...] -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project