| Message ID | 20220728071637.22364-1-peter.wang@mediatek.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ufs: allow vendor disable wb toggle in clock scaling | expand |
> > From: Peter Wang <peter.wang@mediatek.com> > > Mediatek ufs do not want to toggle write booster when clock scaling. > This patch set allow vendor disable wb toggle in clock scaling. > > Peter Wang (2): > ufs: core: interduce a choice of wb toggle in clock scaling > ufs: host: support wb toggle with clock scaling It would make more sense to squash both patches and add it as #6 after - " Support clk-scaling to optimize power consumption" in your earlier " Provide features and fixes in MediaTek platforms" series. Either way, Looks good to me. Thanks, Avri
On 7/28/22 00:16, peter.wang@mediatek.com wrote: > Mediatek ufs do not want to toggle write booster when clock scaling. > This patch set allow vendor disable wb toggle in clock scaling. I don't like this approach. Whether or not to toggle the write booster when scaling the clock is not dependent on the host controller and hence should not depend on the host controller driver. Has it been considered to add a sysfs attribute in the UFS driver core to control this behavior? Thanks, Bart.
On Thu, 2022-07-28 at 14:09 -0700, Bart Van Assche wrote: > On 7/28/22 00:16, peter.wang@mediatek.com wrote: > > Mediatek ufs do not want to toggle write booster when clock > > scaling. > > This patch set allow vendor disable wb toggle in clock scaling. > > I don't like this approach. Whether or not to toggle the write > booster > when scaling the clock is not dependent on the host controller and > hence > should not depend on the host controller driver. > > Has it been considered to add a sysfs attribute in the UFS driver > core > to control this behavior? > Bart, we already have wb_on sysfs node, but it only allows to write this node when clock scaling is not supported. static ssize_t wb_on_store(..) { struct ufs_hba *hba = dev_get_drvdata(dev); unsigned int wb_enable; ssize_t res; if (ufshcd_is_clkscaling_supported(hba)) { /* * If the platform supports UFSHCD_CAP_AUTO_BKOPS_SUSPEND, * turn WB on/off will be done while clock scaling up/down. */ dev_warn(dev, "To control WB through wb_on is not allowed!\n"); return -EOPNOTSUPP; } Kind regards, Bean > Thanks, > > Bart.
Hi, On Fri, Jul 29, 2022 at 5:27 AM Bean Huo <huobean@gmail.com> wrote: > > On Thu, 2022-07-28 at 14:09 -0700, Bart Van Assche wrote: > > On 7/28/22 00:16, peter.wang@mediatek.com wrote: > > > Mediatek ufs do not want to toggle write booster when clock > > > scaling. > > > This patch set allow vendor disable wb toggle in clock scaling. > > > > I don't like this approach. Whether or not to toggle the write > > booster > > when scaling the clock is not dependent on the host controller and > > hence > > should not depend on the host controller driver. > > > > Has it been considered to add a sysfs attribute in the UFS driver > > core > > to control this behavior? > > > > Bart, > we already have wb_on sysfs node, but it only allows to write this node > when clock scaling is not supported. > > > static ssize_t wb_on_store(..) > { > struct ufs_hba *hba = dev_get_drvdata(dev); > unsigned int wb_enable; > ssize_t res; > > if (ufshcd_is_clkscaling_supported(hba)) { > /* > * If the platform supports > UFSHCD_CAP_AUTO_BKOPS_SUSPEND, > * turn WB on/off will be done while clock scaling > up/down. > */ > dev_warn(dev, "To control WB through wb_on is not > allowed!\n"); > return -EOPNOTSUPP; > } > > > Kind regards, > Bean > > > Thanks, > > > > Bart. Acked to this patch series. Clk-Scaling is aimed to save power by fine-tuning any possible resources depending on the workload. Currently below items would be tuned, 1. Clk rate 2. Gear 3. Write Booster switch on/off The truth is that each host (and device) vendor has different designs to reduce the power, therefore those tunings may not be suitable or need different ways for all host platforms. Take below cases for example, 1. The clk cannot be set at different rates directly because it is shared with other users. 2. The Write Booster feature does not need to be disabled because this impacts the performance too much. Performance may be even worse than the case when clk-scaling is disabled. In addition, system power may be not reduced if a long data write period is harmful to the system low-power design. Thanks, Stanley
On 7/29/22 4:43 AM, Avri Altman wrote: >> From: Peter Wang <peter.wang@mediatek.com> >> >> Mediatek ufs do not want to toggle write booster when clock scaling. >> This patch set allow vendor disable wb toggle in clock scaling. >> >> Peter Wang (2): >> ufs: core: interduce a choice of wb toggle in clock scaling >> ufs: host: support wb toggle with clock scaling > It would make more sense to squash both patches and add it as #6 after - > " Support clk-scaling to optimize power consumption" in your earlier > " Provide features and fixes in MediaTek platforms" series. > > Either way, Looks good to me. > > Thanks, > Avri Hi Arvi, Yes, it make more sense to enable clock scaling in mediatek platform first. I will wait "Provide features and fixes in MediaTek platforms" patch set accepted than update this patch. Thanks. Peter
On 7/29/22 5:09 AM, Bart Van Assche wrote: > On 7/28/22 00:16, peter.wang@mediatek.com wrote: >> Mediatek ufs do not want to toggle write booster when clock scaling. >> This patch set allow vendor disable wb toggle in clock scaling. > > I don't like this approach. Whether or not to toggle the write booster > when scaling the clock is not dependent on the host controller and > hence should not depend on the host controller driver. > > Has it been considered to add a sysfs attribute in the UFS driver core > to control this behavior? > > Thanks, > > Bart. Hi Bart, Write booster binding with clock scaling is not make sense. Clock scaling should always do clock scaling related things, and write bootster is not related to clock, right? So Mediatek don't want to toggle wb with clock scaling. Consider legacy design is binding, so we provide a flag to decouple them instead remove ufshcd_wb_toggle directly. Or, do you think we can direct remove ufshcd_wb_toggle in clock scaling and only let sysfs to control wb behavior? Thanks. Peter
On 8/1/22 07:30, Peter Wang wrote: > Or, do you think we can direct remove ufshcd_wb_toggle in clock scaling > and only let sysfs to control wb behavior? I think it's worth asking the people who introduced this feature whether it can be removed. Hi Asutosh, Commit 3d17b9b5ab11 ("scsi: ufs: Add write booster feature support") introduced the following code in ufshcd_devfreq_scale(): + /* Enable Write Booster if we have scaled up else disable it */ + up_write(&hba->clk_scaling_lock); + ufshcd_wb_ctrl(hba, scale_up); + down_write(&hba->clk_scaling_lock); Would you mind if the code for enabling/disabling the WriteBooster is removed again from ufshcd_devfreq_scale() and that a new mechanism is introduced for controlling the WriteBooster mechanism? Thanks, Bart.
Please fix up your wording. A vendor can't do anything at all in Linux. A driver might be able to disable things for a specific device, though.
Hello, On 8/1/2022 9:43 AM, Bart Van Assche wrote: > On 8/1/22 07:30, Peter Wang wrote: >> Or, do you think we can direct remove ufshcd_wb_toggle in clock >> scaling and only let sysfs to control wb behavior? > > I think it's worth asking the people who introduced this feature whether > it can be removed. > > Hi Asutosh, > > Commit 3d17b9b5ab11 ("scsi: ufs: Add write booster feature support") > introduced the following code in ufshcd_devfreq_scale(): > > + /* Enable Write Booster if we have scaled up else disable it */ > + up_write(&hba->clk_scaling_lock); > + ufshcd_wb_ctrl(hba, scale_up); > + down_write(&hba->clk_scaling_lock); > > Would you mind if the code for enabling/disabling the WriteBooster is > removed again from ufshcd_devfreq_scale() and that a new mechanism is > introduced for controlling the WriteBooster mechanism? > Qualcomm would need the load based toggling of WB during scaling. What would the new mechanism be for controlling WB mechanism? > Thanks, > > Bart.
On 8/1/22 10:57, Christoph Hellwig wrote: > Please fix up your wording. A vendor can't do anything at all in Linux. > A driver might be able to disable things for a specific device, though. Agreed that the description should become more clear. I think Peter wanted to refer to the "vendor driver" where he wrote "vendor". Bart.
On Mon, Aug 01, 2022 at 11:12:17AM -0700, Bart Van Assche wrote: > Agreed that the description should become more clear. I think Peter wanted > to refer to the "vendor driver" where he wrote "vendor". vendor driver makes just as little sense. hardware interfaces do not map to vendors in any sensible way.
On 8/2/22 2:14 AM, Christoph Hellwig wrote: > On Mon, Aug 01, 2022 at 11:12:17AM -0700, Bart Van Assche wrote: >> Agreed that the description should become more clear. I think Peter wanted >> to refer to the "vendor driver" where he wrote "vendor". > vendor driver makes just as little sense. hardware interfaces do not > map to vendors in any sensible way. Hi Christoph, Will change commit message. Thanks Peter
From: Peter Wang <peter.wang@mediatek.com> Mediatek ufs do not want to toggle write booster when clock scaling. This patch set allow vendor disable wb toggle in clock scaling. Peter Wang (2): ufs: core: interduce a choice of wb toggle in clock scaling ufs: host: support wb toggle with clock scaling drivers/ufs/core/ufs-sysfs.c | 3 ++- drivers/ufs/core/ufshcd.c | 8 +++++--- drivers/ufs/host/ufs-qcom.c | 2 +- include/ufs/ufshcd.h | 7 +++++++ 4 files changed, 15 insertions(+), 5 deletions(-)