Message-ID: <MWHPR07MB3502A73F57AA5A69847EB6D9D8170@MWHPR07MB3502.namprd07.prod.outlook.com>
Date:   Mon, 24 Sep 2018 09:08:51 +0000
From:   Alan Douglas <adouglas@...ence.com>
To:     Kishon Vijay Abraham I <kishon@...com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: RE: [RFC PATCH v2 2/2] phy: cadence: Add driver for Sierra PHY

Hi,

On 20 September 2018 11:10, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Thursday 06 September 2018 08:12 PM, Alan Douglas wrote:
> > Add a Sierra PHY driver with PCIe and USB support.
> >
> > The PHY has multiple lanes, which can be configured into
> > groups, and a generic PHY device is created for each group.
> >
> > There are two resets controlling the overall PHY block, one
> > to enable the APB interface for programming registers, and
> > another to enable the PHY itself.  Additionally there are
> > resets for each PHY lane.
> >
> > The PHY can be configured in hardware to read register
> > settings from ROM, or they can be written by the driver.
> >
> > The sequence of operation on startup is to enable the APB
> > bus, write the PHY registers (if required)  for each lane
> > group, and then enable the PHY.  Each group of lanes
> > can then be individually controlled using the power_on()/
> > power_off() function for that generic PHY
> >
> > 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      |   2 +
> >  drivers/phy/cadence/cdns-sierra.c | 385 ++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 398 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..098df0f
> > --- /dev/null
> > +++ b/drivers/phy/cadence/Kconfig
> > @@ -0,0 +1,9 @@
> > +#
> > +# Phy drivers for Cadence PHYs
> > +#
> > +config CDNS_SIERRA_PHY
> > +	tristate "Cadence Sierra PHY Driver"
> > +	depends on OF && HAS_IOMEM && RESET_CONTROLLER
> > +	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..c396c69
> > --- /dev/null
> > +++ b/drivers/phy/cadence/Makefile
> > @@ -0,0 +1,2 @@
> > +# 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..83568b4
> > --- /dev/null
> > +++ b/drivers/phy/cadence/cdns-sierra.c
> > @@ -0,0 +1,385 @@
> > +// 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>
> > +
> > +/* PHY register offsets */
> > +#define SIERRA_PHY_PLL_CFG		(0xc00e << 2)
> > +#define SIERRA_DET_STANDEC_A		(0x4000 << 2)
> > +#define SIERRA_DET_STANDEC_B		(0x4001 << 2)
> > +#define SIERRA_DET_STANDEC_C		(0x4002 << 2)
> > +#define SIERRA_DET_STANDEC_D		(0x4003 << 2)
> > +#define SIERRA_DET_STANDEC_E		(0x4004 << 2)
> > +#define SIERRA_PSM_LANECAL		(0x4008 << 2)
> > +#define SIERRA_PSM_DIAG			(0x4015 << 2)
> > +#define SIERRA_PSC_TX_A0		(0x4028 << 2)
> > +#define SIERRA_PSC_TX_A1		(0x4029 << 2)
> > +#define SIERRA_PSC_TX_A2		(0x402A << 2)
> > +#define SIERRA_PSC_TX_A3		(0x402B << 2)
> > +#define SIERRA_PSC_RX_A0		(0x4030 << 2)
> > +#define SIERRA_PSC_RX_A1		(0x4031 << 2)
> > +#define SIERRA_PSC_RX_A2		(0x4032 << 2)
> > +#define SIERRA_PSC_RX_A3		(0x4033 << 2)
> > +#define SIERRA_PLLCTRL_SUBRATE		(0x403A << 2)
> > +#define SIERRA_PLLCTRL_GEN_D		(0x403E << 2)
> > +#define SIERRA_DRVCTRL_ATTEN		(0x406A << 2)
> > +#define SIERRA_CLKPATHCTRL_TMR		(0x4081 << 2)
> > +#define SIERRA_RX_CREQ_FLTR_A_MODE1	(0x4087 << 2)
> > +#define SIERRA_RX_CREQ_FLTR_A_MODE0	(0x4088 << 2)
> > +#define SIERRA_CREQ_CCLKDET_MODE01	(0x408E << 2)
> > +#define SIERRA_RX_CTLE_MAINTENANCE	(0x4091 << 2)
> > +#define SIERRA_CREQ_FSMCLK_SEL		(0x4092 << 2)
> > +#define SIERRA_CTLELUT_CTRL		(0x4098 << 2)
> > +#define SIERRA_DFE_ECMP_RATESEL		(0x40C0 << 2)
> > +#define SIERRA_DFE_SMP_RATESEL		(0x40C1 << 2)
> > +#define SIERRA_DEQ_VGATUNE_CTRL		(0x40E1 << 2)
> > +#define SIERRA_TMRVAL_MODE3		(0x416E << 2)
> > +#define SIERRA_TMRVAL_MODE2		(0x416F << 2)
> > +#define SIERRA_TMRVAL_MODE1		(0x4170 << 2)
> > +#define SIERRA_TMRVAL_MODE0		(0x4171 << 2)
> > +#define SIERRA_PICNT_MODE1		(0x4174 << 2)
> > +#define SIERRA_CPI_OUTBUF_RATESEL	(0x417C << 2)
> > +#define SIERRA_LFPSFILT_NS		(0x418A << 2)
> > +#define SIERRA_LFPSFILT_RD		(0x418B << 2)
> > +#define SIERRA_LFPSFILT_MP		(0x418C << 2)
> > +#define SIERRA_SDFILT_H2L_A		(0x4191 << 2)
> > +
> > +#define SIERRA_MACRO_ID			0x00007364
> > +#define SIERRA_MAX_LANES		4
> > +
> > +struct cdns_sierra_inst {
> > +	struct phy *phy;
> > +	u32 phy_type;
> > +	u32 num_lanes;
> > +	u32 mlane;
> > +	struct reset_control *lnk_rst;
> > +};
> > +
> > +struct cdns_reg_pairs {
> > +	u16 val;
> > +	u32 off;
> > +};
> > +
> > +struct cdns_sierra_data {
> > +		u32 id_value;
> > +		u32 pcie_regs;
> > +		u32 usb_regs;
> > +		struct cdns_reg_pairs *pcie_vals;
> > +		struct cdns_reg_pairs  *usb_vals;
> > +};
> > +
> > +struct cdns_sierra_phy {
> > +	struct device *dev;
> > +	void __iomem *base;
> > +	struct cdns_sierra_data *init_data;
> > +	struct cdns_sierra_inst phys[SIERRA_MAX_LANES];
> > +	struct reset_control *phy_rst;
> > +	struct reset_control *apb_rst;
> > +	struct clk *clk;
> > +	struct phy *pcie_phy;
> > +	bool autoconf;
> > +};
> > +
> > +static void cdns_sierra_phy_init(struct phy *gphy)
> > +{
> > +	struct cdns_sierra_inst *ins = phy_get_drvdata(gphy);
> > +	struct cdns_sierra_phy *phy = dev_get_drvdata(gphy->dev.parent);
> > +	int i, j;
> > +	struct cdns_reg_pairs *vals;
> > +	u32 num_regs;
> > +
> > +	if (ins->phy_type == PHY_TYPE_PCIE) {
> > +		num_regs = phy->init_data->pcie_regs;
> > +		vals = phy->init_data->pcie_vals;
> > +	} else if (ins->phy_type == PHY_TYPE_USB3) {
> > +		num_regs = phy->init_data->usb_regs;
> > +		vals = phy->init_data->usb_vals;
> > +	} else {
> > +		return;
> > +	}
> > +	for (i = 0; i < ins->num_lanes; i++)
> > +		for (j = 0; j < num_regs ; j++)
> > +			writel(vals[j].val, phy->base +
> > +				vals[j].off + (i + ins->mlane) * 0x800);
> > +}
> > +
> > +static int cdns_sierra_phy_on(struct phy *gphy)
> > +{
> > +	struct cdns_sierra_inst *ins = phy_get_drvdata(gphy);
> > +	int ret;
> > +
> > +	/* Take the PHY out of reset */
> > +	ret = reset_control_deassert(ins->lnk_rst);
> > +
> > +	if (ret)
> 
> This check is unncessary.
I'll remove it, thanks for pointing out.

> > +		return ret;
> > +
> > +	return ret;
> > +}
> > +
> > +static int cdns_sierra_phy_off(struct phy *gphy)
> > +{
> > +	struct cdns_sierra_inst *ins = phy_get_drvdata(gphy);
> > +
> > +	reset_control_assert(ins->lnk_rst);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct phy_ops ops = {
> > +	.power_on	= cdns_sierra_phy_on,
> > +	.power_off	= cdns_sierra_phy_off,
> > +	.owner		= THIS_MODULE,
> > +};
> > +
> > +static int cdns_sierra_get_optional(struct cdns_sierra_inst *inst,
> > +				    struct device_node *child)
> > +{
> > +	if (of_property_read_u32(child, "reg", &inst->mlane))
> > +		return -EINVAL;
> > +
> > +	if (of_property_read_u32(child, "cdns,num-lanes", &inst->num_lanes))
> > +		return -EINVAL;
> > +
> > +	if (of_property_read_u32(child, "cdns,phy-type", &inst->phy_type))
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id cdns_sierra_id_table[];
> > +
> > +static int cdns_sierra_phy_probe(struct platform_device *pdev)
> > +{
> > +	struct cdns_sierra_phy *sp;
> > +	struct phy_provider *phy_provider;
> > +	struct device *dev = &pdev->dev;
> > +	const struct of_device_id *match;
> > +	struct resource *res;
> > +	int ret, node = 0;
> > +	struct device_node *dn = dev->of_node, *child;
> > +
> > +	if (of_get_child_count(dn) == 0)
> > +		return -ENODEV;
> > +
> > +	sp = devm_kzalloc(dev, sizeof(*sp), GFP_KERNEL);
> > +	if (!sp)
> > +		return -ENOMEM;
> > +	dev_set_drvdata(dev, sp);
> > +	sp->dev = dev;
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "reg");
> > +	sp->base = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(sp->base)) {
> > +		dev_err(dev, "missing \"reg\"\n");
> > +		return PTR_ERR(sp->base);
> > +	}
> > +
> > +	/* Get init data for this PHY */
> > +	match = of_match_device(cdns_sierra_id_table, dev);
> > +	if (!match)
> > +		return -EINVAL;
> > +	sp->init_data = (struct cdns_sierra_data *)match->data;
> > +
> > +	platform_set_drvdata(pdev, sp);
> > +
> > +	sp->clk = devm_clk_get(dev, "phy_clk");
> > +	if (IS_ERR(sp->clk)) {
> > +		dev_err(dev, "failed to get clock phy_clk\n");
> > +		return PTR_ERR(sp->clk);
> > +	}
> > +
> > +	sp->phy_rst = devm_reset_control_get(dev, "sierra_reset");
> > +	if (IS_ERR(sp->phy_rst)) {
> > +		dev_err(dev, "failed to get reset\n");
> > +		return PTR_ERR(sp->phy_rst);
> > +	}
> > +
> > +	sp->apb_rst = devm_reset_control_get(dev, "sierra_apb");
> > +	if (IS_ERR(sp->apb_rst)) {
> > +		dev_err(dev, "failed to get apb reset\n");
> > +		return PTR_ERR(sp->apb_rst);
> > +	}
> > +
> > +	ret = clk_prepare_enable(sp->clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Enable APB */
> > +	reset_control_deassert(sp->apb_rst);
> > +
> > +	/* Check that PHY is present */
> > +	if  (sp->init_data->id_value != readl(sp->base)) {
> > +		goto clk_disable;
> > +		return -EINVAL;
> > +	}
> > +
> > +	sp->autoconf = of_property_read_bool(dn, "cdns,autoconf");
> > +
> > +	for_each_available_child_of_node(dn, child) {
> > +		struct phy *gphy;
> > +
> > +		if (IS_ERR(sp->phys[node].lnk_rst)) {
> > +			dev_err(dev, "failed to get reset %s\n", child->name);
> > +			return PTR_ERR(sp->phys[node].lnk_rst);
> > +		}
> > +		sp->phys[node].lnk_rst =
> > +				devm_reset_control_get(dev, child->name);
> > +
> > +		if (!sp->autoconf) {
> > +			ret = cdns_sierra_get_optional(&sp->phys[node], child);
> > +			if (ret) {
> > +				dev_err(dev, "missing property in node %s\n",
> > +					child->name);
> > +				goto put_child;
> > +			}
> > +		}
> > +
> > +		gphy = devm_phy_create(dev, child, &ops);
> > +
> > +		if (IS_ERR(gphy)) {
> > +			ret = PTR_ERR(gphy);
> > +			goto put_child;
> > +		}
> > +		sp->phys[node].phy = gphy;
> > +		phy_set_drvdata(gphy, &sp->phys[node]);
> > +
> > +		/* Initialise the PHY registers, unless auto configured */
> > +		if (!sp->autoconf)
> > +			cdns_sierra_phy_init(gphy);
> > +
> > +		node++;
> > +	}
> > +
> > +	/* If more than one subnode, configure the PHY as multilink */
> > +	if (!sp->autoconf && of_get_child_count(dn) > 1)
> > +		writel(2, sp->base + SIERRA_PHY_PLL_CFG);
> > +
> > +	pm_runtime_enable(dev);
> > +	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> > +	reset_control_deassert(sp->phy_rst);
> > +	return PTR_ERR_OR_ZERO(phy_provider);
> > +put_child:
> > +	of_node_put(child);
> > +clk_disable:
> > +	clk_disable_unprepare(sp->clk);
> > +	reset_control_assert(sp->apb_rst);
> > +	return ret;
> > +}
> > +
> > +static int cdns_sierra_phy_remove(struct platform_device *pdev)
> > +{
> > +	struct cdns_sierra_phy *phy = dev_get_drvdata(pdev->dev.parent);
> > +
> > +	reset_control_assert(phy->phy_rst);
> > +	reset_control_assert(phy->apb_rst);
> > +	pm_runtime_disable(&pdev->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct cdns_reg_pairs cdns_usb_regs[] = {
> > +	/*
> > +	 * Write USB configuration parameters to the PHY.
> > +	 * These values are specific to this specific hardware
> > +	 * configuration.
> 
> Your intention is to have different arrays for different hardware configurations?
Ideally we don't need to add any, if implementors are using different values the
recommendation would be to use autoconfiguration and load them from
hardware.  However, if this is not possible then yes, I'd expect we'd need to add
new arrays for different hardware configurations, and add a new compatible
string.


> > +	 */
> > +	{0xFE0A, SIERRA_DET_STANDEC_A},
> > +	{0x000F, SIERRA_DET_STANDEC_B},
> > +	{0x55A5, SIERRA_DET_STANDEC_C},
> > +	{0x69AD, SIERRA_DET_STANDEC_D},
> > +	{0x0241, SIERRA_DET_STANDEC_E},
> > +	{0x0110, SIERRA_PSM_LANECAL},
> > +	{0xCF00, SIERRA_PSM_DIAG},
> > +	{0x001F, SIERRA_PSC_TX_A0},
> > +	{0x0007, SIERRA_PSC_TX_A1},
> > +	{0x0003, SIERRA_PSC_TX_A2},
> > +	{0x0003, SIERRA_PSC_TX_A3},
> > +	{0x0FFF, SIERRA_PSC_RX_A0},
> > +	{0x0003, SIERRA_PSC_RX_A1},
> > +	{0x0003, SIERRA_PSC_RX_A2},
> > +	{0x0001, SIERRA_PSC_RX_A3},
> > +	{0x0001, SIERRA_PLLCTRL_SUBRATE},
> > +	{0x0406, SIERRA_PLLCTRL_GEN_D},
> > +	{0x0000, SIERRA_DRVCTRL_ATTEN},
> > +	{0x823E, SIERRA_CLKPATHCTRL_TMR},
> > +	{0x078F, SIERRA_RX_CREQ_FLTR_A_MODE1},
> > +	{0x078F, SIERRA_RX_CREQ_FLTR_A_MODE0},
> > +	{0x7B3C, SIERRA_CREQ_CCLKDET_MODE01},
> > +	{0x023C, SIERRA_RX_CTLE_MAINTENANCE},
> > +	{0x3232, SIERRA_CREQ_FSMCLK_SEL},
> > +	{0x8452, SIERRA_CTLELUT_CTRL},
> > +	{0x4121, SIERRA_DFE_ECMP_RATESEL},
> > +	{0x4121, SIERRA_DFE_SMP_RATESEL},
> > +	{0x9999, SIERRA_DEQ_VGATUNE_CTRL},
> > +	{0x0330, SIERRA_TMRVAL_MODE0},
> > +	{0x01FF, SIERRA_PICNT_MODE1},
> > +	{0x0009, SIERRA_CPI_OUTBUF_RATESEL},
> > +	{0x000F, SIERRA_LFPSFILT_NS},
> > +	{0x0009, SIERRA_LFPSFILT_RD},
> > +	{0x0001, SIERRA_LFPSFILT_MP},
> > +	{0x8013, SIERRA_SDFILT_H2L_A},
> > +	{0x0400, SIERRA_TMRVAL_MODE1},
> 
> Are all of them 16 bit value fields? Or some of them coalesced to form 16 bit
> values?
Yes, 16 bit value fields.

> 
> Thanks
> Kishon

Powered by blists - more mailing lists