> Subject: RE: [PATCH 2/4] firmware: imx: add resource management api > > > From: Peng Fan <peng....@nxp.com> > > Sent: Tuesday, June 2, 2020 12:51 PM > > > > > > > From: Peng Fan <peng....@nxp.com> > > > > Sent: Monday, June 1, 2020 8:40 PM > > > > > > > > > > > From: Peng Fan <peng....@nxp.com> > > > > > > Sent: Friday, April 24, 2020 9:12 AM > > > > > > > > > > > > > > > From: Peng Fan <peng....@nxp.com> > > > > > > > > Sent: Thursday, April 23, 2020 6:57 PM > > > > > > > > > > From: Peng Fan <peng....@nxp.com> > > > > > > > > > > Sent: Thursday, April 23, 2020 3:00 PM > > > > > > > > > > > > > > > > > > > > Add resource management API, when we have multiple > > > > > > > > > > partition running together, resources not owned to > > > > > > > > > > current partition should not be > > > > > > > used. > > > > > > > > > > > > > > > > > > > > Reviewed-by: Leonard Crestez <leonard.cres...@nxp.com> > > > > > > > > > > Signed-off-by: Peng Fan <peng....@nxp.com> > > > > > > > > > > > > > > > > > > Right now I'm still not quite understand if we really need > > > > > > > > > this. > > > > > > > > > As current resource is bound to power domains, if it's > > > > > > > > > not owned by one specific SW execution environment, > > > > > > > > > power on will also > > > > fail. > > > > > > > > > Can you clarify if any exceptions? > > > > > > > > > > > > > > > > There will be lots of failures when boot Linux domu if no check. > > > > > > > > > > > > > > > > > > > > > > What kind of features did you mean? > > > > > > > Could you clarify a bit more? I'd like to understand this issue > better. > > > > > > > > > > > > When supporting hypervisor with dual Linux running, 1st Linux > > > > > > and the 2nd Linux will have their own dedicated resources. > > > > > > > > > > > > If no resource check, that means 1st/2nd Linux will register > > > > > > all the resources, then both will see fail logs when register > > > > > > resources not owned to > > > > > itself. > > > > > > > > > > > > Same to partitioned m4 case. > > > > > > > > > > > > Would it be better without the failure log? > > > > > > > > > > > > > > > > Is it power up failure? > > > > > If yes, it's expected because we actually use power up to check > > > > > if resources are owned by this partition. It functions the same > > > > > as calling resource check API. > > > > > That's current design. > > > > > > > > > > Or it's other failures? Log? > > > > Sorry for long late reply. > > > > > > > > Part of my XEN DomU log, there are lots of failure log. I think > > > > better not have such logs by add a resource own check. > > > > > > Those error logs are intended. > > > Resource owner check actually behaves the same as power up. > > > > If resource is not owned, it will not register the power domain. > > > > Without resource own check, it will continue register the power domain > > and hook it into genpd list. > > > > That's the intended behavior to save the resource owner checking as it's very > time cost to send SCU cmd for each domain during booting which benefits > nothing except not exposing them in genpd list. > > > I prefer we not expose the power domain not owned to current partition > > and keep the err msg for people to easy see where it goes wrong. > > If we really want to to do, I wonder probably another better approach is > trying to re-use the partition Information built by bootloader as uboot > already > did that one time, not necessary to re-do It again for kernel as it's time > cost. > e.g. introduce a resource partition property in dt and initialized and passed > by > bootloarder to kernel to use later.
This will not work for hypervisor based VM runtime partition create and resource assignment. > Then we can still save those huge number of resource owner check CMDs. So we have to live with them even I only need one SDHC for a VM? With so many clk-scu entries and power domain entries there. Thanks, Peng. > > Regards > Aisheng > > > > > Regards, > > Peng. > > > So not quite necessary to add a double check. > > > If we're concerning about the error log, we can change it to dev_dbg(). > > > > > > BTW, as resource management will be needed by seco drivers later, So > > > I will continue to review the patch. > > > > > > Regards > > > Aisheng > > > > > > > > > > > [ 2.034774] imx6q-pcie 5f000000.pcie: IO > 0x6ff80000..0x6ff8ffff > > -> > > > > 0x00000000 > > > > [ 2.034801] imx6q-pcie 5f000000.pcie: MEM > > 0x60000000..0x6fefffff > > > -> > > > > 0x60000000 > > > > [ 2.035072] sdhc1: failed to power up resource 249 ret -13 > > > > [ 2.035619] sdhc2: failed to power up resource 250 ret -13 > > > > [ 2.036126] enet0: failed to power up resource 251 ret -13 > > > > [ 2.036584] enet1: failed to power up resource 252 ret -13 > > > > [ 2.037040] mlb0: failed to power up resource 253 ret -13 > > > > [ 2.037495] nand: failed to power up resource 265 ret -13 > > > > [ 2.037951] nand: failed to power up resource 265 ret -13 > > > > [ 2.038416] pwm0: failed to power up resource 191 ret -13 > > > > [ 2.038868] pwm1: failed to power up resource 192 ret -13 > > > > [ 2.039320] pwm2: failed to power up resource 193 ret -13 > > > > [ 2.039786] pwm3: failed to power up resource 194 ret -13 > > > > [ 2.040239] pwm4: failed to power up resource 195 ret -13 > > > > [ 2.040692] pwm5: failed to power up resource 196 ret -13 > > > > [ 2.041142] pwm6: failed to power up resource 197 ret -13 > > > > [ 2.041596] pwm7: failed to power up resource 198 ret -13 > > > > [ 2.042073] amix: failed to power up resource 458 ret -13 > > > > [ 2.042558] lpspi0: failed to power up resource 53 ret -13 > > > > [ 2.043033] lpspi1: failed to power up resource 54 ret -13 > > > > [ 2.043501] lpspi2: failed to power up resource 55 ret -13 > > > > [ 2.043992] lpspi3: failed to power up resource 56 ret -13 > > > > [ 2.044460] lpuart0: failed to power up resource 57 ret -13 > > > > [ 2.044935] lpuart2: failed to power up resource 59 ret -13 > > > > [ 2.045409] lpuart3: failed to power up resource 60 ret -13 > > > > [ 2.055014] sim0: failed to power up resource 62 ret -13 > > > > [ 2.055510] adc0: failed to power up resource 101 ret -13 > > > > [ 2.056467] lpi2c0: failed to power up resource 96 ret -13 > > > > [ 2.056946] lpi2c1: failed to power up resource 97 ret -13 > > > > [ 2.057424] lpi2c2: failed to power up resource 98 ret -13 > > > > [ 2.057898] lpi2c3: failed to power up resource 99 ret -13 > > > > [ 2.058371] can0: failed to power up resource 105 ret -13 > > > > [ 2.059289] can1: failed to power up resource 106 ret -13 > > > > [ 2.059801] can2: failed to power up resource 107 ret -13 > > > > [ 2.060281] nand: failed to power up resource 265 ret -13 > > > > [ 2.062764] dpu-core 56180000.dpu: driver probed > > > > > > > > Thanks, > > > > Peng. > > > > > > > > > > > > > > Regards > > > > > Aisheng > > > > > > > > > > > Thanks, > > > > > > Peng. > > > > > > > > > > > > > > > > > > > > > > > > > > Regards > > > > > > > Aisheng > > > > > > > > > > > > > > > Thanks, > > > > > > > > Peng. > > > > > > > > > > > > > > > > > > > > > > > > > > Regards > > > > > > > > > Aisheng > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > drivers/firmware/imx/Makefile | 2 +- > > > > > > > > > > drivers/firmware/imx/rm.c | 40 > > > > > > > +++++++++++++++++++++ > > > > > > > > > > include/linux/firmware/imx/sci.h | 1 + > > > > > > > > > > include/linux/firmware/imx/svc/rm.h | 69 > > > > > > > > > > +++++++++++++++++++++++++++++++++++++ > > > > > > > > > > 4 files changed, 111 insertions(+), 1 deletion(-) > > > > > > > > > > create mode > > > > > > > > > > 100644 drivers/firmware/imx/rm.c create mode 100644 > > > > > > > > > > include/linux/firmware/imx/svc/rm.h > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/firmware/imx/Makefile > > > > > > > > > > b/drivers/firmware/imx/Makefile index > > > > > > > > > > 08bc9ddfbdfb..17ea3613e142 > > > > > > > > > > 100644 > > > > > > > > > > --- a/drivers/firmware/imx/Makefile > > > > > > > > > > +++ b/drivers/firmware/imx/Makefile > > > > > > > > > > @@ -1,4 +1,4 @@ > > > > > > > > > > # SPDX-License-Identifier: GPL-2.0 > > > > > > > > > > obj-$(CONFIG_IMX_DSP) += imx-dsp.o > > > > > > > > > > -obj-$(CONFIG_IMX_SCU) += imx-scu.o misc.o > > imx-scu-irq.o > > > > > > > > > > +obj-$(CONFIG_IMX_SCU) += imx-scu.o misc.o > > > imx-scu-irq.o > > > > > rm.o > > > > > > > > > > obj-$(CONFIG_IMX_SCU_PD) += scu-pd.o > > > > > > > > > > diff --git a/drivers/firmware/imx/rm.c > > > > > > > > > > b/drivers/firmware/imx/rm.c new file mode 100644 index > > > > > > > > > > 000000000000..7b0334de5486 > > > > > > > > > > --- /dev/null > > > > > > > > > > +++ b/drivers/firmware/imx/rm.c > > > > > > > > > > @@ -0,0 +1,40 @@ > > > > > > > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > > > > > > > +/* > > > > > > > > > > + * Copyright 2020 NXP > > > > > > > > > > + * > > > > > > > > > > + * File containing client-side RPC functions for the RM > service. > > > > > > > > > > +These > > > > > > > > > > + * function are ported to clients that communicate to the > > > > > > > > > > SC. > > > > > > > > > > + */ > > > > > > > > > > + > > > > > > > > > > +#include <linux/firmware/imx/svc/rm.h> > > > > > > > > > > + > > > > > > > > > > +struct imx_sc_msg_rm_rsrc_owned { > > > > > > > > > > + struct imx_sc_rpc_msg hdr; > > > > > > > > > > + u16 resource; > > > > > > > > > > +} __packed __aligned(4); > > > > > > > > > > + > > > > > > > > > > +/* > > > > > > > > > > + * This function check @resource is owned by current > > > > > > > > > > +partition or not > > > > > > > > > > + * > > > > > > > > > > + * @param[in] ipc IPC handle > > > > > > > > > > + * @param[in] resource resource the control is > > > > > associated > > > > > > > with > > > > > > > > > > + * > > > > > > > > > > + * @return Returns 0 for success and < 0 for errors. > > > > > > > > > > + */ > > > > > > > > > > +bool imx_sc_rm_is_resource_owned(struct imx_sc_ipc > > > > > > > > > > +*ipc, > > > > > > > > > > +u16 > > > > > > > > > > +resource) { > > > > > > > > > > + struct imx_sc_msg_rm_rsrc_owned msg; > > > > > > > > > > + struct imx_sc_rpc_msg *hdr = &msg.hdr; > > > > > > > > > > + > > > > > > > > > > + hdr->ver = IMX_SC_RPC_VERSION; > > > > > > > > > > + hdr->svc = IMX_SC_RPC_SVC_RM; > > > > > > > > > > + hdr->func = IMX_SC_RM_FUNC_IS_RESOURCE_OWNED; > > > > > > > > > > + hdr->size = 2; > > > > > > > > > > + > > > > > > > > > > + msg.resource = resource; > > > > > > > > > > + > > > > > > > > > > + imx_scu_call_rpc(ipc, &msg, true); > > > > > > > > > > + > > > > > > > > > > + return hdr->func; > > > > > > > > > > +} > > > > > > > > > > +EXPORT_SYMBOL(imx_sc_rm_is_resource_owned); > > > > > > > > > > diff --git a/include/linux/firmware/imx/sci.h > > > > > > > > > > b/include/linux/firmware/imx/sci.h > > > > > > > > > > index 17ba4e405129..b5c5a56f29be 100644 > > > > > > > > > > --- a/include/linux/firmware/imx/sci.h > > > > > > > > > > +++ b/include/linux/firmware/imx/sci.h > > > > > > > > > > @@ -15,6 +15,7 @@ > > > > > > > > > > > > > > > > > > > > #include <linux/firmware/imx/svc/misc.h> #include > > > > > > > > > > <linux/firmware/imx/svc/pm.h> > > > > > > > > > > +#include <linux/firmware/imx/svc/rm.h> > > > > > > > > > > > > > > > > > > > > int imx_scu_enable_general_irq_channel(struct device > > > > > > > > > > *dev); int imx_scu_irq_register_notifier(struct > > > > > > > > > > notifier_block *nb); diff --git > > > > > > > > > > a/include/linux/firmware/imx/svc/rm.h > > > > > > > > > > b/include/linux/firmware/imx/svc/rm.h > > > > > > > > > > new file mode 100644 > > > > > > > > > > index 000000000000..fc6ea62d9d0e > > > > > > > > > > --- /dev/null > > > > > > > > > > +++ b/include/linux/firmware/imx/svc/rm.h > > > > > > > > > > @@ -0,0 +1,69 @@ > > > > > > > > > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > > > > > > > > > +/* > > > > > > > > > > + * Copyright (C) 2016 Freescale Semiconductor, Inc. > > > > > > > > > > + * Copyright 2017-2019 NXP > > > > > > > > > > + * > > > > > > > > > > + * Header file containing the public API for the > > > > > > > > > > +System Controller > > > > > > > > > > +(SC) > > > > > > > > > > + * Power Management (PM) function. This includes > > > > > > > > > > +functions for power state > > > > > > > > > > + * control, clock control, reset control, and wake-up > > > > > > > > > > +event > > > control. > > > > > > > > > > + * > > > > > > > > > > + * RM_SVC (SVC) Resource Management Service > > > > > > > > > > + * > > > > > > > > > > + * Module for the Resource Management (RM) service. > > > > > > > > > > + */ > > > > > > > > > > + > > > > > > > > > > +#ifndef _SC_RM_API_H > > > > > > > > > > +#define _SC_RM_API_H > > > > > > > > > > + > > > > > > > > > > +#include <linux/firmware/imx/sci.h> > > > > > > > > > > + > > > > > > > > > > +/* > > > > > > > > > > + * This type is used to indicate RPC RM function calls. > > > > > > > > > > + */ > > > > > > > > > > +enum imx_sc_rm_func { > > > > > > > > > > + IMX_SC_RM_FUNC_UNKNOWN = 0, > > > > > > > > > > + IMX_SC_RM_FUNC_PARTITION_ALLOC = 1, > > > > > > > > > > + IMX_SC_RM_FUNC_SET_CONFIDENTIAL = 31, > > > > > > > > > > + IMX_SC_RM_FUNC_PARTITION_FREE = 2, > > > > > > > > > > + IMX_SC_RM_FUNC_GET_DID = 26, > > > > > > > > > > + IMX_SC_RM_FUNC_PARTITION_STATIC = 3, > > > > > > > > > > + IMX_SC_RM_FUNC_PARTITION_LOCK = 4, > > > > > > > > > > + IMX_SC_RM_FUNC_GET_PARTITION = 5, > > > > > > > > > > + IMX_SC_RM_FUNC_SET_PARENT = 6, > > > > > > > > > > + IMX_SC_RM_FUNC_MOVE_ALL = 7, > > > > > > > > > > + IMX_SC_RM_FUNC_ASSIGN_RESOURCE = 8, > > > > > > > > > > + IMX_SC_RM_FUNC_SET_RESOURCE_MOVABLE = 9, > > > > > > > > > > + IMX_SC_RM_FUNC_SET_SUBSYS_RSRC_MOVABLE = 28, > > > > > > > > > > + IMX_SC_RM_FUNC_SET_MASTER_ATTRIBUTES = 10, > > > > > > > > > > + IMX_SC_RM_FUNC_SET_MASTER_SID = 11, > > > > > > > > > > + IMX_SC_RM_FUNC_SET_PERIPHERAL_PERMISSIONS = > 12, > > > > > > > > > > + IMX_SC_RM_FUNC_IS_RESOURCE_OWNED = 13, > > > > > > > > > > + IMX_SC_RM_FUNC_GET_RESOURCE_OWNER = 33, > > > > > > > > > > + IMX_SC_RM_FUNC_IS_RESOURCE_MASTER = 14, > > > > > > > > > > + IMX_SC_RM_FUNC_IS_RESOURCE_PERIPHERAL = 15, > > > > > > > > > > + IMX_SC_RM_FUNC_GET_RESOURCE_INFO = 16, > > > > > > > > > > + IMX_SC_RM_FUNC_MEMREG_ALLOC = 17, > > > > > > > > > > + IMX_SC_RM_FUNC_MEMREG_SPLIT = 29, > > > > > > > > > > + IMX_SC_RM_FUNC_MEMREG_FRAG = 32, > > > > > > > > > > + IMX_SC_RM_FUNC_MEMREG_FREE = 18, > > > > > > > > > > + IMX_SC_RM_FUNC_FIND_MEMREG = 30, > > > > > > > > > > + IMX_SC_RM_FUNC_ASSIGN_MEMREG = 19, > > > > > > > > > > + IMX_SC_RM_FUNC_SET_MEMREG_PERMISSIONS = 20, > > > > > > > > > > + IMX_SC_RM_FUNC_IS_MEMREG_OWNED = 21, > > > > > > > > > > + IMX_SC_RM_FUNC_GET_MEMREG_INFO = 22, > > > > > > > > > > + IMX_SC_RM_FUNC_ASSIGN_PAD = 23, > > > > > > > > > > + IMX_SC_RM_FUNC_SET_PAD_MOVABLE = 24, > > > > > > > > > > + IMX_SC_RM_FUNC_IS_PAD_OWNED = 25, > > > > > > > > > > + IMX_SC_RM_FUNC_DUMP = 27, }; > > > > > > > > > > + > > > > > > > > > > +#if IS_ENABLED(CONFIG_IMX_SCU) bool > > > > > > > > > > +imx_sc_rm_is_resource_owned(struct imx_sc_ipc *ipc, > > > > > > > > > > +u16 resource); #else static inline bool > > > > > > > > > > +imx_sc_rm_is_resource_owned(struct imx_sc_ipc *ipc, > > > > > > > > > > +u16 > > > > > resource) { > > > > > > > > > > + return true; > > > > > > > > > > +} > > > > > > > > > > +#endif > > > > > > > > > > +#endif > > > > > > > > > > -- > > > > > > > > > > 2.16.4