2020-01-09 20:44:58

by Jolly Shah

[permalink] [raw]
Subject: [PATCH 0/2] arch: arm64: xilinx: Make zynqmp_firmware driver optional

From: Tejas Patel <[email protected]>

Zynqmp firmware driver requires firmware to be present in system.
Zynqmp firmware driver will crash if firmware is not present in system.
For example single arch QEMU, may not have firmware, with such setup
Linux booting fails.

So make zynqmp_firmware driver as optional to disable it if user don't
have firmware in system.

Tejas Patel (2):
include: linux: firmware: Correct config dependency of zynqmp_eemi_ops
arch: arm64: xilinx: Make zynqmp_firmware driver optional

arch/arm64/Kconfig.platforms | 1 -
drivers/firmware/xilinx/Kconfig | 2 ++
include/linux/firmware/xlnx-zynqmp.h | 2 +-
3 files changed, 3 insertions(+), 2 deletions(-)

--
2.7.4


2020-01-09 21:03:03

by Jolly Shah

[permalink] [raw]
Subject: [PATCH 2/2] arch: arm64: xilinx: Make zynqmp_firmware driver optional

From: Tejas Patel <[email protected]>

Zynqmp firmware driver requires firmware to be present in system.
Zynqmp firmware driver will crash if firmware is not present in system.
For example single arch QEMU, may not have firmware, with such setup
Linux booting fails.

So make zynqmp_firmware driver as optional to disable it if user don't
have firmware in system.

Signed-off-by: Tejas Patel <[email protected]>
Signed-off-by: Jolly Shah <[email protected]>
---
arch/arm64/Kconfig.platforms | 1 -
drivers/firmware/xilinx/Kconfig | 2 ++
2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index b2b504e..563c93d 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -301,7 +301,6 @@ config ARCH_ZX

config ARCH_ZYNQMP
bool "Xilinx ZynqMP Family"
- select ZYNQMP_FIRMWARE
help
This enables support for Xilinx ZynqMP Family

diff --git a/drivers/firmware/xilinx/Kconfig b/drivers/firmware/xilinx/Kconfig
index bd33bbf..9a9bd19 100644
--- a/drivers/firmware/xilinx/Kconfig
+++ b/drivers/firmware/xilinx/Kconfig
@@ -6,6 +6,8 @@ menu "Zynq MPSoC Firmware Drivers"

config ZYNQMP_FIRMWARE
bool "Enable Xilinx Zynq MPSoC firmware interface"
+ depends on ARCH_ZYNQMP
+ default y if ARCH_ZYNQMP
select MFD_CORE
help
Firmware interface driver is used by different
--
2.7.4

2020-01-09 21:03:03

by Jolly Shah

[permalink] [raw]
Subject: [PATCH 1/2] include: linux: firmware: Correct config dependency of zynqmp_eemi_ops

From: Tejas Patel <[email protected]>

zynqmp_eemi_ops will be compiled only when CONFIG_ZYNQMP_FIRMWARE is
enabled. So check for CONFIG_ZYNQMP_FIRMWARE instead of checking for
CONFIG_ARCH_ZYNQMP.

Signed-off-by: Tejas Patel <[email protected]>
Signed-off-by: Jolly Shah <[email protected]>
---
include/linux/firmware/xlnx-zynqmp.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
index e41ad9e..a50a30b 100644
--- a/include/linux/firmware/xlnx-zynqmp.h
+++ b/include/linux/firmware/xlnx-zynqmp.h
@@ -311,7 +311,7 @@ struct zynqmp_eemi_ops {
int zynqmp_pm_invoke_fn(u32 pm_api_id, u32 arg0, u32 arg1,
u32 arg2, u32 arg3, u32 *ret_payload);

-#if IS_REACHABLE(CONFIG_ARCH_ZYNQMP)
+#if IS_REACHABLE(CONFIG_ZYNQMP_FIRMWARE)
const struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void);
#else
static inline struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void)
--
2.7.4

