Received-SPF: pass (google.com: domain of linux-kernel+bounces-141294-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 11 Apr 2024 10:12:35 -0600
From: Mathieu Poirier <mathieu.poirier@linaro.org>
To: Tanmay Shah <tanmay.shah@amd.com>
Cc: andersson@kernel.org, robh@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	michal.simek@amd.com, ben.levinsky@amd.com,
	linux-remoteproc@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v14 4/4] remoteproc: zynqmp: parse TCM from device tree
Message-ID: <ZhgL8/hJZTJnDYuN@p14s>
References: <20240408205313.3552165-1-tanmay.shah@amd.com>
 <20240408205313.3552165-5-tanmay.shah@amd.com>
 <ZhbFfA7toAkUATfg@p14s>
 <0f62cb47-23bd-41bd-a713-ac9f37a1ec7b@amd.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <0f62cb47-23bd-41bd-a713-ac9f37a1ec7b@amd.com>

On Wed, Apr 10, 2024 at 05:36:30PM -0500, Tanmay Shah wrote:
> 
> 
> On 4/10/24 11:59 AM, Mathieu Poirier wrote:
> > On Mon, Apr 08, 2024 at 01:53:14PM -0700, Tanmay Shah wrote:
> >> ZynqMP TCM information was fixed in driver. Now ZynqMP TCM information
> >> is available in device-tree. Parse TCM information in driver
> >> as per new bindings.
> >> 
> >> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> >> ---
> >> 
> >> Changes in v14:
> >>   - Add Versal platform support
> >>   - Add Versal-NET platform support
> >>   - Maintain backward compatibility for ZynqMP platform and use hardcode
> >>     TCM addresses
> >>   - Configure TCM based on xlnx,tcm-mode property for Versal
> >>   - Avoid TCM configuration if that property isn't available in DT 
> >> 
> >>  drivers/remoteproc/xlnx_r5_remoteproc.c | 173 ++++++++++++++++++------
> >>  1 file changed, 132 insertions(+), 41 deletions(-)
> >> 
> >> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
> >> index 0f942440b4e2..504492f930ac 100644
> >> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> >> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> >> @@ -74,8 +74,8 @@ struct mbox_info {
> >>  };
> >>  
> >>  /*
> >> - * Hardcoded TCM bank values. This will be removed once TCM bindings are
> >> - * accepted for system-dt specifications and upstreamed in linux kernel
> >> + * Hardcoded TCM bank values. This will stay in driver to maintain backward
> >> + * compatibility with device-tree that does not have TCM information.
> >>   */
> >>  static const struct mem_bank_data zynqmp_tcm_banks_split[] = {
> >>  	{0xffe00000UL, 0x0, 0x10000UL, PD_R5_0_ATCM, "atcm0"}, /* TCM 64KB each */
> >> @@ -300,36 +300,6 @@ static void zynqmp_r5_rproc_kick(struct rproc *rproc, int vqid)
> >>  		dev_warn(dev, "failed to send message\n");
> >>  }
> >>  
> >> -/*
> >> - * zynqmp_r5_set_mode()
> >> - *
> >> - * set RPU cluster and TCM operation mode
> >> - *
> >> - * @r5_core: pointer to zynqmp_r5_core type object
> >> - * @fw_reg_val: value expected by firmware to configure RPU cluster mode
> >> - * @tcm_mode: value expected by fw to configure TCM mode (lockstep or split)
> >> - *
> >> - * Return: 0 for success and < 0 for failure
> >> - */
> >> -static int zynqmp_r5_set_mode(struct zynqmp_r5_core *r5_core,
> >> -			      enum rpu_oper_mode fw_reg_val,
> >> -			      enum rpu_tcm_comb tcm_mode)
> >> -{
> >> -	int ret;
> >> -
> >> -	ret = zynqmp_pm_set_rpu_mode(r5_core->pm_domain_id, fw_reg_val);
> >> -	if (ret < 0) {
> >> -		dev_err(r5_core->dev, "failed to set RPU mode\n");
> >> -		return ret;
> >> -	}
> >> -
> >> -	ret = zynqmp_pm_set_tcm_config(r5_core->pm_domain_id, tcm_mode);
> >> -	if (ret < 0)
> >> -		dev_err(r5_core->dev, "failed to configure TCM\n");
> >> -
> >> -	return ret;
> >> -}
> >> -
> >>  /*
> >>   * zynqmp_r5_rproc_start()
> >>   * @rproc: single R5 core's corresponding rproc instance
> >> @@ -761,6 +731,103 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
> >>  	return ERR_PTR(ret);
> >>  }
> >>  
> >> +static int zynqmp_r5_get_tcm_node_from_dt(struct zynqmp_r5_cluster *cluster)
> >> +{
> >> +	int i, j, tcm_bank_count, ret, tcm_pd_idx, pd_count;
> >> +	struct of_phandle_args out_args;
> >> +	struct zynqmp_r5_core *r5_core;
> >> +	struct platform_device *cpdev;
> >> +	struct mem_bank_data *tcm;
> >> +	struct device_node *np;
> >> +	struct resource *res;
> >> +	u64 abs_addr, size;
> >> +	struct device *dev;
> >> +
> >> +	for (i = 0; i < cluster->core_count; i++) {
> >> +		r5_core = cluster->r5_cores[i];
> >> +		dev = r5_core->dev;
> >> +		np = r5_core->np;
> >> +
> >> +		pd_count = of_count_phandle_with_args(np, "power-domains",
> >> +						      "#power-domain-cells");
> >> +
> >> +		if (pd_count <= 0) {
> >> +			dev_err(dev, "invalid power-domains property, %d\n", pd_count);
> >> +			return -EINVAL;
> >> +		}
> >> +
> >> +		/* First entry in power-domains list is for r5 core, rest for TCM. */
> >> +		tcm_bank_count = pd_count - 1;
> >> +
> >> +		if (tcm_bank_count <= 0) {
> >> +			dev_err(dev, "invalid TCM count %d\n", tcm_bank_count);
> >> +			return -EINVAL;
> >> +		}
> >> +
> >> +		r5_core->tcm_banks = devm_kcalloc(dev, tcm_bank_count,
> >> +						  sizeof(struct mem_bank_data *),
> >> +						  GFP_KERNEL);
> >> +		if (!r5_core->tcm_banks)
> >> +			return -ENOMEM;
> >> +
> >> +		r5_core->tcm_bank_count = tcm_bank_count;
> >> +		for (j = 0, tcm_pd_idx = 1; j < tcm_bank_count; j++, tcm_pd_idx++) {
> >> +			tcm = devm_kzalloc(dev, sizeof(struct mem_bank_data),
> >> +					   GFP_KERNEL);
> >> +			if (!tcm)
> >> +				return -ENOMEM;
> >> +
> >> +			r5_core->tcm_banks[j] = tcm;
> >> +
> >> +			/* Get power-domains id of TCM. */
> >> +			ret = of_parse_phandle_with_args(np, "power-domains",
> >> +							 "#power-domain-cells",
> >> +							 tcm_pd_idx, &out_args);
> >> +			if (ret) {
> >> +				dev_err(r5_core->dev,
> >> +					"failed to get tcm %d pm domain, ret %d\n",
> >> +					tcm_pd_idx, ret);
> >> +				return ret;
> >> +			}
> >> +			tcm->pm_domain_id = out_args.args[0];
> >> +			of_node_put(out_args.np);
> >> +
> >> +			/* Get TCM address without translation. */
> >> +			ret = of_property_read_reg(np, j, &abs_addr, &size);
> >> +			if (ret) {
> >> +				dev_err(dev, "failed to get reg property\n");
> >> +				return ret;
> >> +			}
> >> +
> >> +			/*
> >> +			 * Remote processor can address only 32 bits
> >> +			 * so convert 64-bits into 32-bits. This will discard
> >> +			 * any unwanted upper 32-bits.
> >> +			 */
> >> +			tcm->da = (u32)abs_addr;
> >> +			tcm->size = (u32)size;
> >> +
> >> +			cpdev = to_platform_device(dev);
> >> +			res = platform_get_resource(cpdev, IORESOURCE_MEM, j);
> >> +			if (!res) {
> >> +				dev_err(dev, "failed to get tcm resource\n");
> >> +				return -EINVAL;
> >> +			}
> >> +
> >> +			tcm->addr = (u32)res->start;
> >> +			tcm->bank_name = (char *)res->name;
> >> +			res = devm_request_mem_region(dev, tcm->addr, tcm->size,
> >> +						      tcm->bank_name);
> >> +			if (!res) {
> >> +				dev_err(dev, "failed to request tcm resource\n");
> >> +				return -EINVAL;
> >> +			}
> >> +		}
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  /**
> >>   * zynqmp_r5_get_tcm_node()
> >>   * Ideally this function should parse tcm node and store information
> >> @@ -839,9 +906,16 @@ static int zynqmp_r5_core_init(struct zynqmp_r5_cluster *cluster,
> >>  	struct zynqmp_r5_core *r5_core;
> >>  	int ret, i;
> >>  
> >> -	ret = zynqmp_r5_get_tcm_node(cluster);
> >> -	if (ret < 0) {
> >> -		dev_err(dev, "can't get tcm node, err %d\n", ret);
> >> +	r5_core = cluster->r5_cores[0];
> >> +
> >> +	/* Maintain backward compatibility for zynqmp by using hardcode TCM address. */
> >> +	if (device_is_compatible(dev, "xlnx,zynqmp-r5fss"))
> > 
> > The previous patch moved the definition of the R5FSS to the new bindings but
> > this is forcing to use the old bindings - did I something?
> 
> Hi Mathieu,
> 
> We need to maintain backward compatibility for zynqmp device. So, using old bindings
> for zynqmp. For new devices (Versal and Versal-NET) new bindings are enforced in driver.
> It's not recommended to map two programming sequences to same device. So for
> "xlnx,zynqmp-r5fss" device old bindings are used.
>

