linux-kernel - Re: [RFC PATCH 12/21] reset: uniphier: add core support for UniPhier reset driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK7LNASpxbW81A7YGQuAqmBdBty4BC6Z8sko8YepXOPnVqSjAQ@mail.gmail.com>
Date:	Wed, 11 May 2016 11:46:10 +0900
From:	Masahiro Yamada <yamada.masahiro@...ionext.com>
To:	Philipp Zabel <p.zabel@...gutronix.de>
Cc:	linux-clk@...r.kernel.org, Arnd Bergmann <arnd@...db.de>,
	Guenter Roeck <linux@...ck-us.net>,
	Kalle Valo <kvalo@...eaurora.org>,
	Jiri Slaby <jslaby@...e.com>,
	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [RFC PATCH 12/21] reset: uniphier: add core support for UniPhier
 reset driver

Hi Philipp,


2016-05-10 22:54 GMT+09:00 Philipp Zabel <p.zabel@...gutronix.de>:
> Am Dienstag, den 10.05.2016, 18:50 +0900 schrieb Masahiro Yamada:
> [...]
>> +static int uniphier_reset_update(struct reset_controller_dev *rcdev,
>> +                              unsigned long id, bool assert)
>> +{
>> +     struct uniphier_reset_priv *priv = to_uniphier_reset_priv(rcdev);
>> +     const struct uniphier_reset_data *p;
>> +     bool handled = false;
>> +
>> +     for (p = priv->data; p->id != UNIPHIER_RESET_ID_END; p++) {
>> +             unsigned int val;
>> +             int ret;
>> +
>> +             if (p->id != id)
>> +                     continue;
>> +
>> +             val = p->deassert_val;
>> +             if (assert)
>> +                     val = ~val;
>> +
>> +             ret = regmap_write_bits(priv->regmap, p->reg, p->mask, val);
>
> What is the difference between mask and deassert_val? Couldn't you just
> assign
>         val = assert ? 0 : p->mask;
> ?


I need to handle both active-high resets and active-low resets.

I thought two ways to do that.


[1] Have mask and a flag indicating active-low/active-high,
    like follows:

     if (flag & UNIPHIER_RST_ACTIVE_LOW)
          assert = !assert;
     val = assert ? 0 : p->mask;

[2] Have mask and deassert_val as in this patch




[1] cannot manage a case where one register contains
active-low bits and active-high bits mixed in it.



For example, let's say reset bits are BIT(1) and BIT(0).

[2] can solve this case as follows:

(a) If both bit1 and bit0 are active-high.
     .mask = BIT(1) | BIT(0);
     .deassert_val = 0;

(b) If bit1 is active-high and bit0 is active-low
     .mask = BIT(1) | BIT(0);
     .deassert_val =  BIT(0);

(c) If bit1 is active-low and bit0 is active-high
     .mask = BIT(1) | BIT(0);
     .deassert_val =  BIT(1);

(d) If both bit1 and bit0 are active-low
     .mask = BIT(1) | BIT(0);
     .deassert_val = BIT(1) | BIT(0);


I have not been hit by such a complicated case though.




>> +static const struct reset_control_ops uniphier_reset_ops = {
>> +     .assert = uniphier_reset_assert,
>> +     .deassert = uniphier_reset_deassert,
>> +     .status = uniphier_reset_status,
>> +};
>> +
>> +int uniphier_reset_probe(struct platform_device *pdev,
>> +                      const struct uniphier_reset_data *data)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     struct uniphier_reset_priv *priv;
>> +     const struct uniphier_reset_data *p;
>> +     struct regmap *regmap;
>> +     unsigned int nr_resets = 0;
>> +
>> +     /* parent should be MFD and syscon node */
>> +     regmap = syscon_node_to_regmap(dev->parent->of_node);
>> +     if (IS_ERR(regmap)) {
>> +             dev_err(dev, "failed to get regmap\n");
>
> syscon_node_to_regmap can return different error codes. It might be
> helpful to use
>         dev_err(dev, "failed to get regmap: %d\n", PTR_ERR(regmap));
> here.


OK. Will do.


-- 
Best Regards
Masahiro Yamada