2020-01-10 11:55:27

by Sudeep Holla

[permalink] [raw]
Subject: Re: [PATCH 0/2] arch: arm64: xilinx: Make zynqmp_firmware driver optional

On Thu, Jan 09, 2020 at 11:06:02AM -0800, Jolly Shah wrote:
> From: Tejas Patel <[email protected]>
>
> Zynqmp firmware driver requires firmware to be present in system.
> Zynqmp firmware driver will crash if firmware is not present in system.
> For example single arch QEMU, may not have firmware, with such setup
> Linux booting fails.
>
> So make zynqmp_firmware driver as optional to disable it if user don't
> have firmware in system.
>

Why can't it be detected runtime ? How do you handle single binary if you
make this compile time option ?

--
Regards,
Sudeep

2020-01-13 06:48:11

by Rajan Vaja

[permalink] [raw]
Subject: RE: [PATCH 0/2] arch: arm64: xilinx: Make zynqmp_firmware driver optional

Hi Sudeep,

Thanks for the reviewing patch.

> -----Original Message-----
> From: Sudeep Holla <[email protected]>
> Sent: 10 January 2020 05:24 PM
> To: Jolly Shah <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Michal Simek <[email protected]>; Rajan Vaja
> <[email protected]>; [email protected]; linux-
> [email protected]; Sudeep Holla <[email protected]>; Tejas Patel
> <[email protected]>
> Subject: Re: [PATCH 0/2] arch: arm64: xilinx: Make zynqmp_firmware driver
> optional
>
> EXTERNAL EMAIL
>
> On Thu, Jan 09, 2020 at 11:06:02AM -0800, Jolly Shah wrote:
> > From: Tejas Patel <[email protected]>
> >
> > Zynqmp firmware driver requires firmware to be present in system.
> > Zynqmp firmware driver will crash if firmware is not present in system.
> > For example single arch QEMU, may not have firmware, with such setup
> > Linux booting fails.
> >
> > So make zynqmp_firmware driver as optional to disable it if user don't
> > have firmware in system.
> >
>
> Why can't it be detected runtime ? How do you handle single binary if you
> make this compile time option ?
[Rajan] There is PMU register which indicates if firmware is present or not, but in case of single arch QEMU that register will not be available so there is no way to detect if firmware is present or not from Linux.
Linux firmware crashes while arm_smccc_smc() call for firmware, but before this call there is no way to identify if firmware is present or not. So we are just giving user an option if they want to use it on single arch
Platform they can disable firmware driver.

Thanks,
Rajan
> --
> Regards,
> Sudeep

2020-01-13 15:15:02

by Sudeep Holla

[permalink] [raw]
Subject: Re: [PATCH 0/2] arch: arm64: xilinx: Make zynqmp_firmware driver optional

On Mon, Jan 13, 2020 at 06:46:52AM +0000, Rajan Vaja wrote:
> Hi Sudeep,
>
> Thanks for the reviewing patch.
>
> > -----Original Message-----
> > From: Sudeep Holla <[email protected]>
> > Sent: 10 January 2020 05:24 PM
> > To: Jolly Shah <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; Michal Simek <[email protected]>; Rajan Vaja
> > <[email protected]>; [email protected]; linux-
> > [email protected]; Sudeep Holla <[email protected]>; Tejas Patel
> > <[email protected]>
> > Subject: Re: [PATCH 0/2] arch: arm64: xilinx: Make zynqmp_firmware driver
> > optional
> >
> > EXTERNAL EMAIL
> >
> > On Thu, Jan 09, 2020 at 11:06:02AM -0800, Jolly Shah wrote:
> > > From: Tejas Patel <[email protected]>
> > >
> > > Zynqmp firmware driver requires firmware to be present in system.
> > > Zynqmp firmware driver will crash if firmware is not present in system.
> > > For example single arch QEMU, may not have firmware, with such setup
> > > Linux booting fails.
> > >
> > > So make zynqmp_firmware driver as optional to disable it if user don't
> > > have firmware in system.
> > >
> >
> > Why can't it be detected runtime ? How do you handle single binary if you
> > make this compile time option ?
> [Rajan] There is PMU register which indicates if firmware is present or not,
> but in case of single arch QEMU that register will not be available so
> there is no way to detect if firmware is present or not from Linux.

