[PATCH v2 2/2] hwrng: hisi: Add support for Hisilicon SoC RNG

[Mathieu Poirier]

On 7 April 2016 at 20:03, Kefeng Wang <wangkefeng.wang at huawei.com> wrote:
>
>
> On 2016/4/7 22:55, Mathieu Poirier wrote:
>> On 7 April 2016 at 02:23, Kefeng Wang <wangkefeng.wang at huawei.com> wrote:
>>> This adds the Hisilicon Random Number Generator(RNG) support,
>>> which is found in Hip04 and Hip05 soc.
>>>
>>> Signed-off-by: Kefeng Wang <wangkefeng.wang at huawei.com>
>>> ---
>>>  drivers/char/hw_random/Kconfig    |  13 ++++
>>>  drivers/char/hw_random/Makefile   |   1 +
>>>  drivers/char/hw_random/hisi-rng.c | 121 ++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 135 insertions(+)
>>>  create mode 100644 drivers/char/hw_random/hisi-rng.c
> [...]
>>> +
>>> +#define RNG_SEED       0x0
>>> +#define RNG_CTRL       0x4
>>> +  #define RNG_SEED_SEL BIT(2)
>>> +  #define RNG_RING_EN  BIT(1)
>>> +  #define RNG_EN       BIT(0)
>>> +#define RNG_RAN_NUM    0x10
>>> +#define RNG_PHY_SEED   0x14
>>> +#define RNG_RELOAD_ERR 0x84
>>> +
>>> +#define to_hisi_rng(p) container_of(p, struct hisi_rng, rng)
>>> +
>>> +static int seed_sel;
>>> +module_param(seed_sel, int, S_IRUGO);
>>> +MODULE_PARM_DESC(seed_sel, "Auto reload seed. 0, use LFSR(default); 1, use ring oscillator.");
>>> +
>>> +struct hisi_rng {
>>> +       void __iomem *base;
>>> +       struct hwrng rng;
>>> +};
>>> +
>>> +static int hisi_rng_init(struct hwrng *rng)
>>> +{
>>> +       struct hisi_rng *hrng = to_hisi_rng(rng);
>>> +       int val = RNG_EN;
>>> +       u32 seed;
>>> +
>>> +       /* get a random number as initial seed */
>>> +       get_random_bytes(&seed, sizeof(seed));
>>> +
>>> +       writel_relaxed(seed, hrng->base + RNG_SEED);
>>> +
>>> +       if (seed_sel == 1)
>>> +               val |= RNG_RING_EN | RNG_SEED_SEL;
>>
>> Hello Kefeng,
>>
>> Please explain what this mode does.  Otherwise reviewers have to make
>> a guess based on the description of the module parameter.
> The seed is reload periodically, there are two choice of seeds, default using
> the val from LFSR, seed_sel = 1  will use the val generated by ring oscillator.

This is the kind of comment that should be in the code.  That way
people can understand what is going on.

>>
>>> +
>>> +       writel_relaxed(val, hrng->base + RNG_CTRL);
>>> +       return 0;
>>> +}
>>> +
>>> +static void hisi_rng_cleanup(struct hwrng *rng)
>>> +{
>>> +       struct hisi_rng *hrng = to_hisi_rng(rng);
>>> +
>>> +       writel_relaxed(0, hrng->base + RNG_CTRL);
>>> +}
>>> +
>>> +static int hisi_rng_read(struct hwrng *rng, void *buf, size_t max,
>>> +                          bool wait)
>>
>> What are variable 'max' and 'wait' used for?  As far as I can tell
>> they can be removed.
> Ok, will use data_read callback in struct hwrng.

Sorry about that, I just noticed this morning the "data_read()"
callback is obsolete [1].  As such you can probably keep the code the
way you had it.

[1]. http://lxr.free-electrons.com/source/include/linux/hw_random.h#L28

>
>>
>>> +{
>>> +       struct hisi_rng *hrng = to_hisi_rng(rng);
>>> +       u32 *data = buf;
>>> +
>>> +       *data = readl_relaxed(hrng->base + RNG_RAN_NUM);
>>> +       return 4;
>>
>> Why not simply doing the readl_relaxed() in the the code rather than
>> introducing a new function?
> Not clear, your mean introduce the data variable?
> Will use following data_read, thanks.
>
> static int hisi_rng_data_read(struct hwrng *rng, u32 *data)
> {
>        struct hisi_rng *hrng = to_hisi_rng(rng);
>        *data = readl_relaxed(hrng->base + RNG_RAN_NUM);
>        return 4;
> }

See my comment above, you were right the first time.

Apology and regards,
Mathieu

>
> Thanks,
> Kefeng
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html