Messages in this thread | | | Date | Mon, 2 Oct 2023 10:12:38 +0200 | Subject | Re: [PATCH 3/7] drivers: soc: xilinx: add check for platform | From | Michal Simek <> |
| |
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
| |