Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 28 Sep 2022 00:04:11 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Colin Foster <colin.foster@in-advantage.com>
Cc:     linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, netdev@vger.kernel.org,
        Russell King <linux@armlinux.org.uk>,
        Linus Walleij <linus.walleij@linaro.org>,
        UNGLinuxDriver@microchip.com,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Claudiu Manoil <claudiu.manoil@nxp.com>,
        Lee Jones <lee@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        "David S. Miller" <davem@davemloft.net>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>
Subject: Re: [PATCH v3 net-next 11/14] mfd: ocelot: add regmaps for ocelot_ext
Message-ID: <20220927210411.6oc3aphlyp4imgsq@skbuf>
References: <20220926002928.2744638-1-colin.foster@in-advantage.com>
 <20220926002928.2744638-12-colin.foster@in-advantage.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220926002928.2744638-12-colin.foster@in-advantage.com>
Precedence: bulk

On Sun, Sep 25, 2022 at 05:29:25PM -0700, Colin Foster wrote:
> The Ocelot switch core driver relies heavily on a fixed array of resources
> for both ports and peripherals. This is in contrast to existing peripherals
> - pinctrl for example - which have a one-to-one mapping of driver <>
> resource. As such, these regmaps must be created differently so that
> enumeration-based offsets are preserved.
> 
> Register the regmaps to the core MFD device unconditionally so they can be
> referenced by the Ocelot switch / Felix DSA systems.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
> 
> v3
>     * No change
> 
> v2
>     * Alignment of variables broken out to a separate patch
>     * Structs now correctly use EXPORT_SYMBOL*
>     * Logic moved and comments added to clear up conditionals around
>       vsc7512_target_io_res[i].start
> 
> v1 from previous RFC:
>     * New patch
> 
> ---
>  drivers/mfd/ocelot-core.c  | 87 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/ocelot.h |  5 +++
>  2 files changed, 92 insertions(+)
> 
> diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> index 013e83173062..702555fbdcc5 100644
> --- a/drivers/mfd/ocelot-core.c
> +++ b/drivers/mfd/ocelot-core.c
> @@ -45,6 +45,45 @@
>  #define VSC7512_SIO_CTRL_RES_START	0x710700f8
>  #define VSC7512_SIO_CTRL_RES_SIZE	0x00000100
>  
> +#define VSC7512_HSIO_RES_START		0x710d0000
> +#define VSC7512_HSIO_RES_SIZE		0x00000128

I don't think you should give the HSIO resource to the switching driver.
In drivers/net/ethernet/mscc/ocelot_vsc7514.c, there is this comment:

