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

["Heiko Stübner" <heiko@sntech.de>]

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?


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);
> >
>