[U-Boot] [PATCH v2 04/13] firmware: zynqmp: Add zynqmp-power support

[Michal Simek]

On 09. 10. 19 17:03, Luca Ceresoli wrote:
> Hi,
> 
> On 02/10/19 15:39, Michal Simek wrote:
>> From: Ibai Erkiaga <ibai.erkiaga-elorza at xilinx.com>
>>
>> zynqmp-power driver for ZynqMP to handle the communication with the PMU
>> firmware. Firmware driver just probes subnodes and power driver handles
>> communication with PMU using the IPI mailbox driver.
>>
>> Signed-off-by: Ibai Erkiaga <ibai.erkiaga-elorza at xilinx.com>
>> Signed-off-by: Michal Simek <michal.simek at xilinx.com>
>> ---
>>
>> Changes in v2:
>> - Check error separately - Reported by Luca
>>
>>  drivers/firmware/Kconfig           |  2 ++
>>  drivers/firmware/firmware-zynqmp.c | 44 ++++++++++++++++++++++++++++++
>>  2 files changed, 46 insertions(+)
>>
>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>> index b70a2063551c..9596ec16c7f7 100644
>> --- a/drivers/firmware/Kconfig
>> +++ b/drivers/firmware/Kconfig
>> @@ -30,6 +30,8 @@ config TI_SCI_PROTOCOL
>>  config ZYNQMP_FIRMWARE
>>  	bool "ZynqMP Firmware interface"
>>  	select FIRMWARE
>> +	select ZYNQMP_IPI
>> +	select DM_MAILBOX
> 
> Nitpick: ZYNQMP_IPI depends on DM_MAILBOX, so I'd list them in opposite
> order. Just for clarity.
> 

This has changed already.

>>  	help
>>  	  Firmware interface driver is used by different
>>  	  drivers to communicate with the firmware for
>> diff --git a/drivers/firmware/firmware-zynqmp.c b/drivers/firmware/firmware-zynqmp.c
>> index 6644a7166ca0..97ac333296ec 100644
>> --- a/drivers/firmware/firmware-zynqmp.c
>> +++ b/drivers/firmware/firmware-zynqmp.c
>> @@ -1,6 +1,50 @@
>>  // SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Xilinx Zynq MPSoC Firmware driver
>> + *
>> + * Copyright (C) 2018-2019 Xilinx, Inc.
>> + */
>>  
>> +#include <common.h>
>>  #include <dm.h>
>> +#include <mailbox.h>
>> +#include <asm/arch/sys_proto.h>
>> +
>> +struct zynqmp_power {
>> +	struct mbox_chan tx_chan;
>> +	struct mbox_chan rx_chan;
>> +} zynqmp_power;
>> +
>> +static int zynqmp_power_probe(struct udevice *dev)
>> +{
>> +	int ret = 0;
> 
> No need to initialize.

Will fix.

> 
>> +
>> +	debug("%s, (dev=%p)\n", __func__, dev);
>> +
>> +	ret = mbox_get_by_name(dev, "tx", &zynqmp_power.tx_chan);
>> +	if (ret) {
>> +		debug("%s, cannot tx mailbox\n", __func__);
> 
> "cannot tx" -> "cannot find tx"?

ditto.

> 
>> +		return ret;
>> +	}
>> +
>> +	ret = mbox_get_by_name(dev, "rx", &zynqmp_power.rx_chan);
>> +	if (ret)
>> +		debug("%s, cannot rx mailbox\n", __func__);
> 
> Ditto.

ditto.

> 
>> +
>> +	return ret;
> 
> return 0;

return ret was correct here because if (ret) doesn't return error value
and that code just print message in case of error.

Thanks,
Michal