diff mbox series

[3/7] drivers: soc: xilinx: add check for platform

Message ID	20230929105558.11893-4-jay.buddhabhatti@amd.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37; Received-SPF: Pass (protection.outlook.com: domain of amd.com designates 165.204.84.17 as permitted sender) receiver=protection.outlook.com; client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C From: Jay Buddhabhatti <jay.buddhabhatti@amd.com> To: <michal.simek@amd.com>, <gregkh@linuxfoundation.org>, <tanmay.shah@amd.com>, <sai.krishna.potthuri@amd.com>, <nava.kishore.manne@amd.com>, <ben.levinsky@amd.com>, <dhaval.r.shah@amd.com>, <marex@denx.de>, <robh@kernel.org>, <arnd@arndb.de>, <izhar.ameer.shaikh@amd.com>, <ruanjinjie@huawei.com>, <mathieu.poirier@linaro.org> CC: <linux-kernel@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, Jay Buddhabhatti <jay.buddhabhatti@amd.com> Subject: [PATCH 3/7] drivers: soc: xilinx: add check for platform Date: Fri, 29 Sep 2023 03:55:54 -0700 Message-ID: <20230929105558.11893-4-jay.buddhabhatti@amd.com> In-Reply-To: <20230929105558.11893-1-jay.buddhabhatti@amd.com> References: <20230929105558.11893-1-jay.buddhabhatti@amd.com> MIME-Version: 1.0 Content-Type: text/plain Precedence: bulk
Series	update for versal net platform \| [0/7] update for versal net platform [1/7] firmware: xilinx: Update firmware call interface to support additional arg [2/7] firmware: xilinx: Expand feature check to support all PLM modules [3/7] drivers: soc: xilinx: add check for platform [4/7] firmware: xilinx: Register event manager driver [5/7] drivers: soc: xilinx: Fix error message on SGI registration failure [6/7] firmware: zynqmp: Add support to handle IPI CRC failure [7/7] drivers: soc: xilinx: update maintainer of event manager driver

Commit Message

Buddhabhatti, Jay Sept. 29, 2023, 10:55 a.m. UTC

  Some error event IDs for Versal and Versal NET are different.
Both the platforms should access their respective error event
IDs so use sub_family_code to check for platform and check
error IDs for respective platforms. The family code is passed
via platform data to avoid platform detection again.
Platform data is setup when even driver is registered.

Signed-off-by: Jay Buddhabhatti <jay.buddhabhatti@amd.com>
---
 drivers/firmware/xilinx/zynqmp.c        |  2 +-
 drivers/soc/xilinx/xlnx_event_manager.c | 25 ++++++++++++++++++++-----
 include/linux/firmware/xlnx-zynqmp.h    | 16 ++++++++++++----
 3 files changed, 33 insertions(+), 10 deletions(-)

Comments

Greg KH Sept. 30, 2023, 7:17 a.m. UTC | #1

On Fri, Sep 29, 2023 at 03:55:54AM -0700, Jay Buddhabhatti wrote:
> Some error event IDs for Versal and Versal NET are different.
> Both the platforms should access their respective error event
> IDs so use sub_family_code to check for platform and check
> error IDs for respective platforms. The family code is passed
> via platform data to avoid platform detection again.
> Platform data is setup when even driver is registered.
> 
> Signed-off-by: Jay Buddhabhatti <jay.buddhabhatti@amd.com>
> ---
>  drivers/firmware/xilinx/zynqmp.c        |  2 +-
>  drivers/soc/xilinx/xlnx_event_manager.c | 25 ++++++++++++++++++++-----
>  include/linux/firmware/xlnx-zynqmp.h    | 16 ++++++++++++----
>  3 files changed, 33 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
> index 5f40288f69a9..6583efa9ac48 100644
> --- a/drivers/firmware/xilinx/zynqmp.c
> +++ b/drivers/firmware/xilinx/zynqmp.c
> @@ -2019,7 +2019,7 @@ static int zynqmp_firmware_probe(struct platform_device *pdev)
>  	np = of_find_compatible_node(NULL, NULL, "xlnx,versal");
>  	if (np) {
>  		em_dev = platform_device_register_data(&pdev->dev, "xlnx_event_manager",
> -						       -1, NULL, 0);
> +						       -1, &pm_sub_family_code, 4);
>  		if (IS_ERR(em_dev))
>  			dev_err_probe(&pdev->dev, PTR_ERR(em_dev), "EM register fail with error\n");
>  	}
> diff --git a/drivers/soc/xilinx/xlnx_event_manager.c b/drivers/soc/xilinx/xlnx_event_manager.c
> index 38cfc161a713..8074ded7b39c 100644
> --- a/drivers/soc/xilinx/xlnx_event_manager.c
> +++ b/drivers/soc/xilinx/xlnx_event_manager.c
> @@ -23,6 +23,7 @@ static DEFINE_PER_CPU_READ_MOSTLY(int, cpu_number1);
>  
>  static int virq_sgi;
>  static int event_manager_availability = -EACCES;
> +static u32 pm_sub_family_code;

