RE: [PATCH 2/4] firmware: imx: add resource management api

From: Aisheng Dong
Date: Tue Jun 02 2020 - 03:28:33 EST

Next message: Daniel Wagner: "[ANNOUNCE] 4.4.225-rt198"
Previous message: Niklas Schnelle: "Re: [PATCH 1/1] s390/pci: Log new handle in clp_disable_fh()"
In reply to: Peng Fan: "RE: [PATCH 2/4] firmware: imx: add resource management api"
Next in thread: Peng Fan: "RE: [PATCH 2/4] firmware: imx: add resource management api"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> From: Peng Fan <peng.fan@xxxxxxx>
> Sent: Tuesday, June 2, 2020 12:51 PM
> >
> > > From: Peng Fan <peng.fan@xxxxxxx>
> > > Sent: Monday, June 1, 2020 8:40 PM
> > > >
> > > > > From: Peng Fan <peng.fan@xxxxxxx>
> > > > > Sent: Friday, April 24, 2020 9:12 AM
> > > > > >
> > > > > > > From: Peng Fan <peng.fan@xxxxxxx>
> > > > > > > Sent: Thursday, April 23, 2020 6:57 PM
> > > > > > > > > From: Peng Fan <peng.fan@xxxxxxx>
> > > > > > > > > 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.crestez@xxxxxxx>
> > > > > > > > > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > > > > > > >
> > > > > > > > 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.
Then we can still save those huge number of resource owner check CMDs.

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

Next message: Daniel Wagner: "[ANNOUNCE] 4.4.225-rt198"
Previous message: Niklas Schnelle: "Re: [PATCH 1/1] s390/pci: Log new handle in clp_disable_fh()"
In reply to: Peng Fan: "RE: [PATCH 2/4] firmware: imx: add resource management api"
Next in thread: Peng Fan: "RE: [PATCH 2/4] firmware: imx: add resource management api"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]