Message-ID: <YzOA35MxZ6S6E6qe@colin-ia-desktop>
Date:   Tue, 27 Sep 2022 16:01:51 -0700
From:   Colin Foster <colin.foster@...advantage.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, netdev@...r.kernel.org,
        Russell King <linux@...linux.org.uk>,
        Linus Walleij <linus.walleij@...aro.org>,
        UNGLinuxDriver@...rochip.com,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Lee Jones <lee@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH v3 net-next 11/14] mfd: ocelot: add regmaps for ocelot_ext

On Wed, Sep 28, 2022 at 12:04:11AM +0300, Vladimir Oltean wrote:
> 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@...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.

It does stand for common clock framework. And this function / comment
keeps me up at night.

Agreed that the HSIO resource isn't currently used, and should be
dropped from this set. The resource will be brought back in as soon as I
add in phy-ocelot-serdes support during the third and final (?) patch
set of adding 7512 copper ethernet support.

My fear is that the lines:

if (ocelot->targets[HSIO])
    ocelot_pll5_init(ocelot);

won't fly inside felix_setup(), and I'm not sure how far that scope will
creep. Maybe it is easier than I suspect.


But I'm getting ahead of myself. I'll remove this for now.

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

I'll change this name to 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.

Yes, this is no longer necessary and I missed this. I think you caught
them all, but I'll do another sweep just in case.

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

Yep. And with that I should be able to add them via the standard
.num_resources, .resources method all the other drivers use. As
mentioned, without the GCB and HSIO entries.

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

Yep. I think it can all go away for the above
ocelot_core_try_add_regmaps() call now.

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

Yes, this include was specifically for TARGET_MAX below. That undef REG
should not be necessary anymore. I'll drop it.

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

Gladly :-)

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

Powered by blists - more mailing lists