Re: [PATCH v3 4/4] misc: fastrpc: Update dma_bits for CDSP support on Kaanapali SoC

[Kumari Pallavi <kumari.pallavi@oss.qualcomm.com>]

On 11/18/2025 10:27 PM, Bjorn Andersson wrote:
> On Mon, Nov 17, 2025 at 02:42:23PM +0530, Kumari Pallavi wrote:
>>
>>
>> On 11/14/2025 9:30 PM, Bjorn Andersson wrote:
>>> On Fri, Nov 14, 2025 at 02:11:42PM +0530, Kumari Pallavi wrote:
>>>> DSP currently supports 32-bit IOVA (32-bit PA + 4-bit SID) for
>>>> both Q6 and user DMA (uDMA) access. This is being upgraded to
>>>> 34-bit PA + 4-bit SID due to a hardware revision in CDSP for
>>>> Kaanapali SoC, which expands the DMA addressable range.
>>>> Update DMA bits configuration in the driver to support CDSP on
>>>> Kaanapali SoC. Set the default `dma_bits` to 32-bit and update
>>>> it to 34-bit based on CDSP and OF matching on the fastrpc node.
>>>>
>>>> Signed-off-by: Kumari Pallavi <kumari.pallavi@oss.qualcomm.com>
>>>> ---
>>>>    drivers/misc/fastrpc.c | 15 +++++++++++++--
>>>>    1 file changed, 13 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>> index bcf3c7f8d3e9..2eb8d37cd9b4 100644
>>>> --- a/drivers/misc/fastrpc.c
>>>> +++ b/drivers/misc/fastrpc.c
>>>> @@ -267,6 +267,8 @@ struct fastrpc_session_ctx {
>>>>    struct fastrpc_soc_data {
>>>>    	u32 sid_pos;
>>>> +	u32 cdsp_dma_bits;
>>>> +	u32 dsp_default_dma_bits;
>>>>    };
>>>>    struct fastrpc_channel_ctx {
>>>> @@ -2186,6 +2188,7 @@ static int fastrpc_cb_probe(struct platform_device *pdev)
>>>>    	int i, sessions = 0;
>>>>    	unsigned long flags;
>>>>    	int rc;
>>>> +	u32 dma_bits;
>>>>    	cctx = dev_get_drvdata(dev->parent);
>>>>    	if (!cctx)
>>>> @@ -2199,12 +2202,16 @@ static int fastrpc_cb_probe(struct platform_device *pdev)
>>>>    		spin_unlock_irqrestore(&cctx->lock, flags);
>>>>    		return -ENOSPC;
>>>>    	}
>>>> +	dma_bits = cctx->soc_data->dsp_default_dma_bits;
>>>>    	sess = &cctx->session[cctx->sesscount++];
>>>>    	sess->used = false;
>>>>    	sess->valid = true;
>>>>    	sess->dev = dev;
>>>>    	dev_set_drvdata(dev, sess);
>>>> +	if (cctx->domain_id == CDSP_DOMAIN_ID)
>>>> +		dma_bits = cctx->soc_data->cdsp_dma_bits;
>>>> +
>>>>    	if (of_property_read_u32(dev->of_node, "reg", &sess->sid))
>>>>    		dev_info(dev, "FastRPC Session ID not specified in DT\n");
>>>> @@ -2219,9 +2226,9 @@ static int fastrpc_cb_probe(struct platform_device *pdev)
>>>>    		}
>>>>    	}
>>>>    	spin_unlock_irqrestore(&cctx->lock, flags);
>>>> -	rc = dma_set_mask(dev, DMA_BIT_MASK(32));
>>>> +	rc = dma_set_mask(dev, DMA_BIT_MASK(dma_bits));
>>>>    	if (rc) {
>>>> -		dev_err(dev, "32-bit DMA enable failed\n");
>>>> +		dev_err(dev, "%u-bit DMA enable failed\n", dma_bits);
>>>>    		return rc;
>>>>    	}
>>>> @@ -2308,10 +2315,14 @@ static int fastrpc_get_domain_id(const char *domain)
>>>>    static const struct fastrpc_soc_data kaanapali_soc_data = {
>>>>    	.sid_pos = 56,
>>>> +	.cdsp_dma_bits = 34,
>>>> +	.dsp_default_dma_bits = 32,
>>>>    };
>>>>    static const struct fastrpc_soc_data default_soc_data = {
>>>>    	.sid_pos = 32,
>>>> +	.cdsp_dma_bits = 32,
>>>> +	.dsp_default_dma_bits = 32,
>>>
>>> So, "dsp_default_dma_bits" specified "what is the dma_mask for the
>>> non-CDSP fastrpc instances"? I don't find "dsp_default" to naturally
>>> mean "not the cdsp".
>>>
>>>
>>> Wouldn't it be better to introduce two different compatibles, one being
>>> the "qcom,kaanapali-fastrpc" and one being the
>>> "qcom,kaanapali-cdsp-fastrpc" and then use that to select things here?
>>>
>>
>> Thank you for the suggestion. In this case, sid_pos is common across all
>> DSP domains on kaanapali Soc. Splitting into two compatibles would lead to
>> duplication of these shared property in the DT schema and driver logic.
>> The only difference here is the DMA address width for CDSP (34-bit) versus
>> other DSPs (32-bit).
>>
>> To address the concern about naming, I can provide:
>>
>> dma_bits_cdsp → clearly indicates this applies to the CDSP domain.
>> dma_bits_non_cdsp (or dma_bits_other_dsp) → for ADSP and other DSP domains.
>> Please let me know if this aligns with your suggestion ?
>>
> 
> This naming is much better.
> 
> I'm not entirely sure about the compatibility part though. The Kaanapali
> CSDP and Kaanapali ADSP doesn't have the same DMA address width, so are
> they then compatible/the same?
> 
> The fact that the two compatibles would refer to something with the same
> sid_pos isn't a concern to me. De-duplicating a single constant at the
> expense of more complicated logic, that is a concern however.
> 

It might be a good idea to replace the domain-based naming with 
functional naming. Currently, CDSP supports an extended DMA address 
width of 34 bits due to specific use cases, but this could change in the 
future for other DSPs as well.
Using names like "dma_addr_bits_extended" and "dma_addr_bits_default" 
would keep the design flexible and independent of domain.

>>>
>>> PS. You store "dma_bits" just for the sake of turning it into a
>>> dma_mask, just store the DMA_BIT_MASK() directly here instead.
>>>
>>
>> The current approach of assigning a value to cdsp_dma_mask allows for
>> adaptable logging behavior, making it easier to trace.
>>
> 
> I presume you mean it allows you to do "%u-bit DMA enable failed"?
> 

Yes, now I can directly use the dma_bits otherwise I have to extract the
dma_bits from DMA_BIT_MASK() just for logging.


> There are only two options here (32 and 34), and the only reason why
> it's not directly obvious which case you're looking at is because you're
> "dynamically" deriving that number from something else.
> 
> Regards,
> Bjorn
> 
>>
>>> Regards,
>>> Bjorn
>>>
>>>>    };
>>>>    static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>> -- 
>>>> 2.34.1
>>>>
>>>>
>>
>> Thanks,
>> Pallavi

Thanks
Pallavi