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

2020-11-06 Thread 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 
> ---
>  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 

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 (0x)
> -- 
> 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

2020-10-23 Thread 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 
---
 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 (0x)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project