Re: [PATCH v4 7/7] phy: qcom-qusb2: Add QUSB2 PHYs support for sdm845

[Manu Gautam <mgautam@codeaurora.org>]

Hi,


On 3/30/2018 2:08 AM, Doug Anderson wrote:
> Hi,
>
> On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wrote:
>> @@ -241,6 +252,18 @@ struct qusb2_phy_cfg {
>>   * @tcsr: TCSR syscon register map
>>   * @cell: nvmem cell containing phy tuning value
>>   *
>> + * @override_imp_res_offset: PHY should use different rescode offset
>> + * @imp_res_offset_value: rescode offset to be updated in IMP_CTRL1 register
>> + *
>> + * @override_hstx_trim: PHY should use different HSTX o/p current value
>> + * @hstx_trim_value: HSTX_TRIM value to be updated in TUNE1 register
>> + *
>> + * @override_preemphasis: PHY should use different pre-amphasis amplitude
>> + * @preemphasis_level: Amplitude Pre-Emphasis to be updated in TUNE1 register
>> + *
>> + * @override_preemphasis_width: PHY should use different pre-emphasis duration
>> + * @preemphasis_width: half/full-width Pre-Emphasis updated via TUNE1
>> + *
> nit: spacing here doesn't match spacing in the structure.  AKA: you've
> smashed together all 8 properties in the structure but not in the
> description.
Will fix it.
>
>
>>   * @cfg: phy config data
>>   * @has_se_clk_scheme: indicate if PHY has single-ended ref clock scheme
>>   * @phy_initialized: indicate if PHY has been initialized
>> @@ -259,12 +282,35 @@ struct qusb2_phy {
>>         struct regmap *tcsr;
>>         struct nvmem_cell *cell;
>>
>> +       bool override_imp_res_offset;
>> +       u8 imp_res_offset_value;
>> +       bool override_hstx_trim;
>> +       u8 hstx_trim_value;
>> +       bool override_preemphasis;
>> +       u8 preemphasis_level;
>> +       bool override_preemphasis_width;
>> +       u8 preemphasis_width;
>> +
>>         const struct qusb2_phy_cfg *cfg;
>>         bool has_se_clk_scheme;
>>         bool phy_initialized;
>>         enum phy_mode mode;
>>  };
>>
>> +static inline void qusb2_write_mask(void __iomem *base, u32 offset,
>> +                                   u32 val, u32 mask)
>> +{
>> +       u32 reg;
>> +
>> +       reg = readl(base + offset);
>> +       reg &= ~mask;
>> +       reg |= val;
> "reg |= (val & mask)" instead of just "reg |= val"
>
> You don't do any bounds checking of the device tree entries and this
> will at least make sure that a bad value for a field won't screw up
> other fields (and so, presumably, it will be easier to find the bug).
>
It makes sense. Will change accordingly.

>
>> +       writel(reg, base + offset);
>> +
>> +       /* Ensure above write is completed */
>> +       readl(base + offset);
> You're using readl() and writel() which have barriers.  Why do you
> need this extra readl()?

This requirement comes from AHB2PHY wrapper designers and HPG.
Also existing qusb2_setbits/clrbits() wrapper already have similar handling.

>
>
>> +}
>> +
>>  static inline void qusb2_setbits(void __iomem *base, u32 offset, u32 val)
>>  {
>>         u32 reg;
>> @@ -305,6 +351,42 @@ void qcom_qusb2_phy_configure(void __iomem *base,
>>  }
>>
>>  /*
>> + * Update board specific PHY tuning override values if specified from
>> + * device tree.
>> + *
> nit: remove extra comment line with just a "*" on it.
Sure.
>
>
>> + */
>> +static void qusb2_phy_override_phy_params(struct qusb2_phy *qphy)
>> +{
>> +       const struct qusb2_phy_cfg *cfg = qphy->cfg;
>> +
>> +       if (qphy->override_imp_res_offset)
>> +               qusb2_write_mask(qphy->base, QUSB2PHY_IMP_CTRL1,
>> +                            qphy->imp_res_offset_value << IMP_RES_OFFSET_SHIFT,
>> +                            IMP_RES_OFFSET_MASK);
>> +
>> +       if (qphy->override_hstx_trim)
>> +               qusb2_write_mask(qphy->base, cfg->regs[QUSB2PHY_PORT_TUNE1],
>> +                                qphy->hstx_trim_value << HSTX_TRIM_SHIFT,
>> +                                HSTX_TRIM_MASK);
>> +
>> +       if (qphy->override_preemphasis)
>> +               qusb2_write_mask(qphy->base, cfg->regs[QUSB2PHY_PORT_TUNE1],
>> +                               qphy->preemphasis_level << PREEMPHASIS_EN_SHIFT,
>> +                               PREEMPHASIS_EN_MASK);
>> +
>> +       if (qphy->override_preemphasis_width) {
>> +               if (qphy->preemphasis_width)
>> +                       qusb2_setbits(qphy->base,
>> +                                     cfg->regs[QUSB2PHY_PORT_TUNE1],
>> +                                     PREEMPH_HALF_WIDTH);
>> +               else
>> +                       qusb2_clrbits(qphy->base,
>> +                                     cfg->regs[QUSB2PHY_PORT_TUNE1],
>> +                                     PREEMPH_HALF_WIDTH);
>> +       }
>> +}
>> +
>> +/*
>>   * Fetches HS Tx tuning value from nvmem and sets the
>>   * QUSB2PHY_PORT_TUNE1/2 register.
>>   * For error case, skip setting the value and use the default value.
>> @@ -525,6 +607,9 @@ static int qusb2_phy_init(struct phy *phy)
>>         qcom_qusb2_phy_configure(qphy->base, cfg->regs, cfg->tbl,
>>                                  cfg->tbl_num);
>>
>> +       /* Override board specific PHY tuning values */
>> +       qusb2_phy_override_phy_params(qphy);
>> +
>>         /* Set efuse value for tuning the PHY */
>>         qusb2_phy_set_tune2_param(qphy);
>>
>> @@ -647,7 +732,7 @@ static int qusb2_phy_exit(struct phy *phy)
>>                 .compatible     = "qcom,msm8996-qusb2-phy",
>>                 .data           = &msm8996_phy_cfg,
>>         }, {
>> -               .compatible     = "qcom,qusb2-v2-phy",
>> +               .compatible     = "qcom,sdm845-qusb2-phy",
>>                 .data           = &qusb2_v2_phy_cfg,
> Can you change the name of the structure to match (AKA include sdm845?)

OK
>
>
>>         },
>>         { },
>> @@ -736,6 +821,31 @@ static int qusb2_phy_probe(struct platform_device *pdev)
>>                 qphy->cell = NULL;
>>                 dev_dbg(dev, "failed to lookup tune2 hstx trim value\n");
>>         }
>> +
>> +       if (of_find_property(dev->of_node, "qcom,imp-res-offset-value", NULL)) {
> Get rid of the extra of_find_property().  Just use the error code from
> the property_read() to know if it was there and you've done two steps
> in one (read it and know if it was there).
>
That is better. Thanks.

>> +               qphy->override_imp_res_offset = true;
>> +               of_property_read_u8(dev->of_node, "qcom,imp-res-offset-value",
>> +                                   &qphy->imp_res_offset_value);
> You probably don't want of_property_read_u8(), even if you intend the
> property to bit in 8 bits.  You probably want to use
> of_property_read_u32() to read into a temporary value and then copy
> that to your 8-bit structure element.
>
> If you use of_property_read_u8 then you have to "/bits/ 8" in the
> device tree and that's ugly and doesn't seem to be what's done very
> often.  People just always stick a u32 in the device tree.

Sure, will do that.

>
>
> -Doug

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project