You are not using two programming sequences for the same device, you are simply
ensuring backward compatibility for older device tree.  The bindings for r5fss
have been updated so the driver should be using the new method.  You have used
"xlnx,tcm-mode" to switch between new and old bindings, do that here too.

I'm also not sure why Versal and Versal-NET are being added to this patchset...
It should be in another patchset of its own.

> Device tree is matching with new bindings. But that's hardware description. I believe,
> driver can still choose to use hardcode addresses to maintain backward compatibility.
> 
> The end result will be same.
> 
> Thanks,
> Tanmay
> 
> > 
> >> +		ret = zynqmp_r5_get_tcm_node(cluster);
> >> +	else
> >> +		ret = zynqmp_r5_get_tcm_node_from_dt(cluster);
> >> +
> >> +	if (ret) {
> >> +		dev_err(dev, "can't get tcm, err %d\n", ret);
> >>  		return ret;
> >>  	}
> >>  
> >> @@ -856,12 +930,18 @@ static int zynqmp_r5_core_init(struct zynqmp_r5_cluster *cluster,
> >>  			return ret;
> >>  		}
> >>  
> >> -		ret = zynqmp_r5_set_mode(r5_core, fw_reg_val, tcm_mode);
> >> -		if (ret) {
> >> -			dev_err(dev, "failed to set r5 cluster mode %d, err %d\n",
> >> -				cluster->mode, ret);
> >> +		ret = zynqmp_pm_set_rpu_mode(r5_core->pm_domain_id, fw_reg_val);
> >> +		if (ret < 0) {
> >> +			dev_err(r5_core->dev, "failed to set RPU mode\n");
> >>  			return ret;
> >>  		}
> >> +
> >> +		if (device_is_compatible(dev, "xlnx,zynqmp-r5fss") ||
> >> +		    of_find_property(dev_of_node(dev), "xlnx,tcm-mode", NULL)) {
> >> +			ret = zynqmp_pm_set_tcm_config(r5_core->pm_domain_id, tcm_mode);
> >> +			if (ret < 0)
> >> +				dev_err(r5_core->dev, "failed to configure TCM\n");
> >> +		}
> >>  	}
> >>  
> >>  	return 0;
> >> @@ -906,16 +986,25 @@ static int zynqmp_r5_cluster_init(struct zynqmp_r5_cluster *cluster)
> >>  	 * fail driver probe if either of that is not set in dts.
> >>  	 */
> >>  	if (cluster_mode == LOCKSTEP_MODE) {
> >> -		tcm_mode = PM_RPU_TCM_COMB;
> >>  		fw_reg_val = PM_RPU_MODE_LOCKSTEP;
> >>  	} else if (cluster_mode == SPLIT_MODE) {
> >> -		tcm_mode = PM_RPU_TCM_SPLIT;
> >>  		fw_reg_val = PM_RPU_MODE_SPLIT;
> >>  	} else {
> >>  		dev_err(dev, "driver does not support cluster mode %d\n", cluster_mode);
> >>  		return -EINVAL;
> >>  	}
> >>  
> >> +	if (device_is_compatible(dev, "xlnx,zynqmp-r5fss")) {
> >> +		if (cluster_mode == LOCKSTEP_MODE)
> >> +			tcm_mode = PM_RPU_TCM_COMB;
> >> +		else
> >> +			tcm_mode = PM_RPU_TCM_SPLIT;
> >> +	} else if (of_find_property(dev_node, "xlnx,tcm-mode", NULL)) {
> >> +		ret = of_property_read_u32(dev_node, "xlnx,tcm-mode", (u32 *)&tcm_mode);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >>  	/*
> >>  	 * Number of cores is decided by number of child nodes of
> >>  	 * r5f subsystem node in dts. If Split mode is used in dts
> >> @@ -1100,6 +1189,8 @@ static int zynqmp_r5_remoteproc_probe(struct platform_device *pdev)
> >>  /* Match table for OF platform binding */
> >>  static const struct of_device_id zynqmp_r5_remoteproc_match[] = {
> >>  	{ .compatible = "xlnx,zynqmp-r5fss", },
> >> +	{ .compatible = "xlnx,versal-r5fss", },
> >> +	{ .compatible = "xlnx,versal-net-r52fss", },
> >>  	{ /* end of list */ },
> >>  };
> >>  MODULE_DEVICE_TABLE(of, zynqmp_r5_remoteproc_match);
> >> -- 
> >> 2.25.1
> >> 
>