Re: [PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Sep 3, 2014 at 8:12 PM, Felipe Balbi <balbi@xxxxxx> wrote:
> Hi,
>
> On Wed, Sep 03, 2014 at 12:59:27PM +0530, Vivek Gautam wrote:
>> > On Tue, Sep 02, 2014 at 04:42:18PM +0530, Vivek Gautam wrote:
>> >> Adding phy calibrate callback, which facilitates setting certain
>> >> PHY settings post initialization of the PHY controller.
>> >> Exynos5420 and Exynos5800 have 28nm USB 3.0 DRD PHY for which
>> >> the Loss-of-Signal (LOS) Detector Threshold Level as well as
>> >> Tx-Vboost-Level should be controlled for Super-Speed operations.
>> >>
>> >> Additionally set proper time to wait for RxDetect measurement,
>> >> for desired PHY reference clock, so as to solve issue with enumeration
>> >> of few USB 3.0 devices, like Samsung SUM-TSB16S 3.0 USB drive
>> >> on the controller.
>> >> We are using CR_port for this purpose to send required data
>> >> to override the LOS values.
>> >>
>> >> On testing with USB 3.0 devices on USB 3.0 port present on
>> >> SMDK5420, and peach-pit boards should see following message:
>> >> usb 2-1: new SuperSpeed USB device number 2 using xhci-hcd
>> >>
>> >> and without this patch, should see below shown message:
>> >> usb 1-1: new high-speed USB device number 2 using xhci-hcd
>> >>
>> >> [Also removed unnecessary extra lines in the register macro definitions]
>> >>
>> >> Signed-off-by: Vivek Gautam <gautam.vivek@xxxxxxxxxxx>
>> >> ---
>> >>  drivers/phy/phy-exynos5-usbdrd.c |  185 ++++++++++++++++++++++++++++++++++----
>> >>  1 file changed, 170 insertions(+), 15 deletions(-)
>> >>
>> >> diff --git a/drivers/phy/phy-exynos5-usbdrd.c b/drivers/phy/phy-exynos5-usbdrd.c
>> >> index 47f47fe..85742b0 100644
>> >> --- a/drivers/phy/phy-exynos5-usbdrd.c
>> >> +++ b/drivers/phy/phy-exynos5-usbdrd.c
>> >> @@ -37,13 +37,11 @@
>> >>
>> >>  /* EXYNOS5: USB 3.0 DRD PHY registers */
>> >>  #define EXYNOS5_DRD_LINKSYSTEM                       0x04
>> >> -
>> >>  #define LINKSYSTEM_FLADJ_MASK                        (0x3f << 1)
>> >>  #define LINKSYSTEM_FLADJ(_x)                 ((_x) << 1)
>> >>  #define LINKSYSTEM_XHCI_VERSION_CONTROL              BIT(27)
>> >>
>> >>  #define EXYNOS5_DRD_PHYUTMI                  0x08
>> >> -
>> >>  #define PHYUTMI_OTGDISABLE                   BIT(6)
>> >>  #define PHYUTMI_FORCESUSPEND                 BIT(1)
>> >>  #define PHYUTMI_FORCESLEEP                   BIT(0)
>> >> @@ -51,26 +49,20 @@
>> >>  #define EXYNOS5_DRD_PHYPIPE                  0x0c
>> >>
>> >>  #define EXYNOS5_DRD_PHYCLKRST                        0x10
>> >> -
>> >>  #define PHYCLKRST_EN_UTMISUSPEND             BIT(31)
>> >> -
>> >>  #define PHYCLKRST_SSC_REFCLKSEL_MASK         (0xff << 23)
>> >>  #define PHYCLKRST_SSC_REFCLKSEL(_x)          ((_x) << 23)
>> >> -
>> >>  #define PHYCLKRST_SSC_RANGE_MASK             (0x03 << 21)
>> >>  #define PHYCLKRST_SSC_RANGE(_x)                      ((_x) << 21)
>> >> -
>> >>  #define PHYCLKRST_SSC_EN                     BIT(20)
>> >>  #define PHYCLKRST_REF_SSP_EN                 BIT(19)
>> >>  #define PHYCLKRST_REF_CLKDIV2                        BIT(18)
>> >> -
>> >>  #define PHYCLKRST_MPLL_MULTIPLIER_MASK               (0x7f << 11)
>> >>  #define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF (0x19 << 11)
>> >>  #define PHYCLKRST_MPLL_MULTIPLIER_50M_REF    (0x32 << 11)
>> >>  #define PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF  (0x68 << 11)
>> >>  #define PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF  (0x7d << 11)
>> >>  #define PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF       (0x02 << 11)
>> >> -
>> >>  #define PHYCLKRST_FSEL_UTMI_MASK             (0x7 << 5)
>> >>  #define PHYCLKRST_FSEL_PIPE_MASK             (0x7 << 8)
>> >>  #define PHYCLKRST_FSEL(_x)                   ((_x) << 5)
>> >> @@ -78,46 +70,68 @@
>> >>  #define PHYCLKRST_FSEL_PAD_24MHZ             (0x2a << 5)
>> >>  #define PHYCLKRST_FSEL_PAD_20MHZ             (0x31 << 5)
>> >>  #define PHYCLKRST_FSEL_PAD_19_2MHZ           (0x38 << 5)
>> >> -
>> >>  #define PHYCLKRST_RETENABLEN                 BIT(4)
>> >> -
>> >>  #define PHYCLKRST_REFCLKSEL_MASK             (0x03 << 2)
>> >>  #define PHYCLKRST_REFCLKSEL_PAD_REFCLK               (0x2 << 2)
>> >>  #define PHYCLKRST_REFCLKSEL_EXT_REFCLK               (0x3 << 2)
>> >> -
>> >>  #define PHYCLKRST_PORTRESET                  BIT(1)
>> >>  #define PHYCLKRST_COMMONONN                  BIT(0)
>> >>
>> >>  #define EXYNOS5_DRD_PHYREG0                  0x14
>> >> +#define PHYREG0_SSC_REF_CLK_SEL                      BIT(21)
>> >> +#define PHYREG0_SSC_RANGE                    BIT(20)
>> >> +#define PHYREG0_CR_WRITE                     BIT(19)
>> >> +#define PHYREG0_CR_READ                              BIT(18)
>> >> +#define PHYREG0_CR_DATA_IN(_x)                       ((_x) << 2)
>> >> +#define PHYREG0_CR_CAP_DATA                  BIT(1)
>> >> +#define PHYREG0_CR_CAP_ADDR                  BIT(0)
>> >> +
>> >>  #define EXYNOS5_DRD_PHYREG1                  0x18
>> >> +#define PHYREG1_CR_DATA_OUT(_x)                      ((_x) << 1)
>> >> +#define PHYREG1_CR_ACK                               BIT(0)
>> >>
>> >>  #define EXYNOS5_DRD_PHYPARAM0                        0x1c
>> >> -
>> >>  #define PHYPARAM0_REF_USE_PAD                        BIT(31)
>> >>  #define PHYPARAM0_REF_LOSLEVEL_MASK          (0x1f << 26)
>> >>  #define PHYPARAM0_REF_LOSLEVEL                       (0x9 << 26)
>> >>
>> >>  #define EXYNOS5_DRD_PHYPARAM1                        0x20
>> >> -
>> >>  #define PHYPARAM1_PCS_TXDEEMPH_MASK          (0x1f << 0)
>> >>  #define PHYPARAM1_PCS_TXDEEMPH                       (0x1c)
>> >>
>> >>  #define EXYNOS5_DRD_PHYTERM                  0x24
>> >>
>> >>  #define EXYNOS5_DRD_PHYTEST                  0x28
>> >> -
>> >>  #define PHYTEST_POWERDOWN_SSP                        BIT(3)
>> >>  #define PHYTEST_POWERDOWN_HSP                        BIT(2)
>> >>
>> >>  #define EXYNOS5_DRD_PHYADP                   0x2c
>> >>
>> >>  #define EXYNOS5_DRD_PHYUTMICLKSEL            0x30
>> >> -
>> >
>> > this and all above are unrelated to $subject.
>>
>> True, but i have mentioned about this change in the commit message.
>> It looked so trivial to me that i included it here in this patch.
>>
>> Will separate it out in a separate patch if you like that. :-)
>
> probably not necessary. Kishon's got the final word for drivers/phy,
> though.

