RE: [PATCH V2 1/2] soc: imx: Add SCU SoC info driver support
Hi, Leonard > -Original Message- > From: Leonard Crestez > Sent: Wednesday, May 15, 2019 6:05 PM > To: Anson Huang > Cc: catalin.mari...@arm.com; will.dea...@arm.com; > shawn...@kernel.org; s.ha...@pengutronix.de; ker...@pengutronix.de; > feste...@gmail.com; agr...@kernel.org; maxime.rip...@bootlin.com; > o...@lixom.net; horms+rene...@verge.net.au; > ja...@amarulasolutions.com; bjorn.anders...@linaro.org; > marc.w.gonza...@free.fr; dingu...@kernel.org; > enric.balle...@collabora.com; l.st...@pengutronix.de; Abel Vesa > ; r...@kernel.org; linux-arm- > ker...@lists.infradead.org; linux-kernel@vger.kernel.org; dl-linux-imx > ; Aisheng Dong > Subject: Re: [PATCH V2 1/2] soc: imx: Add SCU SoC info driver support > > On 15.05.2019 11:32, Anson Huang wrote: > > Add i.MX SCU SoC info driver to support i.MX8QXP SoC, introduce driver > > dependency into Kconfig as CONFIG_IMX_SCU must be selected to support > > i.MX SCU SoC driver, also need to use platform driver model to make > > sure IMX_SCU driver is probed before i.MX SCU SoC driver. > > > > With this patch, SoC info can be read from sysfs: > > > + id = of_match_node(imx_scu_soc_match, root); > > + if (!id) { > > + of_node_put(root); > > + return -ENODEV; > > + } > > Perhaps this check should be moved from imx_scu_soc_probe to > imx_scu_soc_init? As far as I can tell this "probe" function will be attempted > on all SOCs (even non-imx). Better to check if we're on a SCU-based soc early > and avoid temporary allocations. Make sense, I will move this check block into imx_scu_soc_init() function and ONLY register platform driver when check passed. > > > +module_init(imx_scu_soc_init); > > +module_exit(imx_scu_soc_exit); > > Please don't make this a module OK, will fix it in V3. Thanks, Anson. > > -- > Regards, > Leonard
RE: [PATCH V2 1/2] soc: imx: Add SCU SoC info driver support
> -Original Message- > From: Daniel Baluta [mailto:daniel.bal...@gmail.com] > Sent: Wednesday, May 15, 2019 7:47 PM > To: Anson Huang > Cc: catalin.mari...@arm.com; will.dea...@arm.com; > shawn...@kernel.org; s.ha...@pengutronix.de; ker...@pengutronix.de; > feste...@gmail.com; agr...@kernel.org; maxime.rip...@bootlin.com; > o...@lixom.net; horms+rene...@verge.net.au; > ja...@amarulasolutions.com; bjorn.anders...@linaro.org; Leonard Crestez > ; marc.w.gonza...@free.fr; > dingu...@kernel.org; enric.balle...@collabora.com; > l.st...@pengutronix.de; Abel Vesa ; r...@kernel.org; > linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; dl-linux- > imx > Subject: Re: [PATCH V2 1/2] soc: imx: Add SCU SoC info driver support > > Hi Anson, > > Since you are going to send a new version for this please consider my > comment inline. > > > > > +static u32 imx8qxp_soc_revision(void) { > > + struct imx_sc_msg_misc_get_soc_id msg; > > + struct imx_sc_rpc_msg *hdr = &msg.hdr; > > + u32 rev = 0; > > No need to initialize this here. > > > + int ret; > > + > > + hdr->ver = IMX_SC_RPC_VERSION; > > + hdr->svc = IMX_SC_RPC_SVC_MISC; > > + hdr->func = IMX_SC_MISC_FUNC_GET_CONTROL; > > + hdr->size = 3; > > + > > + msg.data.send.control = IMX_SC_C_ID; > > + msg.data.send.resource = IMX_SC_R_SYSTEM; > > + > > + ret = imx_scu_call_rpc(soc_ipc_handle, &msg, true); > > + if (ret) { > > + dev_err(&imx_scu_soc_pdev->dev, > > + "get soc info failed, ret %d\n", ret); > > + /* return 0 means getting revision failed */ > > Just return 0 here. No need for rev. OK, thanks. Anson. > > + return rev; > > + } > > +
Re: [PATCH V2 1/2] soc: imx: Add SCU SoC info driver support
Hi Anson, Since you are going to send a new version for this please consider my comment inline. > +static u32 imx8qxp_soc_revision(void) > +{ > + struct imx_sc_msg_misc_get_soc_id msg; > + struct imx_sc_rpc_msg *hdr = &msg.hdr; > + u32 rev = 0; No need to initialize this here. > + int ret; > + > + hdr->ver = IMX_SC_RPC_VERSION; > + hdr->svc = IMX_SC_RPC_SVC_MISC; > + hdr->func = IMX_SC_MISC_FUNC_GET_CONTROL; > + hdr->size = 3; > + > + msg.data.send.control = IMX_SC_C_ID; > + msg.data.send.resource = IMX_SC_R_SYSTEM; > + > + ret = imx_scu_call_rpc(soc_ipc_handle, &msg, true); > + if (ret) { > + dev_err(&imx_scu_soc_pdev->dev, > + "get soc info failed, ret %d\n", ret); > + /* return 0 means getting revision failed */ Just return 0 here. No need for rev. > + return rev; > + } > +
Re: [PATCH V2 1/2] soc: imx: Add SCU SoC info driver support
On 15.05.2019 11:32, Anson Huang wrote: > Add i.MX SCU SoC info driver to support i.MX8QXP SoC, introduce > driver dependency into Kconfig as CONFIG_IMX_SCU must be > selected to support i.MX SCU SoC driver, also need to use > platform driver model to make sure IMX_SCU driver is probed > before i.MX SCU SoC driver. > > With this patch, SoC info can be read from sysfs: > + id = of_match_node(imx_scu_soc_match, root); > + if (!id) { > + of_node_put(root); > + return -ENODEV; > + } Perhaps this check should be moved from imx_scu_soc_probe to imx_scu_soc_init? As far as I can tell this "probe" function will be attempted on all SOCs (even non-imx). Better to check if we're on a SCU-based soc early and avoid temporary allocations. > +module_init(imx_scu_soc_init); > +module_exit(imx_scu_soc_exit); Please don't make this a module -- Regards, Leonard