[PATCH v1 0/2] Check for device supported event rings and channels

[PATCH v1 0/2] Check for device supported event rings and channels

[Hemant Kumar]

This change is introduced to make sure device supported hardware event ring,
hardware channels, total number of event rings and total number of channels
match with MHI host controller. In case of a mismatch, driver bails out and
does not move MHI device to M0 from Ready state.

Hemant Kumar (2):
  bus: mhi: core: Count number of HW channels supported by controller
  bus: mhi: core: Check for device supported event rings and channels

 drivers/bus/mhi/core/init.c     | 33 +++++++++++++++++++++++++++++++++
 drivers/bus/mhi/core/internal.h |  5 +++++
 include/linux/mhi.h             |  1 +
 3 files changed, 39 insertions(+)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 1/2] bus: mhi: core: Count number of HW channels supported by controller

[Hemant Kumar]

Device provides the total number of HW channels it supports using MHI
configuration register. Host supported HW channels shall not exceed
that value. In order to make this check, a counter is needed to store
total number of HW channels required by host.

Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
---
 drivers/bus/mhi/core/init.c     | 2 ++
 drivers/bus/mhi/core/internal.h | 1 +
 include/linux/mhi.h             | 1 +
 3 files changed, 4 insertions(+)

diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index 0ffdebd..70fd6af 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -725,6 +725,8 @@ static int parse_ch_cfg(struct mhi_controller *mhi_cntrl,
 		mhi_chan = &mhi_cntrl->mhi_chan[chan];
 		mhi_chan->name = ch_cfg->name;
 		mhi_chan->chan = chan;
+		if (chan >= MHI_HW_CHAN_START_IDX)
+			mhi_cntrl->hw_chan++;
 
 		mhi_chan->tre_ring.elements = ch_cfg->num_elements;
 		if (!mhi_chan->tre_ring.elements)
diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index 7989269..3d8e480 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -454,6 +454,7 @@ enum mhi_pm_state {
 #define PRIMARY_CMD_RING		0
 #define MHI_DEV_WAKE_DB			127
 #define MHI_MAX_MTU			0xffff
+#define MHI_HW_CHAN_START_IDX		100
 #define MHI_RANDOM_U32_NONZERO(bmsk)	(prandom_u32_max(bmsk) + 1)
 
 enum mhi_er_type {
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index d4841e5..ea441d2 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -389,6 +389,7 @@ struct mhi_controller {
 	struct list_head lpm_chans;
 	int *irq;
 	u32 max_chan;
+	u32 hw_chan;
 	u32 total_ev_rings;
 	u32 hw_ev_rings;
 	u32 sw_ev_rings;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 1/2] bus: mhi: core: Count number of HW channels supported by controller

[Manivannan Sadhasivam]

On Fri, Oct 23, 2020 at 07:00:42PM -0700, Hemant Kumar wrote:
> Device provides the total number of HW channels it supports using MHI
> configuration register. Host supported HW channels shall not exceed
> that value. In order to make this check, a counter is needed to store
> total number of HW channels required by host.
> 
> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
> ---
>  drivers/bus/mhi/core/init.c     | 2 ++
>  drivers/bus/mhi/core/internal.h | 1 +
>  include/linux/mhi.h             | 1 +
>  3 files changed, 4 insertions(+)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 0ffdebd..70fd6af 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -725,6 +725,8 @@ static int parse_ch_cfg(struct mhi_controller *mhi_cntrl,
>  		mhi_chan = &mhi_cntrl->mhi_chan[chan];
>  		mhi_chan->name = ch_cfg->name;
>  		mhi_chan->chan = chan;
> +		if (chan >= MHI_HW_CHAN_START_IDX)
> +			mhi_cntrl->hw_chan++;
>  
>  		mhi_chan->tre_ring.elements = ch_cfg->num_elements;
>  		if (!mhi_chan->tre_ring.elements)
> diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
> index 7989269..3d8e480 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -454,6 +454,7 @@ enum mhi_pm_state {
>  #define PRIMARY_CMD_RING		0
>  #define MHI_DEV_WAKE_DB			127
>  #define MHI_MAX_MTU			0xffff
> +#define MHI_HW_CHAN_START_IDX		100
>  #define MHI_RANDOM_U32_NONZERO(bmsk)	(prandom_u32_max(bmsk) + 1)
>  
>  enum mhi_er_type {
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index d4841e5..ea441d2 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -389,6 +389,7 @@ struct mhi_controller {
>  	struct list_head lpm_chans;
>  	int *irq;
>  	u32 max_chan;
> +	u32 hw_chan;

Please add Kdoc for this member. With that,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

>  	u32 total_ev_rings;
>  	u32 hw_ev_rings;
>  	u32 sw_ev_rings;
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

[PATCH v1 2/2] bus: mhi: core: Check for device supported event rings and channels

[Hemant Kumar]

It is possible that the device does not support the number of event
rings and channels that the controller would like to use. Read the
MHICFG to determine device-side support and if the controller requests
more than the device supports, bailout without configuring device MMIO
registers.

Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
---
 drivers/bus/mhi/core/init.c     | 31 +++++++++++++++++++++++++++++++
 drivers/bus/mhi/core/internal.h |  4 ++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index 70fd6af..35a6b1d 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -488,6 +488,37 @@ int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
 		{ 0, 0, 0 }
 	};
 
+	/* range check b/w host and device supported ev rings and channels */
+	ret = mhi_read_reg(mhi_cntrl, base, MHICFG, &val);
+	if (ret) {
+		dev_err(dev, "Unable to read MHICFG register\n");
+		return -EIO;
+	}
+
+	if (MHICFG_NHWER(val) < mhi_cntrl->hw_ev_rings) {
+		dev_err(dev, "#HWEV ring: host requires %d dev supports %d\n",
+			mhi_cntrl->hw_ev_rings, MHICFG_NHWER(val));
+		return -EIO;
+	}
+
+	if (MHICFG_NER(val) < mhi_cntrl->total_ev_rings) {
+		dev_err(dev, "#EV ring: host requires %d dev supports %d\n",
+			mhi_cntrl->total_ev_rings, MHICFG_NER(val));
+		return -EIO;
+	}
+
+	if (MHICFG_NHWCH(val) < mhi_cntrl->hw_chan) {
+		dev_err(dev, "#HWCH: host requires %d dev supports %d\n",
+			mhi_cntrl->hw_chan, MHICFG_NHWCH(val));
+		return -EIO;
+	}
+
+	if (MHICFG_NCH(val) < mhi_cntrl->max_chan) {
+		dev_err(dev, "#CH: host requires %d dev supports %d\n",
+			mhi_cntrl->max_chan, MHICFG_NCH(val));
+		return -EIO;
+	}
+
 	dev_dbg(dev, "Initializing MHI registers\n");
 
 	/* Read channel db offset */
diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index 3d8e480..9cbfa71 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -28,6 +28,10 @@ extern struct bus_type mhi_bus_type;
 #define MHICFG_NHWCH_SHIFT (8)
 #define MHICFG_NCH_MASK (0xFF)
 #define MHICFG_NCH_SHIFT (0)
+#define MHICFG_NHWER(n) (((n) & MHICFG_NHWER_MASK) >> MHICFG_NHWER_SHIFT)
+#define MHICFG_NER(n) (((n) & MHICFG_NER_MASK) >> MHICFG_NER_SHIFT)
+#define MHICFG_NHWCH(n) (((n) & MHICFG_NHWCH_MASK) >> MHICFG_NHWCH_SHIFT)
+#define MHICFG_NCH(n) (((n) & MHICFG_NCH_MASK) >> MHICFG_NCH_SHIFT)
 
 #define CHDBOFF (0x18)
 #define CHDBOFF_CHDBOFF_MASK (0xFFFFFFFF)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 2/2] bus: mhi: core: Check for device supported event rings and channels

[Manivannan Sadhasivam]

On Fri, Oct 23, 2020 at 07:00:43PM -0700, Hemant Kumar wrote:
> It is possible that the device does not support the number of event
> rings and channels that the controller would like to use. Read the
> MHICFG to determine device-side support and if the controller requests
> more than the device supports, bailout without configuring device MMIO
> registers.
> 
> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
> ---
>  drivers/bus/mhi/core/init.c     | 31 +++++++++++++++++++++++++++++++
>  drivers/bus/mhi/core/internal.h |  4 ++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 70fd6af..35a6b1d 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -488,6 +488,37 @@ int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
>  		{ 0, 0, 0 }
>  	};
>  
> +	/* range check b/w host and device supported ev rings and channels */
> +	ret = mhi_read_reg(mhi_cntrl, base, MHICFG, &val);
> +	if (ret) {
> +		dev_err(dev, "Unable to read MHICFG register\n");
> +		return -EIO;
> +	}
> +
> +	if (MHICFG_NHWER(val) < mhi_cntrl->hw_ev_rings) {
> +		dev_err(dev, "#HWEV ring: host requires %d dev supports %d\n",
> +			mhi_cntrl->hw_ev_rings, MHICFG_NHWER(val));

Can you please improve this error message? Something like, "Host
requires %d hw event rings but dev supports only %d".

Do this for below errors as well. With that,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> +		return -EIO;
> +	}
> +
> +	if (MHICFG_NER(val) < mhi_cntrl->total_ev_rings) {
> +		dev_err(dev, "#EV ring: host requires %d dev supports %d\n",
> +			mhi_cntrl->total_ev_rings, MHICFG_NER(val));
> +		return -EIO;
> +	}
> +
> +	if (MHICFG_NHWCH(val) < mhi_cntrl->hw_chan) {
> +		dev_err(dev, "#HWCH: host requires %d dev supports %d\n",
> +			mhi_cntrl->hw_chan, MHICFG_NHWCH(val));
> +		return -EIO;
> +	}
> +
> +	if (MHICFG_NCH(val) < mhi_cntrl->max_chan) {
> +		dev_err(dev, "#CH: host requires %d dev supports %d\n",
> +			mhi_cntrl->max_chan, MHICFG_NCH(val));
> +		return -EIO;
> +	}
> +
>  	dev_dbg(dev, "Initializing MHI registers\n");
>  
>  	/* Read channel db offset */
> diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
> index 3d8e480..9cbfa71 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -28,6 +28,10 @@ extern struct bus_type mhi_bus_type;
>  #define MHICFG_NHWCH_SHIFT (8)
>  #define MHICFG_NCH_MASK (0xFF)
>  #define MHICFG_NCH_SHIFT (0)
> +#define MHICFG_NHWER(n) (((n) & MHICFG_NHWER_MASK) >> MHICFG_NHWER_SHIFT)
> +#define MHICFG_NER(n) (((n) & MHICFG_NER_MASK) >> MHICFG_NER_SHIFT)
> +#define MHICFG_NHWCH(n) (((n) & MHICFG_NHWCH_MASK) >> MHICFG_NHWCH_SHIFT)
> +#define MHICFG_NCH(n) (((n) & MHICFG_NCH_MASK) >> MHICFG_NCH_SHIFT)
>  
>  #define CHDBOFF (0x18)
>  #define CHDBOFF_CHDBOFF_MASK (0xFFFFFFFF)
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>