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

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

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.