Hi Lee,

Thank you for all your feedback. I expect to create another RFC in the
next week or two with all the changes you suggest.

On Mon, Jan 10, 2022 at 12:16:59PM +0000, Lee Jones wrote:

On Wed, 29 Dec 2021, Colin Foster wrote:

quoted

On Wed, Dec 29, 2021 at 03:22:24PM +0000, Lee Jones wrote:

[...]

quoted

quoted

quoted

+	tristate "Microsemi Ocelot External Control Support"
+	select MFD_CORE
+	help
+	  Say yes here to add support for Ocelot chips (VSC7511, VSC7512,
+	  VSC7513, VSC7514) controlled externally.

Please describe the device in more detail here.

I'm not sure what an "External Control Support" is.

A second paragraph "All four of these chips can be controlled internally
(MMIO) or externally via SPI, I2C, PCIe. This enables control of these
chips over one or more of these buses"

Where?  Or do you mean that you'll add one?

I meant I added one. Sorry that wasn't clear.

quoted

quoted

Please remove the term 'mfd\|MFD' from everywhere.

"ocelot_init" conflicts with a symbol in
drivers/net/ethernet/mscc/ocelot.o, otherwise I belive I got them all
now.

Then rename the other one.  Or call this one 'core', or something.

I'll add "core" or something to this one.

quoted

quoted

quoted

+struct ocelot_mfd_core {
+	struct ocelot_mfd_config *config;
+	struct regmap *gcb_regmap;
+	struct regmap_field *gcb_regfields[GCB_REGFIELD_MAX];
+};

Not sure about this at all.

Which driver did you take your inspiration from?

Mainly drivers/net/dsa/ocelot/* and drivers/net/ethernet/mscc/*.

I doubt you need it.  Please try to remove it.

Fair point. I'll remove it here.

quoted

quoted

quoted

+static const struct resource vsc7512_gcb_resource = {
+	.start	= 0x71070000,
+	.end	= 0x7107022b,

No magic numbers please.

I've gotten conflicting feedback on this. Several of the ocelot drivers
(drivers/net/dsa/ocelot/felix_vsc9959.c) have these ranges hard-coded.
Others (Documentation/devicetree/bindings/net/mscc-ocelot.txt) have them
all passed in through the device tree. 

https://lore.kernel.org/netdev/20211126213225.okrskqm26lgprxrk@skbuf/ (local)

Ref or quote?

I'm not brain grepping it searching for what you might be referring to.

I'm not sure what you're trying to say here.  I'm asking you to define
this numbers please.

I'll define the numbers as you suggest.


The quote I was referring to is this:

The last option I haven't put much consideration toward would be to
move some of the decision making to the device tree. The main ocelot
driver appears to leave a lot of these addresses out. For instance
Documentation/devicetree/bindings/pinctrl/mscc,ocelot-pinctrl.txt.
That added DT complexity could remove needs for lines like this:

quoted

quoted

+              ocelot->map[GCB][GCB_MIIM_MII_STATUS & REG_MASK],

But that would probably impose DT changes on Seville and Felix, which
is the last thing I want to do.

The thing with putting the targets in the device tree is that you're
inflicting yourself unnecessary pain. Take a look at
Documentation/devicetree/bindings/net/mscc-ocelot.txt, and notice that
they mark the "ptp" target as optional because it wasn't needed when
they first published the device tree, and now they need to maintain
compatibility with those old blobs.

quoted

quoted

quoted

+	.name	= "devcpu_gcb",

What is a 'devcpu_gcb'?

It matches the datasheet of the CPU's general configuation block.

Please could you quote that part for me?

Hmm... I'm not sure exactly what you mean by this.

https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10489.pdf

There are 64 instances of "DEVCPU_GCB" in the main datasheet. 
Chapter 6 of this PDF has an attached PDF around the phrase "Information
about the registers for this product is available in the attached file"

Section 2.3 of that attached PDF is dedicated entirely to DEVCPU_GCB 
registers. Table 1 defines that register block starting at 0x71070000.
The entry from that table is
| DEVCPU_GCB | 0x71070000 | General configuration block. | Page 63 |
This document has 175 references to the phrase "DEVCPU_GCB".

quoted

quoted

quoted

+	ret = regmap_field_write(core->gcb_regfields[GCB_SOFT_RST_CHIP_RST], 1);

No magic numbers please.  I have no idea what this is doing.

I'm not sure how much more verbose it can be... I suppose a define for
"RESET" and "CLEAR" instead of "1" and "0" on that bit. Maybe I'm just
blinded by having stared at this code for the last several months.

Yes please.  '1' could mean anything.

'CLEAR' is just as opaque.

What actually happens when you clear that register bit?

Agreed. I'll fix this.

quoted

quoted

quoted

+	if (ret)
+		return ret;
+
+	/*
+	 * Note: This is adapted from the PCIe reset strategy. The manual doesn't
+	 * suggest how to do a reset over SPI, and the register strategy isn't
+	 * possible.
+	 */
+	msleep(100);
+
+	ret = core->config->init_bus(core->config);

You're not writing a bus.  I doubt you need ops call-backs.

In the case of SPI, the chip needs to be configured both before and
after reset. It sets up the bus for endianness, padding bytes, etc. This
is currently achieved by running "ocelot_spi_init_bus" once during SPI
probe, and once immediately after chip reset.

