Re: [RESEND 1/4] mfd: altera-sysmgr: Add SOCFPGA System Manager abstraction
From: Thor Thayer
Date: Mon Dec 17 2018 - 18:21:20 EST
Hi Arnd,
On 12/14/18 6:36 AM, Arnd Bergmann wrote:
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.
:) Thank you for the review and comments!
+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.
Thank you. I'm not completely clear on this. Are you saying this
function should test for a new compatible that is assigned to Stratix10
in the binding ("altr,sys-mgr-s10") instead of the machine name?
+
+/**
+ * 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?
Yes. I will change.
+ * 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", ®_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.
Yes. I will change.
+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.
Yes. It is more complex but I was concerned about re-implementing large
parts of syscon for the ARM32 case.
However, re-implementing it will simplify the driver and keep both ARM32
and ARM64 together. Thanks for the suggestion - I will change it.
+ 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.
OK. I will change this.
+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
Yes. I will make these changes.
Thanks so much for the review!