I am still not that convinced yet.

> Linux firmware crashes while arm_smccc_smc() call for firmware, but before
> this call there is no way to identify if firmware is present or not. So we
> are just giving user an option if they want to use it on single arch

So IIUC this platform used SMC as transport for EEMI communication. And
PSCI will act as bypass and send the command to PMU. If so why can't
platform PSCI implementation send error to OSPM if it is not implemented.

> Platform they can disable firmware driver.
>

Not an option. With this enable, single binary should work fine on both
QEMU and your platform with this EEMI firmware support. You need to find
a way to detect it dynamically.

--
Regards,
Sudeep

2020-02-24 10:35:30

by Michal Simek

[permalink] [raw]
Subject: Re: [PATCH 0/2] arch: arm64: xilinx: Make zynqmp_firmware driver optional

On 13. 01. 20 7:46, Rajan Vaja wrote:
> Hi Sudeep,
>
> Thanks for the reviewing patch.
>
>> -----Original Message-----
>> From: Sudeep Holla <[email protected]>
>> Sent: 10 January 2020 05:24 PM
>> To: Jolly Shah <[email protected]>
>> Cc: [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; Michal Simek <[email protected]>; Rajan Vaja
>> <[email protected]>; [email protected]; linux-
>> [email protected]; Sudeep Holla <[email protected]>; Tejas Patel
>> <[email protected]>
>> Subject: Re: [PATCH 0/2] arch: arm64: xilinx: Make zynqmp_firmware driver
>> optional
>>
>> EXTERNAL EMAIL
>>
>> On Thu, Jan 09, 2020 at 11:06:02AM -0800, Jolly Shah wrote:
>>> From: Tejas Patel <[email protected]>
>>>
>>> Zynqmp firmware driver requires firmware to be present in system.
>>> Zynqmp firmware driver will crash if firmware is not present in system.
>>> For example single arch QEMU, may not have firmware, with such setup
>>> Linux booting fails.
>>>
>>> So make zynqmp_firmware driver as optional to disable it if user don't
>>> have firmware in system.
>>>
>>
>> Why can't it be detected runtime ? How do you handle single binary if you
>> make this compile time option ?
> [Rajan] There is PMU register which indicates if firmware is present or not, but in case of single arch QEMU that register will not be available so there is no way to detect if firmware is present or not from Linux.
> Linux firmware crashes while arm_smccc_smc() call for firmware, but before this call there is no way to identify if firmware is present or not. So we are just giving user an option if they want to use it on single arch
> Platform they can disable firmware driver.

If this is just for qemu case why not just add this register for single
instance solution.

Edgar: Can we map this register when there is no second instance to PMU MB?

Thanks,
Michal

2020-02-24 10:36:54

by Michal Simek

[permalink] [raw]
Subject: Re: [PATCH 1/2] include: linux: firmware: Correct config dependency of zynqmp_eemi_ops

On 09. 01. 20 20:06, Jolly Shah wrote:
> From: Tejas Patel <[email protected]>
>
> zynqmp_eemi_ops will be compiled only when CONFIG_ZYNQMP_FIRMWARE is
> enabled. So check for CONFIG_ZYNQMP_FIRMWARE instead of checking for
> CONFIG_ARCH_ZYNQMP.
>
> Signed-off-by: Tejas Patel <[email protected]>
> Signed-off-by: Jolly Shah <[email protected]>
> ---
> include/linux/firmware/xlnx-zynqmp.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
> index e41ad9e..a50a30b 100644
> --- a/include/linux/firmware/xlnx-zynqmp.h
> +++ b/include/linux/firmware/xlnx-zynqmp.h
> @@ -311,7 +311,7 @@ struct zynqmp_eemi_ops {
> int zynqmp_pm_invoke_fn(u32 pm_api_id, u32 arg0, u32 arg1,
> u32 arg2, u32 arg3, u32 *ret_payload);
>
> -#if IS_REACHABLE(CONFIG_ARCH_ZYNQMP)
> +#if IS_REACHABLE(CONFIG_ZYNQMP_FIRMWARE)
> const struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void);
> #else
> static inline struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void)
>

