Re: [RFC PATCH 1/6] phy: exynos-pcie: Add support for Exynos PCIe phy

[Jaehoon Chung]

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().
> 
> [...]
> 
> 
>