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

[Kefeng Wang <wangkefeng.wang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>]

On 2016/4/7 22:55, Mathieu Poirier wrote:
> On 7 April 2016 at 02:23, Kefeng Wang <wangkefeng.wang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> 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-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>>  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.
> 
>> +
>> +       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.

> 
>> +{
>> +       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;
}

Thanks,
Kefeng


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html