| 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: <e39faddb23c80a5cfa600109ae2d20335435c234.camel@nxp.com>
Date: Tue, 05 Jul 2022 23:10:29 +0800
From: Liu Ying <victor.liu@....com>
To: Vinod Koul <vkoul@...nel.org>
Cc: linux-phy@...ts.infradead.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
kishon@...com, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, shawnguo@...nel.org,
s.hauer@...gutronix.de, kernel@...gutronix.de, festevam@...il.com,
linux-imx@....com, krzysztof.kozlowski@...aro.org
Subject: Re: [PATCH v3 3/3] phy: freescale: Add i.MX8qm Mixel LVDS PHY
support
On Tue, 2022-07-05 at 14:24 +0530, Vinod Koul wrote:
> On 20-06-22, 20:38, Liu Ying wrote:
> > Add Freescale i.MX8qm LVDS PHY support.
> > The PHY IP is from Mixel, Inc.
> >
> > Signed-off-by: Liu Ying <victor.liu@....com>
> > ---
> > v2->v3:
> > * No change.
> >
> > v1->v2:
> > * Drop 'This patch' from commit message. (Krzysztof)
> > * Make dev_err_probe() function calls as one-liners. (Krzysztof)
> > * Drop unnecessary debug messages. (Krzysztof)
> > * Get phy_ref_clk without name specified due to 'clock-names'
> > dropped from
> > dt-binding.
> >
> > drivers/phy/freescale/Kconfig | 9 +
> > drivers/phy/freescale/Makefile | 1 +
> > .../phy/freescale/phy-fsl-imx8qm-lvds-phy.c | 440
> > ++++++++++++++++++
> > 3 files changed, 450 insertions(+)
> > create mode 100644 drivers/phy/freescale/phy-fsl-imx8qm-lvds-phy.c
> >
> > diff --git a/drivers/phy/freescale/Kconfig
> > b/drivers/phy/freescale/Kconfig
> > index f9c54cd02036..853958fb2c06 100644
> > --- a/drivers/phy/freescale/Kconfig
> > +++ b/drivers/phy/freescale/Kconfig
> > @@ -8,6 +8,15 @@ config PHY_FSL_IMX8MQ_USB
> > select GENERIC_PHY
> > default ARCH_MXC && ARM64
> >
> > +config PHY_MIXEL_LVDS_PHY
> > + tristate "Mixel LVDS PHY support"
> > + depends on OF
> > + select GENERIC_PHY
> > + select REGMAP_MMIO
> > + help
> > + Enable this to add support for the Mixel LVDS PHY as found
> > + on NXP's i.MX8qm SoC.
> > +
> > config PHY_MIXEL_MIPI_DPHY
> > tristate "Mixel MIPI DSI PHY support"
> > depends on OF && HAS_IOMEM
> > diff --git a/drivers/phy/freescale/Makefile
> > b/drivers/phy/freescale/Makefile
> > index 3518d5dbe8a7..cedb328bc4d2 100644
> > --- a/drivers/phy/freescale/Makefile
> > +++ b/drivers/phy/freescale/Makefile
> > @@ -1,5 +1,6 @@
> > # SPDX-License-Identifier: GPL-2.0-only
> > obj-$(CONFIG_PHY_FSL_IMX8MQ_USB) += phy-fsl-imx8mq-usb.o
> > +obj-$(CONFIG_PHY_MIXEL_LVDS_PHY) += phy-fsl-imx8qm-lvds-phy.o
> > obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY) += phy-fsl-imx8-mipi-dphy.o
> > obj-$(CONFIG_PHY_FSL_IMX8M_PCIE) += phy-fsl-imx8m-pcie.o
> > obj-$(CONFIG_PHY_FSL_LYNX_28G) += phy-fsl-lynx-28g.o
> > diff --git a/drivers/phy/freescale/phy-fsl-imx8qm-lvds-phy.c
> > b/drivers/phy/freescale/phy-fsl-imx8qm-lvds-phy.c
> > new file mode 100644
> > index 000000000000..37f77115ddab
> > --- /dev/null
> > +++ b/drivers/phy/freescale/phy-fsl-imx8qm-lvds-phy.c
> > @@ -0,0 +1,440 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2017-2020,2022 NXP
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/clk.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> > +
> > +#define REG_SET 0x4
> > +#define REG_CLR 0x8
> > +
> > +#define PHY_CTRL 0x0
> > +#define M_MASK GENMASK(18, 17)
> > +#define M(n) FIELD_PREP(M_MASK, (n))
> > +#define CCM_MASK GENMASK(16, 14)
> > +#define CCM(n) FIELD_PREP(CCM_MASK, (n))
> > +#define CA_MASK GENMASK(13, 11)
> > +#define CA(n) FIELD_PREP(CA_MASK, (n))
> > +#define TST_MASK GENMASK(10, 5)
> > +#define TST(n) FIELD_PREP(TST_MASK, (n))
> > +#define CH_EN(id) BIT(3 + (id))
> > +#define NB BIT(2)
> > +#define RFB BIT(1)
> > +#define PD BIT(0)
> > +
> > +/* Power On Reset(POR) value */
> > +#define CTRL_RESET_VAL (M(0x0) | CCM(0x4) | CA(0x4) |
> > TST(0x25))
> > +
> > +/* PHY initialization value and mask */
> > +#define CTRL_INIT_MASK (M_MASK | CCM_MASK | CA_MASK | TST_MASK
> > | NB | RFB)
> > +#define CTRL_INIT_VAL (M(0x0) | CCM(0x5) | CA(0x4) |
> > TST(0x25) | RFB)
> > +
> > +#define PHY_STATUS 0x10
> > +#define LOCK BIT(0)
> > +
> > +#define PHY_NUM 2
> > +
> > +#define MIN_CLKIN_FREQ 25000000
>
> this is 25MHz, so lets write as 25 * MEGA (see units.h)
Will do.
>
> > +#define MAX_CLKIN_FREQ 165000000
>
> here too
Will do.
> > +
> > +#define PLL_LOCK_SLEEP 10
> > +#define PLL_LOCK_TIMEOUT 1000
> > +
> >
[...]
> > +
> > +static int mixel_lvds_phy_power_off(struct phy *phy)
> > +{
> > + struct mixel_lvds_phy_priv *priv = dev_get_drvdata(phy-
> > >dev.parent);
> > + struct mixel_lvds_phy *lvds_phy = phy_get_drvdata(phy);
> > +
> > + mutex_lock(&priv->lock);
> > + regmap_write(priv->regmap, PHY_CTRL + REG_CLR, CH_EN(lvds_phy-
> > >id));
> > + mutex_unlock(&priv->lock);
>
> No check for slave here?
I don't worry too much about separate channel power off. But, it should
be fine to power off the two channels together if the companion is a
slave phy. So, will check for slave here.
>
> > +
> > + clk_disable_unprepare(priv->phy_ref_clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int mixel_lvds_phy_configure(struct phy *phy,
> > + union phy_configure_opts *opts)
> > +{
> > + struct mixel_lvds_phy_priv *priv = dev_get_drvdata(phy-
> > >dev.parent);
> > + struct phy_configure_opts_lvds *cfg = &opts->lvds;
> > + int ret;
> > +
> > + ret = clk_set_rate(priv->phy_ref_clk, cfg-
> > >differential_clk_rate);
> > + if (ret)
> > + dev_err(&phy->dev,
> > + "failed to set PHY reference clock rate(%lu):
> > %d\n",
>
> this can fit in a single line (100 chars is okay)
Will do.
>
> > + cfg->differential_clk_rate, ret);
> > +
> > + return ret;
> > +}
> > +
> >
[...]
> > +static int mixel_lvds_phy_validate(struct phy *phy, enum phy_mode
> > mode,
> > + int submode, union
> > phy_configure_opts *opts)
> > +{
> > + struct mixel_lvds_phy_priv *priv = dev_get_drvdata(phy-
> > >dev.parent);
> > + struct mixel_lvds_phy *lvds_phy = phy_get_drvdata(phy);
> > + struct phy_configure_opts_lvds *cfg = &opts->lvds;
> > + int ret = 0;
> > +
> > + if (mode != PHY_MODE_LVDS) {
> > + dev_err(&phy->dev, "invalid PHY mode(%d)\n", mode);
> > + return -EINVAL;
> > + }
> > +
> > + if (cfg->bits_per_lane_and_dclk_cycle != 7 &&
> > + cfg->bits_per_lane_and_dclk_cycle != 10) {
> > + dev_err(&phy->dev, "invalid bits per data lane(%u)\n",
> > + cfg->bits_per_lane_and_dclk_cycle);
> > + return -EINVAL;
> > + }
> > +
> > + if (cfg->lanes != 4 && cfg->lanes != 3) {
> > + dev_err(&phy->dev, "invalid data lanes(%u)\n", cfg-
> > >lanes);
> > + return -EINVAL;
> > + }
> > +
> > + if (cfg->differential_clk_rate < MIN_CLKIN_FREQ ||
> > + cfg->differential_clk_rate > MAX_CLKIN_FREQ) {
> > + dev_err(&phy->dev, "invalid differential clock
> > rate(%lu)\n",
> > + cfg->differential_clk_rate);
> > + return -EINVAL;
> > + }
> > +
> > + mutex_lock(&priv->lock);
> > + /* cache configuration set of our own for check */
> > + memcpy(&lvds_phy->cfg, cfg, sizeof(*cfg));
> > +
> > + if (cfg->is_slave) {
> > + ret = mixel_lvds_phy_check_slave(phy);
> > + if (ret)
> > + dev_err(&phy->dev,
> > + "failed to check slave PHY: %d\n",
> > ret);
>
> very ugly, single line pls
Will do.
>
> > + }
> > + mutex_unlock(&priv->lock);
> > +
> > + return ret;
> > +}
> > +
> >
[...]
> > +
> > +static int mixel_lvds_phy_reset(struct device *dev)
> > +{
> > + struct mixel_lvds_phy_priv *priv = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + ret = pm_runtime_get_sync(dev);
>
> pm_runtime_resume_and_get() pls
Will do.
>
> > + if (ret < 0) {
> > + dev_err(dev, "failed to get PM runtime: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + regmap_write(priv->regmap, PHY_CTRL, CTRL_RESET_VAL);
> > +
> > + ret = pm_runtime_put(dev);
>
> this seems to be done only around reset, why not in on/off method?
The reason is that phy-core.c does that (See phy_pm_runtime_get_sync
and phy_pm_runtime_put).
Thanks,
Liu Ying
Powered by blists - more mailing lists