Hi Vivek, On 12/28/2016 05:58 PM, Vivek Gautam wrote: > Hi Jaehoon, > > On Wed, Dec 28, 2016 at 8:19 AM, Jaehoon Chung <jh80.chung@xxxxxxxxxxx> 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 <jh80.chung@xxxxxxxxxxx> 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 <jh80.chung@xxxxxxxxxxx> >>>> --- >>>> 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. Right. I understood what you said. > >> >>> >>>> + 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. I will not put PCI_EXYNOS5433, just will use the PCI_EXYNOS. Because it will be supported only one file as pci-exynos.c > > [...] > >>>> + >>>> +#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(). > > [...] > > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html