public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* Re: [PATCH v2 13/13] ArmPlatformPkg: Introduce SCMI protocol
@ 2018-01-11 16:33 Evan Lloyd
  2018-01-12 13:35 ` Ard Biesheuvel
  0 siblings, 1 reply; 4+ messages in thread
From: Evan Lloyd @ 2018-01-11 16:33 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: edk2-devel@lists.01.org,
	"ard.biesheuvel@linaro.org"@arm.com,
	"leif.lindholm@linaro.org"@arm.com,
	"Matteo.Carlini@arm.com"@arm.com,
	"nd@arm.com"@arm.com



> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: 23 December 2017 14:06
> To: Evan Lloyd <Evan.Lloyd@arm.com>
> Cc: edk2-devel@lists.01.org; "ard.biesheuvel@linaro.org"@arm.com;
> "leif.lindholm@linaro.org"@arm.com;
> "Matteo.Carlini@arm.com"@arm.com; "nd@arm.com"@arm.com
> Subject: Re: [PATCH v2 13/13] ArmPlatformPkg: Introduce SCMI protocol
>
> , couOn 22 December 2017 at 18:34,  <evan.lloyd@arm.com> wrote:
> > From: Girish Pathak <girish.pathak@arm.com>
> >
> > This change introduces a new SCMI protocol driver for
> > Arm Platforms. The driver currently supports only clock
> > and performance management protocols. Other protocols
> > will be added as and when needed.
> >
> > Clock management protocol is used to configure the HDLCD clock
> > on Juno platforms.
> >
> > Whereas performance management protocol allows adjustment
> > of various performance domains to evaluate performance of the
> > Juno platform.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Girish Pathak <girish.pathak@arm.com>
> > ---
...
> > +
> > +#ifndef ARM_SCMI_BASE_PROTOCOL_PRIVATE_H_
> > +#define ARM_SCMI_BASE_PROTOCOL_PRIVATE_H_
> > +
> > +// Return values of BASE_DISCOVER_LIST_PROTOCOLS command.
> > +typedef struct {
> > +  UINT32 NumProtocols;
> > +  // Array of four protocols in each element
> > +  // Total elements = 1 + (NumProtocols-1)/4
> > +  UINT8 Protocols[];
>
> For paleontological reasons, EDK2 code does not allow arrays of
> unspecified length at the end of a struct. I don't know which version
> of which toolchain used to be the issue here, and I would be surprised
> if anyone went through the trouble of writing that down, but it is the
> reason that EDK2 only allows a [1] array, and hence care needs to be
> taken to add/substract 1 as appropriate when sizing the variable.

 [[Evan Lloyd]] Whilst not disputing your claim, we have failed to find any such restriction in the coding standard.  Do you have a reference?

>
> So now, it's my turn to cut /you/ a deal here. If you stop whingeing
> about frivolous patches that only move whitespace around or change //
> for /*, or move ASSERT()s into if () conditions on cold-as-ice code
> paths, I am not going to complain about the arrays of unspecified
> length in this patch, simply because I don't take the coding standard
> as gospel, and feel that the Tianocore could do with a bit more
> pragmatism when it comes to matters like these.

 [[Evan Lloyd]] I'm happy to take the deal.  You know why some of the formatting changes are there, and why the ASSERT moves are relevant, so I can't promise we'll stop doing it.

>
>
> > +} BASE_DISCOVER_LIST;
> > +
> > +#endif /* ARM_SCMI_BASE_PROTOCOL_PRIVATE_H_ */
> > diff --git
> a/ArmPlatformPkg/Drivers/ArmScmiDxe/ArmScmiClockProtocolPrivate.h
> b/ArmPlatformPkg/Drivers/ArmScmiDxe/ArmScmiClockProtocolPrivate.h
> > new file mode 100644
> > index
> 0000000000000000000000000000000000000000..2807b6b476ac1b8cf8
> 21a29ca7a59a78e9188c52
> > --- /dev/null
> > +++
> b/ArmPlatformPkg/Drivers/ArmScmiDxe/ArmScmiClockProtocolPrivate.h
....
> > +
> > +#endif /* SCMI_PRIVATE_H_ */
> > diff --git a/ArmPlatformPkg/Include/Drivers/ArmScmi.h
> b/ArmPlatformPkg/Include/Drivers/ArmScmi.h
>
> Please don't put stuff in Include/Drivers.
>
> Things derived from industry specs belong in Include/IndustryStandard,
> but given that this header only defines SCMI_MAX_STR_LEN, could you
> move it into the protocol header instead?

 [[Evan Lloyd]] Can do.  However, this point raises some interesting thoughts.
e.g. Does SBBR count as an "Industry" standard?  How about SCMI?
Also, I believe we aim to eventually put ArmPlatformPkg out of our misery.
So, would the SCMI driver belong in ArmPkg (as an arm ATG standard), or should it be in edk2-platforms?

>
>
> > new file mode 100644
> > index
> 0000000000000000000000000000000000000000..04ea3de5b34157ed45
> 9ee47440abbcaa7114e93a
> > --- /dev/null
> > +++ b/ArmPlatformPkg/Include/Drivers/ArmScmi.h
> > @@ -0,0 +1,27 @@
...
> > diff --git a/ArmPlatformPkg/Include/Drivers/ArmScmiBaseProtocol.h
> b/ArmPlatformPkg/Include/Drivers/ArmScmiBaseProtocol.h
>
> This belongs in Include/Protocol, and needs to be declared in the
> package .dec file as well, along with its GUID.
>

 [[Evan Lloyd]]   We can do that, but can I check that is actually what you want.
This file relates to an SCMI (communications) protocol definition, not a UEFI protocol (although Girish has used the familiar style).
Also, is that Include/Protocol in ArmPkg, ArmPlatformsPkg, or edk2-platforms/Platform/ARM ?

>
...
> > +#define ARM_SCMI_BASE_PROTOCOL_GUID  { 0xd7e5abe9, 0x33ab,
> 0x418e, { 0x9f, 0x91, 0x72, 0xda, 0xe2, 0xba, 0x8e, 0x2f } }
> > +
>
> Please wrap this line
[[Evan Lloyd]] Can do, but I don't think it needs to be in the .h file at all.
>
> > +extern EFI_GUID gArmScmiBaseProtocolGuid;
> > +
...
> > +/** Initialize Base protocol and install protocol on a given handle.
> > +
> > +   @param[in] Handle              Handle to install Base protocol.
> > +
> > +   @retval EFI_SUCCESS            Base protocol interface installed
> > +                                  successfully.
> > +**/
> > +EFI_STATUS
> > +ScmiBaseProtocolInit (
> > +  IN OUT EFI_HANDLE* Handle
> > +  );
> > +
>
> This is not part of the protocol so it needs to be moved elsewhere.

 [[Evan Lloyd]]  It initialises the SCMI Base Protocol - not a UEFI protocol.


>
> > +#endif /* ARM_SCMI_BASE_PROTOCOL_H_ */
> > +
> > diff --git a/ArmPlatformPkg/Include/Drivers/ArmScmiClockProtocol.h
> b/ArmPlatformPkg/Include/Drivers/ArmScmiClockProtocol.h
>
> Please move to Include/Protocol as well, and add declaration to the
> package file.
>
> > new file mode 100644
> > index
> 0000000000000000000000000000000000000000..a97728e4dfe8efc3cd8
> dc29dc94987c1cc6c6a80
> > --- /dev/null
> > +++ b/ArmPlatformPkg/Include/Drivers/ArmScmiClockProtocol.h
...
> > +/** Initialize clock management protocol and install protocol on a given
> handle.
> > +
> > +  @param[in] Handle              Handle to install clock management
> protocol.
> > +
> > +  @retval EFI_SUCCESS            Clock protocol interface installed
> successfully.
> > +**/
> > +EFI_STATUS
> > +ScmiClockProtocolInit (
> > +  IN EFI_HANDLE *Handle
> > +  );
> > +
>
> Please move this into a separate header, it is not part of the protocol.
[[Evan Lloyd]] ditto
>
> > +#endif /* ARM_SCMI_CLOCK_PROTOCOL_H_ */
> > +
> > diff --git
> a/ArmPlatformPkg/Include/Drivers/ArmScmiPerformanceProtocol.h
> b/ArmPlatformPkg/Include/Drivers/ArmScmiPerformanceProtocol.h
>
> Same as above
>
> > new file mode 100644
> > index
> 0000000000000000000000000000000000000000..cb4aa6bf71df86cfd7a0
> dabb354112c5a38c978f
> > --- /dev/null
> > +++ b/ArmPlatformPkg/Include/Drivers/ArmScmiPerformanceProtocol.h
> > @@ -0,0 +1,274 @@
...
> > +/** Initialize performance management protocol and install on a given
> Handle.
> > +
> > +  @param[in] Handle              Handle to install performance management
> > +                                 protocol.
> > +
> > +  @retval EFI_SUCCESS            Performance protocol installed successfully.
> > +**/
> > +EFI_STATUS
> > +ScmiPerformanceProtocolInit (
> > +  IN EFI_HANDLE* Handle
> > +  );
> > +
>
> Same as above
>
> > +#endif /* ARM_SCMI_PERFORMANCE_PROTOCOL_H_ */
> > +
> > diff --git a/ArmPlatformPkg/Include/Library/ArmMtl.h
> b/ArmPlatformPkg/Include/Library/ArmMtl.h
>
> Please rename to ArmMtlLib, and declare it as a library class in the
> package file.
[[Evan Lloyd]] Quite right
>
> > new file mode 100644
> > index
> 0000000000000000000000000000000000000000..9be65cfa0a1dcf0d984f
> 29e5d95aedf5e0afac2b
> > --- /dev/null
> > +++ b/ArmPlatformPkg/Include/Library/ArmMtl.h
> > @@ -0,0 +1,132 @@
> > +/** @file
...
> > --
> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> >
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

^ permalink raw reply	[flat|nested] 4+ messages in thread
* [PATCH v2 00/13] ArmPlatformPkg: Update GOP
@ 2017-12-22 18:34 evan.lloyd
  2017-12-22 18:34 ` [PATCH v2 13/13] ArmPlatformPkg: Introduce SCMI protocol evan.lloyd
  0 siblings, 1 reply; 4+ messages in thread