sure,

>
>>
>> >
>> >>  #define PHYUTMICLKSEL_UTMI_CLKSEL            BIT(2)
>> >>
>> >>  #define EXYNOS5_DRD_PHYRESUME                        0x34
>> >>  #define EXYNOS5_DRD_LINKPORT                 0x44
>> >>
>> >> +/* USB 3.0 DRD PHY SS Function Control Reg; accessed by CR_PORT */
>> >> +#define EXYNOS5_DRD_PHYSS_LOSLEVEL_OVRD_IN           (0x15)
>> >> +#define LOSLEVEL_OVRD_IN_LOS_BIAS_5420                       (0x5 << 13)
>> >> +#define LOSLEVEL_OVRD_IN_LOS_BIAS_DEFAULT            (0x0 << 13)
>> >> +#define LOSLEVEL_OVRD_IN_EN                          (0x1 << 10)
>> >> +#define LOSLEVEL_OVRD_IN_LOS_LEVEL_DEFAULT           (0x9 << 0)
>> >> +
>> >> +#define EXYNOS5_DRD_PHYSS_TX_VBOOSTLEVEL_OVRD_IN     (0x12)
>> >> +#define TX_VBOOSTLEVEL_OVRD_IN_VBOOST_5420           (0x5 << 13)
>> >> +#define TX_VBOOSTLEVEL_OVRD_IN_VBOOST_DEFAULT                (0x4 << 13)
>> >> +
>> >> +#define EXYNOS5_DRD_PHYSS_LANE0_TX_DEBUG             (0x1010)
>> >> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_19M2_20M              (0x4 << 4)
>> >> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_24M           (0x8 << 4)
>> >> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_25M_26M               (0x8 << 4)
>> >> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_48M_50M_52M   (0x20 << 4)
>> >> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_62M5          (0x20 << 4)
>> >> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_96M_100M              (0x40 << 4)
>> >> +
>> >>  #define KHZ  1000
>> >>  #define MHZ  (KHZ * KHZ)
>> >>
>> >> @@ -135,12 +149,14 @@ struct exynos5_usbdrd_phy_config {
>> >>       void (*phy_isol)(struct phy_usb_instance *inst, u32 on);
>> >>       void (*phy_init)(struct exynos5_usbdrd_phy *phy_drd);
>> >>       unsigned int (*set_refclk)(struct phy_usb_instance *inst);
>> >> +     int (*phy_calibrate)(struct phy_usb_instance *inst);
>> >>  };
>> >>
>> >>  struct exynos5_usbdrd_phy_drvdata {
>> >>       const struct exynos5_usbdrd_phy_config *phy_cfg;
>> >>       u32 pmu_offset_usbdrd0_phy;
>> >>       u32 pmu_offset_usbdrd1_phy;
>> >> +     void (*phy_exynos_calibrate)(struct exynos5_usbdrd_phy *phy_drd);
>> >>  };
>> >>
>> >>  /**
>> >> @@ -487,6 +503,142 @@ static int exynos5_usbdrd_phy_power_off(struct phy *phy)
>> >>       return 0;
>> >>  }
>> >>
>> >> +static void crport_handshake(struct exynos5_usbdrd_phy *phy_drd,
>> >> +                                             u32 val, u32 cmd)
>> >> +{
>> >> +     u32 usec = 100;
>> >> +     u32 result;
>> >> +
>> >> +     writel(val | cmd, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
>> >> +
>> >> +     do {
>> >> +             result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
>> >> +             if (result & PHYREG1_CR_ACK)
>> >> +                     break;
>> >> +
>> >> +             udelay(1);
>> >> +     } while (usec-- > 0);
>> >> +
>> >> +     if (!usec)
>> >> +             dev_err(phy_drd->dev,
>> >> +                     "CRPORT handshake timeout1 (0x%08x)\n", val);
>> >
>> > if handshake timed out, shouldn't you error out here ? IOW, make this
>> > function return an int and return an error code from this point ? Why
>> > would you even continue to the next step ?
>>
>> Point taken, we should have error'ed out here once timeout occured.
>> Will update it accordingly.
>>
>> >
>> >> +
>> >> +     usec = 100;
>> >> +
>> >> +     writel(val, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
>> >> +
>> >> +     do {
>> >> +             result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
>> >> +             if (!(result & PHYREG1_CR_ACK))
>> >> +                     break;
>> >> +
>> >> +             udelay(1);
>> >> +     } while (usec-- > 0);
>> >> +
>> >> +     if (!usec)
>> >> +             dev_err(phy_drd->dev,
>> >> +                     "CRPORT handshake timeout2 (0x%08x)\n", val);
>> >
>> > return an error ?
>> sure
>>
>> >
>> >> +}
>> >> +
>> >> +static void crport_ctrl_write(struct exynos5_usbdrd_phy *phy_drd,
>> >> +                                             u32 addr, u32 data)
>> >> +{
>> >> +     /* Write Address */
>> >> +     writel(PHYREG0_CR_DATA_IN(addr),
>> >> +                     phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
>> >> +     crport_handshake(phy_drd, PHYREG0_CR_DATA_IN(addr),
>> >> +                      PHYREG0_CR_CAP_ADDR);
>> >> +
>> >> +     /* Write Data */
>> >> +     writel(PHYREG0_CR_DATA_IN(data),
>> >> +                     phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
>> >> +     crport_handshake(phy_drd, PHYREG0_CR_DATA_IN(data),
>> >> +                      PHYREG0_CR_CAP_DATA);
>> >> +     crport_handshake(phy_drd, PHYREG0_CR_DATA_IN(data),
>> >> +                      PHYREG0_CR_WRITE);
>> >> +}
>> >> +
>> >> +/*
>> >> + * Override PHY paramaeters using CR_PORT register to calibrate settings
>> >> + * to meet meet SuperSpeed requirements, on Exynos5420 and Exynos5800 systems,
>> >> + * which have 28nm USB 3.0 DRD PHY.
>> >> + */
>> >> +static void exynos5420_usbdrd_phy_calibrate(struct exynos5_usbdrd_phy *phy_drd)
>> >> +{
>> >> +     u32 temp;
>> >> +
>> >> +     /*
>> >> +      * Change los_bias to (0x5) for 28nm PHY from a
>> >> +      * default value (0x0); los_level is set as default
>> >> +      * (0x9) as also reflected in los_level[30:26] bits
>> >> +      * of PHYPARAM0 register.
>> >> +      */
>> >> +     temp = LOSLEVEL_OVRD_IN_LOS_BIAS_5420 |
>> >> +             LOSLEVEL_OVRD_IN_EN |
>> >> +             LOSLEVEL_OVRD_IN_LOS_LEVEL_DEFAULT;
>> >> +     crport_ctrl_write(phy_drd,
>> >> +                       EXYNOS5_DRD_PHYSS_LOSLEVEL_OVRD_IN,
>> >> +                       temp);
>> >> +
>> >> +     /*
>> >> +      * Set tx_vboost_lvl to (0x5) for 28nm PHY Tuning,
>> >> +      * to raise Tx signal level from its default value of (0x4)
>> >> +      */
>> >> +     temp = TX_VBOOSTLEVEL_OVRD_IN_VBOOST_5420;
>> >> +     crport_ctrl_write(phy_drd,
>> >> +                       EXYNOS5_DRD_PHYSS_TX_VBOOSTLEVEL_OVRD_IN,
>> >> +                       temp);
>> >> +
>> >> +     /*
>> >> +      * Set proper time to wait for RxDetect measurement, for
>> >> +      * desired reference clock of PHY, by tuning the CRPORT
>> >> +      * register LANE0.TX_DEBUG which is internal to PHY.
>> >> +      * This fixes issue with few USB 3.0 devices, which are
>> >> +      * not detected (not even generate interrupts on the bus
>> >> +      * on insertion) without this change.
>> >> +      * e.g. Samsung SUM-TSB16S 3.0 USB drive.
>> >> +      */
>> >> +     switch (phy_drd->extrefclk) {
>> >> +     case EXYNOS5_FSEL_50MHZ:
>> >> +             temp = LANE0_TX_DEBUG_RXDET_MEAS_TIME_48M_50M_52M;
>> >> +             break;
>> >> +     case EXYNOS5_FSEL_20MHZ:
>> >> +     case EXYNOS5_FSEL_19MHZ2:
>> >> +             temp = LANE0_TX_DEBUG_RXDET_MEAS_TIME_19M2_20M;
>> >> +             break;
>> >> +     case EXYNOS5_FSEL_24MHZ:
>> >
>> > why can't this be done during probe() ? Are you going to change external
>> > reference clock frequency during runtime ? And even if you do,
>> > wouldn't it be easier to subscribe to clock rate change notifiers
>> > instead of adding this ->calibrate() ?
>>
>> Wait, did this patch give an impression that we are changing the reference
>> clock settings ?
>
> not, it gives the impression that you need to change RX Detect timeout
> value based on external reference clock frequency :-)
>
>> We are actually trying to override some of the PHY parameters, which
>> is possible only after the controller has been reset.
>> These PHY parameter overriding (on PHY registers which are not exposed
>> outside) is done post initialization, only through
>> EXYNOS5_DRD_PHYREG0.
>
> so it _has_ to be done after dwc3's reset ?

By  controller reset i meant XHCI reset in fact.
What we see with this PHY controller on Exynos5420/5800 that it's only when
the host controller has been reset, the PHY parameters (which we are
calibrating here)
settings remain intact.
We have already tried earlier moving this calibration afte DWC3 core
reset occurs,
but that doesn't help.
That's the reason we moved the calibration stuff to usb_add_hcd() and
usb_bus_resume()
functions.

> Don't we have phy_power_on()
> for that ? It looks like you could just as well do this from
> phy_power_on() ?

No, unfortunately keeping these calibration settings in phy_power_on()
doesn't help, since we need to do this after XHCI reset has happened.

>
>> > It still looks, to me, that this ->calibrate() thing is unnecessary. We
>> > have a single user for it and this user could very well get this sorted
>> > out during probe() or by subscribing to clock rate notifiers.
>>
>> We can't program these parameters during PHY probe(), since they reset to
>> their default value after the controller reset.
>> So we needed a way to do this post initialization, once the controller
>> has been reset.
>
> right, and we already have a call to phy_power_on() after core has been
> reset. Can't you use that ?
>
> --
> balbi



-- 
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux