| 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
| ||
|
Date: Wed, 13 Apr 2022 18:16:56 +0800
From: Liu Ying <victor.liu@....com>
To: Vinod Koul <vkoul@...nel.org>
Cc: linux-phy@...ts.infradead.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, dri-devel@...ts.freedesktop.org,
linux-arm-kernel@...ts.infradead.org, kishon@...com,
robh+dt@...nel.org, andrzej.hajda@...el.com,
narmstrong@...libre.com, Laurent.pinchart@...asonboard.com,
jonas@...boo.se, jernej.skrabec@...il.com, airlied@...ux.ie,
daniel@...ll.ch, shawnguo@...nel.org, s.hauer@...gutronix.de,
kernel@...gutronix.de, festevam@...il.com, linux-imx@....com,
agx@...xcpu.org, robert.chiras@....com, martin.kepplinger@...i.sm,
robert.foss@...aro.org
Subject: Re: [PATCH v6 resend 5/5] phy: freescale: phy-fsl-imx8-mipi-dphy:
Add i.MX8qxp LVDS PHY mode support
Hi Vinod,
On Wed, 2022-04-13 at 11:51 +0530, Vinod Koul wrote:
> On 02-04-22, 13:24, Liu Ying wrote:
> > i.MX8qxp SoC embeds a Mixel MIPI DPHY + LVDS PHY combo which
> > supports
> > either a MIPI DSI display or a LVDS display. The PHY mode is
> > controlled
> > by SCU firmware and the driver would call a SCU firmware function
> > to
> > configure the PHY mode. The single LVDS PHY has 4 data lanes to
> > support
> > a LVDS display. Also, with a master LVDS PHY and a slave LVDS PHY,
> > they
> > may work together to support a LVDS display with 8 data
> > lanes(usually, dual
> > LVDS link display). Note that this patch supports the LVDS PHY
> > mode only
> > for the i.MX8qxp Mixel combo PHY, i.e., the MIPI DPHY mode is yet
> > to be
> > supported, so for now error would be returned from ->set_mode() if
> > MIPI
> > DPHY mode is passed over to it for the combo PHY.
> >
> > Cc: Guido Günther <agx@...xcpu.org>
> > Cc: Robert Chiras <robert.chiras@....com>
> > Cc: Kishon Vijay Abraham I <kishon@...com>
> > Cc: Vinod Koul <vkoul@...nel.org>
> > Cc: Shawn Guo <shawnguo@...nel.org>
> > Cc: Sascha Hauer <s.hauer@...gutronix.de>
> > Cc: Pengutronix Kernel Team <kernel@...gutronix.de>
> > Cc: Fabio Estevam <festevam@...il.com>
> > Cc: NXP Linux Team <linux-imx@....com>
> > Reviewed-by: Guido Günther <agx@...xcpu.org>
> > Signed-off-by: Liu Ying <victor.liu@....com>
> > ---
> > v5->v6:
> > * No change.
> >
> > v4->v5:
> > * No change.
> >
> > v3->v4:
> > * Add Guido's R-b tag.
> >
> > v2->v3:
> > * Improve readability of mixel_dphy_set_mode(). (Guido)
> >
> > v1->v2:
> > * Print invalid PHY mode in dmesg. (Guido)
> >
> > .../phy/freescale/phy-fsl-imx8-mipi-dphy.c | 269
> > +++++++++++++++++-
> > 1 file changed, 258 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > index a95572b397ca..af1ecda091c7 100644
> > --- a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > +++ b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> > @@ -4,17 +4,31 @@
> > * Copyright 2019 Purism SPC
> > */
> >
> > +#include <linux/bitfield.h>
> > #include <linux/clk.h>
> > #include <linux/clk-provider.h>
> > #include <linux/delay.h>
> > +#include <linux/firmware/imx/ipc.h>
> > +#include <linux/firmware/imx/svc/misc.h>
> > #include <linux/io.h>
> > #include <linux/kernel.h>
> > +#include <linux/mfd/syscon.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > #include <linux/of_platform.h>
> > #include <linux/phy/phy.h>
> > #include <linux/platform_device.h>
> > #include <linux/regmap.h>
> > +#include <dt-bindings/firmware/imx/rsrc.h>
> > +
> > +/* Control and Status Registers(CSR) */
> > +#define PHY_CTRL 0x00
> > +#define CCM_MASK GENMASK(7, 5)
> > +#define CCM(n) FIELD_PREP(CCM_MASK,
> > (n))
> > +#define CA_MASK GENMASK(4, 2)
> > +#define CA(n) FIELD_PREP(CA_MASK,
> > (n))
> > +#define RFB BIT(1)
> > +#define LVDS_EN BIT(0)
> >
> > /* DPHY registers */
> > #define DPHY_PD_DPHY 0x00
> > @@ -55,8 +69,15 @@
> > #define PWR_ON 0
> > #define PWR_OFF 1
> >
> > +#define MIN_VCO_FREQ 640000000
> > +#define MAX_VCO_FREQ 1500000000
> > +
> > +#define MIN_LVDS_REFCLK_FREQ 24000000
> > +#define MAX_LVDS_REFCLK_FREQ 150000000
> > +
> > enum mixel_dphy_devtype {
> > MIXEL_IMX8MQ,
> > + MIXEL_IMX8QXP,
> > };
> >
> > struct mixel_dphy_devdata {
> > @@ -65,6 +86,7 @@ struct mixel_dphy_devdata {
> > u8 reg_rxlprp;
> > u8 reg_rxcdrp;
> > u8 reg_rxhs_settle;
> > + bool is_combo; /* MIPI DPHY and LVDS PHY combo */
> > };
> >
> > static const struct mixel_dphy_devdata mixel_dphy_devdata[] = {
> > @@ -74,6 +96,10 @@ static const struct mixel_dphy_devdata
> > mixel_dphy_devdata[] = {
> > .reg_rxlprp = 0x40,
> > .reg_rxcdrp = 0x44,
> > .reg_rxhs_settle = 0x48,
> > + .is_combo = false,
> > + },
> > + [MIXEL_IMX8QXP] = {
> > + .is_combo = true,
> > },
> > };
> >
> > @@ -95,8 +121,12 @@ struct mixel_dphy_cfg {
> > struct mixel_dphy_priv {
> > struct mixel_dphy_cfg cfg;
> > struct regmap *regmap;
> > + struct regmap *lvds_regmap;
> > struct clk *phy_ref_clk;
> > const struct mixel_dphy_devdata *devdata;
> > + struct imx_sc_ipc *ipc_handle;
> > + bool is_slave;
> > + int id;
> > };
> >
> > static const struct regmap_config mixel_dphy_regmap_config = {
> > @@ -317,7 +347,8 @@ static int mixel_dphy_set_pll_params(struct phy
> > *phy)
> > return 0;
> > }
> >
> > -static int mixel_dphy_configure(struct phy *phy, union
> > phy_configure_opts *opts)
> > +static int
> > +mixel_dphy_configure_mipi_dphy(struct phy *phy, union
> > phy_configure_opts *opts)
>
> Please dont change things like this.. A patch should do only one
> thing :)
The function name is changed by purpose - the phy in i.MX8qxp SoC is a
combo phy(lvds or mipi dphy). The new function name can be
distinguished from the added function mixel_dphy_configure_lvds_phy().
>
> > {
> > struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > struct mixel_dphy_cfg cfg = { 0 };
> > @@ -345,15 +376,121 @@ static int mixel_dphy_configure(struct phy
> > *phy, union phy_configure_opts *opts)
> > return 0;
> > }
> >
> > +static int
> > +mixel_dphy_configure_lvds_phy(struct phy *phy, union
> > phy_configure_opts *opts)
> > +{
> > + struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> > + struct phy_configure_opts_lvds *lvds_opts = &opts->lvds;
> > + unsigned long data_rate;
> > + unsigned long fvco;
> > + u32 rsc;
> > + u32 co;
> > + int ret;
> > +
> > + priv->is_slave = lvds_opts->is_slave;
>
> I think you should check if lvds_opts is valid before dereferencing
> it..
In next verstion, I may check !opts in mixel_dphy_configure() for both
lvds and mipi dphy.
>
> > +
> > + /* LVDS interface pins */
> > + regmap_write(priv->lvds_regmap, PHY_CTRL, CCM(0x5) | CA(0x4) |
> > RFB);
>
> can we avoid magic numbers?
Will add macros CCM_1_2V and CA_3_51MA in next version.
>
> > +
> > + /* enable MODE8 only for slave LVDS PHY */
> > + rsc = priv->id ? IMX_SC_R_MIPI_1 : IMX_SC_R_MIPI_0;
> > + ret = imx_sc_misc_set_control(priv->ipc_handle, rsc,
> > IMX_SC_C_DUAL_MODE,
> > + lvds_opts->is_slave);
> > + if (ret) {
> > + dev_err(&phy->dev, "Failed to configure MODE8: %d\n",
> > ret);
> > + return ret;
> > + }
> > +
> > + /*
> > + * Choose an appropriate divider ratio to meet the requirement
> > of
> > + * PLL VCO frequency range.
> > + *
> > + * ----- 640MHz ~ 1500MHz ------------ ---------------
> > + * | VCO | ----------------> | CO divider | -> | LVDS data
> > rate|
> > + * ----- FVCO ------------ ---------------
> > + * 1/2/4/8 div 7 *
> > differential_clk_rate
> > + */
> > + data_rate = 7 * lvds_opts->differential_clk_rate;
> > + for (co = 1; co <= 8; co *= 2) {
> > + fvco = data_rate * co;
> > +
> > + if (fvco >= MIN_VCO_FREQ)
> > + break;
> > + }
> > +
> > + if (fvco < MIN_VCO_FREQ || fvco > MAX_VCO_FREQ) {
> > + dev_err(&phy->dev, "VCO frequency %lu is out of
> > range\n", fvco);
> > + return -ERANGE;
> > + }
> > +
> > + /*
> > + * CO is configurable, while CN and CM are not,
> > + * as fixed ratios 1 and 7 are applied respectively.
> > + */
> > + phy_write(phy, __ffs(co), DPHY_CO);
> > +
> > + /* set reference clock rate */
> > + clk_set_rate(priv->phy_ref_clk, lvds_opts-
> > >differential_clk_rate);
> > +
> > + return ret;
> > +}
> > +
> > +static int mixel_dphy_configure(struct phy *phy, union
> > phy_configure_opts *opts)
> > +{
> > + if (phy->attrs.mode == PHY_MODE_MIPI_DPHY)
> > + return mixel_dphy_configure_mipi_dphy(phy, opts);
> > + else if (phy->attrs.mode == PHY_MODE_LVDS)
> > + return mixel_dphy_configure_lvds_phy(phy, opts);
> > +
> > + dev_err(&phy->dev,
> > + "Failed to configure PHY with invalid PHY mode: %d\n",
> > + phy-
> > >attrs.mode);
>
> single line for this please
Will do.
>
> > + return -EINVAL;
> > +}
> > +
> > +static int
> > +mixel_dphy_validate_lvds_phy(struct phy *phy, union
> > phy_configure_opts *opts)
> > +{
> > + struct phy_configure_opts_lvds *lvds_cfg = &opts->lvds;
> > +
> > + if (lvds_cfg->bits_per_lane_and_dclk_cycle != 7) {
> > + dev_err(&phy->dev, "Invalid bits per LVDS data lane:
> > %u\n",
> > + lvds_cfg-
> > >bits_per_lane_and_dclk_cycle);
> > + return -EINVAL;
> > + }
> > +
> > + if (lvds_cfg->lanes != 4) {
> > + dev_err(&phy->dev, "Invalid LVDS data lanes: %u\n",
> > + lvds_cfg->lanes);
>
> here too, make it better read
Ok.
>
> > + return -EINVAL;
> > + }
> > +
> > + if (lvds_cfg->differential_clk_rate < MIN_LVDS_REFCLK_FREQ ||
> > + lvds_cfg->differential_clk_rate > MAX_LVDS_REFCLK_FREQ) {
> > + dev_err(&phy->dev,
> > + "Invalid LVDS differential clock rate: %lu\n",
> > + lvds_cfg-
> > >differential_clk_rate);
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int mixel_dphy_validate(struct phy *phy, enum phy_mode
> > mode, int submode,
> > union phy_configure_opts *opts)
> > {
> > - struct mixel_dphy_cfg cfg = { 0 };
> > + if (mode == PHY_MODE_MIPI_DPHY) {
> > + struct mixel_dphy_cfg mipi_dphy_cfg = { 0 };
> >
> > - if (mode != PHY_MODE_MIPI_DPHY)
> > - return -EINVAL;
> > + return mixel_dphy_config_from_opts(phy, &opts-
> > >mipi_dphy,
> > + &mipi_dphy_cfg)
> > ;
>
> the line should be justified to previos line open brace (checkpatch
> --strict will warn you about this style miss)
Ok. Will suppress all 'checkpatch --strict' warnings.
Regards,
Liu Ying
Powered by blists - more mailing lists