On Fri, Jul 11, 2025 at 12:25:30PM GMT, Krishna Chaitanya Chundru wrote:
[...]
> > > > > +static int mhi_init_bw_scale(struct mhi_controller *mhi_cntrl,
> > > > > + int bw_scale_db)
> > > > > +{
> > > > > + struct device *dev = &mhi_cntrl->mhi_dev->dev;
> > > > > + u32 bw_cfg_offset, val;
> > > > > + int ret, er_index;
> > > > > +
> > > > > + ret = mhi_find_capability(mhi_cntrl, MHI_BW_SCALE_CAP_ID, &bw_cfg_offset);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + er_index = mhi_get_er_index(mhi_cntrl, MHI_ER_BW_SCALE);
> > > > > + if (er_index < 0)
> > > > > + return er_index;
> > > > > +
> > > > > + bw_cfg_offset += MHI_BW_SCALE_CFG_OFFSET;
> > > > > +
> > > > > + /* Advertise host support */
> > > > > + val = (__force u32)cpu_to_le32(FIELD_PREP(MHI_BW_SCALE_DB_CHAN_ID, bw_scale_db) |
> > > > > + FIELD_PREP(MHI_BW_SCALE_ER_INDEX, er_index) |
> > > > > + MHI_BW_SCALE_ENABLED);
> > > > > +
> > > >
> > > > It is wrong to store the value of cpu_to_le32() in a non-le32 variable.
> > > > mhi_write_reg() accepts the 'val' in native endian. writel used in the
> > > > controller drivers should take care of converting to LE before writing to the
> > > > device.
> > > >
> > > ok then I will revert to u32.
> > >
> > > I think we need a patch in the controller drivers seperately to handle
> > > this.
> >
> > Why?
> >
> what I understood from your previous comment is from here we need to
> send u32 only and the controller drivers should take care of
> converting u32 to le32.
> As of today controller drivers are not considering this and writing
> u32 only.
> So we need a seperate patch in the controller driver to convert it to
> le32.
No. All controller drivers are using writel() to write the register. Since
writel() converts the value to little endian from native endian, you do not need
to do anything.
> > > > > + mhi_write_reg(mhi_cntrl, mhi_cntrl->regs, bw_cfg_offset, val);
> > > > > +
> > > > > + dev_dbg(dev, "Bandwidth scaling setup complete with event ring: %d\n",
> > > > > + er_index);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
[...]
> > > > > + link_info.target_link_speed = MHI_TRE_GET_EV_LINKSPEED(dev_rp);
> > > > > + link_info.target_link_width = MHI_TRE_GET_EV_LINKWIDTH(dev_rp);
> > > > > + link_info.sequence_num = MHI_TRE_GET_EV_BW_REQ_SEQ(dev_rp);
> > > > > +
> > > > > + dev_dbg(dev, "Received BW_REQ with seq:%d link speed:0x%x width:0x%x\n",
> > > > > + link_info.sequence_num,
> > > > > + link_info.target_link_speed,
> > > > > + link_info.target_link_width);
> > > > > +
> > > > > + /* Bring host and device out of suspended states */
> > > > > + ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
> > > >
> > > > Looks like mhi_device_get_sync() is going runtime_get()/runtime_put() inside
> > > > mhi_trigger_resume(). I'm wondering why that is necessary.
> > > >
> > > Before mhi_trigger_resume we are doing wake_get, which will make sure
> > > device will not transition to the low power modes while servicing this
> > > event. And also make sure mhi state is in M0 only.
> > >
> > > As we are in workqueue this can be scheduled at some later time and by
> > > that time mhi can go to m1 or m2 state.
> > >
> >
> > My comment was more about the behavior of mhi_trigger_resume(). Why does it call
> > get() and put()? Sorry if I was not clear.
> > Get() needed for bringing out of suspend, put() is for balancing the
> get() I belive the intention here is to trigger resume only and balance
> pm framework.
>
> That said mhi_device_get_sync() has a bug we are doing wake_get() before
> triggering resume and also trigger resume will not guarantee that device
> had resumed, it is not safe to do wake_get untill device is out of
> suspend, we might need to introduce runtime_get_sync() function and new
> API for this purpose.
> mhi_trigger_resume makes sense in the place like this[1], where ever
> there are register writes after trigger resume it may need to be
> replaced with runtime_get_sync().
>
> This function needs to do mhi_cntrl->runtime_get_sync(mhi_cntrl); first
> to make sure controller is not in suspend and then mhi_device_get_sync()
> to make sure device is staying in M0. and in the end we balance these
> with mhi_cntrl->runtime_put(mhi_cntrl) & mhi_device_put().
>
Sounds good to me.
- Mani
--
மணிவண்ணன் சதாசிவம்
