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 <[email protected]> >> >> 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 <[email protected]> >> Signed-off-by: Michal Simek <[email protected]> > > ... > >> +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 _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
