Message-ID: <59288dbf-bea2-a12e-93d2-c7c95f5568f6@axentia.se>
Date:   Tue, 10 May 2022 21:38:05 +0200
From:   Peter Rosin <peda@...ntia.se>
To:     Kavyasree.Kotagiri@...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, krzysztof.kozlowski+dt@...aro.org,
        Nicolas.Ferre@...rochip.com, alexandre.belloni@...tlin.com,
        Claudiu.Beznea@...rochip.com
Subject: Re: [PATCH v2 4/4] mux: lan966: Add support for flexcom mux
 controller

Hi!

2022-05-10 at 16:59, Kavyasree.Kotagiri@...rochip.com 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.
>>
> 
> Yes, mux is kept selected until configuring register is done. Please see below log where
> I added debug prints just for understanding:
> # dmesg | grep KK
>  [    0.779827] KK: Func: atmel_flexcom_probe ***** [START flx muxing] ********
> [    0.779875] KK: Func: atmel_flexcom_probe i = 0 args[0] = 0
> [    0.779890] KK: Func: mux_control_select [Entered]
> [    0.779902] KK: Func: mux_lan966x_set [Entered] state = 0
> [    0.779977] KK: Func: mux_lan966x_set [Read] = 0x1fffef   <<<----- setting mux_lan966x[state].ss_pin "4" which is passed from dts
> [    0.779992] KK: Func: mux_lan966x_set [Exit]
> [    0.780002] KK: Func: mux_control_select [Exit]
> [    0.780011] KK: Func: mux_control_deselect [Entered]
> [    0.780021] KK: Func: mux_control_deselect [Exit]

You misunderstand. The mux control is only "selected" between the call 
to mux_control_select() and the following call to 
mux_control_deselect().

After that, the mux control is "idle". However, in your case the 
"idle-state" is configured to "as-is" (which is the default), so the 
mux control (naturally) remains in the previously selected state while 
idle. But you are not documenting that limitation, and with this 
implementation you *must* have a mux control that uses "as-is" as its 
idle state. But that is an unexpected and broken limitation, and a 
much better solution is to simply keep the mux control "selected" for 
the complete duration when you rely on it to be in whatever state you 
need it to be in.

>>> +                     } 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?
>>
> Yes, I took mmio.c  driver as reference.
> 

So, then the above copyright and authorship info is not complete.

>>> + */
>>> +
>>> +#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).
>>
> Ok. I will try to see if this is relevant and change accordingly.
> 
>>> +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.
>>
> Ok. I will return error condition in case of trying to access none existing entry.
>>> +
>>> +     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.
>>
> LAN966 has 5 flexcoms(which can be used as USART/SPI/I2C interface).
> FLEXCOM has two chip-select I/O lines namely CS0 and CS1
> in SPI mode, CTS and RTS in USART mode. These FLEXCOM pins are optional.
> These chip-selects can be mapped to flexcom shared pin [0-21] which can be
> done by configuring flexcom multiplexer register(FLEXCOM_SHARED[0-4]:SS_MASK[0-1])
> Driver explanation:
> "flx_shared_base" is used to get base address of Flexcom shared multiplexer
> "mux-offset-pin" property is used to get cs0/cs1 offset and flexcom shared pin[0-21] of a flexcom.
> state value passed from atmel-flexcom is used to configure respective 
> FLEXCOM_SHARED[0-4]:SS_MASK[0-1] register with offset and flexcom shared pin.

Ok, let me try to interpret that. You wish enable a "fan out" of these
two chip-selects for any of the 5 flexcoms (in SPI mode?) such that
these 10 internal chip-selects can be connected to any of 21 external
pins?

If that's correct and if you wish to interface with e.g. 20 chips,
then it would be possible to have 20 states for one mux control and
then reach the 20 chips using only CS0 from FLEXCOM0, or it would be
possible to have 2 states for 10 mux controls, one each for CS0/CS1 of
all five flexcoms and also reach 20 chips. Or any wild combo you
imagine is useful.

Is that correctly understood?

Assuming so, then you can have a maximum of 10 mux controls, and for
each mux control you need a variable number of legitimate states. Each
mux control would also need to know at what address/offset its SS_MASK
register is at and what pins/states are valid.

But your code does not implemnent the above. You allocate num_fields
mux controls, which is what confuses the hell out of me. num_fields is
the number of states, not the number of mux controls! And you also
need to specify an individual offset for each state, and that makes no
sense at all, at least not to me.

So, instead, I think you want to have:

struct mux_lan966x {
	u32 ss_mask;
	int num_pins;
	u32 ss_pin[];
};

And then do:

	mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_lan966x) + num_pins * sizeof(u32));

(or however that size is best spelled, I haven't kept up)

The set operation would be along the lines of:

static int mux_lan966x_set(struct mux_control *mux, int state)
{
	struct mux_lan966x *mux_lan966x = mux_chip_priv(mux->chip);
	u32 val;

	if (state < 0 || state >= mux_lan966x->num_pins)
		return -1;

	val = ~(1 << mux_lan966x->ss_pin[state]) & FLEX_SHRD_MASK;
	writel(val, flx_shared_base + mux_lan966x->ss_mask);

	return 0;
}

Because, one mux control should only ever need to know about one offset,
as it should only ever write to its own SS_MASK register. And you will
need some private space such that you can keep track of which states
are legit.

I would also separate out the SS_MASK offset from the mux-offset-pin
property and have one property for that value and then a straight
array for the valid pin numbers in another property (no longer named
mux-offset-pin of course).

Or something like that and assuming I understand how the FLEXCOMs work
and what you want to do etc.

Cheers,
Peter


>>> +     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