Re: [PATCH 2/2] remoteproc: imx_rproc: Switch iMX8MN/MP from SMCCC to MMIO

[Marek Vasut <marex@denx.de>]

On 7/17/23 19:10, Mathieu Poirier wrote:
> Good morning,
> 
> On Sat, Jul 08, 2023 at 01:26:26AM +0200, Marek Vasut wrote:
>> The MX8M CM7 boot via SMC call is problematic, since not all versions
>> of ATF support this interface. Extend the MMIO support so it can boot
>> the CM7 on MX8MN/MP instead and discern the two alternatives using DT
>> compatible strings.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: Bjorn Andersson <andersson@kernel.org>
>> Cc: Conor Dooley <conor+dt@kernel.org>
>> Cc: Fabio Estevam <festevam@gmail.com>
>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Cc: NXP Linux Team <linux-imx@nxp.com>
>> Cc: Peng Fan <peng.fan@nxp.com>
>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>> Cc: Shawn Guo <shawnguo@kernel.org>
>> Cc: devicetree@vger.kernel.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-remoteproc@vger.kernel.org
>> ---
>>   drivers/remoteproc/imx_rproc.c | 53 ++++++++++++++++++++++++++++++++--
>>   drivers/remoteproc/imx_rproc.h |  2 ++
>>   2 files changed, 53 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
>> index d0eb96d6a4fe1..09589f664a2be 100644
>> --- a/drivers/remoteproc/imx_rproc.c
>> +++ b/drivers/remoteproc/imx_rproc.c
>> @@ -41,6 +41,12 @@
>>   #define IMX7D_M4_STOP			(IMX7D_ENABLE_M4 | IMX7D_SW_M4C_RST | \
>>   					 IMX7D_SW_M4C_NON_SCLR_RST)
>>   
>> +#define IMX8M_M7_STOP			(IMX7D_ENABLE_M4 | IMX7D_SW_M4C_RST)
>> +#define IMX8M_M7_POLL			IMX7D_ENABLE_M4
>> +
>> +#define IMX8M_GPR22			0x58
>> +#define IMX8M_GPR22_CM7_CPUWAIT		BIT(0)
>> +
>>   /* Address: 0x020D8000 */
>>   #define IMX6SX_SRC_SCR			0x00
>>   #define IMX6SX_ENABLE_M4		BIT(22)
>> @@ -92,6 +98,7 @@ static int imx_rproc_detach_pd(struct rproc *rproc);
>>   struct imx_rproc {
>>   	struct device			*dev;
>>   	struct regmap			*regmap;
>> +	struct regmap			*gpr;
>>   	struct rproc			*rproc;
>>   	const struct imx_rproc_dcfg	*dcfg;
>>   	struct imx_rproc_mem		mem[IMX_RPROC_MEM_MAX];
>> @@ -287,6 +294,18 @@ static const struct imx_rproc_att imx_rproc_att_imx6sx[] = {
>>   	{ 0x80000000, 0x80000000, 0x60000000, 0 },
>>   };
>>   
>> +static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mn_mmio = {
>> +	.src_reg	= IMX7D_SRC_SCR,
>> +	.src_mask	= IMX7D_M4_RST_MASK,
>> +	.src_start	= IMX7D_M4_START,
>> +	.src_stop	= IMX8M_M7_STOP,
>> +	.gpr_reg	= IMX8M_GPR22,
>> +	.gpr_wait	= IMX8M_GPR22_CM7_CPUWAIT,
>> +	.att		= imx_rproc_att_imx8mn,
>> +	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8mn),
>> +	.method		= IMX_RPROC_MMIO,
>> +};
>> +
>>   static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mn = {
>>   	.att		= imx_rproc_att_imx8mn,
>>   	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8mn),
>> @@ -367,8 +386,14 @@ static int imx_rproc_start(struct rproc *rproc)
>>   
>>   	switch (dcfg->method) {
>>   	case IMX_RPROC_MMIO:
>> -		ret = regmap_update_bits(priv->regmap, dcfg->src_reg, dcfg->src_mask,
>> -					 dcfg->src_start);
>> +		if (priv->gpr) {
>> +			ret = regmap_clear_bits(priv->gpr, dcfg->gpr_reg,
>> +						dcfg->gpr_wait);
>> +		} else {
>> +			ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
>> +						 dcfg->src_mask,
>> +						 dcfg->src_start);
>> +		}
>>   		break;
>>   	case IMX_RPROC_SMC:
>>   		arm_smccc_smc(IMX_SIP_RPROC, IMX_SIP_RPROC_START, 0, 0, 0, 0, 0, 0, &res);
>> @@ -400,6 +425,16 @@ static int imx_rproc_stop(struct rproc *rproc)
>>   
>>   	switch (dcfg->method) {
>>   	case IMX_RPROC_MMIO:
>> +		if (priv->gpr) {
>> +			ret = regmap_set_bits(priv->gpr, dcfg->gpr_reg,
>> +					      dcfg->gpr_wait);
>> +			if (ret) {
>> +				dev_err(priv->dev,
>> +					"Failed to quiescence M4 platform!\n");
>> +				return ret;
>> +			}
>> +		}
>> +
>>   		ret = regmap_update_bits(priv->regmap, dcfg->src_reg, dcfg->src_mask,
>>   					 dcfg->src_stop);
>>   		break;
>> @@ -988,6 +1023,10 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
>>   		break;
>>   	}
>>   
>> +	priv->gpr = syscon_regmap_lookup_by_phandle(dev->of_node, "gpr");
>> +	if (IS_ERR(priv->gpr))
>> +		priv->gpr = NULL;
>> +
>>   	regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
>>   	if (IS_ERR(regmap)) {
>>   		dev_err(dev, "failed to find syscon\n");
>> @@ -997,6 +1036,14 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
>>   	priv->regmap = regmap;
>>   	regmap_attach_dev(dev, regmap, &config);
>>   
>> +	if (priv->gpr) {
>> +		ret = regmap_read(priv->gpr, dcfg->gpr_reg, &val);
>> +		if (val & dcfg->gpr_wait) {
>> +			imx_rproc_stop(priv->rproc);
>> +			return 0;
>> +		}
>> +	}
> 
> Can you provide a comment to explain what is happening above?  In what kind of
> scenario would a GPR be set with the wait bit on startup?

This happens right after power-on reset, the core indicates its in wait 
state in the GPR, but its not fully stopped, so we stop it and then the 
firmware can be loaded into it.