Re: [PATCH v4 4/5] phy: ti: Add a new SERDES driver for TI's AM654x SoC

Re: [PATCH v4 4/5] phy: ti: Add a new SERDES driver for TI's AM654x SoC

[Peter Rosin]

Hi!

[I know this has already been merged upstream, but I only just
 now noticed the code and went to the archives to find the
 originating mail. I hope I managed to set in-reply-to correctly...]

The mux handling is problematic and does not follow the rules.
It needs to be fixed, or you may face deadlocks. See below.

On 2019-04-05 11:08, Kishon Vijay Abraham I wrote:
> Add a new SERDES driver for TI's AM654x SoC which configures
> the SERDES only for PCIe. Support fo USB3 will be added later.
> 
> SERDES in am654x has three input clocks (left input, externel reference
> clock and right input) and two output clocks (left output and right
> output) in addition to a PLL mux clock which the SERDES uses for Clock
> Multiplier Unit (CMU refclock).
> 
> The PLL mux clock can select from one of the three input clocks.
> The right output can select between left input and external reference
> clock while the left output can select between the right input and
> external reference clock.
> 
> The driver has support to select PLL mux and left/right output mux as
> specified in device tree.
> 
> [rogerq@ti.com: Fix boot lockup caused by accessing a structure member
> (hw->init) allocated in stack of probe() and accessed in get_parent]
> [rogerq@ti.com: Fix "Failed to find the parent" warnings]
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/phy/ti/Kconfig            |  12 +
>  drivers/phy/ti/Makefile           |   1 +
>  drivers/phy/ti/phy-am654-serdes.c | 624 ++++++++++++++++++++++++++++++
>  3 files changed, 637 insertions(+)
>  create mode 100644 drivers/phy/ti/phy-am654-serdes.c
> 
> diff --git a/drivers/phy/ti/Kconfig b/drivers/phy/ti/Kconfig
> index 7cdc35f8c862..6931c87235b9 100644
> --- a/drivers/phy/ti/Kconfig
> +++ b/drivers/phy/ti/Kconfig
> @@ -20,6 +20,18 @@ config PHY_DM816X_USB
>  	help
>  	  Enable this for dm816x USB to work.
>  
> +config PHY_AM654_SERDES
> +	tristate "TI AM654 SERDES support"
> +	depends on OF && ARCH_K3 || COMPILE_TEST
> +	depends on COMMON_CLK
> +	select GENERIC_PHY
> +	select MULTIPLEXER
> +	select REGMAP_MMIO
> +	select MUX_MMIO
> +	help
> +	  This option enables support for TI AM654 SerDes PHY used for
> +	  PCIe.
> +
>  config OMAP_CONTROL_PHY
>  	tristate "OMAP CONTROL PHY Driver"
>  	depends on ARCH_OMAP2PLUS || COMPILE_TEST
> diff --git a/drivers/phy/ti/Makefile b/drivers/phy/ti/Makefile
> index 368de8e1548d..601bbd88f35e 100644
> --- a/drivers/phy/ti/Makefile
> +++ b/drivers/phy/ti/Makefile
> @@ -6,5 +6,6 @@ obj-$(CONFIG_OMAP_USB2)			+= phy-omap-usb2.o
>  obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
>  obj-$(CONFIG_PHY_TUSB1210)		+= phy-tusb1210.o
>  obj-$(CONFIG_TWL4030_USB)		+= phy-twl4030-usb.o
> +obj-$(CONFIG_PHY_AM654_SERDES)		+= phy-am654-serdes.o
>  obj-$(CONFIG_PHY_TI_GMII_SEL)		+= phy-gmii-sel.o
>  obj-$(CONFIG_PHY_TI_KEYSTONE_SERDES)	+= phy-keystone-serdes.o
> diff --git a/drivers/phy/ti/phy-am654-serdes.c b/drivers/phy/ti/phy-am654-serdes.c
> new file mode 100644
> index 000000000000..4817c67abbbb
> --- /dev/null
> +++ b/drivers/phy/ti/phy-am654-serdes.c
> @@ -0,0 +1,624 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * PCIe SERDES driver for AM654x SoC
> + *
> + * Copyright (C) 2018 - 2019 Texas Instruments Incorporated - http://www.ti.com/
> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
> + */
> +
> +#include <dt-bindings/phy/phy.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/mux/consumer.h>
> +#include <linux/of_address.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +
> +#define CMU_R07C		0x7c
> +
> +#define COMLANE_R138		0xb38
> +#define VERSION			0x70
> +
> +#define COMLANE_R190		0xb90
> +
> +#define COMLANE_R194		0xb94
> +
> +#define SERDES_CTRL		0x1fd0
> +
> +#define WIZ_LANEXCTL_STS	0x1fe0
> +#define TX0_DISABLE_STATE	0x4
> +#define TX0_SLEEP_STATE		0x5
> +#define TX0_SNOOZE_STATE	0x6
> +#define TX0_ENABLE_STATE	0x7
> +
> +#define RX0_DISABLE_STATE	0x4
> +#define RX0_SLEEP_STATE		0x5
> +#define RX0_SNOOZE_STATE	0x6
> +#define RX0_ENABLE_STATE	0x7
> +
> +#define WIZ_PLL_CTRL		0x1ff4
> +#define PLL_DISABLE_STATE	0x4
> +#define PLL_SLEEP_STATE		0x5
> +#define PLL_SNOOZE_STATE	0x6
> +#define PLL_ENABLE_STATE	0x7
> +
> +#define PLL_LOCK_TIME		100000	/* in microseconds */
> +#define SLEEP_TIME		100	/* in microseconds */
> +
> +#define LANE_USB3		0x0
> +#define LANE_PCIE0_LANE0	0x1
> +
> +#define LANE_PCIE1_LANE0	0x0
> +#define LANE_PCIE0_LANE1	0x1
> +
> +#define SERDES_NUM_CLOCKS	3
> +
> +struct serdes_am654_clk_mux {
> +	struct clk_hw	hw;
> +	struct regmap	*regmap;
> +	unsigned int	reg;
> +	int		*table;
> +	u32		mask;
> +	u8		shift;
> +	struct clk_init_data clk_data;
> +};
> +
> +#define to_serdes_am654_clk_mux(_hw)	\
> +		container_of(_hw, struct serdes_am654_clk_mux, hw)
> +
> +static struct regmap_config serdes_am654_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.fast_io = true,
> +};
> +
> +static const struct reg_field cmu_master_cdn_o = REG_FIELD(CMU_R07C, 24, 24);
> +static const struct reg_field config_version = REG_FIELD(COMLANE_R138, 16, 23);
> +static const struct reg_field l1_master_cdn_o = REG_FIELD(COMLANE_R190, 9, 9);
> +static const struct reg_field cmu_ok_i_0 = REG_FIELD(COMLANE_R194, 19, 19);
> +static const struct reg_field por_en = REG_FIELD(SERDES_CTRL, 29, 29);
> +static const struct reg_field tx0_enable = REG_FIELD(WIZ_LANEXCTL_STS, 29, 31);
> +static const struct reg_field rx0_enable = REG_FIELD(WIZ_LANEXCTL_STS, 13, 15);
> +static const struct reg_field pll_enable = REG_FIELD(WIZ_PLL_CTRL, 29, 31);
> +static const struct reg_field pll_ok = REG_FIELD(WIZ_PLL_CTRL, 28, 28);
> +
> +struct serdes_am654 {
> +	struct regmap		*regmap;
> +	struct regmap_field	*cmu_master_cdn_o;
> +	struct regmap_field	*config_version;
> +	struct regmap_field	*l1_master_cdn_o;
> +	struct regmap_field	*cmu_ok_i_0;
> +	struct regmap_field	*por_en;
> +	struct regmap_field	*tx0_enable;
> +	struct regmap_field	*rx0_enable;
> +	struct regmap_field	*pll_enable;
> +	struct regmap_field	*pll_ok;
> +
> +	struct device		*dev;
> +	struct mux_control	*control;
> +	bool			busy;
> +	u32			type;
> +	struct device_node	*of_node;
> +	struct clk_onecell_data	clk_data;
> +	struct clk		*clks[SERDES_NUM_CLOCKS];
> +};
> +
> +static int serdes_am654_enable_pll(struct serdes_am654 *phy)
> +{
> +	int ret;
> +	u32 val;
> +
> +	ret = regmap_field_write(phy->pll_enable, PLL_ENABLE_STATE);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_field_read_poll_timeout(phy->pll_ok, val, val, 1000,
> +					      PLL_LOCK_TIME);
> +}
> +
> +static void serdes_am654_disable_pll(struct serdes_am654 *phy)
> +{
> +	struct device *dev = phy->dev;
> +	int ret;
> +
> +	ret = regmap_field_write(phy->pll_enable, PLL_DISABLE_STATE);
> +	if (ret)
> +		dev_err(dev, "Failed to disable PLL\n");
> +}
> +
> +static int serdes_am654_enable_txrx(struct serdes_am654 *phy)
> +{
> +	int ret;
> +
> +	/* Enable TX */
> +	ret = regmap_field_write(phy->tx0_enable, TX0_ENABLE_STATE);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable RX */
> +	ret = regmap_field_write(phy->rx0_enable, RX0_ENABLE_STATE);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int serdes_am654_disable_txrx(struct serdes_am654 *phy)
> +{
> +	int ret;
> +
> +	/* Disable TX */
> +	ret = regmap_field_write(phy->tx0_enable, TX0_DISABLE_STATE);
> +	if (ret)
> +		return ret;
> +
> +	/* Disable RX */
> +	ret = regmap_field_write(phy->rx0_enable, RX0_DISABLE_STATE);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int serdes_am654_power_on(struct phy *x)
> +{
> +	struct serdes_am654 *phy = phy_get_drvdata(x);
> +	struct device *dev = phy->dev;
> +	int ret;
> +	u32 val;
> +
> +	ret = serdes_am654_enable_pll(phy);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable PLL\n");
> +		return ret;
> +	}
> +
> +	ret = serdes_am654_enable_txrx(phy);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable TX RX\n");
> +		return ret;
> +	}
> +
> +	return regmap_field_read_poll_timeout(phy->cmu_ok_i_0, val, val,
> +					      SLEEP_TIME, PLL_LOCK_TIME);
> +}
> +
> +static int serdes_am654_power_off(struct phy *x)
> +{
> +	struct serdes_am654 *phy = phy_get_drvdata(x);
> +
> +	serdes_am654_disable_txrx(phy);
> +	serdes_am654_disable_pll(phy);
> +
> +	return 0;
> +}
> +
> +static int serdes_am654_init(struct phy *x)
> +{
> +	struct serdes_am654 *phy = phy_get_drvdata(x);
> +	int ret;
> +
> +	ret = regmap_field_write(phy->config_version, VERSION);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_field_write(phy->cmu_master_cdn_o, 0x1);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_field_write(phy->l1_master_cdn_o, 0x1);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int serdes_am654_reset(struct phy *x)
> +{
> +	struct serdes_am654 *phy = phy_get_drvdata(x);
> +	int ret;
> +
> +	ret = regmap_field_write(phy->por_en, 0x1);
> +	if (ret)
> +		return ret;
> +
> +	mdelay(1);
> +
> +	ret = regmap_field_write(phy->por_en, 0x0);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static void serdes_am654_release(struct phy *x)
> +{
> +	struct serdes_am654 *phy = phy_get_drvdata(x);
> +
> +	phy->type = PHY_NONE;
> +	phy->busy = false;
> +	mux_control_deselect(phy->control);

Here you unconditionally deselect the mux, and that seems
dangerous. Are you *sure* that ->release may not be called
without a successful xlate call?

I'm not 100% sure of that, but I have not looked at the phy
code before today, so it may very well be the case that this
is safe...

> +}
> +
> +struct phy *serdes_am654_xlate(struct device *dev, struct of_phandle_args
> +				 *args)
> +{
> +	struct serdes_am654 *am654_phy;
> +	struct phy *phy;
> +	int ret;
> +
> +	phy = of_phy_simple_xlate(dev, args);
> +	if (IS_ERR(phy))
> +		return phy;
> +
> +	am654_phy = phy_get_drvdata(phy);
> +	if (am654_phy->busy)
> +		return ERR_PTR(-EBUSY);
> +
> +	ret = mux_control_select(am654_phy->control, args->args[1]);
> +	if (ret) {
> +		dev_err(dev, "Failed to select SERDES Lane Function\n");
> +		return ERR_PTR(ret);
> +	}

*However*

Here you select the mux as the last action, good, but, a mux must
be handled with that same care as a locking primitive, i.e.
successful selects must be perfectly balanced with deselects. I
see no guarantee of that here, since there are other failures
possible after the xlate call. So, being last in the function
does not really help. If I read the code correctly, the
phy core may fail if try_module_get fails in phy_get(). If that
ever happens, a successful call to mux_control_select is simply
forgotten, and the mux will be locked indefinitely.

am654_phy->busy will also be set indefinitely, so you will get
-EBUSY and not a hard deadlock. At least here, but if the now
locked mux control happens to also control some other muxes
(probably unlikely, but if), then their consumers will potentially
deadlock hard. But that's just after a cursory reading, so I may
completely miss something...

Thinking some more, the above mentioned forgotten phy problem
in fact looks like a generic leak in phy_get. It will simply
leak any and all phys where the owner module is not yet
available. Or, am I missing something?

Perhaps fixing that is all that is needed to make the mux
select/deselect calls guaranteed to be balanced?

Cheers,
Peter

> +
> +	am654_phy->busy = true;
> +	am654_phy->type = args->args[0];
> +
> +	return phy;
> +}
> +
> +static const struct phy_ops ops = {
> +	.reset		= serdes_am654_reset,
> +	.init		= serdes_am654_init,
> +	.power_on	= serdes_am654_power_on,
> +	.power_off	= serdes_am654_power_off,
> +	.release	= serdes_am654_release,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static u8 serdes_am654_clk_mux_get_parent(struct clk_hw *hw)
> +{
> +	struct serdes_am654_clk_mux *mux = to_serdes_am654_clk_mux(hw);
> +	unsigned int num_parents = clk_hw_get_num_parents(hw);
> +	struct regmap *regmap = mux->regmap;
> +	unsigned int reg = mux->reg;
> +	unsigned int val;
> +	int i;
> +
> +	regmap_read(regmap, reg, &val);
> +	val >>= mux->shift;
> +	val &= mux->mask;
> +
> +	for (i = 0; i < num_parents; i++)
> +		if (mux->table[i] == val)
> +			return i;
> +
> +	/*
> +	 * No parent? This should never happen!
> +	 * Verify if we set a valid parent in serdes_am654_clk_register()
> +	 */
> +	WARN(1, "Failed to find the parent of %s clock\n", hw->init->name);
> +
> +	/* Make the parent lookup to fail */
> +	return num_parents;
> +}
> +
> +static int serdes_am654_clk_mux_set_parent(struct clk_hw *hw, u8 index)
> +{
> +	struct serdes_am654_clk_mux *mux = to_serdes_am654_clk_mux(hw);
> +	struct regmap *regmap = mux->regmap;
> +	unsigned int reg = mux->reg;
> +	int val;
> +	int ret;
> +
> +	val = mux->table[index];
> +
> +	if (val == -1)
> +		return -EINVAL;
> +
> +	val <<= mux->shift;
> +	ret = regmap_update_bits(regmap, reg, mux->mask << mux->shift, val);
> +
> +	return ret;
> +}
> +
> +static const struct clk_ops serdes_am654_clk_mux_ops = {
> +	.set_parent = serdes_am654_clk_mux_set_parent,
> +	.get_parent = serdes_am654_clk_mux_get_parent,
> +};
> +
> +static int mux_table[SERDES_NUM_CLOCKS][3] = {
> +	/*
> +	 * The entries represent values for selecting between
> +	 * {left input, external reference clock, right input}
> +	 * Only one of Left Output or Right Output should be used since
> +	 * both left and right output clock uses the same bits and modifying
> +	 * one clock will impact the other.
> +	 */
> +	{ BIT(2),               0, BIT(0) }, /* Mux of CMU refclk */
> +	{     -1,          BIT(3), BIT(1) }, /* Mux of Left Output */
> +	{ BIT(1), BIT(3) | BIT(1),     -1 }, /* Mux of Right Output */
> +};
> +
> +static int mux_mask[SERDES_NUM_CLOCKS] = { 0x5, 0xa, 0xa };
> +
> +static int serdes_am654_clk_register(struct serdes_am654 *am654_phy,
> +				     const char *clock_name, int clock_num)
> +{
> +	struct device_node *node = am654_phy->of_node;
> +	struct device *dev = am654_phy->dev;
> +	struct serdes_am654_clk_mux *mux;
> +	struct device_node *regmap_node;
> +	const char **parent_names;
> +	struct clk_init_data *init;
> +	unsigned int num_parents;
> +	struct regmap *regmap;
> +	const __be32 *addr;
> +	unsigned int reg;
> +	struct clk *clk;
> +
> +	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
> +	if (!mux)
> +		return -ENOMEM;
> +
> +	init = &mux->clk_data;
> +
> +	regmap_node = of_parse_phandle(node, "ti,serdes-clk", 0);
> +	of_node_put(regmap_node);
> +	if (!regmap_node) {
> +		dev_err(dev, "Fail to get serdes-clk node\n");
> +		return -ENODEV;
> +	}
> +
> +	regmap = syscon_node_to_regmap(regmap_node->parent);
> +	if (IS_ERR(regmap)) {
> +		dev_err(dev, "Fail to get Syscon regmap\n");
> +		return PTR_ERR(regmap);
> +	}
> +
> +	num_parents = of_clk_get_parent_count(node);
> +	if (num_parents < 2) {
> +		dev_err(dev, "SERDES clock must have parents\n");
> +		return -EINVAL;
> +	}
> +
> +	parent_names = devm_kzalloc(dev, (sizeof(char *) * num_parents),
> +				    GFP_KERNEL);
> +	if (!parent_names)
> +		return -ENOMEM;
> +
> +	of_clk_parent_fill(node, parent_names, num_parents);
> +
> +	addr = of_get_address(regmap_node, 0, NULL, NULL);
> +	if (!addr)
> +		return -EINVAL;
> +
> +	reg = be32_to_cpu(*addr);
> +
> +	init->ops = &serdes_am654_clk_mux_ops;
> +	init->flags = CLK_SET_RATE_NO_REPARENT;
> +	init->parent_names = parent_names;
> +	init->num_parents = num_parents;
> +	init->name = clock_name;
> +
> +	mux->table = mux_table[clock_num];
> +	mux->regmap = regmap;
> +	mux->reg = reg;
> +	mux->shift = 4;
> +	mux->mask = mux_mask[clock_num];
> +	mux->hw.init = init;
> +
> +	/*
> +	 * setup a sane default so get_parent() call evaluates
> +	 * to a valid parent. Index 1 is the safest choice as
> +	 * the default as it is valid value for all of serdes's
> +	 * output clocks.
> +	 */
> +	serdes_am654_clk_mux_set_parent(&mux->hw, 1);
> +	clk = devm_clk_register(dev, &mux->hw);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	am654_phy->clks[clock_num] = clk;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id serdes_am654_id_table[] = {
> +	{
> +		.compatible = "ti,phy-am654-serdes",
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, serdes_am654_id_table);
> +
> +static int serdes_am654_regfield_init(struct serdes_am654 *am654_phy)
> +{
> +	struct regmap *regmap = am654_phy->regmap;
> +	struct device *dev = am654_phy->dev;
> +
> +	am654_phy->cmu_master_cdn_o = devm_regmap_field_alloc(dev, regmap,
> +							      cmu_master_cdn_o);
> +	if (IS_ERR(am654_phy->cmu_master_cdn_o)) {
> +		dev_err(dev, "CMU_MASTER_CDN_O reg field init failed\n");
> +		return PTR_ERR(am654_phy->cmu_master_cdn_o);
> +	}
> +
> +	am654_phy->config_version = devm_regmap_field_alloc(dev, regmap,
> +							    config_version);
> +	if (IS_ERR(am654_phy->config_version)) {
> +		dev_err(dev, "CONFIG_VERSION reg field init failed\n");
> +		return PTR_ERR(am654_phy->config_version);
> +	}
> +
> +	am654_phy->l1_master_cdn_o = devm_regmap_field_alloc(dev, regmap,
> +							     l1_master_cdn_o);
> +	if (IS_ERR(am654_phy->l1_master_cdn_o)) {
> +		dev_err(dev, "L1_MASTER_CDN_O reg field init failed\n");
> +		return PTR_ERR(am654_phy->l1_master_cdn_o);
> +	}
> +
> +	am654_phy->cmu_ok_i_0 = devm_regmap_field_alloc(dev, regmap,
> +							cmu_ok_i_0);
> +	if (IS_ERR(am654_phy->cmu_ok_i_0)) {
> +		dev_err(dev, "CMU_OK_I_0 reg field init failed\n");
> +		return PTR_ERR(am654_phy->cmu_ok_i_0);
> +	}
> +
> +	am654_phy->por_en = devm_regmap_field_alloc(dev, regmap, por_en);
> +	if (IS_ERR(am654_phy->por_en)) {
> +		dev_err(dev, "POR_EN reg field init failed\n");
> +		return PTR_ERR(am654_phy->por_en);
> +	}
> +
> +	am654_phy->tx0_enable = devm_regmap_field_alloc(dev, regmap,
> +							tx0_enable);
> +	if (IS_ERR(am654_phy->tx0_enable)) {
> +		dev_err(dev, "TX0_ENABLE reg field init failed\n");
> +		return PTR_ERR(am654_phy->tx0_enable);
> +	}
> +
> +	am654_phy->rx0_enable = devm_regmap_field_alloc(dev, regmap,
> +							rx0_enable);
> +	if (IS_ERR(am654_phy->rx0_enable)) {
> +		dev_err(dev, "RX0_ENABLE reg field init failed\n");
> +		return PTR_ERR(am654_phy->rx0_enable);
> +	}
> +
> +	am654_phy->pll_enable = devm_regmap_field_alloc(dev, regmap,
> +							pll_enable);
> +	if (IS_ERR(am654_phy->pll_enable)) {
> +		dev_err(dev, "PLL_ENABLE reg field init failed\n");
> +		return PTR_ERR(am654_phy->pll_enable);
> +	}
> +
> +	am654_phy->pll_ok = devm_regmap_field_alloc(dev, regmap, pll_ok);
> +	if (IS_ERR(am654_phy->pll_ok)) {
> +		dev_err(dev, "PLL_OK reg field init failed\n");
> +		return PTR_ERR(am654_phy->pll_ok);
> +	}
> +
> +	return 0;
> +}
> +
> +static int serdes_am654_probe(struct platform_device *pdev)
> +{
> +	struct phy_provider *phy_provider;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	struct clk_onecell_data *clk_data;
> +	struct serdes_am654 *am654_phy;
> +	struct mux_control *control;
> +	const char *clock_name;
> +	struct regmap *regmap;
> +	void __iomem *base;
> +	struct phy *phy;
> +	int ret;
> +	int i;
> +
> +	am654_phy = devm_kzalloc(dev, sizeof(*am654_phy), GFP_KERNEL);
> +	if (!am654_phy)
> +		return -ENOMEM;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	regmap = devm_regmap_init_mmio(dev, base, &serdes_am654_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err(dev, "Failed to initialize regmap\n");
> +		return PTR_ERR(regmap);
> +	}
> +
> +	control = devm_mux_control_get(dev, NULL);
> +	if (IS_ERR(control))
> +		return PTR_ERR(control);
> +
> +	am654_phy->dev = dev;
> +	am654_phy->of_node = node;
> +	am654_phy->regmap = regmap;
> +	am654_phy->control = control;
> +	am654_phy->type = PHY_NONE;
> +
> +	ret = serdes_am654_regfield_init(am654_phy);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize regfields\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, am654_phy);
> +
> +	for (i = 0; i < SERDES_NUM_CLOCKS; i++) {
> +		ret = of_property_read_string_index(node, "clock-output-names",
> +						    i, &clock_name);
> +		if (ret) {
> +			dev_err(dev, "Failed to get clock name\n");
> +			return ret;
> +		}
> +
> +		ret = serdes_am654_clk_register(am654_phy, clock_name, i);
> +		if (ret) {
> +			dev_err(dev, "Failed to initialize clock %s\n",
> +				clock_name);
> +			return ret;
> +		}
> +	}
> +
> +	clk_data = &am654_phy->clk_data;
> +	clk_data->clks = am654_phy->clks;
> +	clk_data->clk_num = SERDES_NUM_CLOCKS;
> +	ret = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_enable(dev);
> +
> +	phy = devm_phy_create(dev, NULL, &ops);
> +	if (IS_ERR(phy))
> +		return PTR_ERR(phy);
> +
> +	phy_set_drvdata(phy, am654_phy);
> +	phy_provider = devm_of_phy_provider_register(dev, serdes_am654_xlate);
> +	if (IS_ERR(phy_provider)) {
> +		ret = PTR_ERR(phy_provider);
> +		goto clk_err;
> +	}
> +
> +	return 0;
> +
> +clk_err:
> +	of_clk_del_provider(node);
> +
> +	return ret;
> +}
> +
> +static int serdes_am654_remove(struct platform_device *pdev)
> +{
> +	struct serdes_am654 *am654_phy = platform_get_drvdata(pdev);
> +	struct device_node *node = am654_phy->of_node;
> +
> +	pm_runtime_disable(&pdev->dev);
> +	of_clk_del_provider(node);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver serdes_am654_driver = {
> +	.probe		= serdes_am654_probe,
> +	.remove		= serdes_am654_remove,
> +	.driver		= {
> +		.name	= "phy-am654",
> +		.of_match_table = serdes_am654_id_table,
> +	},
> +};
> +module_platform_driver(serdes_am654_driver);
> +
> +MODULE_AUTHOR("Texas Instruments Inc.");
> +MODULE_DESCRIPTION("TI AM654x SERDES driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v4 4/5] phy: ti: Add a new SERDES driver for TI's AM654x SoC

[Kishon Vijay Abraham I]

Hi Peter,

On 13/06/19 4:20 AM, Peter Rosin wrote:
> Hi!
> 
> [I know this has already been merged upstream, but I only just
>  now noticed the code and went to the archives to find the
>  originating mail. I hope I managed to set in-reply-to correctly...]
> 
> The mux handling is problematic and does not follow the rules.
> It needs to be fixed, or you may face deadlocks. See below.
> 
> On 2019-04-05 11:08, Kishon Vijay Abraham I wrote:
>> Add a new SERDES driver for TI's AM654x SoC which configures
>> the SERDES only for PCIe. Support fo USB3 will be added later.
>>
>> SERDES in am654x has three input clocks (left input, externel reference
>> clock and right input) and two output clocks (left output and right
>> output) in addition to a PLL mux clock which the SERDES uses for Clock
>> Multiplier Unit (CMU refclock).
>>
>> The PLL mux clock can select from one of the three input clocks.
>> The right output can select between left input and external reference
>> clock while the left output can select between the right input and
>> external reference clock.
>>
>> The driver has support to select PLL mux and left/right output mux as
>> specified in device tree.
>>
>> [rogerq@ti.com: Fix boot lockup caused by accessing a structure member
>> (hw->init) allocated in stack of probe() and accessed in get_parent]
>> [rogerq@ti.com: Fix "Failed to find the parent" warnings]
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/phy/ti/Kconfig            |  12 +
>>  drivers/phy/ti/Makefile           |   1 +
>>  drivers/phy/ti/phy-am654-serdes.c | 624 ++++++++++++++++++++++++++++++
>>  3 files changed, 637 insertions(+)
>>  create mode 100644 drivers/phy/ti/phy-am654-serdes.c
>>
>> diff --git a/drivers/phy/ti/Kconfig b/drivers/phy/ti/Kconfig
>> index 7cdc35f8c862..6931c87235b9 100644
>> --- a/drivers/phy/ti/Kconfig
>> +++ b/drivers/phy/ti/Kconfig
>> @@ -20,6 +20,18 @@ config PHY_DM816X_USB
>>  	help
>>  	  Enable this for dm816x USB to work.
>>  
>> +config PHY_AM654_SERDES
>> +	tristate "TI AM654 SERDES support"
>> +	depends on OF && ARCH_K3 || COMPILE_TEST
>> +	depends on COMMON_CLK
>> +	select GENERIC_PHY
>> +	select MULTIPLEXER
>> +	select REGMAP_MMIO
>> +	select MUX_MMIO
>> +	help
>> +	  This option enables support for TI AM654 SerDes PHY used for
>> +	  PCIe.
>> +
>>  config OMAP_CONTROL_PHY
>>  	tristate "OMAP CONTROL PHY Driver"
>>  	depends on ARCH_OMAP2PLUS || COMPILE_TEST
>> diff --git a/drivers/phy/ti/Makefile b/drivers/phy/ti/Makefile
>> index 368de8e1548d..601bbd88f35e 100644
>> --- a/drivers/phy/ti/Makefile
>> +++ b/drivers/phy/ti/Makefile
>> @@ -6,5 +6,6 @@ obj-$(CONFIG_OMAP_USB2)			+= phy-omap-usb2.o
>>  obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
>>  obj-$(CONFIG_PHY_TUSB1210)		+= phy-tusb1210.o
>>  obj-$(CONFIG_TWL4030_USB)		+= phy-twl4030-usb.o
>> +obj-$(CONFIG_PHY_AM654_SERDES)		+= phy-am654-serdes.o
>>  obj-$(CONFIG_PHY_TI_GMII_SEL)		+= phy-gmii-sel.o
>>  obj-$(CONFIG_PHY_TI_KEYSTONE_SERDES)	+= phy-keystone-serdes.o
>> diff --git a/drivers/phy/ti/phy-am654-serdes.c b/drivers/phy/ti/phy-am654-serdes.c
>> new file mode 100644
>> index 000000000000..4817c67abbbb
>> --- /dev/null
>> +++ b/drivers/phy/ti/phy-am654-serdes.c
>> @@ -0,0 +1,624 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/**
>> + * PCIe SERDES driver for AM654x SoC
>> + *
>> + * Copyright (C) 2018 - 2019 Texas Instruments Incorporated - http://www.ti.com/
>> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
>> + */
>> +
>> +#include <dt-bindings/phy/phy.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/delay.h>
>> +#include <linux/module.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/mux/consumer.h>
>> +#include <linux/of_address.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +
>> +#define CMU_R07C		0x7c
>> +
>> +#define COMLANE_R138		0xb38
>> +#define VERSION			0x70
>> +
>> +#define COMLANE_R190		0xb90
>> +
>> +#define COMLANE_R194		0xb94
>> +
>> +#define SERDES_CTRL		0x1fd0
>> +
>> +#define WIZ_LANEXCTL_STS	0x1fe0
>> +#define TX0_DISABLE_STATE	0x4
>> +#define TX0_SLEEP_STATE		0x5
>> +#define TX0_SNOOZE_STATE	0x6
>> +#define TX0_ENABLE_STATE	0x7
>> +
>> +#define RX0_DISABLE_STATE	0x4
>> +#define RX0_SLEEP_STATE		0x5
>> +#define RX0_SNOOZE_STATE	0x6
>> +#define RX0_ENABLE_STATE	0x7
>> +
>> +#define WIZ_PLL_CTRL		0x1ff4
>> +#define PLL_DISABLE_STATE	0x4
>> +#define PLL_SLEEP_STATE		0x5
>> +#define PLL_SNOOZE_STATE	0x6
>> +#define PLL_ENABLE_STATE	0x7
>> +
>> +#define PLL_LOCK_TIME		100000	/* in microseconds */
>> +#define SLEEP_TIME		100	/* in microseconds */
>> +
>> +#define LANE_USB3		0x0
>> +#define LANE_PCIE0_LANE0	0x1
>> +
>> +#define LANE_PCIE1_LANE0	0x0
>> +#define LANE_PCIE0_LANE1	0x1
>> +
>> +#define SERDES_NUM_CLOCKS	3
>> +
>> +struct serdes_am654_clk_mux {
>> +	struct clk_hw	hw;
>> +	struct regmap	*regmap;
>> +	unsigned int	reg;
>> +	int		*table;
>> +	u32		mask;
>> +	u8		shift;
>> +	struct clk_init_data clk_data;
>> +};
>> +
>> +#define to_serdes_am654_clk_mux(_hw)	\
>> +		container_of(_hw, struct serdes_am654_clk_mux, hw)
>> +
>> +static struct regmap_config serdes_am654_regmap_config = {
>> +	.reg_bits = 32,
>> +	.val_bits = 32,
>> +	.reg_stride = 4,
>> +	.fast_io = true,
>> +};
>> +
>> +static const struct reg_field cmu_master_cdn_o = REG_FIELD(CMU_R07C, 24, 24);
>> +static const struct reg_field config_version = REG_FIELD(COMLANE_R138, 16, 23);
>> +static const struct reg_field l1_master_cdn_o = REG_FIELD(COMLANE_R190, 9, 9);
>> +static const struct reg_field cmu_ok_i_0 = REG_FIELD(COMLANE_R194, 19, 19);
>> +static const struct reg_field por_en = REG_FIELD(SERDES_CTRL, 29, 29);
>> +static const struct reg_field tx0_enable = REG_FIELD(WIZ_LANEXCTL_STS, 29, 31);
>> +static const struct reg_field rx0_enable = REG_FIELD(WIZ_LANEXCTL_STS, 13, 15);
>> +static const struct reg_field pll_enable = REG_FIELD(WIZ_PLL_CTRL, 29, 31);
>> +static const struct reg_field pll_ok = REG_FIELD(WIZ_PLL_CTRL, 28, 28);
>> +
>> +struct serdes_am654 {
>> +	struct regmap		*regmap;
>> +	struct regmap_field	*cmu_master_cdn_o;
>> +	struct regmap_field	*config_version;
>> +	struct regmap_field	*l1_master_cdn_o;
>> +	struct regmap_field	*cmu_ok_i_0;
>> +	struct regmap_field	*por_en;
>> +	struct regmap_field	*tx0_enable;
>> +	struct regmap_field	*rx0_enable;
>> +	struct regmap_field	*pll_enable;
>> +	struct regmap_field	*pll_ok;
>> +
>> +	struct device		*dev;
>> +	struct mux_control	*control;
>> +	bool			busy;
>> +	u32			type;
>> +	struct device_node	*of_node;
>> +	struct clk_onecell_data	clk_data;
>> +	struct clk		*clks[SERDES_NUM_CLOCKS];
>> +};
>> +
>> +static int serdes_am654_enable_pll(struct serdes_am654 *phy)
>> +{
>> +	int ret;
>> +	u32 val;
>> +
>> +	ret = regmap_field_write(phy->pll_enable, PLL_ENABLE_STATE);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return regmap_field_read_poll_timeout(phy->pll_ok, val, val, 1000,
>> +					      PLL_LOCK_TIME);
>> +}
>> +
>> +static void serdes_am654_disable_pll(struct serdes_am654 *phy)
>> +{
>> +	struct device *dev = phy->dev;
>> +	int ret;
>> +
>> +	ret = regmap_field_write(phy->pll_enable, PLL_DISABLE_STATE);
>> +	if (ret)
>> +		dev_err(dev, "Failed to disable PLL\n");
>> +}
>> +
>> +static int serdes_am654_enable_txrx(struct serdes_am654 *phy)
>> +{
>> +	int ret;
>> +
>> +	/* Enable TX */
>> +	ret = regmap_field_write(phy->tx0_enable, TX0_ENABLE_STATE);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Enable RX */
>> +	ret = regmap_field_write(phy->rx0_enable, RX0_ENABLE_STATE);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int serdes_am654_disable_txrx(struct serdes_am654 *phy)
>> +{
>> +	int ret;
>> +
>> +	/* Disable TX */
>> +	ret = regmap_field_write(phy->tx0_enable, TX0_DISABLE_STATE);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Disable RX */
>> +	ret = regmap_field_write(phy->rx0_enable, RX0_DISABLE_STATE);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int serdes_am654_power_on(struct phy *x)
>> +{
>> +	struct serdes_am654 *phy = phy_get_drvdata(x);
>> +	struct device *dev = phy->dev;
>> +	int ret;
>> +	u32 val;
>> +
>> +	ret = serdes_am654_enable_pll(phy);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to enable PLL\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = serdes_am654_enable_txrx(phy);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to enable TX RX\n");
>> +		return ret;
>> +	}
>> +
>> +	return regmap_field_read_poll_timeout(phy->cmu_ok_i_0, val, val,
>> +					      SLEEP_TIME, PLL_LOCK_TIME);
>> +}
>> +
>> +static int serdes_am654_power_off(struct phy *x)
>> +{
>> +	struct serdes_am654 *phy = phy_get_drvdata(x);
>> +
>> +	serdes_am654_disable_txrx(phy);
>> +	serdes_am654_disable_pll(phy);
>> +
>> +	return 0;
>> +}
>> +
>> +static int serdes_am654_init(struct phy *x)
>> +{
>> +	struct serdes_am654 *phy = phy_get_drvdata(x);
>> +	int ret;
>> +
>> +	ret = regmap_field_write(phy->config_version, VERSION);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_field_write(phy->cmu_master_cdn_o, 0x1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_field_write(phy->l1_master_cdn_o, 0x1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int serdes_am654_reset(struct phy *x)
>> +{
>> +	struct serdes_am654 *phy = phy_get_drvdata(x);
>> +	int ret;
>> +
>> +	ret = regmap_field_write(phy->por_en, 0x1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	mdelay(1);
>> +
>> +	ret = regmap_field_write(phy->por_en, 0x0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static void serdes_am654_release(struct phy *x)
>> +{
>> +	struct serdes_am654 *phy = phy_get_drvdata(x);
>> +
>> +	phy->type = PHY_NONE;
>> +	phy->busy = false;
>> +	mux_control_deselect(phy->control);
> 
> Here you unconditionally deselect the mux, and that seems
> dangerous. Are you *sure* that ->release may not be called
> without a successful xlate call?

Yeah, without a successful xlate(), the consumer will never get a reference to
the PHY and the ->release() is invoked only from phy_put() which needs a
reference to the PHY.
> 
> I'm not 100% sure of that, but I have not looked at the phy
> code before today, so it may very well be the case that this
> is safe...
> 
>> +}
>> +
>> +struct phy *serdes_am654_xlate(struct device *dev, struct of_phandle_args
>> +				 *args)
>> +{
>> +	struct serdes_am654 *am654_phy;
>> +	struct phy *phy;
>> +	int ret;
>> +
>> +	phy = of_phy_simple_xlate(dev, args);
>> +	if (IS_ERR(phy))
>> +		return phy;
>> +
>> +	am654_phy = phy_get_drvdata(phy);
>> +	if (am654_phy->busy)
>> +		return ERR_PTR(-EBUSY);
>> +
>> +	ret = mux_control_select(am654_phy->control, args->args[1]);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to select SERDES Lane Function\n");
>> +		return ERR_PTR(ret);
>> +	}
> 
> *However*
> 
> Here you select the mux as the last action, good, but, a mux must
> be handled with that same care as a locking primitive, i.e.
> successful selects must be perfectly balanced with deselects. I
> see no guarantee of that here, since there are other failures
> possible after the xlate call. So, being last in the function
> does not really help. If I read the code correctly, the
> phy core may fail if try_module_get fails in phy_get(). If that
> ever happens, a successful call to mux_control_select is simply
> forgotten, and the mux will be locked indefinitely.

Good catch. While adding ->release() ops which is only invoked from phy_put,
perhaps this was missed. Ideally it should be invoked from other places where
there is a failure after phy_get.
> 
> am654_phy->busy will also be set indefinitely, so you will get
> -EBUSY and not a hard deadlock. At least here, but if the now
> locked mux control happens to also control some other muxes
> (probably unlikely, but if), then their consumers will potentially
> deadlock hard. But that's just after a cursory reading, so I may
> completely miss something...

The ->busy here should prevent two consumers trying to control the same mux.
> 
> Thinking some more, the above mentioned forgotten phy problem
> in fact looks like a generic leak in phy_get. It will simply
> leak any and all phys where the owner module is not yet
> available. Or, am I missing something?

yes you are right. Ideally we should have invoked ->release() after phy_get()
failures.
> 
> Perhaps fixing that is all that is needed to make the mux
> select/deselect calls guaranteed to be balanced?

That's correct. Thanks for reporting this.

Cheers
Kishon

Re: [PATCH v4 4/5] phy: ti: Add a new SERDES driver for TI's AM654x SoC

[Peter Rosin]

Hi,

On 2019-06-13 06:57, Kishon Vijay Abraham I wrote:
> Hi Peter,
> 
> On 13/06/19 4:20 AM, Peter Rosin wrote:
>> Hi!
>>
>> [I know this has already been merged upstream, but I only just
>>  now noticed the code and went to the archives to find the
>>  originating mail. I hope I managed to set in-reply-to correctly...]
>>
>> The mux handling is problematic and does not follow the rules.
>> It needs to be fixed, or you may face deadlocks. See below.
>>
>> On 2019-04-05 11:08, Kishon Vijay Abraham I wrote:
>>> Add a new SERDES driver for TI's AM654x SoC which configures
>>> the SERDES only for PCIe. Support fo USB3 will be added later.
>>>
>>> SERDES in am654x has three input clocks (left input, externel reference
>>> clock and right input) and two output clocks (left output and right
>>> output) in addition to a PLL mux clock which the SERDES uses for Clock
>>> Multiplier Unit (CMU refclock).
>>>
>>> The PLL mux clock can select from one of the three input clocks.
>>> The right output can select between left input and external reference
>>> clock while the left output can select between the right input and
>>> external reference clock.
>>>
>>> The driver has support to select PLL mux and left/right output mux as
>>> specified in device tree.
>>>
>>> [rogerq@ti.com: Fix boot lockup caused by accessing a structure member
>>> (hw->init) allocated in stack of probe() and accessed in get_parent]
>>> [rogerq@ti.com: Fix "Failed to find the parent" warnings]
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

*snip*

>>> +static void serdes_am654_release(struct phy *x)
>>> +{
>>> +	struct serdes_am654 *phy = phy_get_drvdata(x);
>>> +
>>> +	phy->type = PHY_NONE;
>>> +	phy->busy = false;
>>> +	mux_control_deselect(phy->control);
>>
>> Here you unconditionally deselect the mux, and that seems
>> dangerous. Are you *sure* that ->release may not be called
>> without a successful xlate call?
> 
> Yeah, without a successful xlate(), the consumer will never get a reference to
> the PHY and the ->release() is invoked only from phy_put() which needs a
> reference to the PHY.

Yes, I thought it might be ok, but good that you can confirm it.

>> I'm not 100% sure of that, but I have not looked at the phy
>> code before today, so it may very well be the case that this
>> is safe...
>>
>>> +}
>>> +
>>> +struct phy *serdes_am654_xlate(struct device *dev, struct of_phandle_args
>>> +				 *args)
>>> +{
>>> +	struct serdes_am654 *am654_phy;
>>> +	struct phy *phy;
>>> +	int ret;
>>> +
>>> +	phy = of_phy_simple_xlate(dev, args);
>>> +	if (IS_ERR(phy))
>>> +		return phy;
>>> +
>>> +	am654_phy = phy_get_drvdata(phy);
>>> +	if (am654_phy->busy)
>>> +		return ERR_PTR(-EBUSY);
>>> +
>>> +	ret = mux_control_select(am654_phy->control, args->args[1]);
>>> +	if (ret) {
>>> +		dev_err(dev, "Failed to select SERDES Lane Function\n");
>>> +		return ERR_PTR(ret);
>>> +	}
>>
>> *However*
>>
>> Here you select the mux as the last action, good, but, a mux must
>> be handled with that same care as a locking primitive, i.e.
>> successful selects must be perfectly balanced with deselects. I
>> see no guarantee of that here, since there are other failures
>> possible after the xlate call. So, being last in the function
>> does not really help. If I read the code correctly, the
>> phy core may fail if try_module_get fails in phy_get(). If that
>> ever happens, a successful call to mux_control_select is simply
>> forgotten, and the mux will be locked indefinitely.
> 
> Good catch. While adding ->release() ops which is only invoked from phy_put,
> perhaps this was missed. Ideally it should be invoked from other places where
> there is a failure after phy_get.
>>
>> am654_phy->busy will also be set indefinitely, so you will get
>> -EBUSY and not a hard deadlock. At least here, but if the now
>> locked mux control happens to also control some other muxes
>> (probably unlikely, but if), then their consumers will potentially
>> deadlock hard. But that's just after a cursory reading, so I may
>> completely miss something...
> 
> The ->busy here should prevent two consumers trying to control the same mux.

Aha, you do not seem to be aware that one mux controller can
have multiple independent consumers (a mux is not like a gpio
or a pwm in that aspect). What I'm talking about is a single
mux control that controls several parallel muxes, each with its
own consumer. In the specific case you are targeting, that may
not be possible due to some hardware reason or something, but
looking at just this driver *it* cannot know that the mux will
be available just because it has a local ->busy flag.

For this case, I get the feeling that the mux may be selected for
a very long time, right? It is never about selecting the mux,
doing something for x milli/microseconds or so and then deselecting
the mux. Perhaps the mux will typically sit in the same state for
the entire uptime of the machine?

If you have these very long access patterns, the sharing capability
of the mux controls are pretty much useless, and I have
contemplating a mux mode to support this case. I.e. where you
lock/unlock the mux control once at probe/release (or similar),
and then basically instead of a shared mux get an exclusive mux
where the consumer is responsible for not making parallel accesses.
In other words, just like a gpio or a pwm.

The problem is that I then need a definition of "long" and "short"
accesses, and I split the mux universe in two...

>> Thinking some more, the above mentioned forgotten phy problem
>> in fact looks like a generic leak in phy_get. It will simply
>> leak any and all phys where the owner module is not yet
>> available. Or, am I missing something?
> 
> yes you are right. Ideally we should have invoked ->release() after phy_get()
> failures.
>>
>> Perhaps fixing that is all that is needed to make the mux
>> select/deselect calls guaranteed to be balanced?
> 
> That's correct. Thanks for reporting this.

No problem!

Cheers,
Peter

Re: [PATCH v4 4/5] phy: ti: Add a new SERDES driver for TI's AM654x SoC

[Kishon Vijay Abraham I]

Hi,

On 13/06/19 1:22 PM, Peter Rosin wrote:
> Hi,
> 
> On 2019-06-13 06:57, Kishon Vijay Abraham I wrote:
>> Hi Peter,
>>
>> On 13/06/19 4:20 AM, Peter Rosin wrote:
>>> Hi!
>>>
>>> [I know this has already been merged upstream, but I only just
>>>  now noticed the code and went to the archives to find the
>>>  originating mail. I hope I managed to set in-reply-to correctly...]
>>>
>>> The mux handling is problematic and does not follow the rules.
>>> It needs to be fixed, or you may face deadlocks. See below.
>>>
>>> On 2019-04-05 11:08, Kishon Vijay Abraham I wrote:
>>>> Add a new SERDES driver for TI's AM654x SoC which configures
>>>> the SERDES only for PCIe. Support fo USB3 will be added later.
>>>>
>>>> SERDES in am654x has three input clocks (left input, externel reference
>>>> clock and right input) and two output clocks (left output and right
>>>> output) in addition to a PLL mux clock which the SERDES uses for Clock
>>>> Multiplier Unit (CMU refclock).
>>>>
>>>> The PLL mux clock can select from one of the three input clocks.
>>>> The right output can select between left input and external reference
>>>> clock while the left output can select between the right input and
>>>> external reference clock.
>>>>
>>>> The driver has support to select PLL mux and left/right output mux as
>>>> specified in device tree.
>>>>
>>>> [rogerq@ti.com: Fix boot lockup caused by accessing a structure member
>>>> (hw->init) allocated in stack of probe() and accessed in get_parent]
>>>> [rogerq@ti.com: Fix "Failed to find the parent" warnings]
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> 
> *snip*
> 
>>>> +static void serdes_am654_release(struct phy *x)
>>>> +{
>>>> +	struct serdes_am654 *phy = phy_get_drvdata(x);
>>>> +
>>>> +	phy->type = PHY_NONE;
>>>> +	phy->busy = false;
>>>> +	mux_control_deselect(phy->control);
>>>
>>> Here you unconditionally deselect the mux, and that seems
>>> dangerous. Are you *sure* that ->release may not be called
>>> without a successful xlate call?
>>
>> Yeah, without a successful xlate(), the consumer will never get a reference to
>> the PHY and the ->release() is invoked only from phy_put() which needs a
>> reference to the PHY.
> 
> Yes, I thought it might be ok, but good that you can confirm it.
> 
>>> I'm not 100% sure of that, but I have not looked at the phy
>>> code before today, so it may very well be the case that this
>>> is safe...
>>>
>>>> +}
>>>> +
>>>> +struct phy *serdes_am654_xlate(struct device *dev, struct of_phandle_args
>>>> +				 *args)
>>>> +{
>>>> +	struct serdes_am654 *am654_phy;
>>>> +	struct phy *phy;
>>>> +	int ret;
>>>> +
>>>> +	phy = of_phy_simple_xlate(dev, args);
>>>> +	if (IS_ERR(phy))
>>>> +		return phy;
>>>> +
>>>> +	am654_phy = phy_get_drvdata(phy);
>>>> +	if (am654_phy->busy)
>>>> +		return ERR_PTR(-EBUSY);
>>>> +
>>>> +	ret = mux_control_select(am654_phy->control, args->args[1]);
>>>> +	if (ret) {
>>>> +		dev_err(dev, "Failed to select SERDES Lane Function\n");
>>>> +		return ERR_PTR(ret);
>>>> +	}
>>>
>>> *However*
>>>
>>> Here you select the mux as the last action, good, but, a mux must
>>> be handled with that same care as a locking primitive, i.e.
>>> successful selects must be perfectly balanced with deselects. I
>>> see no guarantee of that here, since there are other failures
>>> possible after the xlate call. So, being last in the function
>>> does not really help. If I read the code correctly, the
>>> phy core may fail if try_module_get fails in phy_get(). If that
>>> ever happens, a successful call to mux_control_select is simply
>>> forgotten, and the mux will be locked indefinitely.
>>
>> Good catch. While adding ->release() ops which is only invoked from phy_put,
>> perhaps this was missed. Ideally it should be invoked from other places where
>> there is a failure after phy_get.
>>>
>>> am654_phy->busy will also be set indefinitely, so you will get
>>> -EBUSY and not a hard deadlock. At least here, but if the now
>>> locked mux control happens to also control some other muxes
>>> (probably unlikely, but if), then their consumers will potentially
>>> deadlock hard. But that's just after a cursory reading, so I may
>>> completely miss something...
>>
>> The ->busy here should prevent two consumers trying to control the same mux.
> 
> Aha, you do not seem to be aware that one mux controller can
> have multiple independent consumers (a mux is not like a gpio
> or a pwm in that aspect). What I'm talking about is a single
> mux control that controls several parallel muxes, each with its
> own consumer. In the specific case you are targeting, that may
> not be possible due to some hardware reason or something, but
> looking at just this driver *it* cannot know that the mux will
> be available just because it has a local ->busy flag.

I think this should be okay atleast for my case.

My dt node will looks something like this
serdes_mux: mux-controller {
	compatible = "mmio-mux";
        #mux-control-cells = <1>;
        mux-reg-masks = <0x4080 0x3>, /* SERDES0 lane select */
                        <0x4090 0x3>; /* SERDES1 lane select */
}

Here SERDES lane0 is muxed between PCIe0 Lane0, ICSS2 SGMII Lane0.
The consumers of SERDES0 (in this case PCIe and ICSS), will have to invoke
->xlate of AM654 SERDES driver to select the mux and ->busy should be able to
tell whether it is available.

Only when multiple drivers try to get devm_mux_control_get and use
mux_control_select, the problem might occur. For my case, only SERDES is the
consumer of mux.
> 
> For this case, I get the feeling that the mux may be selected for
> a very long time, right? It is never about selecting the mux,
> doing something for x milli/microseconds or so and then deselecting
> the mux. Perhaps the mux will typically sit in the same state for
> the entire uptime of the machine?

Yes right. It'll be for the entire up time. Well, with dt overlay this could
change.
> 
> If you have these very long access patterns, the sharing capability
> of the mux controls are pretty much useless, and I have
> contemplating a mux mode to support this case. I.e. where you
> lock/unlock the mux control once at probe/release (or similar),
> and then basically instead of a shared mux get an exclusive mux
> where the consumer is responsible for not making parallel accesses.
> In other words, just like a gpio or a pwm.

Yes, having an exclusive mux will make sure no one can accidentally modify the mux.

> 
> The problem is that I then need a definition of "long" and "short"
> accesses, and I split the mux universe in two...

Are you trying to make sure there is no deadlock when two different consumers
try to control a shared mux?

Thanks
Kishon

Re: [PATCH v4 4/5] phy: ti: Add a new SERDES driver for TI's AM654x SoC

[Peter Rosin]

Hi,

Sorry for the late reply, $-work interfered...

On 2019-06-13 12:21, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On 13/06/19 1:22 PM, Peter Rosin wrote:
>> Hi,
>>
>> On 2019-06-13 06:57, Kishon Vijay Abraham I wrote:
>>> Hi Peter,
>>>
>>> On 13/06/19 4:20 AM, Peter Rosin wrote:
>>>> Hi!
>>>>
>>>> [I know this has already been merged upstream, but I only just
>>>>  now noticed the code and went to the archives to find the
>>>>  originating mail. I hope I managed to set in-reply-to correctly...]
>>>>
>>>> The mux handling is problematic and does not follow the rules.
>>>> It needs to be fixed, or you may face deadlocks. See below.
>>>>
>>>> On 2019-04-05 11:08, Kishon Vijay Abraham I wrote:
>>>>> Add a new SERDES driver for TI's AM654x SoC which configures
>>>>> the SERDES only for PCIe. Support fo USB3 will be added later.
>>>>>
>>>>> SERDES in am654x has three input clocks (left input, externel reference
>>>>> clock and right input) and two output clocks (left output and right
>>>>> output) in addition to a PLL mux clock which the SERDES uses for Clock
>>>>> Multiplier Unit (CMU refclock).
>>>>>
>>>>> The PLL mux clock can select from one of the three input clocks.
>>>>> The right output can select between left input and external reference
>>>>> clock while the left output can select between the right input and
>>>>> external reference clock.
>>>>>
>>>>> The driver has support to select PLL mux and left/right output mux as
>>>>> specified in device tree.
>>>>>
>>>>> [rogerq@ti.com: Fix boot lockup caused by accessing a structure member
>>>>> (hw->init) allocated in stack of probe() and accessed in get_parent]
>>>>> [rogerq@ti.com: Fix "Failed to find the parent" warnings]
>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>
>> *snip*
>>
>>>>> +static void serdes_am654_release(struct phy *x)
>>>>> +{
>>>>> +	struct serdes_am654 *phy = phy_get_drvdata(x);
>>>>> +
>>>>> +	phy->type = PHY_NONE;
>>>>> +	phy->busy = false;
>>>>> +	mux_control_deselect(phy->control);
>>>>
>>>> Here you unconditionally deselect the mux, and that seems
>>>> dangerous. Are you *sure* that ->release may not be called
>>>> without a successful xlate call?
>>>
>>> Yeah, without a successful xlate(), the consumer will never get a reference to
>>> the PHY and the ->release() is invoked only from phy_put() which needs a
>>> reference to the PHY.
>>
>> Yes, I thought it might be ok, but good that you can confirm it.
>>
>>>> I'm not 100% sure of that, but I have not looked at the phy
>>>> code before today, so it may very well be the case that this
>>>> is safe...
>>>>
>>>>> +}
>>>>> +
>>>>> +struct phy *serdes_am654_xlate(struct device *dev, struct of_phandle_args
>>>>> +				 *args)
>>>>> +{
>>>>> +	struct serdes_am654 *am654_phy;
>>>>> +	struct phy *phy;
>>>>> +	int ret;
>>>>> +
>>>>> +	phy = of_phy_simple_xlate(dev, args);
>>>>> +	if (IS_ERR(phy))
>>>>> +		return phy;
>>>>> +
>>>>> +	am654_phy = phy_get_drvdata(phy);
>>>>> +	if (am654_phy->busy)
>>>>> +		return ERR_PTR(-EBUSY);
>>>>> +
>>>>> +	ret = mux_control_select(am654_phy->control, args->args[1]);
>>>>> +	if (ret) {
>>>>> +		dev_err(dev, "Failed to select SERDES Lane Function\n");
>>>>> +		return ERR_PTR(ret);
>>>>> +	}
>>>>
>>>> *However*
>>>>
>>>> Here you select the mux as the last action, good, but, a mux must
>>>> be handled with that same care as a locking primitive, i.e.
>>>> successful selects must be perfectly balanced with deselects. I
>>>> see no guarantee of that here, since there are other failures
>>>> possible after the xlate call. So, being last in the function
>>>> does not really help. If I read the code correctly, the
>>>> phy core may fail if try_module_get fails in phy_get(). If that
>>>> ever happens, a successful call to mux_control_select is simply
>>>> forgotten, and the mux will be locked indefinitely.
>>>
>>> Good catch. While adding ->release() ops which is only invoked from phy_put,
>>> perhaps this was missed. Ideally it should be invoked from other places where
>>> there is a failure after phy_get.
>>>>
>>>> am654_phy->busy will also be set indefinitely, so you will get
>>>> -EBUSY and not a hard deadlock. At least here, but if the now
>>>> locked mux control happens to also control some other muxes
>>>> (probably unlikely, but if), then their consumers will potentially
>>>> deadlock hard. But that's just after a cursory reading, so I may
>>>> completely miss something...
>>>
>>> The ->busy here should prevent two consumers trying to control the same mux.
>>
>> Aha, you do not seem to be aware that one mux controller can
>> have multiple independent consumers (a mux is not like a gpio
>> or a pwm in that aspect). What I'm talking about is a single
>> mux control that controls several parallel muxes, each with its
>> own consumer. In the specific case you are targeting, that may
>> not be possible due to some hardware reason or something, but
>> looking at just this driver *it* cannot know that the mux will
>> be available just because it has a local ->busy flag.
> 
> I think this should be okay atleast for my case.
> 
> My dt node will looks something like this
> serdes_mux: mux-controller {
> 	compatible = "mmio-mux";
>         #mux-control-cells = <1>;
>         mux-reg-masks = <0x4080 0x3>, /* SERDES0 lane select */
>                         <0x4090 0x3>; /* SERDES1 lane select */
> }
> 
> Here SERDES lane0 is muxed between PCIe0 Lane0, ICSS2 SGMII Lane0.
> The consumers of SERDES0 (in this case PCIe and ICSS), will have to invoke
> ->xlate of AM654 SERDES driver to select the mux and ->busy should be able to
> tell whether it is available.

Yes, for a single mux consumer everything is fine.

> Only when multiple drivers try to get devm_mux_control_get and use
> mux_control_select, the problem might occur. For my case, only SERDES is the
> consumer of mux.

For your device tree that is true, indeed, but someone else might extend
that DT with an additional mux consumer or reuse your driver in some
unexpected manner. Arguably this person gets to keep the pieces when it
breaks. And with all likelihood, the register that controls this mux
does not control any other mux, so there is probably very little reason
for someone to extend the DT in this way.

>>
>> For this case, I get the feeling that the mux may be selected for
>> a very long time, right? It is never about selecting the mux,
>> doing something for x milli/microseconds or so and then deselecting
>> the mux. Perhaps the mux will typically sit in the same state for
>> the entire uptime of the machine?
> 
> Yes right. It'll be for the entire up time. Well, with dt overlay this could
> change.
>>
>> If you have these very long access patterns, the sharing capability
>> of the mux controls are pretty much useless, and I have
>> contemplating a mux mode to support this case. I.e. where you
>> lock/unlock the mux control once at probe/release (or similar),
>> and then basically instead of a shared mux get an exclusive mux
>> where the consumer is responsible for not making parallel accesses.
>> In other words, just like a gpio or a pwm.
> 
> Yes, having an exclusive mux will make sure no one can accidentally modify the mux.
> 
>>
>> The problem is that I then need a definition of "long" and "short"
>> accesses, and I split the mux universe in two...
> 
> Are you trying to make sure there is no deadlock when two different consumers
> try to control a shared mux?

I'm just noticing that when I added the mux framework, the big motivation
was to serialize multiple (two in my case) consumers since the HW used
the same GPIO lines to control muxes for both an I2C bus and ADC signals.
But, all other users seem to use the mux framework for more longstanding
selects and are just hindered by the locking aspect of select/deselect.
I.e., it seems that most users (you included) would be much better off
with an API that grabs the mux and is then free to just select its desired
position (or deselect it if that is beneficial for some power consumption
reason or something), without the requirement of balanced select/deselect
calls.

So, I'm basically just poking around in how the mux framework is used
and what its users need...

Cheers,
Peter

[PATCH v4 4/5] phy: ti: Add a new SERDES driver for TI's AM654x SoC

[Kishon Vijay Abraham I]

Add a new SERDES driver for TI's AM654x SoC which configures
the SERDES only for PCIe. Support fo USB3 will be added later.

SERDES in am654x has three input clocks (left input, externel reference
clock and right input) and two output clocks (left output and right
output) in addition to a PLL mux clock which the SERDES uses for Clock
Multiplier Unit (CMU refclock).

The PLL mux clock can select from one of the three input clocks.
The right output can select between left input and external reference
clock while the left output can select between the right input and
external reference clock.

The driver has support to select PLL mux and left/right output mux as
specified in device tree.

[rogerq@ti.com: Fix boot lockup caused by accessing a structure member
(hw->init) allocated in stack of probe() and accessed in get_parent]
[rogerq@ti.com: Fix "Failed to find the parent" warnings]
Signed-off-by: Roger Quadros <rogerq@ti.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/phy/ti/Kconfig            |  12 +
 drivers/phy/ti/Makefile           |   1 +
 drivers/phy/ti/phy-am654-serdes.c | 624 ++++++++++++++++++++++++++++++
 3 files changed, 637 insertions(+)
 create mode 100644 drivers/phy/ti/phy-am654-serdes.c

diff --git a/drivers/phy/ti/Kconfig b/drivers/phy/ti/Kconfig
index 7cdc35f8c862..6931c87235b9 100644
--- a/drivers/phy/ti/Kconfig
+++ b/drivers/phy/ti/Kconfig
@@ -20,6 +20,18 @@ config PHY_DM816X_USB
 	help
 	  Enable this for dm816x USB to work.
 
+config PHY_AM654_SERDES
+	tristate "TI AM654 SERDES support"
+	depends on OF && ARCH_K3 || COMPILE_TEST
+	depends on COMMON_CLK
+	select GENERIC_PHY
+	select MULTIPLEXER
+	select REGMAP_MMIO
+	select MUX_MMIO
+	help
+	  This option enables support for TI AM654 SerDes PHY used for
+	  PCIe.
+
 config OMAP_CONTROL_PHY
 	tristate "OMAP CONTROL PHY Driver"
 	depends on ARCH_OMAP2PLUS || COMPILE_TEST
diff --git a/drivers/phy/ti/Makefile b/drivers/phy/ti/Makefile
index 368de8e1548d..601bbd88f35e 100644
--- a/drivers/phy/ti/Makefile
+++ b/drivers/phy/ti/Makefile
@@ -6,5 +6,6 @@ obj-$(CONFIG_OMAP_USB2)			+= phy-omap-usb2.o
 obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
 obj-$(CONFIG_PHY_TUSB1210)		+= phy-tusb1210.o
 obj-$(CONFIG_TWL4030_USB)		+= phy-twl4030-usb.o
+obj-$(CONFIG_PHY_AM654_SERDES)		+= phy-am654-serdes.o
 obj-$(CONFIG_PHY_TI_GMII_SEL)		+= phy-gmii-sel.o
 obj-$(CONFIG_PHY_TI_KEYSTONE_SERDES)	+= phy-keystone-serdes.o
diff --git a/drivers/phy/ti/phy-am654-serdes.c b/drivers/phy/ti/phy-am654-serdes.c
new file mode 100644
index 000000000000..4817c67abbbb
--- /dev/null
+++ b/drivers/phy/ti/phy-am654-serdes.c
@@ -0,0 +1,624 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * PCIe SERDES driver for AM654x SoC
+ *
+ * Copyright (C) 2018 - 2019 Texas Instruments Incorporated - http://www.ti.com/
+ * Author: Kishon Vijay Abraham I <kishon@ti.com>
+ */
+
+#include <dt-bindings/phy/phy.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mux/consumer.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+
+#define CMU_R07C		0x7c
+
+#define COMLANE_R138		0xb38
+#define VERSION			0x70
+
+#define COMLANE_R190		0xb90
+
+#define COMLANE_R194		0xb94
+
+#define SERDES_CTRL		0x1fd0
+
+#define WIZ_LANEXCTL_STS	0x1fe0
+#define TX0_DISABLE_STATE	0x4
+#define TX0_SLEEP_STATE		0x5
+#define TX0_SNOOZE_STATE	0x6
+#define TX0_ENABLE_STATE	0x7
+
+#define RX0_DISABLE_STATE	0x4
+#define RX0_SLEEP_STATE		0x5
+#define RX0_SNOOZE_STATE	0x6
+#define RX0_ENABLE_STATE	0x7
+
+#define WIZ_PLL_CTRL		0x1ff4
+#define PLL_DISABLE_STATE	0x4
+#define PLL_SLEEP_STATE		0x5
+#define PLL_SNOOZE_STATE	0x6
+#define PLL_ENABLE_STATE	0x7
+
+#define PLL_LOCK_TIME		100000	/* in microseconds */
+#define SLEEP_TIME		100	/* in microseconds */
+
+#define LANE_USB3		0x0
+#define LANE_PCIE0_LANE0	0x1
+
+#define LANE_PCIE1_LANE0	0x0
+#define LANE_PCIE0_LANE1	0x1
+
+#define SERDES_NUM_CLOCKS	3
+
+struct serdes_am654_clk_mux {
+	struct clk_hw	hw;
+	struct regmap	*regmap;
+	unsigned int	reg;
+	int		*table;
+	u32		mask;
+	u8		shift;
+	struct clk_init_data clk_data;
+};
+
+#define to_serdes_am654_clk_mux(_hw)	\
+		container_of(_hw, struct serdes_am654_clk_mux, hw)
+
+static struct regmap_config serdes_am654_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.fast_io = true,
+};
+
+static const struct reg_field cmu_master_cdn_o = REG_FIELD(CMU_R07C, 24, 24);
+static const struct reg_field config_version = REG_FIELD(COMLANE_R138, 16, 23);
+static const struct reg_field l1_master_cdn_o = REG_FIELD(COMLANE_R190, 9, 9);
+static const struct reg_field cmu_ok_i_0 = REG_FIELD(COMLANE_R194, 19, 19);
+static const struct reg_field por_en = REG_FIELD(SERDES_CTRL, 29, 29);
+static const struct reg_field tx0_enable = REG_FIELD(WIZ_LANEXCTL_STS, 29, 31);
+static const struct reg_field rx0_enable = REG_FIELD(WIZ_LANEXCTL_STS, 13, 15);
+static const struct reg_field pll_enable = REG_FIELD(WIZ_PLL_CTRL, 29, 31);
+static const struct reg_field pll_ok = REG_FIELD(WIZ_PLL_CTRL, 28, 28);
+
+struct serdes_am654 {
+	struct regmap		*regmap;
+	struct regmap_field	*cmu_master_cdn_o;
+	struct regmap_field	*config_version;
+	struct regmap_field	*l1_master_cdn_o;
+	struct regmap_field	*cmu_ok_i_0;
+	struct regmap_field	*por_en;
+	struct regmap_field	*tx0_enable;
+	struct regmap_field	*rx0_enable;
+	struct regmap_field	*pll_enable;
+	struct regmap_field	*pll_ok;
+
+	struct device		*dev;
+	struct mux_control	*control;
+	bool			busy;
+	u32			type;
+	struct device_node	*of_node;
+	struct clk_onecell_data	clk_data;
+	struct clk		*clks[SERDES_NUM_CLOCKS];
+};
+
+static int serdes_am654_enable_pll(struct serdes_am654 *phy)
+{
+	int ret;
+	u32 val;
+
+	ret = regmap_field_write(phy->pll_enable, PLL_ENABLE_STATE);
+	if (ret)
+		return ret;
+
+	return regmap_field_read_poll_timeout(phy->pll_ok, val, val, 1000,
+					      PLL_LOCK_TIME);
+}
+
+static void serdes_am654_disable_pll(struct serdes_am654 *phy)
+{
+	struct device *dev = phy->dev;
+	int ret;
+
+	ret = regmap_field_write(phy->pll_enable, PLL_DISABLE_STATE);
+	if (ret)
+		dev_err(dev, "Failed to disable PLL\n");
+}
+
+static int serdes_am654_enable_txrx(struct serdes_am654 *phy)
+{
+	int ret;
+
+	/* Enable TX */
+	ret = regmap_field_write(phy->tx0_enable, TX0_ENABLE_STATE);
+	if (ret)
+		return ret;
+
+	/* Enable RX */
+	ret = regmap_field_write(phy->rx0_enable, RX0_ENABLE_STATE);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int serdes_am654_disable_txrx(struct serdes_am654 *phy)
+{
+	int ret;
+
+	/* Disable TX */
+	ret = regmap_field_write(phy->tx0_enable, TX0_DISABLE_STATE);
+	if (ret)
+		return ret;
+
+	/* Disable RX */
+	ret = regmap_field_write(phy->rx0_enable, RX0_DISABLE_STATE);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int serdes_am654_power_on(struct phy *x)
+{
+	struct serdes_am654 *phy = phy_get_drvdata(x);
+	struct device *dev = phy->dev;
+	int ret;
+	u32 val;
+
+	ret = serdes_am654_enable_pll(phy);
+	if (ret) {
+		dev_err(dev, "Failed to enable PLL\n");
+		return ret;
+	}
+
+	ret = serdes_am654_enable_txrx(phy);
+	if (ret) {
+		dev_err(dev, "Failed to enable TX RX\n");
+		return ret;
+	}
+
+	return regmap_field_read_poll_timeout(phy->cmu_ok_i_0, val, val,
+					      SLEEP_TIME, PLL_LOCK_TIME);
+}
+
+static int serdes_am654_power_off(struct phy *x)
+{
+	struct serdes_am654 *phy = phy_get_drvdata(x);
+
+	serdes_am654_disable_txrx(phy);
+	serdes_am654_disable_pll(phy);
+
+	return 0;
+}
+
+static int serdes_am654_init(struct phy *x)
+{
+	struct serdes_am654 *phy = phy_get_drvdata(x);
+	int ret;
+
+	ret = regmap_field_write(phy->config_version, VERSION);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_write(phy->cmu_master_cdn_o, 0x1);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_write(phy->l1_master_cdn_o, 0x1);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int serdes_am654_reset(struct phy *x)
+{
+	struct serdes_am654 *phy = phy_get_drvdata(x);
+	int ret;
+
+	ret = regmap_field_write(phy->por_en, 0x1);
+	if (ret)
+		return ret;
+
+	mdelay(1);
+
+	ret = regmap_field_write(phy->por_en, 0x0);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void serdes_am654_release(struct phy *x)
+{
+	struct serdes_am654 *phy = phy_get_drvdata(x);
+
+	phy->type = PHY_NONE;
+	phy->busy = false;
+	mux_control_deselect(phy->control);
+}
+
+struct phy *serdes_am654_xlate(struct device *dev, struct of_phandle_args
+				 *args)
+{
+	struct serdes_am654 *am654_phy;
+	struct phy *phy;
+	int ret;
+
+	phy = of_phy_simple_xlate(dev, args);
+	if (IS_ERR(phy))
+		return phy;
+
+	am654_phy = phy_get_drvdata(phy);
+	if (am654_phy->busy)
+		return ERR_PTR(-EBUSY);
+
+	ret = mux_control_select(am654_phy->control, args->args[1]);
+	if (ret) {
+		dev_err(dev, "Failed to select SERDES Lane Function\n");
+		return ERR_PTR(ret);
+	}
+
+	am654_phy->busy = true;
+	am654_phy->type = args->args[0];
+
+	return phy;
+}
+
+static const struct phy_ops ops = {
+	.reset		= serdes_am654_reset,
+	.init		= serdes_am654_init,
+	.power_on	= serdes_am654_power_on,
+	.power_off	= serdes_am654_power_off,
+	.release	= serdes_am654_release,
+	.owner		= THIS_MODULE,
+};
+
+static u8 serdes_am654_clk_mux_get_parent(struct clk_hw *hw)
+{
+	struct serdes_am654_clk_mux *mux = to_serdes_am654_clk_mux(hw);
+	unsigned int num_parents = clk_hw_get_num_parents(hw);
+	struct regmap *regmap = mux->regmap;
+	unsigned int reg = mux->reg;
+	unsigned int val;
+	int i;
+
+	regmap_read(regmap, reg, &val);
+	val >>= mux->shift;
+	val &= mux->mask;
+
+	for (i = 0; i < num_parents; i++)
+		if (mux->table[i] == val)
+			return i;
+
+	/*
+	 * No parent? This should never happen!
+	 * Verify if we set a valid parent in serdes_am654_clk_register()
+	 */
+	WARN(1, "Failed to find the parent of %s clock\n", hw->init->name);
+
+	/* Make the parent lookup to fail */
+	return num_parents;
+}
+
+static int serdes_am654_clk_mux_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct serdes_am654_clk_mux *mux = to_serdes_am654_clk_mux(hw);
+	struct regmap *regmap = mux->regmap;
+	unsigned int reg = mux->reg;
+	int val;
+	int ret;
+
+	val = mux->table[index];
+
+	if (val == -1)
+		return -EINVAL;
+
+	val <<= mux->shift;
+	ret = regmap_update_bits(regmap, reg, mux->mask << mux->shift, val);
+
+	return ret;
+}
+
+static const struct clk_ops serdes_am654_clk_mux_ops = {
+	.set_parent = serdes_am654_clk_mux_set_parent,
+	.get_parent = serdes_am654_clk_mux_get_parent,
+};
+
+static int mux_table[SERDES_NUM_CLOCKS][3] = {
+	/*
+	 * The entries represent values for selecting between
+	 * {left input, external reference clock, right input}
+	 * Only one of Left Output or Right Output should be used since
+	 * both left and right output clock uses the same bits and modifying
+	 * one clock will impact the other.
+	 */
+	{ BIT(2),               0, BIT(0) }, /* Mux of CMU refclk */
+	{     -1,          BIT(3), BIT(1) }, /* Mux of Left Output */
+	{ BIT(1), BIT(3) | BIT(1),     -1 }, /* Mux of Right Output */
+};
+
+static int mux_mask[SERDES_NUM_CLOCKS] = { 0x5, 0xa, 0xa };
+
+static int serdes_am654_clk_register(struct serdes_am654 *am654_phy,
+				     const char *clock_name, int clock_num)
+{
+	struct device_node *node = am654_phy->of_node;
+	struct device *dev = am654_phy->dev;
+	struct serdes_am654_clk_mux *mux;
+	struct device_node *regmap_node;
+	const char **parent_names;
+	struct clk_init_data *init;
+	unsigned int num_parents;
+	struct regmap *regmap;
+	const __be32 *addr;
+	unsigned int reg;
+	struct clk *clk;
+
+	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
+	if (!mux)
+		return -ENOMEM;
+
+	init = &mux->clk_data;
+
+	regmap_node = of_parse_phandle(node, "ti,serdes-clk", 0);
+	of_node_put(regmap_node);
+	if (!regmap_node) {
+		dev_err(dev, "Fail to get serdes-clk node\n");
+		return -ENODEV;
+	}
+
+	regmap = syscon_node_to_regmap(regmap_node->parent);
+	if (IS_ERR(regmap)) {
+		dev_err(dev, "Fail to get Syscon regmap\n");
+		return PTR_ERR(regmap);
+	}
+
+	num_parents = of_clk_get_parent_count(node);
+	if (num_parents < 2) {
+		dev_err(dev, "SERDES clock must have parents\n");
+		return -EINVAL;
+	}
+
+	parent_names = devm_kzalloc(dev, (sizeof(char *) * num_parents),
+				    GFP_KERNEL);
+	if (!parent_names)
+		return -ENOMEM;
+
+	of_clk_parent_fill(node, parent_names, num_parents);
+
+	addr = of_get_address(regmap_node, 0, NULL, NULL);
+	if (!addr)
+		return -EINVAL;
+
+	reg = be32_to_cpu(*addr);
+
+	init->ops = &serdes_am654_clk_mux_ops;
+	init->flags = CLK_SET_RATE_NO_REPARENT;
+	init->parent_names = parent_names;
+	init->num_parents = num_parents;
+	init->name = clock_name;
+
+	mux->table = mux_table[clock_num];
+	mux->regmap = regmap;
+	mux->reg = reg;
+	mux->shift = 4;
+	mux->mask = mux_mask[clock_num];
+	mux->hw.init = init;
+
+	/*
+	 * setup a sane default so get_parent() call evaluates
+	 * to a valid parent. Index 1 is the safest choice as
+	 * the default as it is valid value for all of serdes's
+	 * output clocks.
+	 */
+	serdes_am654_clk_mux_set_parent(&mux->hw, 1);
+	clk = devm_clk_register(dev, &mux->hw);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	am654_phy->clks[clock_num] = clk;
+
+	return 0;
+}
+
+static const struct of_device_id serdes_am654_id_table[] = {
+	{
+		.compatible = "ti,phy-am654-serdes",
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, serdes_am654_id_table);
+
+static int serdes_am654_regfield_init(struct serdes_am654 *am654_phy)
+{
+	struct regmap *regmap = am654_phy->regmap;
+	struct device *dev = am654_phy->dev;
+
+	am654_phy->cmu_master_cdn_o = devm_regmap_field_alloc(dev, regmap,
+							      cmu_master_cdn_o);
+	if (IS_ERR(am654_phy->cmu_master_cdn_o)) {
+		dev_err(dev, "CMU_MASTER_CDN_O reg field init failed\n");
+		return PTR_ERR(am654_phy->cmu_master_cdn_o);
+	}
+
+	am654_phy->config_version = devm_regmap_field_alloc(dev, regmap,
+							    config_version);
+	if (IS_ERR(am654_phy->config_version)) {
+		dev_err(dev, "CONFIG_VERSION reg field init failed\n");
+		return PTR_ERR(am654_phy->config_version);
+	}
+
+	am654_phy->l1_master_cdn_o = devm_regmap_field_alloc(dev, regmap,
+							     l1_master_cdn_o);
+	if (IS_ERR(am654_phy->l1_master_cdn_o)) {
+		dev_err(dev, "L1_MASTER_CDN_O reg field init failed\n");
+		return PTR_ERR(am654_phy->l1_master_cdn_o);
+	}
+
+	am654_phy->cmu_ok_i_0 = devm_regmap_field_alloc(dev, regmap,
+							cmu_ok_i_0);
+	if (IS_ERR(am654_phy->cmu_ok_i_0)) {
+		dev_err(dev, "CMU_OK_I_0 reg field init failed\n");
+		return PTR_ERR(am654_phy->cmu_ok_i_0);
+	}
+
+	am654_phy->por_en = devm_regmap_field_alloc(dev, regmap, por_en);
+	if (IS_ERR(am654_phy->por_en)) {
+		dev_err(dev, "POR_EN reg field init failed\n");
+		return PTR_ERR(am654_phy->por_en);
+	}
+
+	am654_phy->tx0_enable = devm_regmap_field_alloc(dev, regmap,
+							tx0_enable);
+	if (IS_ERR(am654_phy->tx0_enable)) {
+		dev_err(dev, "TX0_ENABLE reg field init failed\n");
+		return PTR_ERR(am654_phy->tx0_enable);
+	}
+
+	am654_phy->rx0_enable = devm_regmap_field_alloc(dev, regmap,
+							rx0_enable);
+	if (IS_ERR(am654_phy->rx0_enable)) {
+		dev_err(dev, "RX0_ENABLE reg field init failed\n");
+		return PTR_ERR(am654_phy->rx0_enable);
+	}
+
+	am654_phy->pll_enable = devm_regmap_field_alloc(dev, regmap,
+							pll_enable);
+	if (IS_ERR(am654_phy->pll_enable)) {
+		dev_err(dev, "PLL_ENABLE reg field init failed\n");
+		return PTR_ERR(am654_phy->pll_enable);
+	}
+
+	am654_phy->pll_ok = devm_regmap_field_alloc(dev, regmap, pll_ok);
+	if (IS_ERR(am654_phy->pll_ok)) {
+		dev_err(dev, "PLL_OK reg field init failed\n");
+		return PTR_ERR(am654_phy->pll_ok);
+	}
+
+	return 0;
+}
+
+static int serdes_am654_probe(struct platform_device *pdev)
+{
+	struct phy_provider *phy_provider;
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	struct clk_onecell_data *clk_data;
+	struct serdes_am654 *am654_phy;
+	struct mux_control *control;
+	const char *clock_name;
+	struct regmap *regmap;
+	void __iomem *base;
+	struct phy *phy;
+	int ret;
+	int i;
+
+	am654_phy = devm_kzalloc(dev, sizeof(*am654_phy), GFP_KERNEL);
+	if (!am654_phy)
+		return -ENOMEM;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	regmap = devm_regmap_init_mmio(dev, base, &serdes_am654_regmap_config);
+	if (IS_ERR(regmap)) {
+		dev_err(dev, "Failed to initialize regmap\n");
+		return PTR_ERR(regmap);
+	}
+
+	control = devm_mux_control_get(dev, NULL);
+	if (IS_ERR(control))
+		return PTR_ERR(control);
+
+	am654_phy->dev = dev;
+	am654_phy->of_node = node;
+	am654_phy->regmap = regmap;
+	am654_phy->control = control;
+	am654_phy->type = PHY_NONE;
+
+	ret = serdes_am654_regfield_init(am654_phy);
+	if (ret) {
+		dev_err(dev, "Failed to initialize regfields\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, am654_phy);
+
+	for (i = 0; i < SERDES_NUM_CLOCKS; i++) {
+		ret = of_property_read_string_index(node, "clock-output-names",
+						    i, &clock_name);
+		if (ret) {
+			dev_err(dev, "Failed to get clock name\n");
+			return ret;
+		}
+
+		ret = serdes_am654_clk_register(am654_phy, clock_name, i);
+		if (ret) {
+			dev_err(dev, "Failed to initialize clock %s\n",
+				clock_name);
+			return ret;
+		}
+	}
+
+	clk_data = &am654_phy->clk_data;
+	clk_data->clks = am654_phy->clks;
+	clk_data->clk_num = SERDES_NUM_CLOCKS;
+	ret = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
+	if (ret)
+		return ret;
+
+	pm_runtime_enable(dev);
+
+	phy = devm_phy_create(dev, NULL, &ops);
+	if (IS_ERR(phy))
+		return PTR_ERR(phy);
+
+	phy_set_drvdata(phy, am654_phy);
+	phy_provider = devm_of_phy_provider_register(dev, serdes_am654_xlate);
+	if (IS_ERR(phy_provider)) {
+		ret = PTR_ERR(phy_provider);
+		goto clk_err;
+	}
+
+	return 0;
+
+clk_err:
+	of_clk_del_provider(node);
+
+	return ret;
+}
+
+static int serdes_am654_remove(struct platform_device *pdev)
+{
+	struct serdes_am654 *am654_phy = platform_get_drvdata(pdev);
+	struct device_node *node = am654_phy->of_node;
+
+	pm_runtime_disable(&pdev->dev);
+	of_clk_del_provider(node);
+
+	return 0;
+}
+
+static struct platform_driver serdes_am654_driver = {
+	.probe		= serdes_am654_probe,
+	.remove		= serdes_am654_remove,
+	.driver		= {
+		.name	= "phy-am654",
+		.of_match_table = serdes_am654_id_table,
+	},
+};
+module_platform_driver(serdes_am654_driver);
+
+MODULE_AUTHOR("Texas Instruments Inc.");
+MODULE_DESCRIPTION("TI AM654x SERDES driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1