RE: [PATCH v5 2/2] phy: zynqmp: Add phy driver for xilinx zynqmp phy core
From: Anurag Kumar Vulisha
Date: Wed Jan 23 2019 - 11:10:20 EST
Hi Kishon,
>-----Original Message-----
>From: Kishon Vijay Abraham I [mailto:kishon@xxxxxx]
>Sent: Monday, January 21, 2019 11:16 AM
>To: Anurag Kumar Vulisha <anuragku@xxxxxxxxxx>; robh+dt@xxxxxxxxxx; Mark
>Rutland <mark.rutland@xxxxxxx>; vivek.gautam@xxxxxxxxxxxxxx
>Cc: Michal Simek <michals@xxxxxxxxxx>; v.anuragkumar@xxxxxxxxx; sundeep
>subbaraya <sundeep.lkml@xxxxxxxxx>; Ajay Yugalkishore Pandey
><APANDEY@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; linux-arm-
>kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH v5 2/2] phy: zynqmp: Add phy driver for xilinx zynqmp phy core
>
>Hi Anurag,
>
>On 17/01/19 9:39 PM, Anurag Kumar Vulisha wrote:
>> Hi Kishon,
>>
>>> -----Original Message-----
>>> From: Kishon Vijay Abraham I [mailto:kishon@xxxxxx]
>>> Sent: Wednesday, January 16, 2019 1:38 PM
>>> To: Anurag Kumar Vulisha <anuragku@xxxxxxxxxx>; robh+dt@xxxxxxxxxx; Mark
>>> Rutland <mark.rutland@xxxxxxx>; vivek.gautam@xxxxxxxxxxxxxx
>>> Cc: Michal Simek <michals@xxxxxxxxxx>; v.anuragkumar@xxxxxxxxx; sundeep
>>> subbaraya <sundeep.lkml@xxxxxxxxx>; Ajay Yugalkishore Pandey
>>> <APANDEY@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; linux-arm-
>>> kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
>>> Subject: Re: [PATCH v5 2/2] phy: zynqmp: Add phy driver for xilinx zynqmp phy core
>>>
>>> Hi,
>>>
>>> On 18/12/18 7:15 PM, Anurag Kumar Vulisha wrote:
>>>> ZynqMP SoC has a Gigabit Transceiver with four lanes. All the high
>>>> speed peripherals such as USB, SATA, PCIE, Display Port and Ethernet
>>>> SGMII can rely on any of the four GT lanes for PHY layer. This patch
>>>> adds driver for that ZynqMP GT core.
>>>>
>>>> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xxxxxxxxxx>
>>>> ---
>>>> Changes in v5:
>>>> 1. No functional changes. Added missing Author name
>>>>
>>>> Changes in v4:
>>>> 1. Moved include/dt-bindings/phy/phy.h into patch 1 as suggested by
>>>> "Rob Herring"
>>>>
>>>> Changes in v3:
>>>> 1. Corrected the Documentation as suggested by "Vivek Gautam"
>>>>
>>>> Changes in v2:
>>>> 1. Fixed the compilation error when compiled phy-zynqmp.c as a module
>>>> 2. Added CONFIG_PM macro in phy-zynqmp.c driver
>>>> ---
>>>> drivers/phy/Kconfig | 8 +
>>>> drivers/phy/Makefile | 1 +
>>>> drivers/phy/phy-zynqmp.c | 1582
>>> ++++++++++++++++++++++++++++++++++++++++
>>>> include/linux/phy/phy-zynqmp.h | 52 ++
>>>> 4 files changed, 1643 insertions(+)
>>>> create mode 100644 drivers/phy/phy-zynqmp.c create mode 100644
>>>> include/linux/phy/phy-zynqmp.h
>>>>
>>>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig index
>>>> 60f949e..7a3c900 100644
>>>> --- a/drivers/phy/Kconfig
>>>> +++ b/drivers/phy/Kconfig
>>>> @@ -40,6 +40,14 @@ config PHY_XGENE
>>>> help
>>>> This option enables support for APM X-Gene SoC multi-purpose PHY.
>>>>
>>>> +config PHY_XILINX_ZYNQMP
>>>> + tristate "Xilinx ZynqMP PHY driver"
>>>> + depends on ARCH_ZYNQMP
>>>> + select GENERIC_PHY
>>>> + help
>>>> + Enable this to support ZynqMP High Speed Gigabit Transceiver
>>>> + that is part of ZynqMP SoC.
>>>> +
>>>> source "drivers/phy/allwinner/Kconfig"
>>>> source "drivers/phy/amlogic/Kconfig"
>>>> source "drivers/phy/broadcom/Kconfig"
>>>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile index
>>>> 0301e25..2335e85 100644
>>>> --- a/drivers/phy/Makefile
>>>> +++ b/drivers/phy/Makefile
>>>> +/**
>>>> +
>>>> +/**
>>>> + * xpsgtr_override_deemph - override PIPE TX de-emphasis
>>>> + * @phy: pointer to phy
>>>> + * @plvl: pre-emphasis level
>>>> + * @vlvl: voltage swing level
>>>> + *
>>>> + * Return: None
>>>> + */
>>>> +void xpsgtr_override_deemph(struct phy *phy, u8 plvl, u8 vlvl) {
>>>> + struct xpsgtr_phy *gtr_phy = phy_get_drvdata(phy);
>>>> + struct xpsgtr_dev *gtr_dev = gtr_phy->data;
>>>> + static u8 pe[4][4] = { { 0x2, 0x2, 0x2, 0x2 },
>>>> + { 0x1, 0x1, 0x1, 0xFF },
>>>> + { 0x0, 0x0, 0xFF, 0xFF },
>>>> + { 0xFF, 0xFF, 0xFF, 0xFF } };
>>>> +
>>>> + writel(pe[plvl][vlvl],
>>>> + gtr_dev->serdes + gtr_phy->lane * L0_TX_ANA_TM_18_OFFSET +
>>>> + L0_TX_ANA_TM_18);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(xpsgtr_override_deemph);
>>>
>>> I thought I gave a feedback to get rid of export symbol. This will make the
>consumer
>>> driver tied to this PHY driver.
>>>
>>
>> Thanks a lot for spending your time in reviewing this patch. With the current
>implementation,
>> if phy-zynqmp.c driver is not compiled and consumer driver calls
>xpsgtr_override_deemph()
>> routine, static inline function in phy-zynqmp.h gets called and error -ENODEV is
>returned. So,
>> with the current implementation the consumer driver is already depending on phy-
>zynqmp.c
>> driver. Please correct me if my understanding is wrong
>
>If the same consumer is used with 5 different PHYs, we would be adding 5
>different APIs for PHY functionality. We would also like to avoid the compiling
>out option if we use something like a multi_v7_defconfig where a single image
>is used for multiple platforms.
>
Thanks for your comment. As of now the Xilinx Display Port driver is designed to work
with this phy-zynqmp driver (may be generic in future). Along with Display Port , this
phy driver is being used by 4 other consumer drivers and compiling phy driver as module
might fail if it is tightly coupled with Display Port driver.
Instead , can we use the platform data of the phy->dev to provide the phy platform
specific API's to the consumer drivers. The consumer drivers will include the
phy-zynqmp.h and call dev_get_platdata(&phy->dev) and call the respective phy
specific APIs. In this way we can get rid of export symbols from driver. The below
is the pseudo code changes for using platform data. Please let me know your opinion
on this,
--- a/include/linux/phy/phy-zynqmp.h
+++ b/include/linux/phy/phy-zynqmp.h
+struct xpsgtr_pdata
+ int (* xpsgtr_override_deemph)(struct phy *phy, u8 plvl, u8 vlvl);
+ int (* xpsgtr_margining_factor)(struct phy *phy, u8 plvl, u8 vlvl);
+ int (* xpsgtr_wait_pll_lock)(struct phy *phy);
+ int (* xpsgtr_usb_crst_assert)(struct phy *phy);
+ int (* xpsgtr_usb_crst_release)(struct phy *phy);
+ void *pdata;
+};
--- a/drivers/phy/phy-zynqmp.c
+++ b/drivers/phy/phy-zynqmp.c
-int xpsgtr_override_deemph(struct phy *phy, u8 plvl, u8 vlvl)
+static int xpsgtr_override_deemph(struct phy *phy, u8 plvl, u8 vlvl)
{
struct xpsgtr_phy *gtr_phy = phy_get_drvdata(phy);
struct xpsgtr_dev *gtr_dev = gtr_phy->data;
@@ -327,9 +327,8 @@ int xpsgtr_override_deemph(struct phy *phy, u8 plvl, u8 vlvl)
return 0;
}
-EXPORT_SYMBOL_GPL(xpsgtr_override_deemph);
-int xpsgtr_margining_factor(struct phy *phy, u8 plvl, u8 vlvl)
+static int xpsgtr_margining_factor(struct phy *phy, u8 plvl, u8 vlvl)
{
struct xpsgtr_phy *gtr_phy = phy_get_drvdata(phy);
struct xpsgtr_dev *gtr_dev = gtr_phy->data;
@@ -344,7 +343,6 @@ int xpsgtr_margining_factor(struct phy *phy, u8 plvl, u8 vlvl)
return 0;
}
-EXPORT_SYMBOL_GPL(xpsgtr_margining_factor);
/**
* xpsgtr_set_txwidth - This function sets the tx bus width of the lane
@@ -1111,6 +1107,13 @@ static int xpsgtr_phy_init(struct phy *phy)
if (gtr_phy->protocol == ICM_PROTOCOL_SGMII)
xpsgtr_misc_sgmii(gtr_phy);
+ struct xpsgtr_pdata * pdata =
+ kmalloc(sizeof(struct xpsgtr_pdata), GFP_KERNEL);
+ pdata->xpsgtr_override_deemph = xpsgtr_override_deemph;
+ pdata->xpsgtr_margining_factor = xpsgtr_margining_factor;
+ pdata->xpsgtr_wait_pll_lock = xpsgtr_wait_pll_lock;
+ phy->dev.platform_data = (void *)pdata;
+
/* Bring controller out of reset */
ret = xpsgtr_controller_release_reset(gtr_phy);
if (ret != 0) {
@@ -1306,6 +1309,8 @@ static int xpsgtr_phy_exit(struct phy *phy)
/* As we are exiting, clear skip_phy_init flag */
gtr_phy->skip_phy_init = false;
+ kfree(phy->dev.platform_data);
+
return 0;
}
Thanks,
Anurag Kumar Vulisha