Message ID | 20210723135239.388325-3-clement.leger@bootlin.com |
---|---|
State | New |
Headers | show |
Series | add SMC based regmap driver for secure syscon access | expand |
On Fri, 23 Jul 2021, Clément Léger wrote: > System controllers can be placed under secure monitor control when running > under them. In order to keep existing code which accesses such system > controllers using a syscon, add support for "syscon-smc" compatible. > > When enable, the syscon will handle this new compatible and look for an > "arm,smc-id" property to execute the appropriate SMC. A SMC regmap is then > created to forward register access to the secure monitor. > > Signed-off-by: Clément Léger <clement.leger@bootlin.com> > --- > drivers/mfd/syscon.c | 170 ++++++++++++++++++++++++++++++++++++------- > 1 file changed, 145 insertions(+), 25 deletions(-) I'm going to let Arnd have at this, but just a couple of points. > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c > index 765c0210cb52..eb727b146315 100644 > --- a/drivers/mfd/syscon.c > +++ b/drivers/mfd/syscon.c > @@ -40,7 +40,15 @@ static const struct regmap_config syscon_regmap_config = { > .reg_stride = 4, > }; > > -static struct syscon *of_syscon_register(struct device_node *np, bool check_clk) > +static void syscon_add(struct syscon *syscon) > +{ > + spin_lock(&syscon_list_slock); > + list_add_tail(&syscon->list, &syscon_list); > + spin_unlock(&syscon_list_slock); > +} > + > +static struct syscon *of_syscon_register_mmio(struct device_node *np, > + bool check_clk) > { > struct clk *clk; > struct syscon *syscon; > @@ -132,10 +140,6 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk) > syscon->regmap = regmap; > syscon->np = np; > > - spin_lock(&syscon_list_slock); > - list_add_tail(&syscon->list, &syscon_list); > - spin_unlock(&syscon_list_slock); > - > return syscon; > > err_attach: > @@ -150,8 +154,49 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk) > return ERR_PTR(ret); > } > > +#ifdef CONFIG_REGMAP_SMCCC > +static struct syscon *of_syscon_register_smccc(struct device_node *np) > +{ > + struct syscon *syscon; > + struct regmap *regmap; > + u32 reg_io_width = 4, smc_id; > + int ret; > + struct regmap_config syscon_config = syscon_regmap_config; > + > + ret = of_property_read_u32(np, "arm,smc-id", &smc_id); So this is Arm specific. Not sure we want to be creating bespoke support for specific architectures/platforms in the generic syscon implementation. > + if (ret) > + return ERR_PTR(-ENODEV); > + > + syscon = kzalloc(sizeof(*syscon), GFP_KERNEL); > + if (!syscon) > + return ERR_PTR(-ENOMEM); > + > + of_property_read_u32(np, "reg-io-width", ®_io_width); > + > + syscon_config.name = kasprintf(GFP_KERNEL, "%pOFn@smc%x", np, smc_id); > + syscon_config.val_bits = reg_io_width * 8; > + > + regmap = regmap_init_smccc(NULL, smc_id, &syscon_config); > + if (IS_ERR(regmap)) { > + ret = PTR_ERR(regmap); > + goto err_regmap; > + } > + > + syscon->regmap = regmap; > + syscon->np = np; > + > + return syscon; > + > +err_regmap: > + kfree(syscon_config.name); > + kfree(syscon); > + > + return ERR_PTR(ret); > +} > +#endif > + > static struct regmap *device_node_get_regmap(struct device_node *np, > - bool check_clk) > + bool check_clk, bool use_smccc) > { > struct syscon *entry, *syscon = NULL; > > @@ -165,8 +210,19 @@ static struct regmap *device_node_get_regmap(struct device_node *np, > > spin_unlock(&syscon_list_slock); > > - if (!syscon) > - syscon = of_syscon_register(np, check_clk); > + if (!syscon) { > + if (use_smccc) > +#ifdef CONFIG_REGMAP_SMCCC > + syscon = of_syscon_register_smccc(np); > +#else > + syscon = NULL; > +#endif ... and we certainly don't want to be doing so using #ifdefery. Please find a better way to support this feature. > + else > + syscon = of_syscon_register_mmio(np, check_clk); > + > + if (!IS_ERR(syscon)) > + syscon_add(syscon); > + } > > if (IS_ERR(syscon)) > return ERR_CAST(syscon); > @@ -176,16 +232,19 @@ static struct regmap *device_node_get_regmap(struct device_node *np, > > struct regmap *device_node_to_regmap(struct device_node *np) > { > - return device_node_get_regmap(np, false); > + return device_node_get_regmap(np, false, false); > } > EXPORT_SYMBOL_GPL(device_node_to_regmap); > > struct regmap *syscon_node_to_regmap(struct device_node *np) > { > - if (!of_device_is_compatible(np, "syscon")) > - return ERR_PTR(-EINVAL); > + if (of_device_is_compatible(np, "syscon")) > + return device_node_get_regmap(np, true, false); > + > + if (of_device_is_compatible(np, "syscon-smc")) > + return device_node_get_regmap(np, true, true); > > - return device_node_get_regmap(np, true); > + return ERR_PTR(-EINVAL); > } > EXPORT_SYMBOL_GPL(syscon_node_to_regmap); > > @@ -273,19 +332,19 @@ struct regmap *syscon_regmap_lookup_by_phandle_optional(struct device_node *np, > } > EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle_optional); > > -static int syscon_probe(struct platform_device *pdev) > +struct syscon_driver_data { > + int (*probe_func)(struct platform_device *pdev, struct device *dev, > + struct syscon *syscon); > +}; > + > +static int syscon_probe_mmio(struct platform_device *pdev, > + struct device *dev, > + struct syscon *syscon) > { > - struct device *dev = &pdev->dev; > - struct syscon_platform_data *pdata = dev_get_platdata(dev); > - struct syscon *syscon; > struct regmap_config syscon_config = syscon_regmap_config; > struct resource *res; > void __iomem *base; > > - syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL); > - if (!syscon) > - return -ENOMEM; > - > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (!res) > return -ENOENT; > @@ -295,23 +354,84 @@ static int syscon_probe(struct platform_device *pdev) > return -ENOMEM; > > syscon_config.max_register = resource_size(res) - 4; > - if (pdata) > - syscon_config.name = pdata->label; > + > syscon->regmap = devm_regmap_init_mmio(dev, base, &syscon_config); > if (IS_ERR(syscon->regmap)) { > dev_err(dev, "regmap init failed\n"); > return PTR_ERR(syscon->regmap); > } > > - platform_set_drvdata(pdev, syscon); > + dev_dbg(dev, "regmap_mmio %pR registered\n", res); > + > + return 0; > +} > + > +static const struct syscon_driver_data syscon_mmio_data = { > + .probe_func = &syscon_probe_mmio, > +}; > + > +#ifdef CONFIG_REGMAP_SMCCC > + > +static int syscon_probe_smc(struct platform_device *pdev, > + struct device *dev, > + struct syscon *syscon) > +{ > + struct regmap_config syscon_config = syscon_regmap_config; > + int smc_id, ret; > + > + ret = of_property_read_u32(dev->of_node, "arm,smc-id", &smc_id); > + if (!ret) > + return -ENODEV; > + > + syscon->regmap = devm_regmap_init_smccc(dev, smc_id, &syscon_config); > + if (IS_ERR(syscon->regmap)) { > + dev_err(dev, "regmap init failed\n"); > + return PTR_ERR(syscon->regmap); > + } > > - dev_dbg(dev, "regmap %pR registered\n", res); > + dev_dbg(dev, "regmap_smccc %x registered\n", smc_id); > + > + return 0; > +} > + > +static const struct syscon_driver_data syscon_smc_data = { > + .probe_func = &syscon_probe_smc, > +}; > +#endif > + > +static int syscon_probe(struct platform_device *pdev) > +{ > + int ret; > + struct device *dev = &pdev->dev; > + struct syscon_platform_data *pdata = dev_get_platdata(dev); > + struct regmap_config syscon_config = syscon_regmap_config; > + struct syscon *syscon; > + const struct syscon_driver_data *driver_data; > + > + if (pdata) > + syscon_config.name = pdata->label; > + > + syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL); > + if (!syscon) > + return -ENOMEM; > + > + driver_data = (const struct syscon_driver_data *) > + platform_get_device_id(pdev)->driver_data; > + > + ret = driver_data->probe_func(pdev, dev, syscon); > + if (ret) > + return ret; > + > + platform_set_drvdata(pdev, syscon); > > return 0; > } > > static const struct platform_device_id syscon_ids[] = { > - { "syscon", }, > + { .name = "syscon", .driver_data = (kernel_ulong_t)&syscon_mmio_data}, > +#ifdef CONFIG_REGMAP_SMCCC > + { .name = "syscon-smc", .driver_data = (kernel_ulong_t)&syscon_smc_data}, > +#endif > { } > }; >
Le Fri, 23 Jul 2021 16:27:59 +0100, Lee Jones <lee.jones@linaro.org> a écrit : > On Fri, 23 Jul 2021, Clément Léger wrote: > > > System controllers can be placed under secure monitor control when > > running under them. In order to keep existing code which accesses > > such system controllers using a syscon, add support for > > "syscon-smc" compatible. > > > > When enable, the syscon will handle this new compatible and look > > for an "arm,smc-id" property to execute the appropriate SMC. A SMC > > regmap is then created to forward register access to the secure > > monitor. > > > > Signed-off-by: Clément Léger <clement.leger@bootlin.com> > > --- > > drivers/mfd/syscon.c | 170 > > ++++++++++++++++++++++++++++++++++++------- 1 file changed, 145 > > insertions(+), 25 deletions(-) > > I'm going to let Arnd have at this, but just a couple of points. > > > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c > > index 765c0210cb52..eb727b146315 100644 > > --- a/drivers/mfd/syscon.c > > +++ b/drivers/mfd/syscon.c > > @@ -40,7 +40,15 @@ static const struct regmap_config > > syscon_regmap_config = { .reg_stride = 4, > > }; > > > > -static struct syscon *of_syscon_register(struct device_node *np, > > bool check_clk) +static void syscon_add(struct syscon *syscon) > > +{ > > + spin_lock(&syscon_list_slock); > > + list_add_tail(&syscon->list, &syscon_list); > > + spin_unlock(&syscon_list_slock); > > +} > > + > > +static struct syscon *of_syscon_register_mmio(struct device_node > > *np, > > + bool check_clk) > > { > > struct clk *clk; > > struct syscon *syscon; > > @@ -132,10 +140,6 @@ static struct syscon > > *of_syscon_register(struct device_node *np, bool check_clk) > > syscon->regmap = regmap; syscon->np = np; > > > > - spin_lock(&syscon_list_slock); > > - list_add_tail(&syscon->list, &syscon_list); > > - spin_unlock(&syscon_list_slock); > > - > > return syscon; > > > > err_attach: > > @@ -150,8 +154,49 @@ static struct syscon > > *of_syscon_register(struct device_node *np, bool check_clk) return > > ERR_PTR(ret); } > > > > +#ifdef CONFIG_REGMAP_SMCCC > > +static struct syscon *of_syscon_register_smccc(struct device_node > > *np) +{ > > + struct syscon *syscon; > > + struct regmap *regmap; > > + u32 reg_io_width = 4, smc_id; > > + int ret; > > + struct regmap_config syscon_config = syscon_regmap_config; > > + > > + ret = of_property_read_u32(np, "arm,smc-id", &smc_id); > > So this is Arm specific. > > Not sure we want to be creating bespoke support for specific > architectures/platforms in the generic syscon implementation. Agreed, I wanted to keep an existing property name but having such thing in the syscon is indeed a bad idea. > > > + if (ret) > > + return ERR_PTR(-ENODEV); > > + > > + syscon = kzalloc(sizeof(*syscon), GFP_KERNEL); > > + if (!syscon) > > + return ERR_PTR(-ENOMEM); > > + > > + of_property_read_u32(np, "reg-io-width", ®_io_width); > > + > > + syscon_config.name = kasprintf(GFP_KERNEL, "%pOFn@smc%x", > > np, smc_id); > > + syscon_config.val_bits = reg_io_width * 8; > > + > > + regmap = regmap_init_smccc(NULL, smc_id, &syscon_config); > > + if (IS_ERR(regmap)) { > > + ret = PTR_ERR(regmap); > > + goto err_regmap; > > + } > > + > > + syscon->regmap = regmap; > > + syscon->np = np; > > + > > + return syscon; > > + > > +err_regmap: > > + kfree(syscon_config.name); > > + kfree(syscon); > > + > > + return ERR_PTR(ret); > > +} > > +#endif > > + > > static struct regmap *device_node_get_regmap(struct device_node > > *np, > > - bool check_clk) > > + bool check_clk, bool > > use_smccc) { > > struct syscon *entry, *syscon = NULL; > > > > @@ -165,8 +210,19 @@ static struct regmap > > *device_node_get_regmap(struct device_node *np, > > spin_unlock(&syscon_list_slock); > > > > - if (!syscon) > > - syscon = of_syscon_register(np, check_clk); > > + if (!syscon) { > > + if (use_smccc) > > +#ifdef CONFIG_REGMAP_SMCCC > > + syscon = of_syscon_register_smccc(np); > > +#else > > + syscon = NULL; > > +#endif > > ... and we certainly don't want to be doing so using #ifdefery. > > Please find a better way to support this feature. Agreed too, best solution would probably be to allow having multiple syscon "backends" split in multiple files which would create the correct regmap according to the devicetree. Clément > > > + else > > + syscon = of_syscon_register_mmio(np, > > check_clk); + > > + if (!IS_ERR(syscon)) > > + syscon_add(syscon); > > + } > > > > if (IS_ERR(syscon)) > > return ERR_CAST(syscon); > > @@ -176,16 +232,19 @@ static struct regmap > > *device_node_get_regmap(struct device_node *np, > > struct regmap *device_node_to_regmap(struct device_node *np) > > { > > - return device_node_get_regmap(np, false); > > + return device_node_get_regmap(np, false, false); > > } > > EXPORT_SYMBOL_GPL(device_node_to_regmap); > > > > struct regmap *syscon_node_to_regmap(struct device_node *np) > > { > > - if (!of_device_is_compatible(np, "syscon")) > > - return ERR_PTR(-EINVAL); > > + if (of_device_is_compatible(np, "syscon")) > > + return device_node_get_regmap(np, true, false); > > + > > + if (of_device_is_compatible(np, "syscon-smc")) > > + return device_node_get_regmap(np, true, true); > > > > - return device_node_get_regmap(np, true); > > + return ERR_PTR(-EINVAL); > > } > > EXPORT_SYMBOL_GPL(syscon_node_to_regmap); > > > > @@ -273,19 +332,19 @@ struct regmap > > *syscon_regmap_lookup_by_phandle_optional(struct device_node *np, } > > EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle_optional); > > > > -static int syscon_probe(struct platform_device *pdev) > > +struct syscon_driver_data { > > + int (*probe_func)(struct platform_device *pdev, struct > > device *dev, > > + struct syscon *syscon); > > +}; > > + > > +static int syscon_probe_mmio(struct platform_device *pdev, > > + struct device *dev, > > + struct syscon *syscon) > > { > > - struct device *dev = &pdev->dev; > > - struct syscon_platform_data *pdata = dev_get_platdata(dev); > > - struct syscon *syscon; > > struct regmap_config syscon_config = syscon_regmap_config; > > struct resource *res; > > void __iomem *base; > > > > - syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL); > > - if (!syscon) > > - return -ENOMEM; > > - > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > if (!res) > > return -ENOENT; > > @@ -295,23 +354,84 @@ static int syscon_probe(struct > > platform_device *pdev) return -ENOMEM; > > > > syscon_config.max_register = resource_size(res) - 4; > > - if (pdata) > > - syscon_config.name = pdata->label; > > + > > syscon->regmap = devm_regmap_init_mmio(dev, base, > > &syscon_config); if (IS_ERR(syscon->regmap)) { > > dev_err(dev, "regmap init failed\n"); > > return PTR_ERR(syscon->regmap); > > } > > > > - platform_set_drvdata(pdev, syscon); > > + dev_dbg(dev, "regmap_mmio %pR registered\n", res); > > + > > + return 0; > > +} > > + > > +static const struct syscon_driver_data syscon_mmio_data = { > > + .probe_func = &syscon_probe_mmio, > > +}; > > + > > +#ifdef CONFIG_REGMAP_SMCCC > > + > > +static int syscon_probe_smc(struct platform_device *pdev, > > + struct device *dev, > > + struct syscon *syscon) > > +{ > > + struct regmap_config syscon_config = syscon_regmap_config; > > + int smc_id, ret; > > + > > + ret = of_property_read_u32(dev->of_node, "arm,smc-id", > > &smc_id); > > + if (!ret) > > + return -ENODEV; > > + > > + syscon->regmap = devm_regmap_init_smccc(dev, smc_id, > > &syscon_config); > > + if (IS_ERR(syscon->regmap)) { > > + dev_err(dev, "regmap init failed\n"); > > + return PTR_ERR(syscon->regmap); > > + } > > > > - dev_dbg(dev, "regmap %pR registered\n", res); > > + dev_dbg(dev, "regmap_smccc %x registered\n", smc_id); > > + > > + return 0; > > +} > > + > > +static const struct syscon_driver_data syscon_smc_data = { > > + .probe_func = &syscon_probe_smc, > > +}; > > +#endif > > + > > +static int syscon_probe(struct platform_device *pdev) > > +{ > > + int ret; > > + struct device *dev = &pdev->dev; > > + struct syscon_platform_data *pdata = dev_get_platdata(dev); > > + struct regmap_config syscon_config = syscon_regmap_config; > > + struct syscon *syscon; > > + const struct syscon_driver_data *driver_data; > > + > > + if (pdata) > > + syscon_config.name = pdata->label; > > + > > + syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL); > > + if (!syscon) > > + return -ENOMEM; > > + > > + driver_data = (const struct syscon_driver_data *) > > + > > platform_get_device_id(pdev)->driver_data; + > > + ret = driver_data->probe_func(pdev, dev, syscon); > > + if (ret) > > + return ret; > > + > > + platform_set_drvdata(pdev, syscon); > > > > return 0; > > } > > > > static const struct platform_device_id syscon_ids[] = { > > - { "syscon", }, > > + { .name = "syscon", .driver_data = > > (kernel_ulong_t)&syscon_mmio_data}, +#ifdef CONFIG_REGMAP_SMCCC > > + { .name = "syscon-smc", .driver_data = > > (kernel_ulong_t)&syscon_smc_data}, +#endif > > { } > > }; > > >
On Fri, Jul 23, 2021 at 3:52 PM Clément Léger <clement.leger@bootlin.com> wrote: > > System controllers can be placed under secure monitor control when running > under them. In order to keep existing code which accesses such system > controllers using a syscon, add support for "syscon-smc" compatible. > > When enable, the syscon will handle this new compatible and look for an > "arm,smc-id" property to execute the appropriate SMC. A SMC regmap is then > created to forward register access to the secure monitor. > > Signed-off-by: Clément Léger <clement.leger@bootlin.com> I don't see anything wrong with the implementation, but this worries me conceptually, because of the ways this might get abused: - this creates one more way to keep device drivers hidden away behind firmware when they should be in the kernel. You can already do that with separate SMC calls, but adding an indirection makes it sneakier. If the 'registers' in here are purely - This may be seen as an easy way out for firmware writers that just expose a bare register-level interface when the correct solution would be to create a high-level interface. There is also a problem with locking: In the case that both firmware and kernel have to access registers within a syscon area, you may need to have a semaphore to protect an atomic sequence of accesses, but since the interface only provides a single register load/store, there is no way for a kernel driver to serialize against a firmware-internal driver. Arnd
On Fri, Jul 23, 2021 at 06:07:44PM +0200, Arnd Bergmann wrote: > There is also a problem with locking: In the case that both firmware and > kernel have to access registers within a syscon area, you may need to > have a semaphore to protect an atomic sequence of accesses, but since > the interface only provides a single register load/store, there is no way for > a kernel driver to serialize against a firmware-internal driver. The standard solution to this for the read/modify/write case would be to expose an explicit update_bits() operation (some hardware does this for concurrency and/or bus bandwidth/latency reasons), though that doesn't help with larger or multi-register sequences (and to be clear as I've been saying I don't think we should do this at all).
Hi "Clément, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on regmap/for-next] [also build test WARNING on robh/for-next v5.14-rc2 next-20210723] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Cl-ment-L-ger/add-SMC-based-regmap-driver-for-secure-syscon-access/20210723-215708 base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next config: riscv-nommu_k210_defconfig (attached as .config) compiler: riscv64-linux-gcc (GCC) 10.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/5b8123662c54263f6fc86b6ef9e296739fe78353 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Cl-ment-L-ger/add-SMC-based-regmap-driver-for-secure-syscon-access/20210723-215708 git checkout 5b8123662c54263f6fc86b6ef9e296739fe78353 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=riscv If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): drivers/mfd/syscon.c: In function 'syscon_probe': >> drivers/mfd/syscon.c:407:23: warning: variable 'syscon_config' set but not used [-Wunused-but-set-variable] 407 | struct regmap_config syscon_config = syscon_regmap_config; | ^~~~~~~~~~~~~ vim +/syscon_config +407 drivers/mfd/syscon.c 401 402 static int syscon_probe(struct platform_device *pdev) 403 { 404 int ret; 405 struct device *dev = &pdev->dev; 406 struct syscon_platform_data *pdata = dev_get_platdata(dev); > 407 struct regmap_config syscon_config = syscon_regmap_config; 408 struct syscon *syscon; 409 const struct syscon_driver_data *driver_data; 410 411 if (pdata) 412 syscon_config.name = pdata->label; 413 414 syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL); 415 if (!syscon) 416 return -ENOMEM; 417 418 driver_data = (const struct syscon_driver_data *) 419 platform_get_device_id(pdev)->driver_data; 420 421 ret = driver_data->probe_func(pdev, dev, syscon); 422 if (ret) 423 return ret; 424 425 platform_set_drvdata(pdev, syscon); 426 427 return 0; 428 } 429 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> Subject: Re: [PATCH 2/3] syscon: add support for "syscon-smc" compatible > > On Fri, Jul 23, 2021 at 3:52 PM Clément Léger <clement.leger@bootlin.com> > wrote: > > > > System controllers can be placed under secure monitor control when > > running under them. In order to keep existing code which accesses such > > system controllers using a syscon, add support for "syscon-smc" compatible. > > > > When enable, the syscon will handle this new compatible and look for > > an "arm,smc-id" property to execute the appropriate SMC. A SMC regmap > > is then created to forward register access to the secure monitor. > > > > Signed-off-by: Clément Léger <clement.leger@bootlin.com> > > I don't see anything wrong with the implementation, I also vote for such an implementation. Such as we have a chip has a misc register space, part as below: 44h USB Wake-up Control Register (DGO 10) (USB_WAKEUP) 48h PTD Pads Compensation Cell Configuration Register 4Ch Lower CA35 TS Timer First Compare Value (TSTMR_CMP0_VAL_L) 50h Upper CA35 TS Timer First Compare Value 54h Lower CA35 TS Timer Second Compare value 58h Upper CA35 TS Timer Second Compare Value 5Ch CA35 Core0 Reset Vector Base Address (DGO 8) (RVBARADDR0) 60h CA35 Core1 Reset Vector Base Address (DGO 9) (RVBARADDR1) 64h Medium Quality Sound Configuration Register (MQS1_CF) 32 RW 0100_0000h It contains several functions, we need protect 5Ch, 60h to avoid Non-secure world modify it. Others could be directly used by Linux kernel. But we could only hide the whole register space in secure world to make 5C/60h register not touch by linux. We not find a good way to provide high-level interface for such a misc register space, provide register level interface would make it easy for various drivers to use. Thanks, Peng. but this worries me > conceptually, because of the ways this might get abused: > > - this creates one more way to keep device drivers hidden away > behind firmware when they should be in the kernel. You can already > do that with separate SMC calls, but adding an indirection makes it > sneakier. If the 'registers' in here are purely > > - This may be seen as an easy way out for firmware writers that just > expose a bare register-level interface when the correct solution would > be to create a high-level interface. > > There is also a problem with locking: In the case that both firmware and > kernel have to access registers within a syscon area, you may need to have a > semaphore to protect an atomic sequence of accesses, but since the interface > only provides a single register load/store, there is no way for a kernel driver to > serialize against a firmware-internal driver. > > Arnd
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c index 765c0210cb52..eb727b146315 100644 --- a/drivers/mfd/syscon.c +++ b/drivers/mfd/syscon.c @@ -40,7 +40,15 @@ static const struct regmap_config syscon_regmap_config = { .reg_stride = 4, }; -static struct syscon *of_syscon_register(struct device_node *np, bool check_clk) +static void syscon_add(struct syscon *syscon) +{ + spin_lock(&syscon_list_slock); + list_add_tail(&syscon->list, &syscon_list); + spin_unlock(&syscon_list_slock); +} + +static struct syscon *of_syscon_register_mmio(struct device_node *np, + bool check_clk) { struct clk *clk; struct syscon *syscon; @@ -132,10 +140,6 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk) syscon->regmap = regmap; syscon->np = np; - spin_lock(&syscon_list_slock); - list_add_tail(&syscon->list, &syscon_list); - spin_unlock(&syscon_list_slock); - return syscon; err_attach: @@ -150,8 +154,49 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk) return ERR_PTR(ret); } +#ifdef CONFIG_REGMAP_SMCCC +static struct syscon *of_syscon_register_smccc(struct device_node *np) +{ + struct syscon *syscon; + struct regmap *regmap; + u32 reg_io_width = 4, smc_id; + int ret; + struct regmap_config syscon_config = syscon_regmap_config; + + ret = of_property_read_u32(np, "arm,smc-id", &smc_id); + if (ret) + return ERR_PTR(-ENODEV); + + syscon = kzalloc(sizeof(*syscon), GFP_KERNEL); + if (!syscon) + return ERR_PTR(-ENOMEM); + + of_property_read_u32(np, "reg-io-width", ®_io_width); + + syscon_config.name = kasprintf(GFP_KERNEL, "%pOFn@smc%x", np, smc_id); + syscon_config.val_bits = reg_io_width * 8; + + regmap = regmap_init_smccc(NULL, smc_id, &syscon_config); + if (IS_ERR(regmap)) { + ret = PTR_ERR(regmap); + goto err_regmap; + } + + syscon->regmap = regmap; + syscon->np = np; + + return syscon; + +err_regmap: + kfree(syscon_config.name); + kfree(syscon); + + return ERR_PTR(ret); +} +#endif + static struct regmap *device_node_get_regmap(struct device_node *np, - bool check_clk) + bool check_clk, bool use_smccc) { struct syscon *entry, *syscon = NULL; @@ -165,8 +210,19 @@ static struct regmap *device_node_get_regmap(struct device_node *np, spin_unlock(&syscon_list_slock); - if (!syscon) - syscon = of_syscon_register(np, check_clk); + if (!syscon) { + if (use_smccc) +#ifdef CONFIG_REGMAP_SMCCC + syscon = of_syscon_register_smccc(np); +#else + syscon = NULL; +#endif + else + syscon = of_syscon_register_mmio(np, check_clk); + + if (!IS_ERR(syscon)) + syscon_add(syscon); + } if (IS_ERR(syscon)) return ERR_CAST(syscon); @@ -176,16 +232,19 @@ static struct regmap *device_node_get_regmap(struct device_node *np, struct regmap *device_node_to_regmap(struct device_node *np) { - return device_node_get_regmap(np, false); + return device_node_get_regmap(np, false, false); } EXPORT_SYMBOL_GPL(device_node_to_regmap); struct regmap *syscon_node_to_regmap(struct device_node *np) { - if (!of_device_is_compatible(np, "syscon")) - return ERR_PTR(-EINVAL); + if (of_device_is_compatible(np, "syscon")) + return device_node_get_regmap(np, true, false); + + if (of_device_is_compatible(np, "syscon-smc")) + return device_node_get_regmap(np, true, true); - return device_node_get_regmap(np, true); + return ERR_PTR(-EINVAL); } EXPORT_SYMBOL_GPL(syscon_node_to_regmap); @@ -273,19 +332,19 @@ struct regmap *syscon_regmap_lookup_by_phandle_optional(struct device_node *np, } EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle_optional); -static int syscon_probe(struct platform_device *pdev) +struct syscon_driver_data { + int (*probe_func)(struct platform_device *pdev, struct device *dev, + struct syscon *syscon); +}; + +static int syscon_probe_mmio(struct platform_device *pdev, + struct device *dev, + struct syscon *syscon) { - struct device *dev = &pdev->dev; - struct syscon_platform_data *pdata = dev_get_platdata(dev); - struct syscon *syscon; struct regmap_config syscon_config = syscon_regmap_config; struct resource *res; void __iomem *base; - syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL); - if (!syscon) - return -ENOMEM; - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!res) return -ENOENT; @@ -295,23 +354,84 @@ static int syscon_probe(struct platform_device *pdev) return -ENOMEM; syscon_config.max_register = resource_size(res) - 4; - if (pdata) - syscon_config.name = pdata->label; + syscon->regmap = devm_regmap_init_mmio(dev, base, &syscon_config); if (IS_ERR(syscon->regmap)) { dev_err(dev, "regmap init failed\n"); return PTR_ERR(syscon->regmap); } - platform_set_drvdata(pdev, syscon); + dev_dbg(dev, "regmap_mmio %pR registered\n", res); + + return 0; +} + +static const struct syscon_driver_data syscon_mmio_data = { + .probe_func = &syscon_probe_mmio, +}; + +#ifdef CONFIG_REGMAP_SMCCC + +static int syscon_probe_smc(struct platform_device *pdev, + struct device *dev, + struct syscon *syscon) +{ + struct regmap_config syscon_config = syscon_regmap_config; + int smc_id, ret; + + ret = of_property_read_u32(dev->of_node, "arm,smc-id", &smc_id); + if (!ret) + return -ENODEV; + + syscon->regmap = devm_regmap_init_smccc(dev, smc_id, &syscon_config); + if (IS_ERR(syscon->regmap)) { + dev_err(dev, "regmap init failed\n"); + return PTR_ERR(syscon->regmap); + } - dev_dbg(dev, "regmap %pR registered\n", res); + dev_dbg(dev, "regmap_smccc %x registered\n", smc_id); + + return 0; +} + +static const struct syscon_driver_data syscon_smc_data = { + .probe_func = &syscon_probe_smc, +}; +#endif + +static int syscon_probe(struct platform_device *pdev) +{ + int ret; + struct device *dev = &pdev->dev; + struct syscon_platform_data *pdata = dev_get_platdata(dev); + struct regmap_config syscon_config = syscon_regmap_config; + struct syscon *syscon; + const struct syscon_driver_data *driver_data; + + if (pdata) + syscon_config.name = pdata->label; + + syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL); + if (!syscon) + return -ENOMEM; + + driver_data = (const struct syscon_driver_data *) + platform_get_device_id(pdev)->driver_data; + + ret = driver_data->probe_func(pdev, dev, syscon); + if (ret) + return ret; + + platform_set_drvdata(pdev, syscon); return 0; } static const struct platform_device_id syscon_ids[] = { - { "syscon", }, + { .name = "syscon", .driver_data = (kernel_ulong_t)&syscon_mmio_data}, +#ifdef CONFIG_REGMAP_SMCCC + { .name = "syscon-smc", .driver_data = (kernel_ulong_t)&syscon_smc_data}, +#endif { } };
System controllers can be placed under secure monitor control when running under them. In order to keep existing code which accesses such system controllers using a syscon, add support for "syscon-smc" compatible. When enable, the syscon will handle this new compatible and look for an "arm,smc-id" property to execute the appropriate SMC. A SMC regmap is then created to forward register access to the secure monitor. Signed-off-by: Clément Léger <clement.leger@bootlin.com> --- drivers/mfd/syscon.c | 170 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 145 insertions(+), 25 deletions(-)