* [PATCH 1/5] MdePkg/SPI: Change function definitions to match their descriptions. @ 2018-02-27 16:49 Marvin Häuser 2018-03-05 2:14 ` Bi, Dandan 0 siblings, 1 reply; 4+ messages in thread From: Marvin Häuser @ 2018-02-27 16:49 UTC (permalink / raw) To: edk2-devel@lists.01.org; +Cc: michael.d.kinney@intel.com, liming.gao@intel.com The PI specification is not continuous with function headers and descriptions for the SPI protocols. Correct and comment these mistakes. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com> --- MdePkg/Include/Protocol/SpiConfiguration.h | 12 ++++++++---- MdePkg/Include/Protocol/SpiHc.h | 14 +++++++++----- MdePkg/Include/Protocol/SpiIo.h | 15 ++++++++++----- 3 files changed, 27 insertions(+), 14 deletions(-) diff --git a/MdePkg/Include/Protocol/SpiConfiguration.h b/MdePkg/Include/Protocol/SpiConfiguration.h index c0df183ec7f0..c36a809f4232 100644 --- a/MdePkg/Include/Protocol/SpiConfiguration.h +++ b/MdePkg/Include/Protocol/SpiConfiguration.h @@ -1,7 +1,7 @@ /** @file This file defines the SPI Configuration Protocol. - Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> + Copyright (c) 2017 - 2018, Intel Corporation. All rights reserved.<BR> 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 @@ -65,6 +65,10 @@ EFI_STATUS IN BOOLEAN PinValue ); +// +// Note: In the PI specification, ClockHz is decorated as only 'IN', which is +// not conforming to the parameter description. +// /** Set up the clock generator to produce the correct clock frequency, phase and polarity for a SPI chip. @@ -78,7 +82,7 @@ EFI_STATUS ClockPhase and ClockPolarity fields. The routine also has access to the names for the SPI bus and chip which can be used during debugging. - @param[in] ClockHz Pointer to the requested clock frequency. The clock + @param[in,out] ClockHz Pointer to the requested clock frequency. The clock generator will choose a supported clock frequency which is less then or equal to this value. Specify zero to turn the clock generator off. @@ -92,8 +96,8 @@ EFI_STATUS **/ typedef EFI_STATUS (EFIAPI *EFI_SPI_CLOCK) ( - IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral, - IN UINT32 *ClockHz + IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral, + IN OUT UINT32 *ClockHz ); /// diff --git a/MdePkg/Include/Protocol/SpiHc.h b/MdePkg/Include/Protocol/SpiHc.h index 12fe5d2dce0a..71c75431e4e8 100644 --- a/MdePkg/Include/Protocol/SpiHc.h +++ b/MdePkg/Include/Protocol/SpiHc.h @@ -1,7 +1,7 @@ /** @file This file defines the SPI Host Controller Protocol. - Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> + Copyright (c) 2017 - 2018, Intel Corporation. All rights reserved.<BR> 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 @@ -66,6 +66,10 @@ typedef EFI_STATUS IN BOOLEAN PinValue ); +// +// Note: In the PI specification, ClockHz is decorated as only 'IN', which is +// not conforming to the parameter description. +// /** Set up the clock generator to produce the correct clock frequency, phase and polarity for a SPI chip. @@ -80,7 +84,7 @@ typedef EFI_STATUS ClockPhase and ClockPolarity fields. The routine also has access to the names for the SPI bus and chip which can be used during debugging. - @param[in] ClockHz Pointer to the requested clock frequency. The SPI + @param[in,out] ClockHz Pointer to the requested clock frequency. The SPI host controller will choose a supported clock frequency which is less then or equal to this value. Specify zero to turn the clock generator @@ -94,9 +98,9 @@ typedef EFI_STATUS **/ typedef EFI_STATUS (EFIAPI *EFI_SPI_HC_PROTOCOL_CLOCK) ( - IN CONST EFI_SPI_HC_PROTOCOL *This, - IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral, - IN UINT32 *ClockHz + IN CONST EFI_SPI_HC_PROTOCOL *This, + IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral, + IN OUT UINT32 *ClockHz ); /** diff --git a/MdePkg/Include/Protocol/SpiIo.h b/MdePkg/Include/Protocol/SpiIo.h index 43e804518f8b..8c5d96bb04b2 100644 --- a/MdePkg/Include/Protocol/SpiIo.h +++ b/MdePkg/Include/Protocol/SpiIo.h @@ -1,7 +1,7 @@ /** @file This file defines the SPI I/O Protocol. - Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> + Copyright (c) 2017 - 2018, Intel Corporation. All rights reserved.<BR> 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 @@ -144,6 +144,11 @@ EFI_STATUS OUT UINT8 *ReadBuffer ); +// +// Note: In the PI specification, 'This' is decorated with 'IN' and 'CONST'. +// However, 'This' needs to be updated in order to reflect the +// Peripheral update. +// /** Update the SPI peripheral associated with this SPI 10 instance. @@ -152,8 +157,8 @@ EFI_STATUS SPI NOR flash parts, where the size and parameters change depending upon device is in the socket. - @param[in] This Pointer to an EFI_SPI_IO_PROTOCOL structure. - @param[in] SpiPeripheral Pointer to an EFI_SPI_PERIPHERAL structure. + @param[in,out] This Pointer to an EFI_SPI_IO_PROTOCOL structure. + @param[in] SpiPeripheral Pointer to an EFI_SPI_PERIPHERAL structure. @retval EFI_SUCCESS The SPI peripheral was updated successfully @retval EFI_INVALID_PARAMETER The SpiPeripheral value is NULL, @@ -165,8 +170,8 @@ EFI_STATUS **/ typedef EFI_STATUS (EFIAPI *EFI_SPI_IO_PROTOCOL_UPDATE_SPI_PERIPHERAL) ( - IN CONST EFI_SPI_IO_PROTOCOL *This, - IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral + IN OUT EFI_SPI_IO_PROTOCOL *This, + IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral ); /// -- 2.16.0.windows.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/5] MdePkg/SPI: Change function definitions to match their descriptions. 2018-02-27 16:49 [PATCH 1/5] MdePkg/SPI: Change function definitions to match their descriptions Marvin Häuser @ 2018-03-05 2:14 ` Bi, Dandan 2018-03-06 13:32 ` Marvin Häuser 0 siblings, 1 reply; 4+ messages in thread From: Bi, Dandan @ 2018-03-05 2:14 UTC (permalink / raw) To: Marvin Häuser, edk2-devel@lists.01.org Cc: Kinney, Michael D, Gao, Liming, Bi, Dandan Hi Marvin, Thank you very much for your contribution. Could you hold this patch series? Since current SPI header files follow PI1.6 spec. For this case, we should submit an ECR to update the PI spec firstly. After the ECR has been approved by PIWG, then we can send patch to update them. Since you have found a lot of missing/incorrect parts in the SPI part of PI Spec. Could you help to submit an ECR to update them? Thanks, Dandan -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Marvin Häuser Sent: Wednesday, February 28, 2018 12:49 AM To: edk2-devel@lists.01.org Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com> Subject: [edk2] [PATCH 1/5] MdePkg/SPI: Change function definitions to match their descriptions. The PI specification is not continuous with function headers and descriptions for the SPI protocols. Correct and comment these mistakes. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com> --- MdePkg/Include/Protocol/SpiConfiguration.h | 12 ++++++++---- MdePkg/Include/Protocol/SpiHc.h | 14 +++++++++----- MdePkg/Include/Protocol/SpiIo.h | 15 ++++++++++----- 3 files changed, 27 insertions(+), 14 deletions(-) diff --git a/MdePkg/Include/Protocol/SpiConfiguration.h b/MdePkg/Include/Protocol/SpiConfiguration.h index c0df183ec7f0..c36a809f4232 100644 --- a/MdePkg/Include/Protocol/SpiConfiguration.h +++ b/MdePkg/Include/Protocol/SpiConfiguration.h @@ -1,7 +1,7 @@ /** @file This file defines the SPI Configuration Protocol. - Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> + Copyright (c) 2017 - 2018, Intel Corporation. All rights + reserved.<BR> 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 @@ -65,6 +65,10 @@ EFI_STATUS IN BOOLEAN PinValue ); +// +// Note: In the PI specification, ClockHz is decorated as only 'IN', which is +// not conforming to the parameter description. +// /** Set up the clock generator to produce the correct clock frequency, phase and polarity for a SPI chip. @@ -78,7 +82,7 @@ EFI_STATUS ClockPhase and ClockPolarity fields. The routine also has access to the names for the SPI bus and chip which can be used during debugging. - @param[in] ClockHz Pointer to the requested clock frequency. The clock + @param[in,out] ClockHz Pointer to the requested clock frequency. The clock generator will choose a supported clock frequency which is less then or equal to this value. Specify zero to turn the clock generator off. @@ -92,8 +96,8 @@ EFI_STATUS **/ typedef EFI_STATUS (EFIAPI *EFI_SPI_CLOCK) ( - IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral, - IN UINT32 *ClockHz + IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral, + IN OUT UINT32 *ClockHz ); /// diff --git a/MdePkg/Include/Protocol/SpiHc.h b/MdePkg/Include/Protocol/SpiHc.h index 12fe5d2dce0a..71c75431e4e8 100644 --- a/MdePkg/Include/Protocol/SpiHc.h +++ b/MdePkg/Include/Protocol/SpiHc.h @@ -1,7 +1,7 @@ /** @file This file defines the SPI Host Controller Protocol. - Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> + Copyright (c) 2017 - 2018, Intel Corporation. All rights + reserved.<BR> 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 @@ -66,6 +66,10 @@ typedef EFI_STATUS IN BOOLEAN PinValue ); +// +// Note: In the PI specification, ClockHz is decorated as only 'IN', which is +// not conforming to the parameter description. +// /** Set up the clock generator to produce the correct clock frequency, phase and polarity for a SPI chip. @@ -80,7 +84,7 @@ typedef EFI_STATUS ClockPhase and ClockPolarity fields. The routine also has access to the names for the SPI bus and chip which can be used during debugging. - @param[in] ClockHz Pointer to the requested clock frequency. The SPI + @param[in,out] ClockHz Pointer to the requested clock frequency. The SPI host controller will choose a supported clock frequency which is less then or equal to this value. Specify zero to turn the clock generator @@ -94,9 +98,9 @@ typedef EFI_STATUS **/ typedef EFI_STATUS (EFIAPI *EFI_SPI_HC_PROTOCOL_CLOCK) ( - IN CONST EFI_SPI_HC_PROTOCOL *This, - IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral, - IN UINT32 *ClockHz + IN CONST EFI_SPI_HC_PROTOCOL *This, + IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral, + IN OUT UINT32 *ClockHz ); /** diff --git a/MdePkg/Include/Protocol/SpiIo.h b/MdePkg/Include/Protocol/SpiIo.h index 43e804518f8b..8c5d96bb04b2 100644 --- a/MdePkg/Include/Protocol/SpiIo.h +++ b/MdePkg/Include/Protocol/SpiIo.h @@ -1,7 +1,7 @@ /** @file This file defines the SPI I/O Protocol. - Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> + Copyright (c) 2017 - 2018, Intel Corporation. All rights + reserved.<BR> 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 @@ -144,6 +144,11 @@ EFI_STATUS OUT UINT8 *ReadBuffer ); +// +// Note: In the PI specification, 'This' is decorated with 'IN' and 'CONST'. +// However, 'This' needs to be updated in order to reflect the +// Peripheral update. +// /** Update the SPI peripheral associated with this SPI 10 instance. @@ -152,8 +157,8 @@ EFI_STATUS SPI NOR flash parts, where the size and parameters change depending upon device is in the socket. - @param[in] This Pointer to an EFI_SPI_IO_PROTOCOL structure. - @param[in] SpiPeripheral Pointer to an EFI_SPI_PERIPHERAL structure. + @param[in,out] This Pointer to an EFI_SPI_IO_PROTOCOL structure. + @param[in] SpiPeripheral Pointer to an EFI_SPI_PERIPHERAL structure. @retval EFI_SUCCESS The SPI peripheral was updated successfully @retval EFI_INVALID_PARAMETER The SpiPeripheral value is NULL, @@ -165,8 +170,8 @@ EFI_STATUS **/ typedef EFI_STATUS (EFIAPI *EFI_SPI_IO_PROTOCOL_UPDATE_SPI_PERIPHERAL) ( - IN CONST EFI_SPI_IO_PROTOCOL *This, - IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral + IN OUT EFI_SPI_IO_PROTOCOL *This, + IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral ); /// -- 2.16.0.windows.2 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/5] MdePkg/SPI: Change function definitions to match their descriptions. 2018-03-05 2:14 ` Bi, Dandan @ 2018-03-06 13:32 ` Marvin Häuser 2018-03-09 1:54 ` Bi, Dandan 0 siblings, 1 reply; 4+ messages in thread From: Marvin Häuser @ 2018-03-06 13:32 UTC (permalink / raw) To: edk2-devel@lists.01.org, Bi, Dandan Hey Dandan, Thanks for your reply. I issued updates for the current headers because I saw activity in edk2-platforms to implement these incomplete protocols and because I haven't heard back for discussed changes in the specification for some time. Sure I could help submitting the ECR, however I'm not sure how. I'm not an UEFI Contributor and neither did I find any design flaws, just typos and missing definitions, which obviously must be bit values due to their non-exclusive nature. If you need me to do something concrete, I will try to do my best. Regards, Marvin. > -----Original Message----- > From: Bi, Dandan <dandan.bi@intel.com> > Sent: Monday, March 5, 2018 3:15 AM > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming > <liming.gao@intel.com>; Bi, Dandan <dandan.bi@intel.com> > Subject: RE: [PATCH 1/5] MdePkg/SPI: Change function definitions to match > their descriptions. > > Hi Marvin, > > Thank you very much for your contribution. Could you hold this patch series? > Since current SPI header files follow PI1.6 spec. > For this case, we should submit an ECR to update the PI spec firstly. After the > ECR has been approved by PIWG, then we can send patch to update them. > Since you have found a lot of missing/incorrect parts in the SPI part of PI > Spec. Could you help to submit an ECR to update them? > > > Thanks, > Dandan > > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Marvin Häuser > Sent: Wednesday, February 28, 2018 12:49 AM > To: edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming > <liming.gao@intel.com> > Subject: [edk2] [PATCH 1/5] MdePkg/SPI: Change function definitions to > match their descriptions. > > The PI specification is not continuous with function headers and descriptions > for the SPI protocols. Correct and comment these mistakes. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com> > --- > MdePkg/Include/Protocol/SpiConfiguration.h | 12 ++++++++---- > MdePkg/Include/Protocol/SpiHc.h | 14 +++++++++----- > MdePkg/Include/Protocol/SpiIo.h | 15 ++++++++++----- > 3 files changed, 27 insertions(+), 14 deletions(-) > > diff --git a/MdePkg/Include/Protocol/SpiConfiguration.h > b/MdePkg/Include/Protocol/SpiConfiguration.h > index c0df183ec7f0..c36a809f4232 100644 > --- a/MdePkg/Include/Protocol/SpiConfiguration.h > +++ b/MdePkg/Include/Protocol/SpiConfiguration.h > @@ -1,7 +1,7 @@ > /** @file > This file defines the SPI Configuration Protocol. > > - Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> > + Copyright (c) 2017 - 2018, Intel Corporation. All rights > + reserved.<BR> > 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 > @@ -65,6 +65,10 @@ EFI_STATUS > IN BOOLEAN PinValue > ); > > +// > +// Note: In the PI specification, ClockHz is decorated as only 'IN', which is > +// not conforming to the parameter description. > +// > /** > Set up the clock generator to produce the correct clock frequency, phase > and > polarity for a SPI chip. > @@ -78,7 +82,7 @@ EFI_STATUS > ClockPhase and ClockPolarity fields. The routine > also has access to the names for the SPI bus and > chip which can be used during debugging. > - @param[in] ClockHz Pointer to the requested clock frequency. The > clock > + @param[in,out] ClockHz Pointer to the requested clock frequency. The > clock > generator will choose a supported clock frequency > which is less then or equal to this value. > Specify zero to turn the clock generator off. > @@ -92,8 +96,8 @@ EFI_STATUS > **/ > typedef EFI_STATUS > (EFIAPI *EFI_SPI_CLOCK) ( > - IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral, > - IN UINT32 *ClockHz > + IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral, > + IN OUT UINT32 *ClockHz > ); > > /// > diff --git a/MdePkg/Include/Protocol/SpiHc.h > b/MdePkg/Include/Protocol/SpiHc.h index 12fe5d2dce0a..71c75431e4e8 > 100644 > --- a/MdePkg/Include/Protocol/SpiHc.h > +++ b/MdePkg/Include/Protocol/SpiHc.h > @@ -1,7 +1,7 @@ > /** @file > This file defines the SPI Host Controller Protocol. > > - Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> > + Copyright (c) 2017 - 2018, Intel Corporation. All rights > + reserved.<BR> > 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 > @@ -66,6 +66,10 @@ typedef EFI_STATUS > IN BOOLEAN PinValue > ); > > +// > +// Note: In the PI specification, ClockHz is decorated as only 'IN', which is > +// not conforming to the parameter description. > +// > /** > Set up the clock generator to produce the correct clock frequency, phase > and > polarity for a SPI chip. > @@ -80,7 +84,7 @@ typedef EFI_STATUS > ClockPhase and ClockPolarity fields. The routine > also has access to the names for the SPI bus and > chip which can be used during debugging. > - @param[in] ClockHz Pointer to the requested clock frequency. The SPI > + @param[in,out] ClockHz Pointer to the requested clock frequency. The > SPI > host controller will choose a supported clock > frequency which is less then or equal to this > value. Specify zero to turn the clock generator @@ -94,9 +98,9 > @@ typedef EFI_STATUS **/ typedef EFI_STATUS (EFIAPI > *EFI_SPI_HC_PROTOCOL_CLOCK) ( > - IN CONST EFI_SPI_HC_PROTOCOL *This, > - IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral, > - IN UINT32 *ClockHz > + IN CONST EFI_SPI_HC_PROTOCOL *This, > + IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral, > + IN OUT UINT32 *ClockHz > ); > > /** > diff --git a/MdePkg/Include/Protocol/SpiIo.h > b/MdePkg/Include/Protocol/SpiIo.h index 43e804518f8b..8c5d96bb04b2 > 100644 > --- a/MdePkg/Include/Protocol/SpiIo.h > +++ b/MdePkg/Include/Protocol/SpiIo.h > @@ -1,7 +1,7 @@ > /** @file > This file defines the SPI I/O Protocol. > > - Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> > + Copyright (c) 2017 - 2018, Intel Corporation. All rights > + reserved.<BR> > 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 > @@ -144,6 +144,11 @@ EFI_STATUS > OUT UINT8 *ReadBuffer > ); > > +// > +// Note: In the PI specification, 'This' is decorated with 'IN' and 'CONST'. > +// However, 'This' needs to be updated in order to reflect the > +// Peripheral update. > +// > /** > Update the SPI peripheral associated with this SPI 10 instance. > > @@ -152,8 +157,8 @@ EFI_STATUS > SPI NOR flash parts, where the size and parameters change depending > upon > device is in the socket. > > - @param[in] This Pointer to an EFI_SPI_IO_PROTOCOL structure. > - @param[in] SpiPeripheral Pointer to an EFI_SPI_PERIPHERAL structure. > + @param[in,out] This Pointer to an EFI_SPI_IO_PROTOCOL structure. > + @param[in] SpiPeripheral Pointer to an EFI_SPI_PERIPHERAL structure. > > @retval EFI_SUCCESS The SPI peripheral was updated successfully > @retval EFI_INVALID_PARAMETER The SpiPeripheral value is NULL, @@ - > 165,8 +170,8 @@ EFI_STATUS **/ typedef EFI_STATUS (EFIAPI > *EFI_SPI_IO_PROTOCOL_UPDATE_SPI_PERIPHERAL) ( > - IN CONST EFI_SPI_IO_PROTOCOL *This, > - IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral > + IN OUT EFI_SPI_IO_PROTOCOL *This, > + IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral > ); > > /// > -- > 2.16.0.windows.2 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/5] MdePkg/SPI: Change function definitions to match their descriptions. 2018-03-06 13:32 ` Marvin Häuser @ 2018-03-09 1:54 ` Bi, Dandan 0 siblings, 0 replies; 4+ messages in thread From: Bi, Dandan @ 2018-03-09 1:54 UTC (permalink / raw) To: Marvin Häuser, edk2-devel@lists.01.org; +Cc: Bi, Dandan Hi Marvin, How about your write the ECR doc firstly then I ask Intel architect to help submit it to PIWG? Thanks, Dandan -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Marvin Häuser Sent: Tuesday, March 6, 2018 9:32 PM To: edk2-devel@lists.01.org; Bi, Dandan <dandan.bi@intel.com> Subject: Re: [edk2] [PATCH 1/5] MdePkg/SPI: Change function definitions to match their descriptions. Hey Dandan, Thanks for your reply. I issued updates for the current headers because I saw activity in edk2-platforms to implement these incomplete protocols and because I haven't heard back for discussed changes in the specification for some time. Sure I could help submitting the ECR, however I'm not sure how. I'm not an UEFI Contributor and neither did I find any design flaws, just typos and missing definitions, which obviously must be bit values due to their non-exclusive nature. If you need me to do something concrete, I will try to do my best. Regards, Marvin. > -----Original Message----- > From: Bi, Dandan <dandan.bi@intel.com> > Sent: Monday, March 5, 2018 3:15 AM > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming > <liming.gao@intel.com>; Bi, Dandan <dandan.bi@intel.com> > Subject: RE: [PATCH 1/5] MdePkg/SPI: Change function definitions to > match their descriptions. > > Hi Marvin, > > Thank you very much for your contribution. Could you hold this patch series? > Since current SPI header files follow PI1.6 spec. > For this case, we should submit an ECR to update the PI spec firstly. > After the ECR has been approved by PIWG, then we can send patch to update them. > Since you have found a lot of missing/incorrect parts in the SPI part > of PI Spec. Could you help to submit an ECR to update them? > > > Thanks, > Dandan > > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Marvin Häuser > Sent: Wednesday, February 28, 2018 12:49 AM > To: edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming > <liming.gao@intel.com> > Subject: [edk2] [PATCH 1/5] MdePkg/SPI: Change function definitions to > match their descriptions. > > The PI specification is not continuous with function headers and > descriptions for the SPI protocols. Correct and comment these mistakes. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com> > --- > MdePkg/Include/Protocol/SpiConfiguration.h | 12 ++++++++---- > MdePkg/Include/Protocol/SpiHc.h | 14 +++++++++----- > MdePkg/Include/Protocol/SpiIo.h | 15 ++++++++++----- > 3 files changed, 27 insertions(+), 14 deletions(-) > > diff --git a/MdePkg/Include/Protocol/SpiConfiguration.h > b/MdePkg/Include/Protocol/SpiConfiguration.h > index c0df183ec7f0..c36a809f4232 100644 > --- a/MdePkg/Include/Protocol/SpiConfiguration.h > +++ b/MdePkg/Include/Protocol/SpiConfiguration.h > @@ -1,7 +1,7 @@ > /** @file > This file defines the SPI Configuration Protocol. > > - Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> > + Copyright (c) 2017 - 2018, Intel Corporation. All rights > + reserved.<BR> > 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 @@ -65,6 +65,10 @@ EFI_STATUS > IN BOOLEAN PinValue > ); > > +// > +// Note: In the PI specification, ClockHz is decorated as only 'IN', which is > +// not conforming to the parameter description. > +// > /** > Set up the clock generator to produce the correct clock frequency, > phase and > polarity for a SPI chip. > @@ -78,7 +82,7 @@ EFI_STATUS > ClockPhase and ClockPolarity fields. The routine > also has access to the names for the SPI bus and > chip which can be used during debugging. > - @param[in] ClockHz Pointer to the requested clock frequency. The > clock > + @param[in,out] ClockHz Pointer to the requested clock frequency. The > clock > generator will choose a supported clock frequency > which is less then or equal to this value. > Specify zero to turn the clock generator off. > @@ -92,8 +96,8 @@ EFI_STATUS > **/ > typedef EFI_STATUS > (EFIAPI *EFI_SPI_CLOCK) ( > - IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral, > - IN UINT32 *ClockHz > + IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral, > + IN OUT UINT32 *ClockHz > ); > > /// > diff --git a/MdePkg/Include/Protocol/SpiHc.h > b/MdePkg/Include/Protocol/SpiHc.h index 12fe5d2dce0a..71c75431e4e8 > 100644 > --- a/MdePkg/Include/Protocol/SpiHc.h > +++ b/MdePkg/Include/Protocol/SpiHc.h > @@ -1,7 +1,7 @@ > /** @file > This file defines the SPI Host Controller Protocol. > > - Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> > + Copyright (c) 2017 - 2018, Intel Corporation. All rights > + reserved.<BR> > 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 @@ -66,6 +66,10 @@ typedef EFI_STATUS > IN BOOLEAN PinValue > ); > > +// > +// Note: In the PI specification, ClockHz is decorated as only 'IN', which is > +// not conforming to the parameter description. > +// > /** > Set up the clock generator to produce the correct clock frequency, > phase and > polarity for a SPI chip. > @@ -80,7 +84,7 @@ typedef EFI_STATUS > ClockPhase and ClockPolarity fields. The routine > also has access to the names for the SPI bus and > chip which can be used during debugging. > - @param[in] ClockHz Pointer to the requested clock frequency. The SPI > + @param[in,out] ClockHz Pointer to the requested clock frequency. The > SPI > host controller will choose a supported clock > frequency which is less then or equal to this > value. Specify zero to turn the clock > generator @@ -94,9 +98,9 @@ typedef EFI_STATUS **/ typedef > EFI_STATUS (EFIAPI > *EFI_SPI_HC_PROTOCOL_CLOCK) ( > - IN CONST EFI_SPI_HC_PROTOCOL *This, > - IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral, > - IN UINT32 *ClockHz > + IN CONST EFI_SPI_HC_PROTOCOL *This, > + IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral, > + IN OUT UINT32 *ClockHz > ); > > /** > diff --git a/MdePkg/Include/Protocol/SpiIo.h > b/MdePkg/Include/Protocol/SpiIo.h index 43e804518f8b..8c5d96bb04b2 > 100644 > --- a/MdePkg/Include/Protocol/SpiIo.h > +++ b/MdePkg/Include/Protocol/SpiIo.h > @@ -1,7 +1,7 @@ > /** @file > This file defines the SPI I/O Protocol. > > - Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> > + Copyright (c) 2017 - 2018, Intel Corporation. All rights > + reserved.<BR> > 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 @@ -144,6 +144,11 @@ EFI_STATUS > OUT UINT8 *ReadBuffer > ); > > +// > +// Note: In the PI specification, 'This' is decorated with 'IN' and 'CONST'. > +// However, 'This' needs to be updated in order to reflect the > +// Peripheral update. > +// > /** > Update the SPI peripheral associated with this SPI 10 instance. > > @@ -152,8 +157,8 @@ EFI_STATUS > SPI NOR flash parts, where the size and parameters change depending > upon > device is in the socket. > > - @param[in] This Pointer to an EFI_SPI_IO_PROTOCOL structure. > - @param[in] SpiPeripheral Pointer to an EFI_SPI_PERIPHERAL structure. > + @param[in,out] This Pointer to an EFI_SPI_IO_PROTOCOL structure. > + @param[in] SpiPeripheral Pointer to an EFI_SPI_PERIPHERAL structure. > > @retval EFI_SUCCESS The SPI peripheral was updated successfully > @retval EFI_INVALID_PARAMETER The SpiPeripheral value is NULL, @@ > - > 165,8 +170,8 @@ EFI_STATUS **/ typedef EFI_STATUS (EFIAPI > *EFI_SPI_IO_PROTOCOL_UPDATE_SPI_PERIPHERAL) ( > - IN CONST EFI_SPI_IO_PROTOCOL *This, > - IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral > + IN OUT EFI_SPI_IO_PROTOCOL *This, > + IN CONST EFI_SPI_PERIPHERAL *SpiPeripheral > ); > > /// > -- > 2.16.0.windows.2 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-03-09 1:48 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-02-27 16:49 [PATCH 1/5] MdePkg/SPI: Change function definitions to match their descriptions Marvin Häuser 2018-03-05 2:14 ` Bi, Dandan 2018-03-06 13:32 ` Marvin Häuser 2018-03-09 1:54 ` Bi, Dandan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox