| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2204003.IdqJV6LE2e@phil>
Date: Tue, 12 May 2015 22:22:56 +0200
From: Heiko Stuebner <heiko@...ech.de>
To: Michael Niewöhner
<mniewoeh@...d.hs-offenburg.de>
Cc: lgirdwood@...il.com, broonie@...nel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH 1/2] regulator: act8865: add restart handler for act8846
Hi Michael,
Am Montag, 11. Mai 2015, 22:54:11 schrieb Michael Niewöhner:
> Rebooting radxa rock which uses act8846 results in kernel panic because the
> bootloader doesn't readjust frequency or voltage for cpu correctly. This
> workaround power cycles the act8846 at restart to reset the board.
Hmm, it might be nice to reword this, as this being a "workaround" for the
radxarock is a bit "short sighted".
The act8846 simply provides means to reset the system when used as main pmic
[hence the reliance on the system-power-controller] and other socs/boards
using it as pmic might also profit from exposing this function - for example if
there is no other means of reset.
> Signed-off-by: Michael Niewoehner <mniewoeh@...d.hs-offenburg.de>
> ---
> drivers/regulator/act8865-regulator.c | 43
> +++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)
>
> diff --git a/drivers/regulator/act8865-regulator.c
> b/drivers/regulator/act8865-regulator.c index 2ff73d7..f8cdff3 100644
> --- a/drivers/regulator/act8865-regulator.c
> +++ b/drivers/regulator/act8865-regulator.c
> @@ -27,6 +27,7 @@
> #include <linux/of_device.h>
> #include <linux/regulator/of_regulator.h>
> #include <linux/regmap.h>
> +#include <linux/reboot.h>
>
> /*
> * ACT8600 Global Register Map.
> @@ -133,10 +134,14 @@
> #define ACT8865_VOLTAGE_NUM 64
> #define ACT8600_SUDCDC_VOLTAGE_NUM 255
>
> +#define ACT8846_SIPC_MASK 0x01
> +
> struct act8865 {
> struct regmap *regmap;
> int off_reg;
> int off_mask;
> + int sipc_reg;
> + int sipc_mask;
> };
>
> static const struct regmap_config act8865_regmap_config = {
> @@ -402,6 +407,17 @@ static void act8865_power_off(void)
> while (1);
> }
>
> +static int act8846_power_cycle(struct notifier_block *this,
> + unsigned long code, void *unused)
> +{
> + struct act8865 *act8846;
> +
> + act8846 = i2c_get_clientdata(act8865_i2c_client);
> + regmap_write(act8846->regmap, act8846->sipc_reg, act8846->sipc_mask);
The act8846 is currently the only one of the three types providing such
functionality, so until the second chip variant surfaces that provides a reset
capability, it might be less intrusive to simply do
regmap_write(act8846->regmap, ACT8846_GLB_OFF_CTRL, ACT8846_SIPC_MASK);
here and skip all the infrastructure like sipc_reg and sipc_mask assignment
and handling? But I guess that is Mark's call what he likes better.
> +
> + return NOTIFY_DONE;
> +}
> +
structs like the restart_handler normally live near the function they
reference and not as static part of some function. And as the restart
functionality is not rockchip-specific, it should also not be named
rockchip_foo as below ;-)
static struct notifier_block act8846_restart_handler = {
.notifier_call = act8846_power_cycle,
.priority = 129,
};
> static int act8865_pmic_probe(struct i2c_client *client,
> const struct i2c_device_id *i2c_id)
> {
> @@ -413,6 +429,7 @@ static int act8865_pmic_probe(struct i2c_client *client,
> struct act8865 *act8865;
> unsigned long type;
> int off_reg, off_mask;
> + int sipc_reg, sipc_mask;
>
> pdata = dev_get_platdata(dev);
>
> @@ -434,18 +451,24 @@ static int act8865_pmic_probe(struct i2c_client
> *client, num_regulators = ARRAY_SIZE(act8600_regulators);
> off_reg = -1;
> off_mask = -1;
> + sipc_reg = -1;
> + sipc_mask = -1;
> break;
> case ACT8846:
> regulators = act8846_regulators;
> num_regulators = ARRAY_SIZE(act8846_regulators);
> off_reg = ACT8846_GLB_OFF_CTRL;
> off_mask = ACT8846_OFF_SYSMASK;
> + sipc_reg = ACT8846_GLB_OFF_CTRL;
> + sipc_mask = ACT8846_SIPC_MASK;
> break;
> case ACT8865:
> regulators = act8865_regulators;
> num_regulators = ARRAY_SIZE(act8865_regulators);
> off_reg = ACT8865_SYS_CTRL;
> off_mask = ACT8865_MSTROFF;
> + sipc_reg = -1;
> + sipc_mask = -1;
> break;
> default:
> dev_err(dev, "invalid device id %lu\n", type);
> @@ -494,6 +517,26 @@ static int act8865_pmic_probe(struct i2c_client
> *client, }
> }
no need to add a second check for of_device_is_system_power_controller() as we
already have a check for this property directly above, simply extend it with
something like the following.
if (of_device_is_system_power_controller(dev->of_node)) {
int ret;
/* already existing code to set poweroff handler */
if (type == ACT8846) {
ret = register_restart_handler(&act8846_restart_handler);
if (ret)
pr_err("%s: cannot register restart handler, %d\n",
__func__, ret);
}
}
>
> + /* Register restart handler */
> + if (of_device_is_system_power_controller(dev->of_node)
> + && sipc_reg > 0) {
> + static struct notifier_block rockchip_restart_handler = {
> + .notifier_call = act8846_power_cycle,
> + .priority = 129,
> + };
> +
> + int ret;
> +
> + act8865_i2c_client = client;
> + act8865->sipc_reg = sipc_reg;
> + act8865->sipc_mask = sipc_mask;
> +
> + ret = register_restart_handler(&rockchip_restart_handler);
> + if (ret)
> + pr_err("%s: cannot register restart handler, %d\n",
> + __func__, ret);
> + }
> +
> /* Finally register devices */
> for (i = 0; i < num_regulators; i++) {
> const struct regulator_desc *desc = ®ulators[i];
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists