On 2016/4/8 22:35, Mathieu Poirier wrote: > On 7 April 2016 at 20:03, Kefeng Wang <wangkefeng.wang@xxxxxxxxxx> wrote: >> >> >> On 2016/4/7 22:55, Mathieu Poirier wrote: >>> On 7 April 2016 at 02:23, Kefeng Wang <wangkefeng.wang@xxxxxxxxxx> 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@xxxxxxxxxx> >>>> --- >>>> 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. > Ok, will add them into code. >>> >>>> + >>>> + 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. Will resend it, thanks for your review, :) > > Apology and regards, > Mathieu > >> >> Thanks, >> Kefeng >> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe devicetree" in >> the body of a message to majordomo@xxxxxxxxxxxxxxx >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > . > -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html