Re: [edk2-platforms][PATCH v2 01/32] Ampere: Initial support for Ampere Altra processor and Mt. Jade platform

["Leif Lindholm" <leif@nuviainc.com>]

On Wed, Jun 09, 2021 at 11:50:07 +0700, Nhi Pham wrote:
> > > diff --git a/Silicon/Ampere/AmpereAltraPkg/Library/MailboxInterfaceLib/MailboxInterfaceLib.c b/Silicon/Ampere/AmpereAltraPkg/Library/MailboxInterfaceLib/MailboxInterfaceLib.c
> > > new file mode 100644
> > > index 000000000000..0da1f90699f6
> > > --- /dev/null
> > > +++ b/Silicon/Ampere/AmpereAltraPkg/Library/MailboxInterfaceLib/MailboxInterfaceLib.c
> > > @@ -0,0 +1,282 @@
> > > +/** @file
> > > +  The library implements the hardware Mailbox (Doorbell) interface for communication
> > > +  between the Application Processor (ARMv8) and the System Control Processors (SMpro/PMpro).
> > > +
> > > +  Copyright (c) 2021, Ampere Computing LLC. All rights reserved.<BR>
> > > +
> > > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +
> > > +**/
> > > +
> > > +#include <Uefi.h>
> > > +
> > > +#include <Library/AmpereCpuLib.h>
> > > +#include <Library/BaseMemoryLib.h>
> > > +#include <Library/DebugLib.h>
> > > +#include <Library/MailboxInterfaceLib.h>
> > > +#include <Library/TimerLib.h>
> > > +#include <Library/IoLib.h>
> > > +
> > > +//
> > > +// Hardware Doorbells
> > > +//
> > > +#define SMPRO_DB0_IRQ_OFST               40
> > > +#define SMPRO_DB0_BASE_ADDRESS           (FixedPcdGet64 (PcdSmproDbBaseReg))
> > > +
> > > +#define PMPRO_DB0_IRQ_OFST               56
> > > +#define PMPRO_DB0_BASE_ADDRESS           (FixedPcdGet64 (PcdPmproDbBaseReg))
> > > +
> > > +#define SLAVE_SOCKET_BASE_ADDRESS_OFFSET 0x400000000000
> > > +
> > > +//
> > > +// The base SPI interrupt number of the Slave socket
> > > +//
> > > +#define SLAVE_SOCKET_SPI_INTERRUPT 352
> > > +
> > > +#define SLAVE_SOCKET_DOORBELL_INTERRUPT_BASE(Socket) ((Socket) * SLAVE_SOCKET_SPI_INTERRUPT - 32)
> > > +
> > > +//
> > > +// Doorbell base register stride size
> > > +//
> > > +#define DB_BASE_REG_STRIDE 0x00001000
> > > +
> > > +#define SMPRO_DBx_ADDRESS(socket, db) \
> > > +        ((socket) * SLAVE_SOCKET_BASE_ADDRESS_OFFSET + SMPRO_DB0_BASE_ADDRESS + DB_BASE_REG_STRIDE * (db))
> > > +
> > > +#define PMPRO_DBx_ADDRESS(socket, db) \
> > > +        ((socket) * SLAVE_SOCKET_BASE_ADDRESS_OFFSET + PMPRO_DB0_BASE_ADDRESS + DB_BASE_REG_STRIDE * (db))
> > > +
> > > +//
> > > +// Doorbell Status Bits
> > > +//
> > > +#define DB_STATUS_AVAIL_BIT       BIT16
> > > +#define DB_STATUS_ACK_BIT         BIT0
> > > +
> > > +/**
> > > +  Get the base address of a doorbell.
> > > +
> > > +  @param[in]  Socket            Active socket index.
> > > +  @param[in]  Doorbell          Doorbell channel for communication with the SMpro/PMpro.
> > > +
> > > +  @retval UINT32                The base address of the doorbell.
> > > +                                The returned value is 0 indicate that the input parameters are invalid.
> > > +
> > > +**/
> > > +UINTN
> > > +EFIAPI
> > > +MailboxGetDoorbellAddress (
> > > +  IN UINT8             Socket,
> > > +  IN DOORBELL_CHANNELS Doorbell
> > > +  )
> > > +{
> > > +  UINTN DoorbellAddress;
> > > +
> > > +  if (Socket >= GetNumberOfActiveSockets ()
> > > +      || Doorbell >= NUMBER_OF_DOORBELLS_PER_SOCKET)
> > > +  {
> > > +    return 0;
> > > +  }
> > Coding style is
> >   if () {
> >   }
> > 
> > This file gets it consistently wrong.
> 
> I agree that it should be consistent. But that coding style conforms to
> "5.2.1.6 Each sub-expression of a complex predicate expression must be on a
> separate line" in EDK II Coding Standards Spec (https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing#5-2-1-6-each-sub-expression-of-a-complex-predicate-expression-must-be-on-a-separate-line).
> 
> "Predicate expressions containing multiple operators with sub-expressions
> joined by && or || must have each sub-expression on a separate line. The
> opening brace, '|{|' of the body shall be on a line by itself and aligned in
> the starting column of the associated keyword.
> 
> while ( ( Code == MEETS_STANDARD)
>   && ( Code == FUNCTIONAL))
> {
>   ShipIt();
> }
> 
> "
> 
> However, I'm OK to change it as your suggestion.

I stand corrected.
I think it's confusing, but if it's not only explicitly permitted but
actually required by the spec, then feel free to leave it in.

Best Regards,

Leif (pondering starting a separate thread on changing that rule in the
      coding style)