lkml.org 
[lkml]   [2018]   [Dec]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RESEND 1/4] mfd: altera-sysmgr: Add SOCFPGA System Manager abstraction
From
Date
Hi Arnd,

On 12/14/18 6:36 AM, Arnd Bergmann wrote:
> On Tue, Nov 13, 2018 at 5:03 PM <thor.thayer@linux.intel.com> wrote:
>>
>> From: Thor Thayer <thor.thayer@linux.intel.com>
>>
>> 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@linux.intel.com>
>> ---
>> 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", &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.
>
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!

\
 
 \ /
  Last update: 2018-12-18 00:22    [W:0.069 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site