Re: [RFC v1 net-next 7/8] mfd: ocelot: add regmaps for ocelot_ext

[Vladimir Oltean]

On Mon, Sep 12, 2022 at 12:04:45PM -0700, Colin Foster wrote:
> That is exactly right. The ocelot_ext version of init_regmap() now uses
> dev_get_regmap() which only cares about the name and essentially drops
> the rest of the information. Previous versions hooked into the
> ocelot-core / ocelot-spi MFD system to request that a new regmap be
> created (with start and end being honored.) A benefit of this design is
> that any regmaps that are named the same are automatically shared. A
> drawback (or maybe a benefit?) is that the users have no control over
> ranges / flags.
> 
> I think if this goes the way of index-based that'll work. I'm happy to
> revert my previous change (sorry it snuck in) but it seems like there'll
> still have to be some trickery... For reference:
> 
> enum ocelot_target {
> 	ANA = 1,
> 	QS,
> 	QSYS,
> 	REW,
> 	SYS,
> 	S0,
> 	S1,
> 	S2,
> 	HSIO,
> 	PTP,
> 	FDMA,
> 	GCB,
> 	DEV_GMII,
> 	TARGET_MAX,
> };
> 
> mfd_add_devices will probably need to add a zero-size resource for HSIO,
> PTP, FDMA, and anything else that might come along in the future. At
> this point, regmap_from_mfd(PTP) might have to look like (pseudocode):
> 
> struct regmap *ocelot_ext_regmap_from_mfd(struct ocelot *ocelot, int index)
> {
>     return ocelot_regmap_from_resource_optional(pdev, index-1, config);
> 
>     /* Note this essentially expands to:
>      * res = platform_get_resource(pdev, IORESOURCE_REG, index-1);
>      * return dev_get_regmap(pdev->dev.parent, res->name);
>      *
>      * This essentially throws away everything that my current
>      * implementation does, except for the IORESOURCE_REG flag
>      */
> }
> 
> Then drivers/net/dsa/felix.c felix_init_structs() would have two loops
> (again, just as an example)
> 
> for (i = ANA; i < TARGET_MAX; i++) {
>     if (felix->info->regmap_from_mfd)
>         target = felix->info->regmap_from_mfd(ocelot, i);
>     else {
>         /* existing logic back to ocelot_regmap_init() */
>     }
> }
> 
> for (port = 0; port < num_phys_ports; port++) {
>     ...
>     if (felix->info->regmap_from_mfd)
>         target = felix->info->regmap_from_mfd(ocelot, TARGET_MAX + port);
>     else {
>         /* existing logic back to ocelot_regmap_init() */
>     }
> }
> 
> And lastly, ocelot_core_init() in drivers/mfd/ocelot-core.c would need a
> mechanism to say "don't add a regmap for cell->resources[PTP], even
> though that resource exists" because... well I suppose it is only in
> drivers/net/ethernet/mscc/ocelot_vsc7514.c for now, but the existance of
> those regmaps invokes different behavior. For instance:
> 
> 	if (ocelot->targets[FDMA])
> 		ocelot_fdma_init(pdev, ocelot);
> 
> I'm not sure whether this last point will have an effect on felix.c in
> the end. My current patch set of adding the SERDES ports uses the
> existance of targets[HSIO] to invoke ocelot_pll5_init() similar to the
> ocelot_vsc7514.c FDMA / PTP conditionals, but I'd understand if that
> gets rejected outright. That's for another day.
> 
> 
> 
> I'm happy to make these changes if you see them valid. I saw the fact
> that dev_get_regmap(dev->parent, res->name) could be used directly in
> ocelot_ext_regmap_init() as an elegant solution to felix / ocelot-lib /
> ocelot-core, but I recognize that the subtle "throw away the
> IORESOURCE_MEM flag and res->{start,end} information" isn't ideal.

Thinking some more about it, there will have to be even more trickery.
Say you solve the problem for the global targets, but then what do you
do for the port targets, how do you reference those by index?
TARGET_MAX + port? Hmm, that isn't great either.

What if we meet half way, and you just get the resources from the
platform device by name, instead of by index? You'd have to modify the
regmap creation procedure to look at a predefined array of strings,
containing names of all targets that are mandatory (a la mscc_ocelot_probe),
and match those
(a) iteration over target_io_res and strcmp(), in the case of vsc9959
    and vsc9953
(b) dev_get_regmap() in the case of ocelot_ext

This way there's still no direct communication between ocelot-mfd and
DSA, and I have the feeling that the problems we both mention are
solved. Hope I'm not missing something.