Re: [PATCH edk2-platforms] Silicon/Bcm2836: add random number generator driver

[Leif Lindholm <leif.lindholm@linaro.org>]

On Thu, Jan 31, 2019 at 06:14:45PM +0100, Ard Biesheuvel wrote:
> On Thu, 31 Jan 2019 at 16:05, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >
> > On Wed, Jan 30, 2019 at 08:39:43PM +0100, Ard Biesheuvel wrote:
> > > Expose the SoC's RNG peripheral via the EFI_RNG_PROTOCOL.
> > > This is used by Linux to seed the KASLR routines.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >
> > Not tested, but looks fine. Only question: could we add those few
> > #defines to IndustryStandard/Bcm2836.h (should that really be
> > #IndustryStandard, btw?) rather than creating a tiny standalone one?
> > (more below)
> >
> 
> Sure.
> 
> Re IndustryStandard/, I deliberately chose something idiomatic for
> EDK2, and this is the least inappropriate one. I could live with
> Chipset/ as well, but dumping headers under Include/ directly is not
> the solution IMO.

I disagree. Dumping the main SoC header under the top-level SoC
directory (and same pattern for platform) is idiomatic.
Dumping all kinds of random files there isn't, I agree (although that
happens too).

BeagleBoardPkg/Include/BeagleBoard.h
OvmfPkg/Include/OvmfPlatforms.h
Vlv2TbltDevicePkg/Include/Platform.h

Silicon/Hisilicon/Hi6220/Include/Hi6220.h

An alternative pattern is an include directory named after the
SoC/Platform.

Omap35xxPkg/Include/Omap3530/

You used 

Silicon/Socionext/SynQuacer/Include/Platform/

which I also don't mind.

If you'd prefer it, I'd be happy with Platform/ and Silicon/.

But we'd better settle on something before Pete changes too much based
on my feedback.

/
    Leif

> > > ---
> > >  Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.c               | 204 ++++++++++++++++++++
> > >  Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf             |  45 +++++
> > >  Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836Rng.h |  26 +++
> > >  3 files changed, 275 insertions(+)
> > >
> >
> > > diff --git a/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836Rng.h b/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836Rng.h
> > > new file mode 100644
> > > index 000000000000..8274e2fe8f77
> > > --- /dev/null
> > > +++ b/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836Rng.h
> > > @@ -0,0 +1,26 @@
> > > + /** @file
> > > + *
> > > + *  Copyright (c) 2019 Linaro, Ltd. All rights reserved.
> > > + *
> > > + *  This program and the accompanying materials
> > > + *  are licensed and made available under the terms and conditions of the BSD License
> > > + *  which accompanies this distribution.  The full text of the license may be found at
> > > + *  http://opensource.org/licenses/bsd-license.php
> > > + *
> > > + *  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > > + *  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > > + *
> > > + **/
> > > +
> > > +#ifndef __BCM2836_RNG_H__
> > > +#define __BCM2836_RNG_H__
> > > +
> > > +#define RNG_BASE_ADDRESS   (BCM2836_SOC_REGISTERS + 0x00104000)
> >
> > If we can't, this file needs to pull in Bcm2836.h anyway.
> >
> 
> Yep.