I think that make sense to fix this dependency no matter if 2/2 is
applied. That's why adding it to my queue.

Thanks,
Michal

2020-02-24 10:44:30

by Michal Simek

[permalink] [raw]
Subject: Re: [PATCH 2/2] arch: arm64: xilinx: Make zynqmp_firmware driver optional

On 09. 01. 20 20:06, Jolly Shah wrote:
> From: Tejas Patel <[email protected]>
>
> Zynqmp firmware driver requires firmware to be present in system.
> Zynqmp firmware driver will crash if firmware is not present in system.
> For example single arch QEMU, may not have firmware, with such setup
> Linux booting fails.


I think that moving it to firmware Kconfig is good solution. What it is
wrong is that description above where I agree with Sudeep.
It means.
1. User should have option to disable zynqmp firmware driver which is
what this patch allows. It means if someone decides to use different
firmware mechanism it can do it directly by simply y/n option.

2. Autodetection of PMUFW presence is another feature which could be
implemented to have this driver enabled but different mechanism can be
used.

3. Doing this because of missing feature in QEMU is IMHO wrong. QEMU
should be fixed and then you don't have any issue if this should be used
or not.

Just a summary. Remove that QEMU example from commit message and talk to
Edgar to fix single QEMU solution to have that regs mapped all the time.

Thanks,
Michal

2020-02-26 00:12:57

by Jolly Shah

[permalink] [raw]
Subject: Re: [PATCH 2/2] arch: arm64: xilinx: Make zynqmp_firmware driver optional

Hi Michal,

> ------Original Message------
> From: Michal Simek <[email protected]>
> Sent: Monday, February 24, 2020 2:43AM
> To: Jolly Shah <[email protected]>, [email protected]
<[email protected]>, [email protected] <[email protected]>, 'Greg
Kh' <[email protected]>, [email protected]
<[email protected]>, [email protected] <[email protected]>,
[email protected] <[email protected]>, [email protected]
<[email protected]>, [email protected]
<[email protected]>, Michal Simek <[email protected]>
> Cc: Rajan Vaja <[email protected]>,
[email protected]
<[email protected]>, [email protected]
<[email protected]>, Tejas Patel <[email protected]>
> Subject: Re: [PATCH 2/2] arch: arm64: xilinx: Make zynqmp_firmware
driver optional
>
> On 09. 01. 20 20:06, Jolly Shah wrote:
>> From: Tejas Patel <[email protected]>
>>
>> Zynqmp firmware driver requires firmware to be present in system.
>> Zynqmp firmware driver will crash if firmware is not present in system.
>> For example single arch QEMU, may not have firmware, with such setup
>> Linux booting fails.
>
>
> I think that moving it to firmware Kconfig is good solution. What it is
> wrong is that description above where I agree with Sudeep.
> It means.
> 1. User should have option to disable zynqmp firmware driver which is
> what this patch allows. It means if someone decides to use different
> firmware mechanism it can do it directly by simply y/n option.
>
> 2. Autodetection of PMUFW presence is another feature which could be
> implemented to have this driver enabled but different mechanism can be
> used.
>
> 3. Doing this because of missing feature in QEMU is IMHO wrong. QEMU
> should be fixed and then you don't have any issue if this should be used
> or not.
>
> Just a summary. Remove that QEMU example from commit message and talk to
> Edgar to fix single QEMU solution to have that regs mapped all the time.

Pushed another patch as suggested. Will sync up with Edgar.

Thanks,
Jolly Shah

>
> Thanks,
> Michal
>