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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
