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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Thu, 30 Aug 2018 14:42:43 +0000
From:   Alan Douglas <adouglas@...ence.com>
To:     Kishon Vijay Abraham I <kishon@...com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     Scott Telford <stelford@...ence.com>
Subject: RE: [RFC PATCH 2/2] phy: cadence: Add driver for Sierra PHY

Hi,

Thanks for your comments.

On 20 August 2018 14:07, Kishon Vijay Abraham wrote:
> Hi,
> 
> On Friday 17 August 2018 06:01 PM, Alan Douglas wrote:
> > Add a Sierra PHY driver with PCIe and USB support.
> > There are two resets to the PHY, one to enable
> > the APB interface for programming registers, and
> > another to enable the PHY itself.
> >
> > The sequence of operation on startup is to enable
> > the APB bus, write the PHY registers and then
> > enable the PHY.
> >
> > On power-down we simply put the PHY and APB bus in
> > reset.
> >
> > Signed-off-by: Alan Douglas <adouglas@...ence.com>
> > ---
> >  drivers/phy/Kconfig               |   1 +
> >  drivers/phy/Makefile              |   1 +
> >  drivers/phy/cadence/Kconfig       |   9 ++
> >  drivers/phy/cadence/Makefile      |   3 +
> >  drivers/phy/cadence/cdns-sierra.c | 326 ++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 340 insertions(+)
> >  create mode 100644 drivers/phy/cadence/Kconfig
> >  create mode 100644 drivers/phy/cadence/Makefile
> >  create mode 100644 drivers/phy/cadence/cdns-sierra.c
> >
> > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> > index 5c8d452..cc47f85 100644
> > --- a/drivers/phy/Kconfig
> > +++ b/drivers/phy/Kconfig
> > @@ -43,6 +43,7 @@ config PHY_XGENE
> >  source "drivers/phy/allwinner/Kconfig"
> >  source "drivers/phy/amlogic/Kconfig"
> >  source "drivers/phy/broadcom/Kconfig"
> > +source "drivers/phy/cadence/Kconfig"
> >  source "drivers/phy/hisilicon/Kconfig"
> >  source "drivers/phy/lantiq/Kconfig"
> >  source "drivers/phy/marvell/Kconfig"
> > diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> > index 84e3bd9..ba48acd 100644
> > --- a/drivers/phy/Makefile
> > +++ b/drivers/phy/Makefile
> > @@ -15,6 +15,7 @@ obj-$(CONFIG_ARCH_RENESAS)		+= renesas/
> >  obj-$(CONFIG_ARCH_ROCKCHIP)		+= rockchip/
> >  obj-$(CONFIG_ARCH_TEGRA)		+= tegra/
> >  obj-y					+= broadcom/	\
> > +					   cadence/	\
> >  					   hisilicon/	\
> >  					   marvell/	\
> >  					   motorola/	\
> > diff --git a/drivers/phy/cadence/Kconfig b/drivers/phy/cadence/Kconfig
> > new file mode 100644
> > index 0000000..6bbe5a2
> > --- /dev/null
> > +++ b/drivers/phy/cadence/Kconfig
> > @@ -0,0 +1,9 @@
> > +#
> > +# Phy drivers for cadence devices
> > +#
> > +config CDNS_SIERRA_PHY
> > +	tristate "CDNS Sierra PHY Driver"
> > +	depends on OF && HAS_IOMEM
> 
> depends on RESET_CONTROLLER?
Yes, I will add this.