From: evan.lloyd @ 2017-12-22 18:34 UTC (permalink / raw)
  To: edk2-devel
  Cc: "ard.biesheuvel, "leif.lindholm, "Matteo.Carlini,
	"nd

From: EvanLloyd <evan.lloyd@arm.com>


This patch series addresses comments on the original
(https://lists.01.org/pipermail/edk2-devel/2017-September/015321.html)
reworking of the Graphics Output Protocol code in ArmPlatformPkg.
It also contains updates for the new SCMI protocol.

After a number of format and quality modifications, several errors
are corrected and new functionality added for Mali DP.

The changes are tested on Juno, and FVP. Edk2-platforms changes will
follow shortly.

Code is available for examination at:
  https://github.com/EvanLloyd/tianocore/tree/166_gop_v2



Girish Pathak (13):
  ArmPlatformPkg: Tidy Lcd code: Coding standard
  ArmPlatformPkg: Tidy Lcd code: Updated comments
  ArmPlatformPkg: PL111 and HDLCD: add const qualifier
  ArmPlatformPkg: HDLCD and PL111: Update debug ASSERTS
  ArmPlatformPkg: PL111Lcd: Replace magic number with macro
  ArmPlatformPkg: Implement LcdIdentify function for HDLCD GOP
  ArmPlatformPkg: Redefine LcdPlatformGetTimings function
  ArmPlatformPkg: Add PCD to select pixel format
  ArmPlatformPkg: PCD to swap red/blue format for HDLCD
  ArmPlatformPkg: Additional display modes
  ArmPlatformPkg: Reserving framebuffer at build
  ArmPlatformPkg: New DP500/DP550/DP650 GOP driver.
  ArmPlatformPkg: Introduce SCMI protocol

 ArmPlatformPkg/ArmPlatformPkg.dec                                     |  18 +
 ArmPlatformPkg/Drivers/ArmScmiDxe/ArmScmiDxe.inf                      |  48 ++
 ArmPlatformPkg/Library/{HdLcd/HdLcd.inf => ArmMaliDp/ArmMaliDp.inf}   |  26 +-
 ArmPlatformPkg/Library/HdLcd/HdLcd.inf                                |   2 +
 ArmPlatformPkg/Drivers/ArmScmiDxe/ArmScmiBaseProtocolPrivate.h        |  29 ++
 ArmPlatformPkg/Drivers/ArmScmiDxe/ArmScmiClockProtocolPrivate.h       |  69 +++
 ArmPlatformPkg/Drivers/ArmScmiDxe/ArmScmiPerformanceProtocolPrivate.h |  39 ++
 ArmPlatformPkg/Drivers/ArmScmiDxe/ScmiDxe.h                           |  41 ++
 ArmPlatformPkg/Drivers/ArmScmiDxe/ScmiPrivate.h                       | 174 ++++++++
 ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.h    |  10 +-
 ArmPlatformPkg/Include/Drivers/ArmScmi.h                              |  27 ++
 ArmPlatformPkg/Include/Drivers/ArmScmiBaseProtocol.h                  | 182 ++++++++
 ArmPlatformPkg/Include/Drivers/ArmScmiClockProtocol.h                 | 225 ++++++++++
 ArmPlatformPkg/Include/Drivers/ArmScmiPerformanceProtocol.h           | 274 ++++++++++++
 ArmPlatformPkg/Include/Library/ArmMtl.h                               | 132 ++++++
 ArmPlatformPkg/Include/Library/LcdPlatformLib.h                       | 181 ++++++--
 ArmPlatformPkg/Library/ArmMaliDp/ArmMaliDp.h                          | 243 +++++++++++
 ArmPlatformPkg/Library/HdLcd/HdLcd.h                                  |  23 +-
 ArmPlatformPkg/Drivers/ArmScmiDxe/Scmi.c                              | 261 +++++++++++
 ArmPlatformPkg/Drivers/ArmScmiDxe/ScmiBaseProtocol.c                  | 320 ++++++++++++++
 ArmPlatformPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c                 | 419 ++++++++++++++++++
 ArmPlatformPkg/Drivers/ArmScmiDxe/ScmiDxe.c                           | 135 ++++++
 ArmPlatformPkg/Drivers/ArmScmiDxe/ScmiPerformanceProtocol.c           | 457 ++++++++++++++++++++
 ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.c    | 197 +++++----
 ArmPlatformPkg/Library/ArmMaliDp/ArmMaliDp.c                          | 414 ++++++++++++++++++
 ArmPlatformPkg/Library/HdLcd/HdLcd.c                                  | 185 ++++----
 ArmPlatformPkg/Library/PL111Lcd/PL111Lcd.c                            | 143 ++++--
 27 files changed, 3984 insertions(+), 290 deletions(-)
 create mode 100644 ArmPlatformPkg/Drivers/ArmScmiDxe/ArmScmiDxe.inf
 copy ArmPlatformPkg/Library/{HdLcd/HdLcd.inf => ArmMaliDp/ArmMaliDp.inf} (61%)
 create mode 100644 ArmPlatformPkg/Drivers/ArmScmiDxe/ArmScmiBaseProtocolPrivate.h
 create mode 100644 ArmPlatformPkg/Drivers/ArmScmiDxe/ArmScmiClockProtocolPrivate.h
 create mode 100644 ArmPlatformPkg/Drivers/ArmScmiDxe/ArmScmiPerformanceProtocolPrivate.h
 create mode 100644 ArmPlatformPkg/Drivers/ArmScmiDxe/ScmiDxe.h
 create mode 100644 ArmPlatformPkg/Drivers/ArmScmiDxe/ScmiPrivate.h
 create mode 100644 ArmPlatformPkg/Include/Drivers/ArmScmi.h
 create mode 100644 ArmPlatformPkg/Include/Drivers/ArmScmiBaseProtocol.h
 create mode 100644 ArmPlatformPkg/Include/Drivers/ArmScmiClockProtocol.h
 create mode 100644 ArmPlatformPkg/Include/Drivers/ArmScmiPerformanceProtocol.h
 create mode 100644 ArmPlatformPkg/Include/Library/ArmMtl.h
 create mode 100644 ArmPlatformPkg/Library/ArmMaliDp/ArmMaliDp.h
 create mode 100644 ArmPlatformPkg/Drivers/ArmScmiDxe/Scmi.c
 create mode 100644 ArmPlatformPkg/Drivers/ArmScmiDxe/ScmiBaseProtocol.c
 create mode 100644 ArmPlatformPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c
 create mode 100644 ArmPlatformPkg/Drivers/ArmScmiDxe/ScmiDxe.c
 create mode 100644 ArmPlatformPkg/Drivers/ArmScmiDxe/ScmiPerformanceProtocol.c
 create mode 100644 ArmPlatformPkg/Library/ArmMaliDp/ArmMaliDp.c

-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")



^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-01-12 13:29 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-11 16:33 [PATCH v2 13/13] ArmPlatformPkg: Introduce SCMI protocol Evan Lloyd
2018-01-12 13:35 ` Ard Biesheuvel
  -- strict thread matches above, loose matches on Subject: below --
2017-12-22 18:34 [PATCH v2 00/13] ArmPlatformPkg: Update GOP evan.lloyd
2017-12-22 18:34 ` [PATCH v2 13/13] ArmPlatformPkg: Introduce SCMI protocol evan.lloyd
2017-12-23 14:05   ` Ard Biesheuvel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox