Re: [PATCH v6 01/12] dmaengine: qcom: bam_dma: Add bam_sw_version register read

[Stephan Gerhold <stephan.gerhold@linaro.org>]

On Wed, Jan 15, 2025 at 03:59:53PM +0530, Md Sadre Alam wrote:
> Add bam_sw_version register read. This will help to
> differentiate b/w some new BAM features across multiple
> BAM IP, feature like LOCK/UNLOCK of BAM pipe.
> 
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> ---
> 
> change in [v6]
> 
> * No change
> 
> change in [v5]
> 
> * No change
> 
> change in [v4]
> 
> * Added BAM_SW_VERSION register read
> 
> change in [v3]
> 
> * This patch was not included in [v3]
> 
> change in [v2]
> 
> * This patch was not included in [v2]
> 
> change in [v1]
> 
> * This patch was not included in [v1]
> 
>  drivers/dma/qcom/bam_dma.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index c14557efd577..daeacd5cb8e9 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -83,6 +83,7 @@ struct bam_async_desc {
>  enum bam_reg {
>  	BAM_CTRL,
>  	BAM_REVISION,
> +	BAM_SW_VERSION,
>  	BAM_NUM_PIPES,
>  	BAM_DESC_CNT_TRSHLD,
>  	BAM_IRQ_SRCS,
> @@ -117,6 +118,7 @@ struct reg_offset_data {
>  static const struct reg_offset_data bam_v1_3_reg_info[] = {
>  	[BAM_CTRL]		= { 0x0F80, 0x00, 0x00, 0x00 },
>  	[BAM_REVISION]		= { 0x0F84, 0x00, 0x00, 0x00 },
> +	[BAM_SW_VERSION]	= { 0x0F88, 0x00, 0x00, 0x00 },
>  	[BAM_NUM_PIPES]		= { 0x0FBC, 0x00, 0x00, 0x00 },
>  	[BAM_DESC_CNT_TRSHLD]	= { 0x0F88, 0x00, 0x00, 0x00 },
>  	[BAM_IRQ_SRCS]		= { 0x0F8C, 0x00, 0x00, 0x00 },
> @@ -146,6 +148,7 @@ static const struct reg_offset_data bam_v1_3_reg_info[] = {
>  static const struct reg_offset_data bam_v1_4_reg_info[] = {
>  	[BAM_CTRL]		= { 0x0000, 0x00, 0x00, 0x00 },
>  	[BAM_REVISION]		= { 0x0004, 0x00, 0x00, 0x00 },
> +	[BAM_SW_VERSION]	= { 0x0008, 0x00, 0x00, 0x00 },
>  	[BAM_NUM_PIPES]		= { 0x003C, 0x00, 0x00, 0x00 },
>  	[BAM_DESC_CNT_TRSHLD]	= { 0x0008, 0x00, 0x00, 0x00 },
>  	[BAM_IRQ_SRCS]		= { 0x000C, 0x00, 0x00, 0x00 },
> @@ -175,6 +178,7 @@ static const struct reg_offset_data bam_v1_4_reg_info[] = {
>  static const struct reg_offset_data bam_v1_7_reg_info[] = {
>  	[BAM_CTRL]		= { 0x00000, 0x00, 0x00, 0x00 },
>  	[BAM_REVISION]		= { 0x01000, 0x00, 0x00, 0x00 },
> +	[BAM_SW_VERSION]	= { 0x01004, 0x00, 0x00, 0x00 },
>  	[BAM_NUM_PIPES]		= { 0x01008, 0x00, 0x00, 0x00 },
>  	[BAM_DESC_CNT_TRSHLD]	= { 0x00008, 0x00, 0x00, 0x00 },
>  	[BAM_IRQ_SRCS]		= { 0x03010, 0x00, 0x00, 0x00 },
> @@ -393,6 +397,7 @@ struct bam_device {
>  	bool controlled_remotely;
>  	bool powered_remotely;
>  	u32 active_channels;
> +	u32 bam_sw_version;
>  
>  	const struct reg_offset_data *layout;
>  
> @@ -1306,6 +1311,9 @@ static int bam_dma_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	bdev->bam_sw_version = readl_relaxed(bam_addr(bdev, 0, BAM_SW_VERSION));
> +	dev_info(bdev->dev, "BAM software version:0x%08x\n", bdev->bam_sw_version);

This will cause crashes for the same reason as your other patch. During
probe(), we can't read from BAM registers if we don't have a clock
assigned. There is no guarantee that the BAM is powered up.

To make this work properly on all platforms, you would need to defer
reading this register until the first channel is requested by the
consumer driver. Or limit this functionality to the if (bdev->bamclk)
case for now.

We should also prioritize fixing the existing regression before adding
new functionality.

Thanks,
Stephan