Message-ID: <AM6PR04MB496662FD230F76CA2492438E808B0@AM6PR04MB4966.eurprd04.prod.outlook.com>
Date:   Tue, 2 Jun 2020 09:07:57 +0000
From:   Aisheng Dong <aisheng.dong@....com>
To:     Peng Fan <peng.fan@....com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>
CC:     "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "festevam@...il.com" <festevam@...il.com>,
        "linux@...pel-privat.de" <linux@...pel-privat.de>,
        Leonard Crestez <leonard.crestez@....com>,
        Daniel Baluta <daniel.baluta@....com>,
        Joakim Zhang <qiangqing.zhang@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH 2/4] firmware: imx: add resource management api

> From: Peng Fan <peng.fan@....com>
> Sent: Tuesday, June 2, 2020 3:48 PM
> >
> > > From: Peng Fan <peng.fan@....com>
> > > Sent: Tuesday, June 2, 2020 12:51 PM
> > > >
> > > > > From: Peng Fan <peng.fan@....com>
> > > > > Sent: Monday, June 1, 2020 8:40 PM
> > > > > >
> > > > > > > From: Peng Fan <peng.fan@....com>
> > > > > > > Sent: Friday, April 24, 2020 9:12 AM
> > > > > > > >
> > > > > > > > > From: Peng Fan <peng.fan@....com>
> > > > > > > > > Sent: Thursday, April 23, 2020 6:57 PM
> > > > > > > > > > > From: Peng Fan <peng.fan@....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.crestez@....com>
> > > > > > > > > > > Signed-off-by: Peng Fan <peng.fan@....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.
> 

For VM case, can't we define them in DT if it's statically defined?

> > 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.

No, clk-scu register will fail automatically if not owned by this partition.

Regards
Aisheng

> 
> 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

Powered by blists - more mailing lists