Why is this not a per-device variable?  Global variables like this for
drivers are almost always wrong.

thanks,

greg k-h

Michal Simek Oct. 2, 2023, 6:09 a.m. UTC | #2

On 9/30/23 09:17, Greg KH wrote:
> On Fri, Sep 29, 2023 at 03:55:54AM -0700, Jay Buddhabhatti wrote:
>> Some error event IDs for Versal and Versal NET are different.
>> Both the platforms should access their respective error event
>> IDs so use sub_family_code to check for platform and check
>> error IDs for respective platforms. The family code is passed
>> via platform data to avoid platform detection again.
>> Platform data is setup when even driver is registered.
>>
>> Signed-off-by: Jay Buddhabhatti <jay.buddhabhatti@amd.com>
>> ---
>>   drivers/firmware/xilinx/zynqmp.c        |  2 +-
>>   drivers/soc/xilinx/xlnx_event_manager.c | 25 ++++++++++++++++++++-----
>>   include/linux/firmware/xlnx-zynqmp.h    | 16 ++++++++++++----
>>   3 files changed, 33 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
>> index 5f40288f69a9..6583efa9ac48 100644
>> --- a/drivers/firmware/xilinx/zynqmp.c
>> +++ b/drivers/firmware/xilinx/zynqmp.c
>> @@ -2019,7 +2019,7 @@ static int zynqmp_firmware_probe(struct platform_device *pdev)
>>   	np = of_find_compatible_node(NULL, NULL, "xlnx,versal");
>>   	if (np) {
>>   		em_dev = platform_device_register_data(&pdev->dev, "xlnx_event_manager",
>> -						       -1, NULL, 0);
>> +						       -1, &pm_sub_family_code, 4);
>>   		if (IS_ERR(em_dev))
>>   			dev_err_probe(&pdev->dev, PTR_ERR(em_dev), "EM register fail with error\n");
>>   	}
>> diff --git a/drivers/soc/xilinx/xlnx_event_manager.c b/drivers/soc/xilinx/xlnx_event_manager.c
>> index 38cfc161a713..8074ded7b39c 100644
>> --- a/drivers/soc/xilinx/xlnx_event_manager.c
>> +++ b/drivers/soc/xilinx/xlnx_event_manager.c
>> @@ -23,6 +23,7 @@ static DEFINE_PER_CPU_READ_MOSTLY(int, cpu_number1);
>>   
>>   static int virq_sgi;
>>   static int event_manager_availability = -EACCES;
>> +static u32 pm_sub_family_code;
> 
> Why is this not a per-device variable?  Global variables like this for
> drivers are almost always wrong.

Not sure I get what you mean by per-device. Can you please elaborate on it?
This is the part of firmware infrastructure and there is only one instance of 
the driver in the system registered from firmware driver itself.

If this should be done in a generic way then there are much more variables which 
should be moved to driver data not just this one.

Thanks,
Michal

Greg KH Oct. 2, 2023, 6:20 a.m. UTC | #3

