Message ID | 4efe8e4ab6b3d4389746a2080cf93bc24ba7293d.1569591296.git.michal.simek@xilinx.com |
---|---|
State | Deferred |
Headers | show |
Series | arm64: zynqmp: Clean communication with PMUFW | expand |
Hi Michal, On 27/09/19 15:34, Michal Simek wrote: > Cleanup PM ID handling by using enum values. > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > Signed-off-by: Ibai Erkiaga <ibai.erkiaga-elorza@xilinx.com> > --- > > arch/arm/mach-zynqmp/include/mach/sys_proto.h | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-zynqmp/include/mach/sys_proto.h b/arch/arm/mach-zynqmp/include/mach/sys_proto.h > index f25d414dcb1e..573c4ffceed9 100644 > --- a/arch/arm/mach-zynqmp/include/mach/sys_proto.h > +++ b/arch/arm/mach-zynqmp/include/mach/sys_proto.h > @@ -10,7 +10,8 @@ > #define PAYLOAD_ARG_CNT 5 > > #define ZYNQMP_CSU_SILICON_VER_MASK 0xF > -#define ZYNQMP_SIP_SVC_PM_SECURE_IMG_LOAD 0xC200002D > +#define ZYNQMP_SIP_SVC_PM_SECURE_IMG_LOAD \ > + (PM_SIP_SVC + PM_SECURE_IMAGE) > #define KEY_PTR_LEN 32 > > #define ZYNQMP_FPGA_BIT_AUTH_DDR 1 > @@ -21,7 +22,8 @@ > > #define ZYNQMP_FPGA_AUTH_DDR 1 > > -#define ZYNQMP_SIP_SVC_GET_API_VERSION 0xC2000001 > +#define ZYNQMP_SIP_SVC_GET_API_VERSION \ > + (PM_SIP_SVC + PM_GET_API_VERSION) > > #define ZYNQMP_PM_VERSION_MAJOR 1 > #define ZYNQMP_PM_VERSION_MINOR 0 > @@ -36,6 +38,13 @@ > > #define PMUFW_V1_0 ((1 << ZYNQMP_PM_VERSION_MAJOR_SHIFT) | 0) > > +#define PM_SIP_SVC 0xc2000000 > + > +enum pm_api_id { > + PM_GET_API_VERSION = 1, > + PM_SECURE_IMAGE = 45, > +}; > + This is a matter of personal taste, but I prefer to define values before using them. So unless there is a good reason to define these values here I'd rather move them before.
On 02. 10. 19 11:34, Luca Ceresoli wrote: > Hi Michal, > > On 27/09/19 15:34, Michal Simek wrote: >> Cleanup PM ID handling by using enum values. >> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >> Signed-off-by: Ibai Erkiaga <ibai.erkiaga-elorza@xilinx.com> >> --- >> >> arch/arm/mach-zynqmp/include/mach/sys_proto.h | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/mach-zynqmp/include/mach/sys_proto.h b/arch/arm/mach-zynqmp/include/mach/sys_proto.h >> index f25d414dcb1e..573c4ffceed9 100644 >> --- a/arch/arm/mach-zynqmp/include/mach/sys_proto.h >> +++ b/arch/arm/mach-zynqmp/include/mach/sys_proto.h >> @@ -10,7 +10,8 @@ >> #define PAYLOAD_ARG_CNT 5 >> >> #define ZYNQMP_CSU_SILICON_VER_MASK 0xF >> -#define ZYNQMP_SIP_SVC_PM_SECURE_IMG_LOAD 0xC200002D >> +#define ZYNQMP_SIP_SVC_PM_SECURE_IMG_LOAD \ >> + (PM_SIP_SVC + PM_SECURE_IMAGE) >> #define KEY_PTR_LEN 32 >> >> #define ZYNQMP_FPGA_BIT_AUTH_DDR 1 >> @@ -21,7 +22,8 @@ >> >> #define ZYNQMP_FPGA_AUTH_DDR 1 >> >> -#define ZYNQMP_SIP_SVC_GET_API_VERSION 0xC2000001 >> +#define ZYNQMP_SIP_SVC_GET_API_VERSION \ >> + (PM_SIP_SVC + PM_GET_API_VERSION) >> >> #define ZYNQMP_PM_VERSION_MAJOR 1 >> #define ZYNQMP_PM_VERSION_MINOR 0 >> @@ -36,6 +38,13 @@ >> >> #define PMUFW_V1_0 ((1 << ZYNQMP_PM_VERSION_MAJOR_SHIFT) | 0) >> >> +#define PM_SIP_SVC 0xc2000000 >> + >> +enum pm_api_id { >> + PM_GET_API_VERSION = 1, >> + PM_SECURE_IMAGE = 45, >> +}; >> + > > This is a matter of personal taste, but I prefer to define values before > using them. So unless there is a good reason to define these values here > I'd rather move them before. ZYNQMP_SIP_SVC.. macros are still used. It is just changing a way how they are defined because PM_SIP_SVC is really just SMC identification. M
Hi Michal, On 02/10/19 11:36, Michal Simek wrote: > On 02. 10. 19 11:34, Luca Ceresoli wrote: >> Hi Michal, >> >> On 27/09/19 15:34, Michal Simek wrote: >>> Cleanup PM ID handling by using enum values. >>> >>> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >>> Signed-off-by: Ibai Erkiaga <ibai.erkiaga-elorza@xilinx.com> >>> --- >>> >>> arch/arm/mach-zynqmp/include/mach/sys_proto.h | 13 +++++++++++-- >>> 1 file changed, 11 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/arm/mach-zynqmp/include/mach/sys_proto.h b/arch/arm/mach-zynqmp/include/mach/sys_proto.h >>> index f25d414dcb1e..573c4ffceed9 100644 >>> --- a/arch/arm/mach-zynqmp/include/mach/sys_proto.h >>> +++ b/arch/arm/mach-zynqmp/include/mach/sys_proto.h >>> @@ -10,7 +10,8 @@ >>> #define PAYLOAD_ARG_CNT 5 >>> >>> #define ZYNQMP_CSU_SILICON_VER_MASK 0xF >>> -#define ZYNQMP_SIP_SVC_PM_SECURE_IMG_LOAD 0xC200002D >>> +#define ZYNQMP_SIP_SVC_PM_SECURE_IMG_LOAD \ >>> + (PM_SIP_SVC + PM_SECURE_IMAGE) >>> #define KEY_PTR_LEN 32 >>> >>> #define ZYNQMP_FPGA_BIT_AUTH_DDR 1 >>> @@ -21,7 +22,8 @@ >>> >>> #define ZYNQMP_FPGA_AUTH_DDR 1 >>> >>> -#define ZYNQMP_SIP_SVC_GET_API_VERSION 0xC2000001 >>> +#define ZYNQMP_SIP_SVC_GET_API_VERSION \ >>> + (PM_SIP_SVC + PM_GET_API_VERSION) >>> >>> #define ZYNQMP_PM_VERSION_MAJOR 1 >>> #define ZYNQMP_PM_VERSION_MINOR 0 >>> @@ -36,6 +38,13 @@ >>> >>> #define PMUFW_V1_0 ((1 << ZYNQMP_PM_VERSION_MAJOR_SHIFT) | 0) >>> >>> +#define PM_SIP_SVC 0xc2000000 >>> + >>> +enum pm_api_id { >>> + PM_GET_API_VERSION = 1, >>> + PM_SECURE_IMAGE = 45, >>> +}; >>> + >> >> This is a matter of personal taste, but I prefer to define values before >> using them. So unless there is a good reason to define these values here >> I'd rather move them before. > > ZYNQMP_SIP_SVC.. macros are still used. It is just changing a way how > they are defined because PM_SIP_SVC is really just SMC identification. My point is about order of lines in the file, not about patch order in the series. Let me clarify. The lines where PM_SECURE_IMAGE is used is above the line there it is declared. Same for PM_GET_API_VERSION. I would (in this same patch) declare enum pm_api_id above in the file. I hope it is clearer now, sorry for the confusion.
On 02. 10. 19 11:44, Luca Ceresoli wrote: > Hi Michal, > > On 02/10/19 11:36, Michal Simek wrote: >> On 02. 10. 19 11:34, Luca Ceresoli wrote: >>> Hi Michal, >>> >>> On 27/09/19 15:34, Michal Simek wrote: >>>> Cleanup PM ID handling by using enum values. >>>> >>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >>>> Signed-off-by: Ibai Erkiaga <ibai.erkiaga-elorza@xilinx.com> >>>> --- >>>> >>>> arch/arm/mach-zynqmp/include/mach/sys_proto.h | 13 +++++++++++-- >>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/arm/mach-zynqmp/include/mach/sys_proto.h b/arch/arm/mach-zynqmp/include/mach/sys_proto.h >>>> index f25d414dcb1e..573c4ffceed9 100644 >>>> --- a/arch/arm/mach-zynqmp/include/mach/sys_proto.h >>>> +++ b/arch/arm/mach-zynqmp/include/mach/sys_proto.h >>>> @@ -10,7 +10,8 @@ >>>> #define PAYLOAD_ARG_CNT 5 >>>> >>>> #define ZYNQMP_CSU_SILICON_VER_MASK 0xF >>>> -#define ZYNQMP_SIP_SVC_PM_SECURE_IMG_LOAD 0xC200002D >>>> +#define ZYNQMP_SIP_SVC_PM_SECURE_IMG_LOAD \ >>>> + (PM_SIP_SVC + PM_SECURE_IMAGE) >>>> #define KEY_PTR_LEN 32 >>>> >>>> #define ZYNQMP_FPGA_BIT_AUTH_DDR 1 >>>> @@ -21,7 +22,8 @@ >>>> >>>> #define ZYNQMP_FPGA_AUTH_DDR 1 >>>> >>>> -#define ZYNQMP_SIP_SVC_GET_API_VERSION 0xC2000001 >>>> +#define ZYNQMP_SIP_SVC_GET_API_VERSION \ >>>> + (PM_SIP_SVC + PM_GET_API_VERSION) >>>> >>>> #define ZYNQMP_PM_VERSION_MAJOR 1 >>>> #define ZYNQMP_PM_VERSION_MINOR 0 >>>> @@ -36,6 +38,13 @@ >>>> >>>> #define PMUFW_V1_0 ((1 << ZYNQMP_PM_VERSION_MAJOR_SHIFT) | 0) >>>> >>>> +#define PM_SIP_SVC 0xc2000000 >>>> + >>>> +enum pm_api_id { >>>> + PM_GET_API_VERSION = 1, >>>> + PM_SECURE_IMAGE = 45, >>>> +}; >>>> + >>> >>> This is a matter of personal taste, but I prefer to define values before >>> using them. So unless there is a good reason to define these values here >>> I'd rather move them before. >> >> ZYNQMP_SIP_SVC.. macros are still used. It is just changing a way how >> they are defined because PM_SIP_SVC is really just SMC identification. > > My point is about order of lines in the file, not about patch order in > the series. > > Let me clarify. The lines where PM_SECURE_IMAGE is used is above the > line there it is declared. Same for PM_GET_API_VERSION. I would (in this > same patch) declare enum pm_api_id above in the file. > > I hope it is clearer now, sorry for the confusion. > Ok. Got you. Let me look at it. It shouldn't be a problem in this patch because that values are going to move anyway. But we can resort these stuff in zynqmp_firmware.h. M
diff --git a/arch/arm/mach-zynqmp/include/mach/sys_proto.h b/arch/arm/mach-zynqmp/include/mach/sys_proto.h index f25d414dcb1e..573c4ffceed9 100644 --- a/arch/arm/mach-zynqmp/include/mach/sys_proto.h +++ b/arch/arm/mach-zynqmp/include/mach/sys_proto.h @@ -10,7 +10,8 @@ #define PAYLOAD_ARG_CNT 5 #define ZYNQMP_CSU_SILICON_VER_MASK 0xF -#define ZYNQMP_SIP_SVC_PM_SECURE_IMG_LOAD 0xC200002D +#define ZYNQMP_SIP_SVC_PM_SECURE_IMG_LOAD \ + (PM_SIP_SVC + PM_SECURE_IMAGE) #define KEY_PTR_LEN 32 #define ZYNQMP_FPGA_BIT_AUTH_DDR 1 @@ -21,7 +22,8 @@ #define ZYNQMP_FPGA_AUTH_DDR 1 -#define ZYNQMP_SIP_SVC_GET_API_VERSION 0xC2000001 +#define ZYNQMP_SIP_SVC_GET_API_VERSION \ + (PM_SIP_SVC + PM_GET_API_VERSION) #define ZYNQMP_PM_VERSION_MAJOR 1 #define ZYNQMP_PM_VERSION_MINOR 0 @@ -36,6 +38,13 @@ #define PMUFW_V1_0 ((1 << ZYNQMP_PM_VERSION_MAJOR_SHIFT) | 0) +#define PM_SIP_SVC 0xc2000000 + +enum pm_api_id { + PM_GET_API_VERSION = 1, + PM_SECURE_IMAGE = 45, +}; + enum { IDCODE, VERSION,