Re: [PATCH v2 14/14] greybus: cpc: add CPC SDIO host driver

["Jérôme Pouiller" <jerome.pouiller@silabs.com>]

On Thursday 15 January 2026 16:58:07 Central European Standard Time Damien Riégel wrote:
> From: Gabriel Beaulieu <gabriel.beaulieu@silabs.com>
> 
> This introduces a new module gb-cpc-sdio, in order to communicate with a
> Greybus CPC device over SDIO.
> 
> Most of the complexity stems from aggregation: packets are aggregated to
> minimize the number of CMD53s. In the first block, the first le32 is the
> number of packets in this transfer. Immediately after that are all the
> packet headers (CPC + Greybus). This lets the device process all the
> headers in a single interrupt, and prepare the ADMA descriptors for all
> the payloads in one go.
> 
> Payloads start at the beginning of the second block and are concatained.
> Their lengths must be 32-bit aligned, so the driver takes care of adding
> and removing padding if necessary.
> 
> Signed-off-by: Gabriel Beaulieu <gabriel.beaulieu@silabs.com>
> Signed-off-by: Damien Riégel <damien.riegel@silabs.com>
> ---
> Changes in v2:
>   - change formatting from %lu to %zu when printing size_t's
>   - remove "/**" kernel-doc marker for static functions not actually
>     using the kernel-doc format
>   - reduce header inclusion list
>   - use reverse christmas tree variable declarations consistently
>   - update aggregation functions to try to be more legible
>   - use define instead of constant value 0x0C for the address where to
>     read the number of bytes the device wants to send
>   - remove padding between headers and payloads when aggregating packets
> 
>  drivers/greybus/cpc/Kconfig  |  12 +
>  drivers/greybus/cpc/Makefile |   3 +
>  drivers/greybus/cpc/sdio.c   | 533 +++++++++++++++++++++++++++++++++++
>  3 files changed, 548 insertions(+)
>  create mode 100644 drivers/greybus/cpc/sdio.c
> 

[...]

> diff --git a/drivers/greybus/cpc/sdio.c b/drivers/greybus/cpc/sdio.c
> new file mode 100644
> index 00000000000..aeeb8378dea
> --- /dev/null
> +++ b/drivers/greybus/cpc/sdio.c
> @@ -0,0 +1,533 @@

[...]

> +static void gb_cpc_sdio_rx_tx(struct cpc_sdio *ctx)
> +{
> +	gb_cpc_sdio_rx(ctx);
> +
> +	set_bit(CPC_SDIO_FLAG_IRQ_RUNNING, &ctx->flags);
> +	gb_cpc_sdio_tx(ctx);
> +	clear_bit(CPC_SDIO_FLAG_IRQ_RUNNING, &ctx->flags);
> +}
> +
> +static void gb_cpc_sdio_tx_work(struct work_struct *work)
> +{
> +	struct cpc_sdio *ctx = container_of(work, struct cpc_sdio, tx_work);
> +
> +	/* Do not execute concurrently to the interrupt */
> +	if (test_bit(CPC_SDIO_FLAG_IRQ_RUNNING, &ctx->flags)) {
> +		set_bit(CPC_SDIO_FLAG_TX_WORK_DELAYED, &ctx->flags);
> +		return;
> +	}
> +
> +	gb_cpc_sdio_tx(ctx);
> +}
> +
> +static struct cpc_hd_driver cpc_sdio_driver = {
> +	.wake_tx = gb_cpc_sdio_wake_tx,
> +};
> +
> +static int cpc_sdio_init(struct sdio_func *func)
> +{
> +	unsigned char rx_data_ready_irq_en_bit = BIT(0);
> +	unsigned int irq_enable_addr = 0x09;
> +	int err;
> +
> +	/* Enable the read data ready interrupt. */
> +	sdio_writeb(func, rx_data_ready_irq_en_bit, irq_enable_addr, &err);
> +	if (err)
> +		dev_err(&func->dev, "failed to set data ready interrupt (%d)\n", err);
> +
> +	return err;
> +}
> +
> +static void cpc_sdio_irq_handler(struct sdio_func *func)
> +{
> +	unsigned int rx_data_pending_irq_bit = 0;
> +	unsigned int irq_status_addr = 0x08;
> +	unsigned long int_status;
> +	struct cpc_sdio *ctx;
> +	struct device *dev;
> +	int err;
> +
> +	ctx = sdio_get_drvdata(func);
> +	dev = &func->dev;
> +
> +	int_status = sdio_readb(func, irq_status_addr, &err);
> +	if (err) {
> +		dev_err(dev, "failed to read interrupt status registers (%d)\n", err);
> +		return;
> +	}
> +
> +	if (__test_and_clear_bit(rx_data_pending_irq_bit, &int_status)) {
> +		/* Clear the IRQ on the device side. */
> +		sdio_writeb(func, BIT(rx_data_pending_irq_bit), irq_status_addr, &err);
> +		if (err) {
> +			dev_err(dev, "failed to clear read interrupt (%d), interrupt will repeat\n",
> +				err);
> +			return;
> +		}
> +
> +		cancel_work_sync(&ctx->tx_work);
> +		gb_cpc_sdio_rx_tx(ctx);
> +
> +		if (test_and_clear_bit(CPC_SDIO_FLAG_TX_WORK_DELAYED, &ctx->flags))
> +			schedule_work(&ctx->tx_work);

From my experience, data path is easier to debug when all the data are
sent (and received) from the same context. Don't you think you could
call gb_cpc_sdio_rx() from the workqueue:

   if (__test_and_clear_bit(0, &int_status)) {
          sdio_writeb(func, BIT(0), irq_status_addr, &err);
	atomic_inc(&rx_wait);
          schedule_work(&ctx->rxtx_work);
   }


-- 
Jérôme Pouiller