[0/4] soc: qcom: aoss: Introduce debugfs interface and cleanup things

The Always On Processor supports a number useful commands for affecting
system resources during in various debug scenarious. Introduce a debugfs
interface for allowing the debugger/tester to send these commands.

While at it, let's make some improvements to the qmp_send() API.

Bjorn Andersson (3):
  soc: qcom: aoss: Move length requirements from caller
  soc: qcom: aoss: Format string in qmp_send()
  soc: qcom: aoss: Tidy up qmp_send() callers

Chris Lew (1):
  soc: qcom: aoss: Add debugfs interface for sending messages

 drivers/net/ipa/ipa_power.c        |  5 +-
 drivers/remoteproc/qcom_q6v5.c     |  8 +--
 drivers/soc/qcom/qcom_aoss.c       | 82 +++++++++++++++++++++---------
 include/linux/soc/qcom/qcom_aoss.h |  4 +-
 4 files changed, 61 insertions(+), 38 deletions(-)

On Sun, Jul 30, 2023 at 09:10:10PM -0700, Bjorn Andersson wrote:
> The existing implementation of qmp_send() requires the caller to provide
> a buffer which is of word-aligned. The underlying reason for this is
> that message ram only supports word accesses, but pushing this
> requirement onto the clients results in the same boiler plate code
> sprinkled in every call site.
> 
> By using a temporary buffer in qmp_send() we can hide the underlying
> hardware limitations from the clients and allow them to pass their
> NUL-terminates C string directly.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>  drivers/net/ipa/ipa_power.c        |  2 +-
>  drivers/remoteproc/qcom_q6v5.c     |  2 +-
>  drivers/soc/qcom/qcom_aoss.c       | 25 ++++++++++++-------------
>  include/linux/soc/qcom/qcom_aoss.h |  4 ++--
>  4 files changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
> index 921eecf3eff6..26181eeed975 100644
> --- a/drivers/net/ipa/ipa_power.c
> +++ b/drivers/net/ipa/ipa_power.c
> @@ -332,7 +332,7 @@ void ipa_power_retention(struct ipa *ipa, bool enable)
>  
>  	(void)snprintf(buf, sizeof(buf), fmt, enable ? '1' : '0');
>  
> -	ret = qmp_send(power->qmp, buf, sizeof(buf));
> +	ret = qmp_send(power->qmp, buf);
>  	if (ret)
>  		dev_err(power->dev, "error %d sending QMP %sable request\n",
>  			ret, enable ? "en" : "dis");
> diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
> index 192c7aa0e39e..8b41a73fa4d1 100644
> --- a/drivers/remoteproc/qcom_q6v5.c
> +++ b/drivers/remoteproc/qcom_q6v5.c
> @@ -35,7 +35,7 @@ static int q6v5_load_state_toggle(struct qcom_q6v5 *q6v5, bool enable)
>  
>  	WARN_ON(ret >= Q6V5_LOAD_STATE_MSG_LEN);
>  
> -	ret = qmp_send(q6v5->qmp, buf, sizeof(buf));
> +	ret = qmp_send(q6v5->qmp, buf);
>  	if (ret)
>  		dev_err(q6v5->dev, "failed to toggle load state\n");
>  
> diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
> index e376c32cc16e..5e74332515cf 100644
> --- a/drivers/soc/qcom/qcom_aoss.c
> +++ b/drivers/soc/qcom/qcom_aoss.c
> @@ -206,36 +206,35 @@ static bool qmp_message_empty(struct qmp *qmp)
>   * qmp_send() - send a message to the AOSS
>   * @qmp: qmp context
>   * @data: message to be sent
> - * @len: length of the message
>   *
>   * Transmit @data to AOSS and wait for the AOSS to acknowledge the message.
> - * @len must be a multiple of 4 and not longer than the mailbox size. Access is
> - * synchronized by this implementation.
> + * data must not be longer than the mailbox size. Access is synchronized by
> + * this implementation.
>   *
>   * Return: 0 on success, negative errno on failure
>   */
> -int qmp_send(struct qmp *qmp, const void *data, size_t len)
> +int qmp_send(struct qmp *qmp, const void *data)
>  {
>  	long time_left;
> +	char buf[QMP_MSG_LEN];
>  	int ret;

Hi Bjorn,

please consider preserving reverse xmas tree - longest line to shortest -
for local variable declarations in this Networking code.

	char buf[QMP_MSG_LEN];
	long time_left;
	int ret;

...

On Mon, Jul 31, 2023 at 02:29:44PM -0700, Chris Lew wrote:
> 
> 
> On 7/30/2023 9:10 PM, Bjorn Andersson wrote:
> > diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
> >   	/* The message RAM only implements 32-bit accesses */
> >   	__iowrite32_copy(qmp->msgram + qmp->offset + sizeof(u32),
> > -			 data, len / sizeof(u32));
> > -	writel(len, qmp->msgram + qmp->offset);
> > +			 buf, sizeof(buf) / sizeof(u32));
> > +	writel(sizeof(buf), qmp->msgram + qmp->offset);
> 
> Looks like we are telling the firmware the packet size will always be
> QMP_MSG_LEN?
> 
> This should be ok but might be a problem when debugging. The AOSS firmware
> only logs size of the message instead of the full string because of memory
> constraints.
> 

Until now ipa_power_retention() has been passing 36 here, everyone else
64, so it is ok.

> We would normally match the firmware and host logs based on size, but won't
> be able to differentiate this way with a fixed size.

I don't mind us changing it to ALIGN(len, 4), but as that would change
the current behavior I'd like to do so in a subsequent patch.

Speaking of behavior, is 64 the max message size? We inherited the 64
from the initial downstream implementation, but qmp->size is quite a bit
bigger.

Regards,
Bjorn