> > +	select GENERIC_PHY
> > +	help
> > +	  Enable this to support the Cadence Sierra PHY driver
> > diff --git a/drivers/phy/cadence/Makefile b/drivers/phy/cadence/Makefile
> > new file mode 100644
> > index 0000000..b384cb3
> > --- /dev/null
> > +++ b/drivers/phy/cadence/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +obj-$(CONFIG_CDNS_SIERRA_PHY)		+= cdns-sierra.o
> > +
> > diff --git a/drivers/phy/cadence/cdns-sierra.c b/drivers/phy/cadence/cdns-sierra.c
> > new file mode 100644
> > index 0000000..0581516
> > --- /dev/null
> > +++ b/drivers/phy/cadence/cdns-sierra.c
> > @@ -0,0 +1,326 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Cadence Sierra PHY Driver
> > + *
> > + * Copyright (c) 2018 Cadence Design Systems
> > + * Author: Alan Douglas <adouglas@...ence.com>
> > + *
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> > +#include <linux/reset.h>
> > +#include <linux/slab.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <dt-bindings/phy/phy.h>
> > +
> > +#define CDNS_PHY_PLL_CFG		(0xc00e << 2)
> > +#define CDNS_DET_STANDEC_A		(0x4000 << 2)
> > +#define CDNS_DET_STANDEC_B		(0x4001 << 2)
> > +#define CDNS_DET_STANDEC_C		(0x4002 << 2)
> > +#define CDNS_DET_STANDEC_D		(0x4003 << 2)
> > +#define CDNS_DET_STANDEC_E		(0x4004 << 2)
> > +#define CDNS_PSM_LANECAL		(0x4008 << 2)
> > +#define CDNS_PSM_DIAG			(0x4015 << 2)
> > +#define CDNS_PSC_TX_A0			(0x4028 << 2)
> > +#define CDNS_PSC_TX_A1			(0x4029 << 2)
> > +#define CDNS_PSC_TX_A2			(0x402A << 2)
> > +#define CDNS_PSC_TX_A3			(0x402B << 2)
> > +#define CDNS_PSC_RX_A0			(0x4030 << 2)
> > +#define CDNS_PSC_RX_A1			(0x4031 << 2)
> > +#define CDNS_PSC_RX_A2			(0x4032 << 2)
> > +#define CDNS_PSC_RX_A3			(0x4033 << 2)
> > +#define CDNS_PLLCTRL_SUBRATE		(0x403A << 2)
> > +#define CDNS_PLLCTRL_GEN_D		(0x403E << 2)
> > +#define CDNS_DRVCTRL_ATTEN		(0x406A << 2)
> > +#define CDNS_CLKPATHCTRL_TMR		(0x4081 << 2)
> > +#define CDNS_RX_CREQ_FLTR_A_MODE1	(0x4087 << 2)
> > +#define CDNS_RX_CREQ_FLTR_A_MODE0	(0x4088 << 2)
> > +#define CDNS_CREQ_CCLKDET_MODE01	(0x408E << 2)
> > +#define CDNS_RX_CTLE_MAINTENANCE	(0x4091 << 2)
> > +#define CDNS_CREQ_FSMCLK_SEL		(0x4092 << 2)
> > +#define CDNS_CTLELUT_CTRL		(0x4098 << 2)
> > +#define CDNS_DFE_ECMP_RATESEL		(0x40C0 << 2)
> > +#define CDNS_DFE_SMP_RATESEL		(0x40C1 << 2)
> > +#define CDNS_DEQ_VGATUNE_CTRL		(0x40E1 << 2)
> > +#define CDNS_TMRVAL_MODE3		(0x416E << 2)
> > +#define CDNS_TMRVAL_MODE2		(0x416F << 2)
> > +#define CDNS_TMRVAL_MODE1		(0x4170 << 2)
> > +#define CDNS_TMRVAL_MODE0		(0x4171 << 2)
> > +#define CDNS_PICNT_MODE1		(0x4174 << 2)
> > +#define CDNS_CPI_OUTBUF_RATESEL		(0x417C << 2)
> > +#define CDNS_LFPSFILT_NS		(0x418A << 2)
> > +#define CDNS_LFPSFILT_RD		(0x418B << 2)
> > +#define CDNS_LFPSFILT_MP		(0x418C << 2)
> > +#define CDNS_SDFILT_H2L_A		(0x4191 << 2)
> > +
> > +#define SIERRA_PHYS_NUM 2
> > +
> > +struct cdns_phy_instance {
> > +	struct phy *phy;
> > +	u32 phy_type;
> > +	u32 nlanes;
> > +	void __iomem *base;
> > +};
> > +
> > +struct cdns_sierra_phy {
> > +	struct device *dev;
> > +	void __iomem *base;
> > +	u32 *init_data;
> > +	struct cdns_phy_instance phys[SIERRA_PHYS_NUM];
> > +	struct reset_control *reset;
> > +	struct reset_control *apb_reset;
> > +	struct clk *clk;
> > +	struct phy *pcie_phy;
> > +};
> > +
> > +static void cdns_sierra_pcie_on(struct cdns_phy_instance *ins)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < (ins->nlanes << 11); i += (1 << 11)) {
> > +		writel(0x891f, ins->base + CDNS_DET_STANDEC_D + i);
> > +		writel(0x0053, ins->base + CDNS_DET_STANDEC_E + i);
> > +		writel(0x0400, ins->base + CDNS_TMRVAL_MODE2 + i);
> > +		writel(0x0200, ins->base + CDNS_TMRVAL_MODE3 + i);
> > +	}
> > +}
> > +
> > +static void cdns_sierra_usb_on(struct cdns_phy_instance *ins)
> > +{
> > +	int i;
> > +
> > +	writel(0x0000, ins->base + CDNS_PHY_PLL_CFG);
> > +	for (i = 0; i < (ins->nlanes << 11); i += (1 << 11)) {
> > +		writel(0xFE0A, ins->base + CDNS_DET_STANDEC_A + i);
> > +		writel(0x000F, ins->base + CDNS_DET_STANDEC_B + i);
> > +		writel(0x55A5, ins->base + CDNS_DET_STANDEC_C + i);
> > +		writel(0x69AD, ins->base + CDNS_DET_STANDEC_D + i);
> > +		writel(0x0241, ins->base + CDNS_DET_STANDEC_E + i);
> > +		writel(0x0110, ins->base + CDNS_PSM_LANECAL + i);
> > +		writel(0xCF00, ins->base + CDNS_PSM_DIAG + i);
> > +		writel(0x001F, ins->base + CDNS_PSC_TX_A0 + i);
> > +		writel(0x0007, ins->base + CDNS_PSC_TX_A1 + i);
> > +		writel(0x0003, ins->base + CDNS_PSC_TX_A2 + i);
> > +		writel(0x0003, ins->base + CDNS_PSC_TX_A3 + i);
> > +		writel(0x0FFF, ins->base + CDNS_PSC_RX_A0 + i);
> > +		writel(0x0003, ins->base + CDNS_PSC_RX_A1 + i);
> > +		writel(0x0003, ins->base + CDNS_PSC_RX_A2 + i);
> > +		writel(0x0001, ins->base + CDNS_PSC_RX_A3 + i);
> > +		writel(0x0001, ins->base + CDNS_PLLCTRL_SUBRATE + i);
> > +		writel(0x0406, ins->base + CDNS_PLLCTRL_GEN_D + i);
> > +		writel(0x0000, ins->base + CDNS_DRVCTRL_ATTEN + i);
> > +		writel(0x823E, ins->base + CDNS_CLKPATHCTRL_TMR + i);
> > +		writel(0x078F, ins->base + CDNS_RX_CREQ_FLTR_A_MODE1 + i);
> > +		writel(0x078F, ins->base + CDNS_RX_CREQ_FLTR_A_MODE0 + i);
> > +		writel(0x7B3C, ins->base + CDNS_CREQ_CCLKDET_MODE01 + i);
> > +		writel(0x023C, ins->base + CDNS_RX_CTLE_MAINTENANCE + i);
> > +		writel(0x3232, ins->base + CDNS_CREQ_FSMCLK_SEL + i);
> > +		writel(0x8452, ins->base + CDNS_CTLELUT_CTRL + i);
> > +		writel(0x4121, ins->base + CDNS_DFE_ECMP_RATESEL + i);
> > +		writel(0x4121, ins->base + CDNS_DFE_SMP_RATESEL + i);
> > +		writel(0x9999, ins->base + CDNS_DEQ_VGATUNE_CTRL + i);
> > +		writel(0x0330, ins->base + CDNS_TMRVAL_MODE0 + i);
> > +		writel(0x01FF, ins->base + CDNS_PICNT_MODE1 + i);
> > +		writel(0x0009, ins->base + CDNS_CPI_OUTBUF_RATESEL + i);
> > +		writel(0x000F, ins->base + CDNS_LFPSFILT_NS + i);
> > +		writel(0x0009, ins->base + CDNS_LFPSFILT_RD + i);
> > +		writel(0x0001, ins->base + CDNS_LFPSFILT_MP + i);
> > +		writel(0x8013, ins->base + CDNS_SDFILT_H2L_A + i);
> > +		writel(0x0400, ins->base + CDNS_TMRVAL_MODE1 + i);
> 
> Are these tuning parameters? Don't they change for different platforms/vendors?
These parameters may vary depending on the actual platform.  I need to rethink how these are included.

> > +	}
> > +}
> > +
> > +static int cdns_sierra_phy_on(struct phy *x)
> > +{
> > +	struct cdns_phy_instance *ins = phy_get_drvdata(x);
> > +	struct cdns_sierra_phy *phy = dev_get_drvdata(x->dev.parent);
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(phy->clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Ensure the PHY is in reset */
> > +	reset_control_assert(phy->reset);
> > +
> > +	/* Enable APB */
> > +	reset_control_deassert(phy->apb_reset);
> > +
> > +	/* Program the PHY registers*/
> > +	if (ins->phy_type == PHY_TYPE_PCIE)
> > +		cdns_sierra_pcie_on(ins);
> > +	else
> > +		cdns_sierra_usb_on(ins);
> > +
> > +	/* Take the PHY out of reset */
> > +	ret = reset_control_assert(phy->reset);
> > +
> > +	if (ret) {
> > +		clk_disable_unprepare(phy->clk);
> > +		return ret;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int cdns_sierra_phy_off(struct phy *x)
> > +{
> > +	struct cdns_sierra_phy *phy = dev_get_drvdata(x->dev.parent);
> > +	int ret = 0;
> > +
> > +	reset_control_assert(phy->reset);
> > +	reset_control_assert(phy->apb_reset);
> > +	clk_disable_unprepare(phy->clk);
> > +	dev_info(phy->dev, "sierra PHY OFF\n");
> 
> dev_vdbg?
I will remove, it was for debug and shouldn't be required.

> > +
> > +	return ret;
> > +}
> > +
> > +static const struct phy_ops ops = {
> > +	.power_on	= cdns_sierra_phy_on,
> > +	.power_off	= cdns_sierra_phy_off,
> > +	.owner		= THIS_MODULE,
> > +};
> > +
> > +static const struct of_device_id cdns_sierra_id_table[];
> > +static struct phy *cdns_sierra_xlate(struct device *dev,
> > +				     struct of_phandle_args *args)
> > +{
> > +	struct cdns_sierra_phy *sphy = dev_get_drvdata(dev);
> > +	struct cdns_phy_instance *ins = NULL;
> > +	int i;
> > +
> > +	if (args->args_count != 2) {
> > +		dev_err(dev, "invalid number of cells in 'phy' property\n");
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	if (WARN_ON(args->args[1] == 0 || args->args[1] > 4))
> > +		return ERR_PTR(-ENODEV);
> > +
> > +	for (i = 0; i < SIERRA_PHYS_NUM; i++) {
> > +		if (sphy->phys[i].phy_type == args->args[0])
> > +			ins = &sphy->phys[i];
> > +	}
> > +
> > +	if (!ins) {
> > +		dev_err(dev, "failed to find appropriate phy\n");
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	ins->nlanes = args->args[1];
> > +
> > +	return ins->phy;
> > +}
> > +
> > +static int cdns_sierra_phy_probe(struct platform_device *pdev)
> > +{
> > +	struct cdns_sierra_phy *sphy;
> > +	struct phy_provider *phy_provider;
> > +	struct device *dev = &pdev->dev;
> > +	const struct of_device_id *match;
> > +	struct resource *res;
> > +	int i;
> > +
> > +	sphy = devm_kzalloc(dev, sizeof(*sphy), GFP_KERNEL);
> > +	if (!sphy)
> > +		return -ENOMEM;
> > +
> > +	sphy->dev = dev;
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "reg");
> > +	sphy->base = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(sphy->base)) {
> > +		dev_err(dev, "missing \"reg\"\n");
> > +		return PTR_ERR(sphy->base);
> > +	}
> > +
> > +	/* Get init data for this PHY */
> > +	match = of_match_device(cdns_sierra_id_table, dev);
> > +	if (!match)
> > +		return -EINVAL;
> > +	sphy->init_data = (u32 *)match->data;
> > +
> > +	/* Check that PHY is present */
> > +	if  (sphy->init_data[0] != readl(sphy->base))
> > +		return -EINVAL;
> > +
> > +	platform_set_drvdata(pdev, sphy);
> > +
> > +	sphy->clk = devm_clk_get(dev, "phy_clk");
> > +	if (IS_ERR(sphy->clk)) {
> > +		dev_err(dev, "failed to get clock phy_clk\n");
> > +		return PTR_ERR(sphy->clk);
> > +	}
> > +
> > +	sphy->reset = devm_reset_control_get(dev, "sierra_reset");
> > +	if (IS_ERR(sphy->reset)) {
> > +		dev_err(dev, "failed to get reset\n");
> > +		return PTR_ERR(sphy->reset);
> > +	}
> > +
> > +	sphy->apb_reset = devm_reset_control_get(dev, "sierra_apb");
> > +	if (IS_ERR(sphy->apb_reset)) {
> > +		dev_err(dev, "failed to get apb reset\n");
> > +		return PTR_ERR(sphy->apb_reset);
> > +	}
> > +
> > +	sphy->phys[0].phy_type = PHY_TYPE_PCIE;
> > +	sphy->phys[1].phy_type = PHY_TYPE_USB3;
> 
> We can have two subnodes, one for PCIe PHY and one for USB3 PHY. That way we
> don't create two PHY's if a vendor hasn't exposed one type of PHY. We also
> don't need a custom xlate then.
Makes sense.  I will make this change and issue a new patch.
Thanks,
Alan
> 
> Thanks
> Kishon

Powered by blists - more mailing lists