| Message ID | 20230929105558.11893-4-jay.buddhabhatti@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | update for versal net platform | expand |
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
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
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
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 --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
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(-)