Re: [PATCH] Revert "bus: mhi: core: Add support for reading MHI info from device"

From: Manivannan Sadhasivam
Date: Wed Feb 21 2024 - 00:47:52 EST


On Mon, Feb 19, 2024 at 11:07:48AM -0700, Jeffrey Hugo wrote:
> This reverts commit 3316ab2b45f6bf4797d8d65b22fda3cc13318890.
>
> The MHI spec owner pointed out that the SOC_HW_VERSION register is part
> of the BHIe segment, and only valid on devices which implement BHIe.
> Only a small subset of MHI devices implement BHIe so blindly accessing
> the register for all devices is not correct. Also, since the BHIe
> segment offset is not used when accessing the register, any
> implementation which moves the BHIe segment will result in accessing
> some other register. We've seen that accessing this register on AIC100
> which does not support BHIe can result in initialization failures.
>
> We could try to put checks into the code to address these issues, but in
> the roughly 4 years this functionality has existed, no one has used it.
> Easier to drop this dead code and address the issues if anyone comes up
> with a real world use for it.
>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@xxxxxxxxxxx>

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

- Mani

> ---
> drivers/bus/mhi/host/init.c | 12 ------------
> drivers/bus/mhi/host/internal.h | 6 ------
> include/linux/mhi.h | 17 -----------------
> 3 files changed, 35 deletions(-)
>
> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> index 944da46e6f11..44f934981de8 100644
> --- a/drivers/bus/mhi/host/init.c
> +++ b/drivers/bus/mhi/host/init.c
> @@ -914,7 +914,6 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
> struct mhi_chan *mhi_chan;
> struct mhi_cmd *mhi_cmd;
> struct mhi_device *mhi_dev;
> - u32 soc_info;
> int ret, i;
>
> if (!mhi_cntrl || !mhi_cntrl->cntrl_dev || !mhi_cntrl->regs ||
> @@ -989,17 +988,6 @@ 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 err_destroy_wq;
> -
> - mhi_cntrl->family_number = FIELD_GET(SOC_HW_VERSION_FAM_NUM_BMSK, soc_info);
> - mhi_cntrl->device_number = FIELD_GET(SOC_HW_VERSION_DEV_NUM_BMSK, soc_info);
> - mhi_cntrl->major_version = FIELD_GET(SOC_HW_VERSION_MAJOR_VER_BMSK, soc_info);
> - mhi_cntrl->minor_version = FIELD_GET(SOC_HW_VERSION_MINOR_VER_BMSK, soc_info);
> -
> mhi_cntrl->index = ida_alloc(&mhi_controller_ida, GFP_KERNEL);
> if (mhi_cntrl->index < 0) {
> ret = mhi_cntrl->index;
> diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
> index 091244cf17c6..5fe49311b8eb 100644
> --- a/drivers/bus/mhi/host/internal.h
> +++ b/drivers/bus/mhi/host/internal.h
> @@ -15,12 +15,6 @@ extern struct bus_type mhi_bus_type;
> #define MHI_SOC_RESET_REQ_OFFSET 0xb0
> #define MHI_SOC_RESET_REQ BIT(0)
>
> -#define SOC_HW_VERSION_OFFS 0x224
> -#define SOC_HW_VERSION_FAM_NUM_BMSK GENMASK(31, 28)
> -#define SOC_HW_VERSION_DEV_NUM_BMSK GENMASK(27, 16)
> -#define SOC_HW_VERSION_MAJOR_VER_BMSK GENMASK(15, 8)
> -#define SOC_HW_VERSION_MINOR_VER_BMSK GENMASK(7, 0)
> -
> struct mhi_ctxt {
> struct mhi_event_ctxt *er_ctxt;
> struct mhi_chan_ctxt *chan_ctxt;
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index 474d32cb0520..77b8c0a26674 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -320,10 +320,6 @@ struct mhi_controller_config {
> * @hw_ev_rings: Number of hardware event rings
> * @sw_ev_rings: Number of software event rings
> * @nr_irqs: Number of IRQ allocated by bus master (required)
> - * @family_number: MHI controller family number
> - * @device_number: MHI controller device number
> - * @major_version: MHI controller major revision number
> - * @minor_version: MHI controller minor revision number
> * @serial_number: MHI controller serial number obtained from BHI
> * @mhi_event: MHI event ring configurations table
> * @mhi_cmd: MHI command ring configurations table
> @@ -368,15 +364,6 @@ struct mhi_controller_config {
> * Fields marked as (required) need to be populated by the controller driver
> * before calling mhi_register_controller(). For the fields marked as (optional)
> * they can be populated depending on the usecase.
> - *
> - * The following fields are present for the purpose of implementing any device
> - * specific quirks or customizations for specific MHI revisions used in device
> - * by the controller drivers. The MHI stack will just populate these fields
> - * during mhi_register_controller():
> - * family_number
> - * device_number
> - * major_version
> - * minor_version
> */
> struct mhi_controller {
> struct device *cntrl_dev;
> @@ -407,10 +394,6 @@ struct mhi_controller {
> u32 hw_ev_rings;
> u32 sw_ev_rings;
> u32 nr_irqs;
> - u32 family_number;
> - u32 device_number;
> - u32 major_version;
> - u32 minor_version;
> u32 serial_number;
>
> struct mhi_event *mhi_event;
> --
> 2.34.1
>

--
மணிவண்ணன் சதாசிவம்