For other control mediums I doubt this is necessary. Perhaps "init_bus"
is a misnomer in this scenario...

Please find a clearer way to do this without function pointers.

Understood.

quoted

quoted

quoted

+void ocelot_mfd_get_resource_name(char *name, const struct resource *res,
+				  int size)
+{
+	if (res->name)
+		snprintf(name, size - 1, "ocelot_mfd-%s", res->name);
+	else
+		snprintf(name, size - 1, "ocelot_mfd@0x%08x", res->start);
+}
+EXPORT_SYMBOL(ocelot_mfd_get_resource_name);

What is this used for?

You should not be hand rolling device resource names like this.

This sort of code belongs in the bus/class API.

Please use those instead.

The idea here was to allow shared regmaps across different devices. The
"devcpu_gcb" might be used in two ways - either everyone shares the same
regmap across the "GCB" range, or everyone creates their own. 

This was more useful when the ocelot-core.c had a copy of the 
"devcpu_org" regmap that was shared with ocelot-spi.c. I was able to
remove that, but also feel like the full switch driver (patch 6 of this
set) ocelot-ext should use the same "devcpu_gcb" regmap instance as
ocelot-core does.

Admittedly, there are complications. There should probably be more
checks added to "ocelot_regmap_init" / "get_regmap" to ensure that the
regmap for ocelot_ext exactly matches the existing regmap for
ocelot_core.

There's yet another complexity with these, and I'm not sure what the
answer is. Currently all regmaps are tied to the ocelot_spi device...
ocelot_spi calls devm_regmap_init. So those regmaps hang around if
they're created by a module that has been removed. At least until the
entire MFD module is removed. Maybe there's something I haven't seen yet
where the devres or similar has a reference count. I don't know the best
path forward on this one.

Why are you worrying about creating them 2 different ways?

If it's possible for them to all create and use their own regmaps,
what's preventing you from just do that all the time?

There isn't really any worry, there just might be efficiencies to be
had if two children share the same regmap. But so long as any regmap is
created with unique names, there's no reason multiple regmaps can't
overlap the same regions. In those cases, maybe syscon would be the best
thing to implement if it becomes needed.

I have nothing against making every child regmap be unique if that's the
desire.

quoted

quoted

quoted

+	/* Create and loop over all child devices here */

These need to all go in now please.

I'll squash them, as I saw you suggested in your other responses. I
tried to keep them separate, especially since adding ocelot_ext to this
commit (which has no functionality until this one) makes it quite a
large single commit. That's why I went the path I did, which was to roll
them in one at a time.

This is not an MFD until they are present.

Sounds good. I'll squash before the next RFC.

quoted

quoted

quoted

+int ocelot_mfd_remove(struct ocelot_mfd_config *config)
+{
+	/* Loop over all children and remove them */

Use devm_* then you won't have to.

Yeah, I was more worried than I needed to be when I wrote that comment.
The only thing called to clean everything up is mfd_remove_devices();

Use devm_mfd_add_devices(), then you don't have to.

[...]

Ooh. Thanks!

quoted

quoted

quoted

+#include <linux/regmap.h>
+
+struct ocelot_mfd_config {
+	struct device *dev;
+	struct regmap *(*get_regmap)(struct ocelot_mfd_config *config,
+				     const struct resource *res,
+				     const char *name);
+	int (*init_bus)(struct ocelot_mfd_config *config);

Please re-work and delete this 'config' concept.

See other drivers in this sub-directory for reference.

Do you have a specific example? I had focused on madera for no specific
reason. But I really dislike the idea of throwing all of the structure
definition for the MFD inside of something like
"include/linux/mfd/ocelot/core.h", especially since all the child
drivers (madera-pinctrl, madera-gpio, etc) heavily rely on this struct. 

It seemed to me that without the concept of
"mfd_get_regmap_from_resource" this sort of back-and-forth was actually
necessary.

Things like regmaps are usually passed in via driver_data or
platform_data.  Almost anything is better than call-backs.

[...]

I'll work to clean this up for the next RFC.

quoted

quoted

quoted

+	if (!ocelot_spi)
+		return -ENOMEM;
+
+	if (spi->max_speed_hz <= 500000) {
+		ocelot_spi->spi_padding_bytes = 0;
+	} else {
+		/*
+		 * Calculation taken from the manual for IF_CFGSTAT:IF_CFG. Err
+		 * on the side of more padding bytes, as having too few can be
+		 * difficult to detect at runtime.
+		 */
+		ocelot_spi->spi_padding_bytes = 1 +
+			(spi->max_speed_hz / 1000000 + 2) / 8;

Please explain what this means or define the values (or both).

I can certainly elaborate the comment. Searching the manual for the term
"if_cfgstat" will take you right to the equation, and a description of
what padding bytes are, etc. 

You shouldn't insist for your readers to RTFM.

If the code doesn't read well or is overly complicated, change it.

If the complexity is required, document it in comments.

Understood. I'll elaborate.

quoted

quoted

quoted

+	ocelot_spi->spi = spi;

Why are you saving this?

This file keeps the regmap_{read,write} implementations, so is needed
for spi_sync() for any regmap. There might be a better way to infer
this... but it seemed pretty nice to have each regmap only carry along
an instance of "ocelot_spi_regmap_context."

I still need Mark to look at your Regmap implementation.

Thank you. And again, I appreciate all the feedback.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog