Re: [RESEND 1/4] mfd: altera-sysmgr: Add SOCFPGA System Manager abstraction

[Arnd Bergmann]

On Tue, Nov 13, 2018 at 5:03 PM <thor.thayer@xxxxxxxxxxxxxxx> wrote:
>
> From: Thor Thayer <thor.thayer@xxxxxxxxxxxxxxx>
>
> The SOCFPGA System Manager register block aggregate different
> peripheral functions into one place.
> On 32 bit ARM parts, the syscon framework fits this problem well.
> On 64 bit ARM parts, the System Manager can only be accessed by
> EL3 secure mode. Since a SMC call to EL3 is required, a new
> driver using regmaps similar to syscon was created that handles
> the SMC call.
> Since regmaps abstract out the underlying register access, the
> changes to drivers using System Manager are minimal.
>
> Signed-off-by: Thor Thayer <thor.thayer@xxxxxxxxxxxxxxx>
> ---
> Resend - update use_single_rw to use_single_read and
>          use_single_write which was added in 4.20.

Sorry for stepping in late here, I forgot to review it earlier and
Lee had to remind me to take a look.

> +static const struct regmap_config s10_sysmgr_regmap_cfg = {
> +       .name = "s10_sysmgr",
> +       .reg_bits = 32,
> +       .reg_stride = 4,
> +       .val_bits = 32,
> +       .reg_read = s10_protected_reg_read,
> +       .reg_write = s10_protected_reg_write,
> +       .fast_io = true,
> +       .use_single_read = true,
> +       .use_single_write = true,
> +};

The new regmap seems fine to me, that looks like a good way
of abstracting the two hardware methods.

> +/**
> + * socfpga_is_s10
> + * Determine if running on Stratix10 platform.
> + * Return: True if running Stratix10, otherwise false.
> + */
> +static int socfpga_is_s10(void)
> +{
> +       return of_machine_is_compatible("altr,socfpga-stratix10");
> +}

I don't really like the way you are checking for a specific here
here though, that is something that should only be done in
an absolute emergency when there is no way of fixing the
device tree files.

Since this is a new driver for a device that is not used in
mainline kernels yet (AFAICT), let's fix the binding and add
a proper detection method here.

> +
> +/**
> + * of_sysmgr_register
> + * Create and register the Altera System Manager regmap.
> + * Return: Pointer to new sysmgr on success.
> + *         Pointer error on failure.
> + */
> +static struct altr_sysmgr *of_sysmgr_register(struct device_node *np)
> +{
> +       struct altr_sysmgr *sysmgr;
> +       struct regmap *regmap;
> +       u32 reg_io_width;
> +       int ret;
> +       struct regmap_config sysmgr_config = s10_sysmgr_regmap_cfg;
> +       struct resource res;
> +
> +       if (!of_device_is_compatible(np, "altr,sys-mgr"))
> +               return ERR_PTR(-EINVAL);
> +
> +       sysmgr = kzalloc(sizeof(*sysmgr), GFP_KERNEL);
> +       if (!sysmgr)
> +               return ERR_PTR(-ENOMEM);
> +
> +       if (of_address_to_resource(np, 0, &res)) {
> +               ret = -ENOMEM;
> +               goto err_map;
> +       }
> +
> +       /* Need physical address for SMCC call */
> +       sysmgr->base = (void __iomem *)res.start;

The cast here seems really ugly. Instead of mixinx up
address spaces, how about adding a resource_size_t
member in the sysmgr structure?

> +        * search for reg-io-width property in DT. If it is not provided,
> +        * default to 4 bytes. regmap_init will return an error if values
> +        * are invalid so there is no need to check them here.
> +        */
> +       ret = of_property_read_u32(np, "reg-io-width", &reg_io_width);
> +       if (ret)
> +               reg_io_width = 4;

How likely is it that this would ever not be four bytes? It looks
like you just copied this from syscon, but it really should not be
needed.

> +struct regmap *altr_sysmgr_node_to_regmap(struct device_node *np)
> +{
> +       struct altr_sysmgr *sysmgr = NULL;
> +
> +       if (!socfpga_is_s10())
> +               return syscon_node_to_regmap(np);

Why do you go through syscon here? Doesn't this add a lot of complexity?

I'd suggest using regmap_init_mmio() directly and open-coding the
initialization you need as you do for the s10 case.

> +       if (!p_sysmgr)
> +               sysmgr = of_sysmgr_register(np);
> +       else
> +               sysmgr = p_sysmgr;
> +
> +       if (IS_ERR_OR_NULL(sysmgr))
> +               return ERR_CAST(sysmgr);

Don't use IS_ERR_OR_NULL(), it's just a sign that your API
is bad. Instead, define the interface either so that you
always return NULL on error or that you always return an
PTR_ERR() value on error.

> +struct regmap *altr_sysmgr_regmap_lookup_by_compatible(const char *s)
> +{
> +       struct device_node *sysmgr_np;
> +       struct regmap *regmap;
> +
> +       if (!socfpga_is_s10())
> +               return syscon_regmap_lookup_by_compatible(s);
> +
> +       sysmgr_np = of_find_compatible_node(NULL, NULL, s);
> +       if (!sysmgr_np)
> +               return ERR_PTR(-ENODEV);
> +
> +       regmap = altr_sysmgr_node_to_regmap(sysmgr_np);
> +       of_node_put(sysmgr_np);
> +
> +       return regmap;
> +}
> +EXPORT_SYMBOL_GPL(altr_sysmgr_regmap_lookup_by_compatible);

That should not be needed, just look it up by phandle and be done
with it. Again, lookup by compatible should only be needed for
compatibility with old DTB files, but you should be able to fix the
binding so you always have a phandle to the correct node here,
at least for the s10 case.

For the older chips with existing DTs, I guess drivers can fall back to
the syscon method directly.

> +EXPORT_SYMBOL_GPL(altr_sysmgr_regmap_lookup_by_pdevname);

Same comment.

        Arnd