Re: [PATCH v2] reset: uniphier: add reset controller drivers for UniPhier SoCs

[Masahiro Yamada <yamada.masahiro@socionext.com>]

Hi Philipp,


2016-07-27 18:17 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:

>> +
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset-controller.h>
>> +
>> +#include "reset-uniphier.h"
>> +
>> +struct uniphier_reset_priv {
>> +     struct reset_controller_dev rcdev;
>> +     struct device *dev;
>> +     struct regmap *regmap;
>> +     const struct uniphier_reset_data *data;
>> +};
>> +
>> +#define to_uniphier_reset_priv(_rcdev) \
>> +                     container_of(_rcdev, struct uniphier_reset_priv, rcdev)
>> +
>> +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->assert_val;
>> +             if (!assert)
>> +                     val = ~val;
>> +
>> +             ret = regmap_write_bits(priv->regmap, p->reg, p->mask, val);
>> +             if (ret)
>> +                     return ret;
>> +
>> +             handled = true;
>
> Why does this continue to walk through the list after the correct id was
> found?

Looks like you already found the answer for this.


>> +#define UNIPHIER_MIO_RESET_USB2(index, ch)                           \
>> +     UNIPHIER_RESETX_SIMPLE((index), 0x110 + 0x200 * (ch), BIT(24)), \
>> +     UNIPHIER_RESETX_SIMPLE((index), 0x114 + 0x200 * (ch), BIT(0))
>
> Ah, so for USB2 reset you have two reset bits in separate registers. Are
> you sure these are controlling the same reset line?


I am not a hardware guy, so I am not sure about the hardware design.

>From my best guess, I think each bit controls a different block.
But both of them must be de-asserted before starting up USB.

There is no use-case where they are asserted/de-asserted independently.

So, I thought it made sense to couple them into a single ID.



> If the USB core does in fact have two separate reset inputs that just
> happen to need asserting at the same time, this should still get two
> separate ids. Same issue for the SD reset above, if the reset lines are
> physically separate, please don't combine them in the driver.

Right.
>From the view of point of Device Tree interface,
it should reflect the hardware design.
I believe they are separate reset signals, so should be given with separate IDs.

But, as a software engineer, it is sometimes difficult to fully understand
the hardware structure.

The hardware document often just says "how to use USB",
but "how clock/reset signals are connected in each block" is not mentioned,
or at least very unclear.

Probably, I will come back with real per-reset-line ID,
but I need some time to take a look.



-- 
Best Regards
Masahiro Yamada