Re: [PATCH v9 4/9] mfd: add base driver for qnap-mcu devices

[Kees Bakker <kees@ijzerbout.nl>]

Op 19-12-2024 om 20:43 schreef Heiko Stübner:
> Hi Kees,
>
> Am Donnerstag, 19. Dezember 2024, 20:18:38 CET schrieb Kees Bakker:
>> Op 07-11-2024 om 12:47 schreef Heiko Stuebner:
>>> These microcontroller units are used in network-attached-storage devices
>>> made by QNAP and provide additional functionality to the system.
>>>
>>> This adds the base driver that implements the serial protocol via
>>> serdev and additionally hooks into the poweroff handlers to turn
>>> off the parts of the system not supplied by the general PMIC.
>>>
>>> Turning off (at least the TSx33 devices using Rockchip SoCs) consists of
>>> two separate actions. Turning off the MCU alone does not turn off the main
>>> SoC and turning off only the SoC/PMIC does not turn off the hard-drives.
>>> Also if the MCU is not turned off, the system also won't start again until
>>> it is unplugged from power.
>>>
>>> So on shutdown the MCU needs to be turned off separately before the
>>> main PMIC.
>>>
>>> The protocol spoken by the MCU is sadly not documented, but was
>>> obtained by listening to the chatter on the serial port, as thankfully
>>> the "hal_app" program from QNAPs firmware allows triggering all/most
>>> MCU actions from the command line.
>>>
>>> The implementation of how to talk to the serial device got some
>>> inspiration from the rave-sp servdev driver.
>>>
>>> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>>> ---
>>>    MAINTAINERS                  |   6 +
>>>    drivers/mfd/Kconfig          |  13 ++
>>>    drivers/mfd/Makefile         |   2 +
>>>    drivers/mfd/qnap-mcu.c       | 338 +++++++++++++++++++++++++++++++++++
>>>    include/linux/mfd/qnap-mcu.h |  26 +++
>>>    5 files changed, 385 insertions(+)
>>>    create mode 100644 drivers/mfd/qnap-mcu.c
>>>    create mode 100644 include/linux/mfd/qnap-mcu.h
>>>
>>> [...]
>>> diff --git a/drivers/mfd/qnap-mcu.c b/drivers/mfd/qnap-mcu.c
>>> new file mode 100644
>>> index 000000000000..4be39d8b2905
>>> --- /dev/null
>>> +++ b/drivers/mfd/qnap-mcu.c
>>> [...]
>>> +int qnap_mcu_exec(struct qnap_mcu *mcu,
>>> +		  const u8 *cmd_data, size_t cmd_data_size,
>>> +		  u8 *reply_data, size_t reply_data_size)
>>> +{
>>> +	unsigned char rx[QNAP_MCU_RX_BUFFER_SIZE];
>>> +	size_t length = reply_data_size + QNAP_MCU_CHECKSUM_SIZE;
>>> +	struct qnap_mcu_reply *reply = &mcu->reply;
>>> +	int ret = 0;
>>> +
>>> +	if (length > sizeof(rx)) {
>>> +		dev_err(&mcu->serdev->dev, "expected data too big for receive buffer");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	mutex_lock(&mcu->bus_lock);
>>> +
>>> +	reply->data = rx,
>>> +	reply->length = length,
>>> +	reply->received = 0,
>>> +	reinit_completion(&reply->done);
>>> +
>>> +	qnap_mcu_write(mcu, cmd_data, cmd_data_size);
>>> +
>>> +	serdev_device_wait_until_sent(mcu->serdev, msecs_to_jiffies(QNAP_MCU_TIMEOUT_MS));
>>> +
>>> +	if (!wait_for_completion_timeout(&reply->done, msecs_to_jiffies(QNAP_MCU_TIMEOUT_MS))) {
>>> +		dev_err(&mcu->serdev->dev, "Command timeout\n");
>>> +		ret = -ETIMEDOUT;
>>> +	} else {
>>> +		u8 crc = qnap_mcu_csum(rx, reply_data_size);
>> Here `rx` is still not initialized.
> The MCU works in a way that a sent command always causes "reply_data_size"
> bytes to be returned.
>
> So for each qnap_mcu_write() above we know that this amount of bytes has
> been returned (and thus written into rx) if the completion above finishes
> sucessfully.
>
> "rx" is assigned to reply->data above (same as the expected received size).
> When characters are received on the serial line, this will trigger
> qnap_mcu_receive_buf() from the serdev and thus fill those elements in rx.
>
> So if we land at the qnap_mcu_csum() call, we do have received the expected
> amount of bytes from the serdev and thus rx is filled accordingly.
>
> If we don't receive the needed amount of bytes, we end up in the timeout
> above that.
>
> What did I miss?
Sorry, my fault. I missed the essential part of the external event (external
for this function that is).
Thanks for explaining.
>
>
> Heiko
>
>>> +
>>> +		if (crc != rx[reply_data_size]) {
>>> +			dev_err(&mcu->serdev->dev,
>>> +				"Invalid Checksum received\n");
>>> +			ret = -EIO;
>>> +		} else {
>>> +			memcpy(reply_data, rx, reply_data_size);
>>> +		}
>>> +	}
>>> +
>>> +	mutex_unlock(&mcu->bus_lock);
>>> +	return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(qnap_mcu_exec);
>>>
>
>
>