Re: [PATCH v3 2/7] bus: mhi: core: Add support for reading MHI info from device

[Jeffrey Hugo]

On 3/24/2020 8:38 AM, Jeffrey Hugo wrote:
> On 3/24/2020 12:10 AM, Manivannan Sadhasivam wrote:
> > The MHI register base has several registers used for getting the MHI
> > specific information such as version, family, major, and minor numbers
> > from the device. This information can be used by the controller drivers
> > for usecases such as applying quirks for a specific revision etc...
> >
> > While at it, let's also rearrange the local variables
> > in mhi_register_controller().
> >
> > Suggested-by: Hemant Kumar <hemantk@xxxxxxxxxxxxxx>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > ---
> > Â drivers/bus/mhi/core/init.cÂÂÂÂ | 19 +++++++++++++++++--
> > Â drivers/bus/mhi/core/internal.h | 10 ++++++++++
> > Â include/linux/mhi.hÂÂÂÂÂÂÂÂÂÂÂÂ | 17 +++++++++++++++++
> > Â 3 files changed, 44 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> > index eb7f556a8531..d136f6c6ca78 100644
> > --- a/drivers/bus/mhi/core/init.c
> > +++ b/drivers/bus/mhi/core/init.c
> > @@ -802,12 +802,12 @@ static int parse_config(struct mhi_controller 
> > *mhi_cntrl,
> > Â int mhi_register_controller(struct mhi_controller *mhi_cntrl,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct mhi_controller_config *config)
> > Â {
> > -ÂÂÂ int ret;
> > -ÂÂÂ int i;
> > ÂÂÂÂÂ struct mhi_event *mhi_event;
> > ÂÂÂÂÂ struct mhi_chan *mhi_chan;
> > ÂÂÂÂÂ struct mhi_cmd *mhi_cmd;
> > ÂÂÂÂÂ struct mhi_device *mhi_dev;
> > +ÂÂÂ u32 soc_info;
> > +ÂÂÂ int ret, i;
> > ÂÂÂÂÂ if (!mhi_cntrl)
> > ÂÂÂÂÂÂÂÂÂ return -EINVAL;
> > @@ -874,6 +874,21 @@ int mhi_register_controller(struct mhi_controller 
> > *mhi_cntrl,
> > ÂÂÂÂÂÂÂÂÂ mhi_cntrl->unmap_single = mhi_unmap_single_no_bb;
> > ÂÂÂÂÂ }
> > +ÂÂÂ /* Read the MHI device info */
> > +ÂÂÂ ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->regs,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SOC_HW_VERSION_OFFS, &soc_info);
> > +ÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂ goto error_alloc_dev;
> > +
> > +ÂÂÂ mhi_cntrl->family_number = (soc_info & 
> > SOC_HW_VERSION_FAM_NUM_BMSK) >>
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SOC_HW_VERSION_FAM_NUM_SHFT;
> > +ÂÂÂ mhi_cntrl->device_number = (soc_info & 
> > SOC_HW_VERSION_DEV_NUM_BMSK) >>
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SOC_HW_VERSION_DEV_NUM_SHFT;
> > +ÂÂÂ mhi_cntrl->major_version = (soc_info & 
> > SOC_HW_VERSION_MAJOR_VER_BMSK) >>
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SOC_HW_VERSION_MAJOR_VER_SHFT;
> > +ÂÂÂ mhi_cntrl->minor_version = (soc_info & 
> > SOC_HW_VERSION_MINOR_VER_BMSK) >>
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SOC_HW_VERSION_MINOR_VER_SHFT;
> > +
> > ÂÂÂÂÂ /* Register controller with MHI bus */
> > ÂÂÂÂÂ mhi_dev = mhi_alloc_device(mhi_cntrl);
> > ÂÂÂÂÂ if (IS_ERR(mhi_dev)) {
> > diff --git a/drivers/bus/mhi/core/internal.h 
> > b/drivers/bus/mhi/core/internal.h
> > index 18066302e6e2..5deadfaa053a 100644
> > --- a/drivers/bus/mhi/core/internal.h
> > +++ b/drivers/bus/mhi/core/internal.h
> > @@ -196,6 +196,16 @@ extern struct bus_type mhi_bus_type;
> > Â #define BHIE_RXVECSTATUS_STATUS_XFER_COMPL (0x02)
> > Â #define BHIE_RXVECSTATUS_STATUS_ERROR (0x03)
> > +#define SOC_HW_VERSION_OFFS (0x224)
> > +#define SOC_HW_VERSION_FAM_NUM_BMSK (0xF0000000)
> > +#define SOC_HW_VERSION_FAM_NUM_SHFT (28)
> > +#define SOC_HW_VERSION_DEV_NUM_BMSK (0x0FFF0000)
> > +#define SOC_HW_VERSION_DEV_NUM_SHFT (16)
> > +#define SOC_HW_VERSION_MAJOR_VER_BMSK (0x0000FF00)
> > +#define SOC_HW_VERSION_MAJOR_VER_SHFT (8)
> > +#define SOC_HW_VERSION_MINOR_VER_BMSK (0x000000FF)
> > +#define SOC_HW_VERSION_MINOR_VER_SHFT (0)
>
> I'm tempted to give reviewed-by, however it occurs to me that I don't 
> see this in the MHI spec. I'm looking at Rev E, which as far as I am 
> aware is the latest.
>
> Hemant, is this in the spec, and if so, what Rev?
>
> I'm concerned that if its not in the spec, we may have an issue with 
> some device not implementing this as expected.

Reviewed-by: Jeffrey Hugo <jhugo@xxxxxxxxxxxxxx>

I talked with Hemant offline, and this is queued up for the next spec 
rev.  I'm satisfied with that.

--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.