On Mon, Oct 02, 2023 at 08:09:50AM +0200, Michal Simek wrote:
> 
> 
> On 9/30/23 09:17, Greg KH wrote:
> > On Fri, Sep 29, 2023 at 03:55:54AM -0700, Jay Buddhabhatti wrote:
> > > Some error event IDs for Versal and Versal NET are different.
> > > Both the platforms should access their respective error event
> > > IDs so use sub_family_code to check for platform and check
> > > error IDs for respective platforms. The family code is passed
> > > via platform data to avoid platform detection again.
> > > Platform data is setup when even driver is registered.
> > > 
> > > Signed-off-by: Jay Buddhabhatti <jay.buddhabhatti@amd.com>
> > > ---
> > >   drivers/firmware/xilinx/zynqmp.c        |  2 +-
> > >   drivers/soc/xilinx/xlnx_event_manager.c | 25 ++++++++++++++++++++-----
> > >   include/linux/firmware/xlnx-zynqmp.h    | 16 ++++++++++++----
> > >   3 files changed, 33 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
> > > index 5f40288f69a9..6583efa9ac48 100644
> > > --- a/drivers/firmware/xilinx/zynqmp.c
> > > +++ b/drivers/firmware/xilinx/zynqmp.c
> > > @@ -2019,7 +2019,7 @@ static int zynqmp_firmware_probe(struct platform_device *pdev)
> > >   	np = of_find_compatible_node(NULL, NULL, "xlnx,versal");
> > >   	if (np) {
> > >   		em_dev = platform_device_register_data(&pdev->dev, "xlnx_event_manager",
> > > -						       -1, NULL, 0);
> > > +						       -1, &pm_sub_family_code, 4);
> > >   		if (IS_ERR(em_dev))
> > >   			dev_err_probe(&pdev->dev, PTR_ERR(em_dev), "EM register fail with error\n");
> > >   	}
> > > diff --git a/drivers/soc/xilinx/xlnx_event_manager.c b/drivers/soc/xilinx/xlnx_event_manager.c
> > > index 38cfc161a713..8074ded7b39c 100644
> > > --- a/drivers/soc/xilinx/xlnx_event_manager.c
> > > +++ b/drivers/soc/xilinx/xlnx_event_manager.c
> > > @@ -23,6 +23,7 @@ static DEFINE_PER_CPU_READ_MOSTLY(int, cpu_number1);
> > >   static int virq_sgi;
> > >   static int event_manager_availability = -EACCES;
> > > +static u32 pm_sub_family_code;
> > 
> > Why is this not a per-device variable?  Global variables like this for
> > drivers are almost always wrong.
> 
> Not sure I get what you mean by per-device. Can you please elaborate on it?

It should not be a global variable (i.e. the same for all devices in the
system.)

> This is the part of firmware infrastructure and there is only one instance
> of the driver in the system registered from firmware driver itself.

Then that should be fixed.

> If this should be done in a generic way then there are much more variables
> which should be moved to driver data not just this one.

Agreed, they should all be moved there.

thanks,

greg k-h

Michal Simek Oct. 2, 2023, 8:12 a.m. UTC | #4

