| Message ID | 1589832241-13867-3-git-send-email-bbhatt@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce features and debugfs/sysfs entries for MHI | expand |
On 5/18/2020 2:03 PM, Bhaumik Bhatt wrote: > Allow independent votes from clients such that they can choose to vote > for either the device or the bus or both. This helps in cases where the > device supports autonomous low power mode wherein it can move to M2 > state without the need to notify the host. Clients can also vote only to > keep the underlying bus active without having the device in M0 state to > support offload use cases. > > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> > --- I wonder, why doesn't this fit with runtimePM?
On 2020-05-20 09:54, Jeffrey Hugo wrote: > On 5/18/2020 2:03 PM, Bhaumik Bhatt wrote: >> Allow independent votes from clients such that they can choose to vote >> for either the device or the bus or both. This helps in cases where >> the >> device supports autonomous low power mode wherein it can move to M2 >> state without the need to notify the host. Clients can also vote only >> to >> keep the underlying bus active without having the device in M0 state >> to >> support offload use cases. >> >> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> >> --- > > I wonder, why doesn't this fit with runtimePM? Hi Jeff, Can you elaborate? In short, with this patch, MHI just wants to give controller the option to choose the vote type so we can implement autonomous low power mode entries on both host and device. Let us know if you need some more information. Thanks, Bhaumik
On 5/20/2020 12:43 PM, bbhatt@codeaurora.org wrote: > On 2020-05-20 09:54, Jeffrey Hugo wrote: >> On 5/18/2020 2:03 PM, Bhaumik Bhatt wrote: >>> Allow independent votes from clients such that they can choose to vote >>> for either the device or the bus or both. This helps in cases where the >>> device supports autonomous low power mode wherein it can move to M2 >>> state without the need to notify the host. Clients can also vote only to >>> keep the underlying bus active without having the device in M0 state to >>> support offload use cases. >>> >>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> >>> --- >> >> I wonder, why doesn't this fit with runtimePM? > Hi Jeff, > > Can you elaborate? > > In short, with this patch, MHI just wants to give controller the option to > choose the vote type so we can implement autonomous low power mode entries > on both host and device. So, you are attempting to manage the power mode of the device. The standard mechanism to do so in Linux is runtime pm. https://elixir.bootlin.com/linux/latest/source/Documentation/driver-api/pm/devices.rst I'm no runtime pm expert, but it feels like your whole voting mechanism, etc is just reimplemeting that. Reimplementing the wheel, when its been a standard thing that the majority of the kernel uses is not usually acceptable. IMO, you need some sort of justification why runtime pm is not applicable for you, because I'm willing to bet Mani/Greg are going to ask the same.
On 2020-05-20 12:06, Jeffrey Hugo wrote: > On 5/20/2020 12:43 PM, bbhatt@codeaurora.org wrote: >> On 2020-05-20 09:54, Jeffrey Hugo wrote: >>> On 5/18/2020 2:03 PM, Bhaumik Bhatt wrote: >>>> Allow independent votes from clients such that they can choose to >>>> vote >>>> for either the device or the bus or both. This helps in cases where >>>> the >>>> device supports autonomous low power mode wherein it can move to M2 >>>> state without the need to notify the host. Clients can also vote >>>> only to >>>> keep the underlying bus active without having the device in M0 state >>>> to >>>> support offload use cases. >>>> >>>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> >>>> --- >>> >>> I wonder, why doesn't this fit with runtimePM? >> Hi Jeff, >> >> Can you elaborate? >> >> In short, with this patch, MHI just wants to give controller the >> option to >> choose the vote type so we can implement autonomous low power mode >> entries >> on both host and device. > > So, you are attempting to manage the power mode of the device. The > standard mechanism to do so in Linux is runtime pm. > > https://elixir.bootlin.com/linux/latest/source/Documentation/driver-api/pm/devices.rst > > I'm no runtime pm expert, but it feels like your whole voting > mechanism, etc is just reimplemeting that. Reimplementing the wheel, > when its been a standard thing that the majority of the kernel uses is > not usually acceptable. > > IMO, you need some sort of justification why runtime pm is not > applicable for you, because I'm willing to bet Mani/Greg are going to > ask the same. I think we can look at the patch as simply expanding the scope of what already exists. The client here has been calling mhi_device_get/put/sync APIs to gain device vote and with new features yet to come in, this introductory change is only re-purposing what voting means going forward. i.e. allowing individual bus and device votes. If you're suggesting using runtimePM APIs to replace the newly introduced bus vote, it would be kind of overkill here IMO. Is that what you were getting at? Because currently, we just have controllers use runtimePM and provide callbacks to them. If you have ideas, we can discuss them.
On 5/20/2020 1:44 PM, bbhatt@codeaurora.org wrote: > On 2020-05-20 12:06, Jeffrey Hugo wrote: >> On 5/20/2020 12:43 PM, bbhatt@codeaurora.org wrote: >>> On 2020-05-20 09:54, Jeffrey Hugo wrote: >>>> On 5/18/2020 2:03 PM, Bhaumik Bhatt wrote: >>>>> Allow independent votes from clients such that they can choose to vote >>>>> for either the device or the bus or both. This helps in cases where >>>>> the >>>>> device supports autonomous low power mode wherein it can move to M2 >>>>> state without the need to notify the host. Clients can also vote >>>>> only to >>>>> keep the underlying bus active without having the device in M0 >>>>> state to >>>>> support offload use cases. >>>>> >>>>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> >>>>> --- >>>> >>>> I wonder, why doesn't this fit with runtimePM? >>> Hi Jeff, >>> >>> Can you elaborate? >>> >>> In short, with this patch, MHI just wants to give controller the >>> option to >>> choose the vote type so we can implement autonomous low power mode >>> entries >>> on both host and device. >> >> So, you are attempting to manage the power mode of the device. The >> standard mechanism to do so in Linux is runtime pm. >> >> https://elixir.bootlin.com/linux/latest/source/Documentation/driver-api/pm/devices.rst >> >> >> I'm no runtime pm expert, but it feels like your whole voting >> mechanism, etc is just reimplemeting that. Reimplementing the wheel, >> when its been a standard thing that the majority of the kernel uses is >> not usually acceptable. >> >> IMO, you need some sort of justification why runtime pm is not >> applicable for you, because I'm willing to bet Mani/Greg are going to >> ask the same. > I think we can look at the patch as simply expanding the scope of what > already exists. > > The client here has been calling mhi_device_get/put/sync APIs to gain > device vote and with > new features yet to come in, this introductory change is only > re-purposing what voting > means going forward. i.e. allowing individual bus and device votes. > > If you're suggesting using runtimePM APIs to replace the newly > introduced bus vote, it > would be kind of overkill here IMO. Is that what you were getting at? > Because currently, > we just have controllers use runtimePM and provide callbacks to them. > > If you have ideas, we can discuss them. Ultimately, yes I think I am suggesting replacing this API with the runtime pm API. As near as I can tell, if I'm a device driver on some other bus, and I want to keep my device alive because I'm doing a DMA to it or something, I would call pm_runtime_get(), and when I have confirmation my activity is done, I would use pm_runtime_put(). As near as I can tell, this is already plumbed into the bus framework, such that the MHI bus would get a callback when the device driver does that. In the MHI bus callback, you would be able to route the request as needed. So, with this API (that has no consumers currently), I call pm_runtime_get() and also mhi_device_get()?
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c index e43a190..b7b5f7f 100644 --- a/drivers/bus/mhi/core/init.c +++ b/drivers/bus/mhi/core/init.c @@ -1062,7 +1062,8 @@ struct mhi_device *mhi_alloc_device(struct mhi_controller *mhi_cntrl) dev->release = mhi_release_device; dev->parent = mhi_cntrl->cntrl_dev; mhi_dev->mhi_cntrl = mhi_cntrl; - mhi_dev->dev_wake = 0; + atomic_set(&mhi_dev->dev_vote, 0); + atomic_set(&mhi_dev->bus_vote, 0); return mhi_dev; } @@ -1079,7 +1080,7 @@ static int mhi_driver_probe(struct device *dev) int ret; /* Bring device out of LPM */ - ret = mhi_device_get_sync(mhi_dev); + ret = mhi_device_get_sync(mhi_dev, MHI_VOTE_DEVICE); if (ret) return ret; @@ -1139,14 +1140,14 @@ static int mhi_driver_probe(struct device *dev) if (dl_chan && dl_chan->auto_start) mhi_prepare_channel(mhi_cntrl, dl_chan); - mhi_device_put(mhi_dev); + mhi_device_put(mhi_dev, MHI_VOTE_DEVICE); return ret; exit_probe: mhi_unprepare_from_transfer(mhi_dev); - mhi_device_put(mhi_dev); + mhi_device_put(mhi_dev, MHI_VOTE_DEVICE); return ret; } @@ -1215,8 +1216,10 @@ static int mhi_driver_remove(struct device *dev) } read_lock_bh(&mhi_cntrl->pm_lock); - while (mhi_dev->dev_wake) - mhi_device_put(mhi_dev); + while (atomic_read(&mhi_dev->dev_vote)) + mhi_device_put(mhi_dev, MHI_VOTE_DEVICE); + while (atomic_read(&mhi_dev->bus_vote)) + mhi_device_put(mhi_dev, MHI_VOTE_BUS); read_unlock_bh(&mhi_cntrl->pm_lock); return 0; diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c index 661d704..4c9812a 100644 --- a/drivers/bus/mhi/core/pm.c +++ b/drivers/bus/mhi/core/pm.c @@ -675,7 +675,8 @@ void mhi_pm_st_worker(struct work_struct *work) int mhi_pm_suspend(struct mhi_controller *mhi_cntrl) { struct mhi_chan *itr, *tmp; - struct device *dev = &mhi_cntrl->mhi_dev->dev; + struct mhi_device *mhi_dev = mhi_cntrl->mhi_dev; + struct device *dev = &mhi_dev->dev; enum mhi_pm_state new_state; int ret; @@ -687,7 +688,8 @@ int mhi_pm_suspend(struct mhi_controller *mhi_cntrl) /* Return busy if there are any pending resources */ if (atomic_read(&mhi_cntrl->dev_wake) || - atomic_read(&mhi_cntrl->pending_pkts)) + atomic_read(&mhi_cntrl->pending_pkts) || + atomic_read(&mhi_dev->bus_vote)) return -EBUSY; /* Take MHI out of M2 state */ @@ -714,7 +716,8 @@ int mhi_pm_suspend(struct mhi_controller *mhi_cntrl) write_lock_irq(&mhi_cntrl->pm_lock); if (atomic_read(&mhi_cntrl->dev_wake) || - atomic_read(&mhi_cntrl->pending_pkts)) { + atomic_read(&mhi_cntrl->pending_pkts) || + atomic_read(&mhi_dev->bus_vote)) { write_unlock_irq(&mhi_cntrl->pm_lock); return -EBUSY; } @@ -1109,42 +1112,74 @@ int mhi_force_rddm_mode(struct mhi_controller *mhi_cntrl) } EXPORT_SYMBOL_GPL(mhi_force_rddm_mode); -void mhi_device_get(struct mhi_device *mhi_dev) +void mhi_device_get(struct mhi_device *mhi_dev, int vote) { struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl; - mhi_dev->dev_wake++; - read_lock_bh(&mhi_cntrl->pm_lock); - mhi_cntrl->wake_get(mhi_cntrl, true); - read_unlock_bh(&mhi_cntrl->pm_lock); + if (vote & MHI_VOTE_DEVICE) { + atomic_inc(&mhi_dev->dev_vote); + read_lock_bh(&mhi_cntrl->pm_lock); + mhi_cntrl->wake_get(mhi_cntrl, true); + read_unlock_bh(&mhi_cntrl->pm_lock); + } + + if (vote & MHI_VOTE_BUS) { + atomic_inc(&mhi_dev->bus_vote); + mhi_cntrl->runtime_get(mhi_cntrl); + } } EXPORT_SYMBOL_GPL(mhi_device_get); -int mhi_device_get_sync(struct mhi_device *mhi_dev) +int mhi_device_get_sync(struct mhi_device *mhi_dev, int vote) { struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl; int ret; + /* bring device out of low power modes regardless of the type of vote */ ret = __mhi_device_get_sync(mhi_cntrl); - if (!ret) - mhi_dev->dev_wake++; + if (ret) + return ret; + + if (vote & MHI_VOTE_DEVICE) { + atomic_inc(&mhi_dev->dev_vote); + } else { + /* remove device vote as it was not requested */ + read_lock_bh(&mhi_cntrl->pm_lock); + mhi_cntrl->wake_put(mhi_cntrl, false); + read_unlock_bh(&mhi_cntrl->pm_lock); + } + + if (vote & MHI_VOTE_BUS) { + atomic_inc(&mhi_dev->bus_vote); + mhi_cntrl->runtime_get(mhi_cntrl); + } return ret; } EXPORT_SYMBOL_GPL(mhi_device_get_sync); -void mhi_device_put(struct mhi_device *mhi_dev) +void mhi_device_put(struct mhi_device *mhi_dev, int vote) { struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl; - mhi_dev->dev_wake--; - read_lock_bh(&mhi_cntrl->pm_lock); - if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) { - mhi_cntrl->runtime_get(mhi_cntrl); - mhi_cntrl->runtime_put(mhi_cntrl); + if (vote & MHI_VOTE_DEVICE) { + atomic_dec(&mhi_dev->dev_vote); + read_lock_bh(&mhi_cntrl->pm_lock); + if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) { + mhi_cntrl->runtime_get(mhi_cntrl); + mhi_cntrl->runtime_put(mhi_cntrl); + } + mhi_cntrl->wake_put(mhi_cntrl, false); + read_unlock_bh(&mhi_cntrl->pm_lock); } - mhi_cntrl->wake_put(mhi_cntrl, false); - read_unlock_bh(&mhi_cntrl->pm_lock); + if (vote & MHI_VOTE_BUS) { + atomic_dec(&mhi_dev->bus_vote); + mhi_cntrl->runtime_put(mhi_cntrl); + + /* notify controller that all bus votes are removed */ + if (!atomic_read(&mhi_dev->bus_vote)) + mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_IDLE); + } } EXPORT_SYMBOL_GPL(mhi_device_put); diff --git a/include/linux/mhi.h b/include/linux/mhi.h index b008914..10fcb52 100644 --- a/include/linux/mhi.h +++ b/include/linux/mhi.h @@ -16,6 +16,9 @@ #include <linux/wait.h> #include <linux/workqueue.h> +#define MHI_VOTE_BUS BIT(0) /* do not disable the mhi bus */ +#define MHI_VOTE_DEVICE BIT(1) /* prevent mhi device from entering lpm */ + struct mhi_chan; struct mhi_event; struct mhi_ctxt; @@ -459,7 +462,8 @@ struct mhi_device { enum mhi_device_type dev_type; int ul_chan_id; int dl_chan_id; - u32 dev_wake; + atomic_t dev_vote; + atomic_t bus_vote; }; /** @@ -644,23 +648,26 @@ void mhi_set_mhi_state(struct mhi_controller *mhi_cntrl, enum mhi_state mhi_get_mhi_state(struct mhi_controller *mhi_cntrl); /** - * mhi_device_get - Disable device low power mode + * mhi_device_get - Disable device and/or bus low power mode * @mhi_dev: Device associated with the channel + * @vote: requested vote (bus, device or both) */ -void mhi_device_get(struct mhi_device *mhi_dev); +void mhi_device_get(struct mhi_device *mhi_dev, int vote); /** - * mhi_device_get_sync - Disable device low power mode. Synchronously + * mhi_device_get_sync - Disable device and/or bus low power mode. Synchronously * take the controller out of suspended state * @mhi_dev: Device associated with the channel + * @vote: requested vote (bus, device or both) */ -int mhi_device_get_sync(struct mhi_device *mhi_dev); +int mhi_device_get_sync(struct mhi_device *mhi_dev, int vote); /** - * mhi_device_put - Re-enable device low power mode + * mhi_device_put - Re-enable device and/or bus low power mode * @mhi_dev: Device associated with the channel + * @vote: vote(s) to remove (bus, device or both) */ -void mhi_device_put(struct mhi_device *mhi_dev); +void mhi_device_put(struct mhi_device *mhi_dev, int vote); /** * mhi_prepare_for_transfer - Setup channel for data transfer
Allow independent votes from clients such that they can choose to vote for either the device or the bus or both. This helps in cases where the device supports autonomous low power mode wherein it can move to M2 state without the need to notify the host. Clients can also vote only to keep the underlying bus active without having the device in M0 state to support offload use cases. Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> --- drivers/bus/mhi/core/init.c | 15 ++++++---- drivers/bus/mhi/core/pm.c | 73 +++++++++++++++++++++++++++++++++------------ include/linux/mhi.h | 21 ++++++++----- 3 files changed, 77 insertions(+), 32 deletions(-)