[U-Boot] [PATCH v2 03/13] mailbox: zynqmp: ipi mailbox driver

[Luca Ceresoli]

Hi Ibai, Michal,

I had half-written a review of this patch and patch 4. Unfortunately I
didn't finish them before they got applied. I'll send them now anyway,
they are mostly nitpicking but you might consider them for a future
improvement. Sorry for the inconvenience.


On 02/10/19 15:39, Michal Simek wrote:
> From: Ibai Erkiaga <ibai.erkiaga-elorza at xilinx.com>
> 
> ZynqMP mailbox driver implementing IPI communication with PMU. This would
> allow U-Boot SPL to communicate with PMUFW to request privileged
> operations.
> 
> Signed-off-by: Ibai Erkiaga <ibai.erkiaga-elorza at xilinx.com>
> Signed-off-by: Michal Simek <michal.simek at xilinx.com>

...

> +static int zynqmp_ipi_probe(struct udevice *dev)
> +{
> +	struct zynqmp_ipi *zynqmp = dev_get_priv(dev);
> +	struct resource res;
> +	ofnode node;
> +
> +	debug("%s(dev=%p)\n", __func__, dev);
> +
> +	/* Get subnode where the regs are defined */
> +	/* Note IPI mailbox node needs to be the first one in DT */
> +	node = ofnode_first_subnode(dev_ofnode(dev));
> +
> +	if (ofnode_read_resource_byname(node, "local_request_region", &res)) {
> +		dev_err(dev, "No reg property for local_request_region\n");
> +		return -EINVAL;
> +	};
> +	zynqmp->local_req_regs = devm_ioremap(dev, res.start,
> +					      (res.start - res.end));
> +
> +	if (ofnode_read_resource_byname(node, "local_response_region", &res)) {
> +		dev_err(dev, "No reg property for local_response_region\n");
> +		return -EINVAL;
> +	};
> +	zynqmp->local_res_regs = devm_ioremap(dev, res.start,
> +					      (res.start - res.end));
> +
> +	if (ofnode_read_resource_byname(node, "remote_request_region", &res)) {
> +		dev_err(dev, "No reg property for remote_request_region\n");
> +		return -EINVAL;
> +	};
> +	zynqmp->remote_req_regs = devm_ioremap(dev, res.start,
> +					       (res.start - res.end));
> +
> +	if (ofnode_read_resource_byname(node, "remote_response_region", &res)) {
> +		dev_err(dev, "No reg property for remote_response_region\n");
> +		return -EINVAL;
> +	};
> +	zynqmp->remote_res_regs = devm_ioremap(dev, res.start,
> +					       (res.start - res.end));

remote_req_regs and remote_res_regs are not used, why adding them in DT?

Should there be a good reason to keep them, I think the above code could
be reorganized to avoid code duplication (assuming binary size of a
bootloader still matters nowadays).

-- 
Luca