[U-Boot] [PATCH v2 03/13] mailbox: zynqmp: ipi mailbox driver
Michal Simek
michal.simek at xilinx.com
Thu Oct 10 09:21:02 UTC 2019
On 09. 10. 19 17:02, Luca Ceresoli wrote:
> 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).
>
Binding is taken from Linux kernel and these are required property
there. I think it is used by Linux driver. It means checking required
property is good thing to do.
Regarding if make sense to map them if they are not used. I think we can
remove this code.
If make sense reorganized the code to make it smaller. Sure of course.
Patches welcome.
Thanks,
Michal
More information about the U-Boot
mailing list