Date:   Mon, 9 May 2022 13:37:59 +0200
From:   Peter Rosin <peda@...ntia.se>
To:     Kavyasree Kotagiri <kavyasree.kotagiri@...rochip.com>,
        krzysztof.kozlowski+dt@...aro.org, nicolas.ferre@...rochip.com,
        alexandre.belloni@...tlin.com, claudiu.beznea@...rochip.com
Cc:     devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, lee.jones@...aro.org,
        linux@...linux.org.uk, Manohar.Puri@...rochip.com,
        UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH v2 4/4] mux: lan966: Add support for flexcom mux
 controller

Hi!

2022-05-09 at 10:49, Kavyasree Kotagiri wrote:
> LAN966 SoC have 5 flexcoms. Each flexcom has 2 chip-selects.
> For each chip select of each flexcom there is a configuration
> register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of
> configuration register is 21 because there are 21 shared pins
> on each of which the chip select can be mapped. Each bit of the
> register represents a different FLEXCOM_SHARED pin.
> 
> Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@...rochip.com>
> ---
>  arch/arm/mach-at91/Kconfig  |   2 +
>  drivers/mfd/atmel-flexcom.c |  55 +++++++++++++++-
>  drivers/mux/Kconfig         |  12 ++++
>  drivers/mux/Makefile        |   2 +
>  drivers/mux/lan966-flx.c    | 121 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 191 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mux/lan966-flx.c
> 
> diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
> index 279810381256..26fb0f4e1b79 100644
> --- a/arch/arm/mach-at91/Kconfig
> +++ b/arch/arm/mach-at91/Kconfig
> @@ -74,6 +74,8 @@ config SOC_LAN966
>  	select DW_APB_TIMER_OF
>  	select ARM_GIC
>  	select MEMORY
> +	select MULTIPLEXER
> +	select MUX_LAN966
>  	help
>  	  This enables support for ARMv7 based Microchip LAN966 SoC family.
>  
> diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
> index 559eb4d352b6..7cfd0fc3f4f0 100644
> --- a/drivers/mfd/atmel-flexcom.c
> +++ b/drivers/mfd/atmel-flexcom.c
> @@ -17,6 +17,7 @@
>  #include <linux/io.h>
>  #include <linux/clk.h>
>  #include <dt-bindings/mfd/atmel-flexcom.h>
> +#include <linux/mux/consumer.h>
>  
>  /* I/O register offsets */
>  #define FLEX_MR		0x0	/* Mode Register */
> @@ -28,6 +29,10 @@
>  #define FLEX_MR_OPMODE(opmode)	(((opmode) << FLEX_MR_OPMODE_OFFSET) &	\
>  				 FLEX_MR_OPMODE_MASK)
>  
> +struct atmel_flex_caps {
> +	bool has_flx_mux;
> +};
> +
>  struct atmel_flexcom {
>  	void __iomem *base;
>  	u32 opmode;
> @@ -37,6 +42,7 @@ struct atmel_flexcom {
>  static int atmel_flexcom_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> +	const struct atmel_flex_caps *caps;
>  	struct resource *res;
>  	struct atmel_flexcom *ddata;
>  	int err;
> @@ -76,13 +82,60 @@ static int atmel_flexcom_probe(struct platform_device *pdev)
>  	 */
>  	writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base + FLEX_MR);
>  
> +	caps = of_device_get_match_data(&pdev->dev);
> +	if (!caps) {
> +		dev_err(&pdev->dev, "Could not retrieve flexcom caps\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Flexcom Mux */
> +	if (caps->has_flx_mux && of_property_read_bool(np, "mux-controls")) {
> +		struct mux_control *flx_mux;
> +		struct of_phandle_args args;
> +		int i, count;
> +
> +		flx_mux = devm_mux_control_get(&pdev->dev, NULL);
> +		if (IS_ERR(flx_mux))
> +			return PTR_ERR(flx_mux);
> +
> +		count = of_property_count_strings(np, "mux-control-names");
> +		for (i = 0; i < count; i++) {
> +			err = of_parse_phandle_with_fixed_args(np, "mux-controls", 1, i, &args);
> +			if (err)
> +				break;
> +
> +			err = mux_control_select(flx_mux, args.args[0]);
> +			if (!err) {
> +				mux_control_deselect(flx_mux);

This is suspect. When you deselect the mux you rely on the mux to be
configured with "as-is" as the idle state. But you do not document that.
This is also fragile in that you silently rely on noone else selecting
the mux to some unwanted state behind your back (mux controls are not
exclusive the way e.g. gpio pins or pwms are). The protocol is that
others may get a ref to the mux control and select it as long as noone
else has selected it.

The only sane thing to do is to keep the mux selected for the whole
duration when you rely on it to be in your desired state.

> +			} else {
> +				dev_err(&pdev->dev, "Failed to select FLEXCOM mux\n");
> +				return err;
> +			}
> +		}
> +	}
> +
>  	clk_disable_unprepare(ddata->clk);
>  
>  	return devm_of_platform_populate(&pdev->dev);
>  }
>  
> +static const struct atmel_flex_caps atmel_flexcom_caps = {};
> +
> +static const struct atmel_flex_caps lan966x_flexcom_caps = {
> +	.has_flx_mux = true,
> +};
> +
>  static const struct of_device_id atmel_flexcom_of_match[] = {
> -	{ .compatible = "atmel,sama5d2-flexcom" },
> +	{
> +		.compatible = "atmel,sama5d2-flexcom",
> +		.data = &atmel_flexcom_caps,
> +	},
> +
> +	{
> +		.compatible = "microchip,lan966-flexcom",
> +		.data = &lan966x_flexcom_caps,
> +	},
> +
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);
> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> index e5c571fd232c..ea09f474bc2f 100644
> --- a/drivers/mux/Kconfig
> +++ b/drivers/mux/Kconfig
> @@ -45,6 +45,18 @@ config MUX_GPIO
>  	  To compile the driver as a module, choose M here: the module will
>  	  be called mux-gpio.
>  
> +config MUX_LAN966
> +	tristate "LAN966 Flexcom multiplexer"
> +	depends on OF || COMPILE_TEST
> +	help
> +	Lan966 Flexcom Multiplexer controller.
> +
> +	The driver supports mapping 2 chip-selects of each of the lan966
> +	flexcoms to 21 flexcom shared pins.
> +
> +	To compile the driver as a module, choose M here: the module will
> +	be called mux-lan966.
> +
>  config MUX_MMIO
>  	tristate "MMIO/Regmap register bitfield-controlled Multiplexer"
>  	depends on OF || COMPILE_TEST
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> index 6e9fa47daf56..53a9840d96fa 100644
> --- a/drivers/mux/Makefile
> +++ b/drivers/mux/Makefile
> @@ -7,10 +7,12 @@ mux-core-objs			:= core.o
>  mux-adg792a-objs		:= adg792a.o
>  mux-adgs1408-objs		:= adgs1408.o
>  mux-gpio-objs			:= gpio.o
> +mux-lan966-objs			:= lan966-flx.o
>  mux-mmio-objs			:= mmio.o
>  
>  obj-$(CONFIG_MULTIPLEXER)	+= mux-core.o
>  obj-$(CONFIG_MUX_ADG792A)	+= mux-adg792a.o
>  obj-$(CONFIG_MUX_ADGS1408)	+= mux-adgs1408.o
>  obj-$(CONFIG_MUX_GPIO)		+= mux-gpio.o
> +obj-$(CONFIG_MUX_LAN966)	+= mux-lan966.o
>  obj-$(CONFIG_MUX_MMIO)		+= mux-mmio.o
> diff --git a/drivers/mux/lan966-flx.c b/drivers/mux/lan966-flx.c
> new file mode 100644
> index 000000000000..2c7dab616a6a
> --- /dev/null
> +++ b/drivers/mux/lan966-flx.c
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LAN966 Flexcom MUX driver
> + *
> + * Copyright (c) 2022 Microchip Inc.
> + *
> + * Author: Kavyasree Kotagiri <kavyasree.kotagiri@...rochip.com>

Looks like it is based on mmio.c?

> + */
> +
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/mux/driver.h>
> +#include <linux/io.h>
> +
> +#define FLEX_SHRD_MASK		0x1FFFFF
> +#define LAN966_MAX_CS		21
> +
> +static void __iomem *flx_shared_base;

I would much prefer to store the combined address (base+offset)
in the mux private data instead of only storing the offset and
then unnecessarily rely on some piece of global state (that
will be clobbered by other instances).

> +struct mux_lan966x {

Why is the file named lan966, but then everything inside lan966x?

> +	u32 offset;
> +	u32 ss_pin;
> +};
> +
> +static int mux_lan966x_set(struct mux_control *mux, int state)
> +{
> +	struct mux_lan966x *mux_lan966x = mux_chip_priv(mux->chip);
> +	u32 val;
> +
> +	val = ~(1 << mux_lan966x[state].ss_pin) & FLEX_SHRD_MASK;
> +	writel(val, flx_shared_base + mux_lan966x[state].offset);

This reads memory you have not allocated, if you select a state
other than zero.

> +
> +	return 0;
> +}
> +
> +static const struct mux_control_ops mux_lan966x_ops = {
> +	.set = mux_lan966x_set,
> +};
> +
> +static const struct of_device_id mux_lan966x_dt_ids[] = {
> +	{ .compatible = "microchip,lan966-flx-mux", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mux_lan966x_dt_ids);
> +
> +static int mux_lan966x_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	struct mux_lan966x *mux_lan966x;
> +	struct mux_chip *mux_chip;
> +	int ret, num_fields, i;
> +
> +	ret = of_property_count_u32_elems(np, "mux-offset-pin");
> +	if (ret == 0 || ret % 2)
> +		ret = -EINVAL;
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +				     "mux-offset-pin property missing or invalid");
> +	num_fields = ret / 2;
> +
> +	mux_chip = devm_mux_chip_alloc(dev, num_fields, sizeof(*mux_lan966x));

I might be thoroughly mistaken and confused by the code, but it seems
very strange that a subsequenct select is not undoing what a previous
select once did. Each state seems to write to its own register offset,
and there is nothing that restores the first register offset with you
switch states.

Care to explain how muxing works and what you are expected to do to
manipulate the mux? Is there some link to public documentation? I did
a quick search for lan966 but came up with nothing relevant.

> +	if (IS_ERR(mux_chip))
> +		return dev_err_probe(dev, PTR_ERR(mux_chip),
> +				     "failed to allocate mux_chips\n");
> +
> +	mux_lan966x = mux_chip_priv(mux_chip);
> +
> +	flx_shared_base = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
> +	if (IS_ERR(flx_shared_base))
> +		return dev_err_probe(dev, PTR_ERR(flx_shared_base),
> +				     "failed to get flexcom shared base address\n");
> +
> +	for (i = 0; i < num_fields; i++) {
> +		struct mux_control *mux = &mux_chip->mux[i];
> +		u32 offset, shared_pin;
> +
> +		ret = of_property_read_u32_index(np, "mux-offset-pin",
> +						 2 * i, &offset);
> +		if (ret == 0)
> +			ret = of_property_read_u32_index(np, "mux-offset-pin",
> +							 2 * i + 1,
> +							 &shared_pin);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "failed to read mux-offset-pin property: %d", i);
> +
> +		if (shared_pin >= LAN966_MAX_CS)
> +			return -EINVAL;
> +
> +		mux_lan966x[i].offset = offset;
> +		mux_lan966x[i].ss_pin = shared_pin;

This clobbers memory you have not allocated, if num_fields >= 1.

Cheers,
Peter

> +
> +		mux->states = LAN966_MAX_CS;
> +	}
> +
> +	mux_chip->ops = &mux_lan966x_ops;
> +
> +	ret = devm_mux_chip_register(dev, mux_chip);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static struct platform_driver mux_lan966x_driver = {
> +	.driver = {
> +		.name = "lan966-mux",
> +		.of_match_table	= of_match_ptr(mux_lan966x_dt_ids),
> +	},
> +	.probe = mux_lan966x_probe,
> +};
> +
> +module_platform_driver(mux_lan966x_driver);
> +
> +MODULE_DESCRIPTION("LAN966 Flexcom multiplexer driver");
> +MODULE_AUTHOR("Kavyasree Kotagiri <kavyasree.kotagiri@...rochip.com>");
> +MODULE_LICENSE("GPL v2");
> +

Powered by blists - more mailing lists