Re: [edk2-platforms][PATCH 1/5] Silicon/Bcm283x: Clean up Bcm2836.h header

["Pete Batard" <pete@akeo.ie>]

On 2019.11.27 13:17, Ard Biesheuvel wrote:
> On Wed, 27 Nov 2019 at 14:09, Pete Batard <pete@akeo.ie> wrote:
>>
>> On 2019.11.27 13:00, Ard Biesheuvel wrote:
>>> On Wed, 27 Nov 2019 at 13:56, Pete Batard <pete@akeo.ie> wrote:
>>>>
>>>> On 2019.11.27 12:48, Ard Biesheuvel wrote:
>>>>> On Wed, 27 Nov 2019 at 13:37, Pete Batard <pete@akeo.ie> wrote:
>>>>>>
>>>>>> Add missing RNG registers, prefer reusing shorter define's
>>>>>> instead of PCDs and clean up spacing.
>>>>>>
>>>>>
>>>>> Is there a source for these register definitions?
>>>>
>>>> I used the most recent Linux driver I could find (but I guess I need to
>>>> point out that it was used for reference with regards to the registers.
>>>> Especially, no code was copied from that source).
>>>>
>>>>> It seems the Linux
>>>>> driver deviates from the below (and the warmup count thing uses the
>>>>> status register as well), so it would be helpful to quote the
>>>>> authoritative reference here.
>>>>
>>>> https://github.com/raspberrypi/linux/blob/rpi-5.4.y/drivers/char/hw_random/iproc-rng200.c#L223-L224
>>>> seems to indicate that RNG_WARMUP_COUNT (0x40000) should go into
>>>> RNG_TOTAL_BIT_COUNT_THRESHOLD_OFFSET (them) / RNG_BIT_COUNT_THRESHOLD
>>>> (us) and not the status register.
>>>>
>>>> Now, of course, since we don't have a public datasheet, it's hard to
>>>> have absolute certainty on what's the proper register to write, but I
>>>> guess the most recent code with a Broadcom Corporation copyright has to
>>>> our next best thing when it comes to authoritative answer...
>>>>
>>>
>>> Well, there is this one too
>>>
>>> https://github.com/raspberrypi/linux/blob/rpi-5.4.y/drivers/char/hw_random/bcm2835-rng.c
>>>
>>> which is what I used as the basis for the existing version.
>>
>> Okay. Looks like 2835 (which has to be the basis for the Bcm2837 used on
>> the Pi 3) and 2838 (basis for Bcm2711) have at least one difference with
>> regards to where the warmup count should go.
>>
>> I guess an easy fix, if we don't want to have to spin 2 separate
>> drivers, is to write to both RNG_STATUS and RNG_BIT_COUNT_THRESHOLD
>> during init.
>>
>> I have tested on Pi 3 and not seen any ill effect to writing to
>> RNG_BIT_COUNT_THRESHOLD.
>>
> 
> I don't like that at all tbh. A misbehaving RNG is virtually
> undetectable (unless it starts returning zeroes), so knowingly
> deviating from the initialization routines like that should really be
> avoided.
> 
>> Would something like this work for you in a v2?
>>
>> Alternatively, I could change the new PcdBcm283xRngUseFifo PCD to
>> PcdBcm283xRng2838Compatible and use that for the conditional code.
>>
> 
> These are two different IP blocks, with different register layouts
> etc, so I don't really see the point in parameterizing the existing
> driver like that. I'd prefer it if we could just clone the existing
> one and make the changes unconditionally.

Okay. I was pretty much expecting that answer...

Regards,

/Pete