| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAD=FV=X70gNedZjCiKQTOqtDf5UttViAiibgRjCoKWVJZqcKEQ@mail.gmail.com>
Date: Thu, 29 Mar 2018 13:38:09 -0700
From: Doug Anderson <dianders@...omium.org>
To: Manu Gautam <mgautam@...eaurora.org>
Cc: Kishon Vijay Abraham I <kishon@...com>,
Rob Herring <robh@...nel.org>,
Stephen Boyd <sboyd@...eaurora.org>,
LKML <linux-kernel@...r.kernel.org>, devicetree@...r.kernel.org,
Rob Herring <robh+dt@...nel.org>,
Vivek Gautam <vivek.gautam@...eaurora.org>,
Evan Green <evgreen@...omium.org>,
linux-arm-msm@...r.kernel.org,
Viresh Kumar <viresh.kumar@...aro.org>
Subject: Re: [PATCH v4 7/7] phy: qcom-qusb2: Add QUSB2 PHYs support for sdm845
Hi,
On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@...eaurora.org> wrote:
> There are two QUSB2 PHYs present on sdm845. In order
> to improve eye diagram for both the PHYs some parameters
> need to be changed. Provide device tree properties to
> override these from board specific device tree files.
>
> Signed-off-by: Manu Gautam <mgautam@...eaurora.org>
> ---
> drivers/phy/qualcomm/phy-qcom-qusb2.c | 112 +++++++++++++++++++++++++++++++++-
> 1 file changed, 111 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> index 40fdef8..adcc3f8 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> @@ -60,6 +60,17 @@
> #define CORE_RESET BIT(5)
> #define CORE_RESET_MUX BIT(6)
>
> +/* QUSB2PHY_IMP_CTRL1 register bits */
> +#define IMP_RES_OFFSET_MASK GENMASK(5, 0)
> +#define IMP_RES_OFFSET_SHIFT 0x0
> +
> +/* QUSB2PHY_PORT_TUNE1 register bits */
> +#define HSTX_TRIM_MASK GENMASK(7, 4)
> +#define HSTX_TRIM_SHIFT 0x4
> +#define PREEMPH_HALF_WIDTH BIT(2)
> +#define PREEMPHASIS_EN_MASK GENMASK(1, 0)
> +#define PREEMPHASIS_EN_SHIFT 0x0
> +
> #define QUSB2PHY_PLL_ANALOG_CONTROLS_TWO 0x04
> #define QUSB2PHY_PLL_CLOCK_INVERTERS 0x18c
> #define QUSB2PHY_PLL_CMODE 0x2c
> @@ -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.
> * @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).
> + 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()?
> +}
> +
> 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.
> + */
> +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?)
> },
> { },
> @@ -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).
> + 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.
-Doug
Powered by blists - more mailing lists