On 10/2/23 08:20, Greg KH wrote:
> On Mon, Oct 02, 2023 at 08:09:50AM +0200, Michal Simek wrote:
>>
>>
>> On 9/30/23 09:17, Greg KH wrote:
>>> On Fri, Sep 29, 2023 at 03:55:54AM -0700, Jay Buddhabhatti wrote:
>>>> Some error event IDs for Versal and Versal NET are different.
>>>> Both the platforms should access their respective error event
>>>> IDs so use sub_family_code to check for platform and check
>>>> error IDs for respective platforms. The family code is passed
>>>> via platform data to avoid platform detection again.
>>>> Platform data is setup when even driver is registered.
>>>>
>>>> Signed-off-by: Jay Buddhabhatti <jay.buddhabhatti@amd.com>
>>>> ---
>>>>    drivers/firmware/xilinx/zynqmp.c        |  2 +-
>>>>    drivers/soc/xilinx/xlnx_event_manager.c | 25 ++++++++++++++++++++-----
>>>>    include/linux/firmware/xlnx-zynqmp.h    | 16 ++++++++++++----
>>>>    3 files changed, 33 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
>>>> index 5f40288f69a9..6583efa9ac48 100644
>>>> --- a/drivers/firmware/xilinx/zynqmp.c
>>>> +++ b/drivers/firmware/xilinx/zynqmp.c
>>>> @@ -2019,7 +2019,7 @@ static int zynqmp_firmware_probe(struct platform_device *pdev)
>>>>    	np = of_find_compatible_node(NULL, NULL, "xlnx,versal");
>>>>    	if (np) {
>>>>    		em_dev = platform_device_register_data(&pdev->dev, "xlnx_event_manager",
>>>> -						       -1, NULL, 0);
>>>> +						       -1, &pm_sub_family_code, 4);
>>>>    		if (IS_ERR(em_dev))
>>>>    			dev_err_probe(&pdev->dev, PTR_ERR(em_dev), "EM register fail with error\n");
>>>>    	}
>>>> diff --git a/drivers/soc/xilinx/xlnx_event_manager.c b/drivers/soc/xilinx/xlnx_event_manager.c
>>>> index 38cfc161a713..8074ded7b39c 100644
>>>> --- a/drivers/soc/xilinx/xlnx_event_manager.c
>>>> +++ b/drivers/soc/xilinx/xlnx_event_manager.c
>>>> @@ -23,6 +23,7 @@ static DEFINE_PER_CPU_READ_MOSTLY(int, cpu_number1);
>>>>    static int virq_sgi;
>>>>    static int event_manager_availability = -EACCES;
>>>> +static u32 pm_sub_family_code;
>>>
>>> Why is this not a per-device variable?  Global variables like this for
>>> drivers are almost always wrong.
>>
>> Not sure I get what you mean by per-device. Can you please elaborate on it?
> 
> It should not be a global variable (i.e. the same for all devices in the
> system.)

You are not changing silicon when you run OS. It means strictly speaking you are 
not changing sub family silicon code.


>> This is the part of firmware infrastructure and there is only one instance
>> of the driver in the system registered from firmware driver itself.
> 
> Then that should be fixed.
> 
>> If this should be done in a generic way then there are much more variables
>> which should be moved to driver data not just this one.
> 
> Agreed, they should all be moved there.

Let us to take a look at what can be done to try to remove all of them.

Jay: Can you please remove this patch from this series and let's deal with it 
separately?

Greg: Are you fine with others?

Thanks,
Michal

diff mbox series

Patch

diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
index 5f40288f69a9..6583efa9ac48 100644
--- a/drivers/firmware/xilinx/zynqmp.c
+++ b/drivers/firmware/xilinx/zynqmp.c
@@ -2019,7 +2019,7 @@  static int zynqmp_firmware_probe(struct platform_device *pdev)
 	np = of_find_compatible_node(NULL, NULL, "xlnx,versal");
 	if (np) {
 		em_dev = platform_device_register_data(&pdev->dev, "xlnx_event_manager",
-						       -1, NULL, 0);
+						       -1, &pm_sub_family_code, 4);
 		if (IS_ERR(em_dev))
 			dev_err_probe(&pdev->dev, PTR_ERR(em_dev), "EM register fail with error\n");
 	}
diff --git a/drivers/soc/xilinx/xlnx_event_manager.c b/drivers/soc/xilinx/xlnx_event_manager.c
index 38cfc161a713..8074ded7b39c 100644
--- a/drivers/soc/xilinx/xlnx_event_manager.c
+++ b/drivers/soc/xilinx/xlnx_event_manager.c
@@ -23,6 +23,7 @@  static DEFINE_PER_CPU_READ_MOSTLY(int, cpu_number1);
 
 static int virq_sgi;
 static int event_manager_availability = -EACCES;
+static u32 pm_sub_family_code;
 
 /* SGI number used for Event management driver */
 #define XLNX_EVENT_SGI_NUM	(15)