static void ocelot_pll5_init(struct ocelot *ocelot)
{
	/* Configure PLL5. This will need a proper CCF driver
	 * The values are coming from the VTSS API for Ocelot
	 */

I believe CCF stands for Common Clock Framework.

> +
> +#define VSC7512_ANA_RES_START		0x71880000
> +#define VSC7512_ANA_RES_SIZE		0x00010000
> +
> +#define VSC7512_QS_RES_START		0x71080000
> +#define VSC7512_QS_RES_SIZE		0x00000100
> +
> +#define VSC7512_QSYS_RES_START		0x71800000
> +#define VSC7512_QSYS_RES_SIZE		0x00200000
> +
> +#define VSC7512_REW_RES_START		0x71030000
> +#define VSC7512_REW_RES_SIZE		0x00010000
> +
> +#define VSC7512_SYS_RES_START		0x71010000
> +#define VSC7512_SYS_RES_SIZE		0x00010000
> +
> +#define VSC7512_S0_RES_START		0x71040000
> +#define VSC7512_S1_RES_START		0x71050000
> +#define VSC7512_S2_RES_START		0x71060000
> +#define VSC7512_S_RES_SIZE		0x00000400

VCAP_RES_SIZE?

> +
> +#define VSC7512_GCB_RES_START		0x71070000
> +#define VSC7512_GCB_RES_SIZE		0x0000022c

Again, I don't think devcpu_gcb should be given to a switching-only
driver. There's nothing switching-related about it.

> +#define VSC7512_PORT_0_RES_START	0x711e0000
> +#define VSC7512_PORT_1_RES_START	0x711f0000
> +#define VSC7512_PORT_2_RES_START	0x71200000
> +#define VSC7512_PORT_3_RES_START	0x71210000
> +#define VSC7512_PORT_4_RES_START	0x71220000
> +#define VSC7512_PORT_5_RES_START	0x71230000
> +#define VSC7512_PORT_6_RES_START	0x71240000
> +#define VSC7512_PORT_7_RES_START	0x71250000
> +#define VSC7512_PORT_8_RES_START	0x71260000
> +#define VSC7512_PORT_9_RES_START	0x71270000
> +#define VSC7512_PORT_10_RES_START	0x71280000
> +#define VSC7512_PORT_RES_SIZE		0x00010000
> +
>  #define VSC7512_GCB_RST_SLEEP_US	100
>  #define VSC7512_GCB_RST_TIMEOUT_US	100000
>  
> @@ -96,6 +135,36 @@ static const struct resource vsc7512_sgpio_resources[] = {
>  	DEFINE_RES_REG_NAMED(VSC7512_SIO_CTRL_RES_START, VSC7512_SIO_CTRL_RES_SIZE, "gcb_sio"),
>  };
>  
> +const struct resource vsc7512_target_io_res[TARGET_MAX] = {
> +	[ANA] = DEFINE_RES_REG_NAMED(VSC7512_ANA_RES_START, VSC7512_ANA_RES_SIZE, "ana"),
> +	[QS] = DEFINE_RES_REG_NAMED(VSC7512_QS_RES_START, VSC7512_QS_RES_SIZE, "qs"),
> +	[QSYS] = DEFINE_RES_REG_NAMED(VSC7512_QSYS_RES_START, VSC7512_QSYS_RES_SIZE, "qsys"),
> +	[REW] = DEFINE_RES_REG_NAMED(VSC7512_REW_RES_START, VSC7512_REW_RES_SIZE, "rew"),
> +	[SYS] = DEFINE_RES_REG_NAMED(VSC7512_SYS_RES_START, VSC7512_SYS_RES_SIZE, "sys"),
> +	[S0] = DEFINE_RES_REG_NAMED(VSC7512_S0_RES_START, VSC7512_S_RES_SIZE, "s0"),
> +	[S1] = DEFINE_RES_REG_NAMED(VSC7512_S1_RES_START, VSC7512_S_RES_SIZE, "s1"),
> +	[S2] = DEFINE_RES_REG_NAMED(VSC7512_S2_RES_START, VSC7512_S_RES_SIZE, "s2"),
> +	[GCB] = DEFINE_RES_REG_NAMED(VSC7512_GCB_RES_START, VSC7512_GCB_RES_SIZE, "devcpu_gcb"),
> +	[HSIO] = DEFINE_RES_REG_NAMED(VSC7512_HSIO_RES_START, VSC7512_HSIO_RES_SIZE, "hsio"),
> +};
> +EXPORT_SYMBOL_NS(vsc7512_target_io_res, MFD_OCELOT);
> +
> +const struct resource vsc7512_port_io_res[] = {

I hope you will merge these 2 arrays now.

> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_0_RES_START, VSC7512_PORT_RES_SIZE, "port0"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_1_RES_START, VSC7512_PORT_RES_SIZE, "port1"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_2_RES_START, VSC7512_PORT_RES_SIZE, "port2"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_3_RES_START, VSC7512_PORT_RES_SIZE, "port3"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_4_RES_START, VSC7512_PORT_RES_SIZE, "port4"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_5_RES_START, VSC7512_PORT_RES_SIZE, "port5"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_6_RES_START, VSC7512_PORT_RES_SIZE, "port6"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_7_RES_START, VSC7512_PORT_RES_SIZE, "port7"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_8_RES_START, VSC7512_PORT_RES_SIZE, "port8"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_9_RES_START, VSC7512_PORT_RES_SIZE, "port9"),
> +	DEFINE_RES_REG_NAMED(VSC7512_PORT_10_RES_START, VSC7512_PORT_RES_SIZE, "port10"),
> +	{}
> +};
> +EXPORT_SYMBOL_NS(vsc7512_port_io_res, MFD_OCELOT);
> +
>  static const struct mfd_cell vsc7512_devs[] = {
>  	{
>  		.name = "ocelot-pinctrl",
> @@ -144,6 +213,7 @@ static void ocelot_core_try_add_regmaps(struct device *dev,
>  
>  int ocelot_core_init(struct device *dev)
>  {
> +	const struct resource *port_res;
>  	int i, ndevs;
>  
>  	ndevs = ARRAY_SIZE(vsc7512_devs);
> @@ -151,6 +221,23 @@ int ocelot_core_init(struct device *dev)
>  	for (i = 0; i < ndevs; i++)
>  		ocelot_core_try_add_regmaps(dev, &vsc7512_devs[i]);
>  
> +	/*
> +	 * Both the target_io_res and the port_io_res structs need to be referenced directly by
> +	 * the ocelot_ext driver, so they can't be attached to the dev directly and referenced by
> +	 * offset like the rest of the drivers. Instead, create these regmaps always and allow any
> +	 * children look these up by name.
> +	 */
> +	for (i = 0; i < TARGET_MAX; i++)
> +		/*
> +		 * The target_io_res array is sparsely populated. Use .start as an indication that
> +		 * the entry isn't defined
> +		 */
> +		if (vsc7512_target_io_res[i].start)
> +			ocelot_core_try_add_regmap(dev, &vsc7512_target_io_res[i]);
> +
> +	for (port_res = vsc7512_port_io_res; port_res->start; port_res++)
> +		ocelot_core_try_add_regmap(dev, port_res);
> +

Will need to be updated.

>  	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, vsc7512_devs, ndevs, NULL, 0, NULL);
>  }
>  EXPORT_SYMBOL_NS(ocelot_core_init, MFD_OCELOT);
> diff --git a/include/linux/mfd/ocelot.h b/include/linux/mfd/ocelot.h
> index dd72073d2d4f..439ff5256cf0 100644
> --- a/include/linux/mfd/ocelot.h
> +++ b/include/linux/mfd/ocelot.h
> @@ -11,8 +11,13 @@
>  #include <linux/regmap.h>
>  #include <linux/types.h>
>  
> +#include <soc/mscc/ocelot.h>
> +

Is this the problematic include that makes it necessary to have the
pinctrl hack? Can we drop the #undef REG now?

>  struct resource;
>  
> +extern const struct resource vsc7512_target_io_res[TARGET_MAX];
> +extern const struct resource vsc7512_port_io_res[];
> +

Will need to be removed.

>  static inline struct regmap *
>  ocelot_regmap_from_resource_optional(struct platform_device *pdev,
>  				     unsigned int index,
> -- 
> 2.25.1
>