Re: [PATCH v4 03/11] bus: mhi: host: Add support to read MHI capabilities

[Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>]

On 7/9/2025 5:50 PM, Ilpo Järvinen wrote:
> On Wed, 9 Jul 2025, Krishna Chaitanya Chundru wrote:
>> On 7/8/2025 10:06 PM, Manivannan Sadhasivam wrote:
>>> On Mon, Jun 09, 2025 at 04:21:24PM GMT, Krishna Chaitanya Chundru wrote:
>>>> From: Vivek Pernamitta <quic_vpernami@quicinc.com>
>>>>
>>>> As per MHI spec v1.2,sec 6.6, MHI has capability registers which are
>>>> located after the ERDB array. The location of this group of registers is
>>>> indicated by the MISCOFF register. Each capability has a capability ID to
>>>> determine which functionality is supported and each capability will point
>>>> to the next capability supported.
>>>>
>>>> Add a basic function to read those capabilities offsets.
>>>>
>>>> Signed-off-by: Vivek Pernamitta <quic_vpernami@quicinc.com>
>>>> Signed-off-by: Krishna Chaitanya Chundru
>>>> <krishna.chundru@oss.qualcomm.com>
>>>> ---
>>>>    drivers/bus/mhi/common.h    | 13 +++++++++++++
>>>>    drivers/bus/mhi/host/init.c | 34 ++++++++++++++++++++++++++++++++++
>>>>    2 files changed, 47 insertions(+)
>>>>
>>>> diff --git a/drivers/bus/mhi/common.h b/drivers/bus/mhi/common.h
>>>> index
>>>> dda340aaed95a5573a2ec776ca712e11a1ed0b52..58f27c6ba63e3e6fa28ca48d6d1065684ed6e1dd
>>>> 100644
>>>> --- a/drivers/bus/mhi/common.h
>>>> +++ b/drivers/bus/mhi/common.h
>>>> @@ -16,6 +16,7 @@
>>>>    #define MHICFG				0x10
>>>>    #define CHDBOFF				0x18
>>>>    #define ERDBOFF				0x20
>>>> +#define MISCOFF				0x24
>>>>    #define BHIOFF				0x28
>>>>    #define BHIEOFF				0x2c
>>>>    #define DEBUGOFF			0x30
>>>> @@ -113,6 +114,9 @@
>>>>    #define MHISTATUS_MHISTATE_MASK		GENMASK(15, 8)
>>>>    #define MHISTATUS_SYSERR_MASK		BIT(2)
>>>>    #define MHISTATUS_READY_MASK		BIT(0)
>>>> +#define MISC_CAP_MASK			GENMASK(31, 0)
>>>> +#define CAP_CAPID_MASK			GENMASK(31, 24)
>>>> +#define CAP_NEXT_CAP_MASK		GENMASK(23, 12)
>>>>      /* Command Ring Element macros */
>>>>    /* No operation command */
>>>> @@ -204,6 +208,15 @@
>>>>    #define MHI_RSCTRE_DATA_DWORD1
>>>> cpu_to_le32(FIELD_PREP(GENMASK(23, 16), \
>>>>    							       MHI_PKT_TYPE_COALESCING))
>>>>    +enum mhi_capability_type {
>>>> +	MHI_CAP_ID_INTX = 0x1,
>>>> +	MHI_CAP_ID_TIME_SYNC = 0x2,
>>>> +	MHI_CAP_ID_BW_SCALE = 0x3,
>>>> +	MHI_CAP_ID_TSC_TIME_SYNC = 0x4,
>>>> +	MHI_CAP_ID_MAX_TRB_LEN = 0x5,
>>>> +	MHI_CAP_ID_MAX,
>>>> +};
>>>> +
>>>>    enum mhi_pkt_type {
>>>>    	MHI_PKT_TYPE_INVALID = 0x0,
>>>>    	MHI_PKT_TYPE_NOOP_CMD = 0x1,
>>>> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
>>>> index
>>>> 13e7a55f54ff45b83b3f18b97e2cdd83d4836fe3..9102ce13a2059f599b46d25ef631f643142642be
>>>> 100644
>>>> --- a/drivers/bus/mhi/host/init.c
>>>> +++ b/drivers/bus/mhi/host/init.c
>>>> @@ -467,6 +467,40 @@ int mhi_init_dev_ctxt(struct mhi_controller
>>>> *mhi_cntrl)
>>>>    	return ret;
>>>>    }
>>>>    +static int mhi_find_capability(struct mhi_controller *mhi_cntrl, u32
>>>> capability, u32 *offset)
>>>> +{
>>>> +	u32 val, cur_cap, next_offset;
>>>> +	int ret;
>>>> +
>>>> +	/* Get the first supported capability offset */
>>>> +	ret = mhi_read_reg_field(mhi_cntrl, mhi_cntrl->regs, MISCOFF,
>>>> MISC_CAP_MASK, offset);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	*offset = (__force u32)le32_to_cpu(*offset);
>>>
>>> Why do you need __force attribute? What does it suppress? Is it because the
>>> pointer is not le32?
>>>
>> yes to suppress warnings.
> 
> I'm pretty sure sparce with endianness checking won't be happy with that
> construct as you pass u32 where le32_to_cpu() expects __le32. Have you
> checked this with sparse? (It might not check endianness with default args.)
> 
>>>> +	do {
>>>> +		if (*offset >= mhi_cntrl->reg_len)
>>>> +			return -ENXIO;
>>>> +
>>>> +		ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->regs, *offset, &val);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +
>>>> +		val = (__force u32)le32_to_cpu(val);
>>>> +		cur_cap = FIELD_GET(CAP_CAPID_MASK, val);
>>>> +		next_offset = FIELD_GET(CAP_NEXT_CAP_MASK, val);
>>>> +		if (cur_cap >= MHI_CAP_ID_MAX)
>>>> +			return -ENXIO;
>>>> +
>>>> +		if (cur_cap == capability)
>>>> +			return 0;
>>>> +
>>>> +		*offset = next_offset;
>>>> +	} while (next_offset);
>>>> +
>>>> +	return -ENXIO;
>>>> +}
> 
> There's a generalization of capability search in Hans Zhang's series,
> can it be used here too?
I Checked this option, the mhi capabilities will not fit in to the
series mentioned for not matching the mask for cap ID, size etc. mhi
capability are similar to PCI, but they vary between mask of cap ID, 
size of the space etc. so going with current approach only.

- Krishna Chaitanya.
> 
>