@@ -77,11 +78,22 @@  struct registered_event_data {
 
 static bool xlnx_is_error_event(const u32 node_id)
 {
-	if (node_id == EVENT_ERROR_PMC_ERR1 ||
-	    node_id == EVENT_ERROR_PMC_ERR2 ||
-	    node_id == EVENT_ERROR_PSM_ERR1 ||
-	    node_id == EVENT_ERROR_PSM_ERR2)
-		return true;
+	if (pm_sub_family_code == VERSAL_SUB_FAMILY_CODE) {
+		if (node_id == VERSAL_EVENT_ERROR_PMC_ERR1 ||
+		    node_id == VERSAL_EVENT_ERROR_PMC_ERR2 ||
+		    node_id == VERSAL_EVENT_ERROR_PSM_ERR1 ||
+		    node_id == VERSAL_EVENT_ERROR_PSM_ERR2)
+			return true;
+	} else {
+		if (node_id == VERSAL_NET_EVENT_ERROR_PMC_ERR1 ||
+		    node_id == VERSAL_NET_EVENT_ERROR_PMC_ERR2 ||
+		    node_id == VERSAL_NET_EVENT_ERROR_PMC_ERR3 ||
+		    node_id == VERSAL_NET_EVENT_ERROR_PSM_ERR1 ||
+		    node_id == VERSAL_NET_EVENT_ERROR_PSM_ERR2 ||
+		    node_id == VERSAL_NET_EVENT_ERROR_PSM_ERR3 ||
+		    node_id == VERSAL_NET_EVENT_ERROR_PSM_ERR4)
+			return true;
+	}
 
 	return false;
 }
@@ -625,6 +637,7 @@  static void xlnx_event_cleanup_sgi(struct platform_device *pdev)
 static int xlnx_event_manager_probe(struct platform_device *pdev)
 {
 	int ret;
+	u32 *platform_data;
 
 	ret = zynqmp_pm_feature(PM_REGISTER_NOTIFIER);
 	if (ret < 0) {
@@ -659,6 +672,8 @@  static int xlnx_event_manager_probe(struct platform_device *pdev)
 	}
 
 	event_manager_availability = 0;
+	platform_data = (u32 *)dev_get_platdata((const struct device *)&pdev->dev);
+	pm_sub_family_code = *platform_data;
 
 	dev_info(&pdev->dev, "SGI %d Registered over TF-A\n", sgi_num);
 	dev_info(&pdev->dev, "Xilinx Event Management driver probed\n");
diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
index 86fb73269d95..5d4c81790091 100644
--- a/include/linux/firmware/xlnx-zynqmp.h
+++ b/include/linux/firmware/xlnx-zynqmp.h
@@ -95,10 +95,18 @@ 
 /*
  * Node IDs for the Error Events.
  */
-#define EVENT_ERROR_PMC_ERR1	(0x28100000U)
-#define EVENT_ERROR_PMC_ERR2	(0x28104000U)
-#define EVENT_ERROR_PSM_ERR1	(0x28108000U)
-#define EVENT_ERROR_PSM_ERR2	(0x2810C000U)
+#define VERSAL_EVENT_ERROR_PMC_ERR1	(0x28100000U)
+#define VERSAL_EVENT_ERROR_PMC_ERR2	(0x28104000U)
+#define VERSAL_EVENT_ERROR_PSM_ERR1	(0x28108000U)
+#define VERSAL_EVENT_ERROR_PSM_ERR2	(0x2810C000U)
+
+#define VERSAL_NET_EVENT_ERROR_PMC_ERR1	(0x28100000U)
+#define VERSAL_NET_EVENT_ERROR_PMC_ERR2	(0x28104000U)
+#define VERSAL_NET_EVENT_ERROR_PMC_ERR3	(0x28108000U)
+#define VERSAL_NET_EVENT_ERROR_PSM_ERR1	(0x2810C000U)
+#define VERSAL_NET_EVENT_ERROR_PSM_ERR2	(0x28110000U)
+#define VERSAL_NET_EVENT_ERROR_PSM_ERR3	(0x28114000U)
+#define VERSAL_NET_EVENT_ERROR_PSM_ERR4	(0x28118000U)
 
 /* ZynqMP SD tap delay tuning */
 #define SD_ITAPDLY	0xFF180314