| Message ID | 20181106105314.30069-3-acelan.kao@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Power consumption during s2idle is higher than long idle(sk hynix) | expand |
On 11/06/18 11:53, AceLan Kao wrote: > BugLink: https://bugs.launchpad.net/bugs/1801875 > > Call nvme_dev_disable() function leads to the power consumption goes > up to 2.2 Watt during suspend-to-idle, and from SK hynix FE, they > suggest us to use its own APST feature to do the power management during > s2idle. > After D3 is diabled and nvme_dev_disable() is not called while > suspending, the power consumption drops to 0.77 Watt during s2idle. > > Signed-off-by: AceLan Kao <acelan.kao@canonical.com> > --- > drivers/nvme/host/nvme.h | 5 +++++ > drivers/nvme/host/pci.c | 8 +++++++- > 2 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h > index a9ec71cce3cc..c2f71cb23700 100644 > --- a/drivers/nvme/host/nvme.h > +++ b/drivers/nvme/host/nvme.h > @@ -86,6 +86,11 @@ enum nvme_quirks { > * Set MEDIUM priority on SQ creation > */ > NVME_QUIRK_MEDIUM_PRIO_SQ = (1 << 7), > + > + /* > + * Do not disable nvme when suspending(s2idle) > + */ > + NVME_QUIRK_NO_DISABLE = (1 << 8), > }; > > /* > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > index 3be974ff4261..5f797d595e52 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -29,6 +29,7 @@ > #include <linux/types.h> > #include <linux/io-64-nonatomic-lo-hi.h> > #include <linux/sed-opal.h> > +#include <linux/suspend.h> > > #include "nvme.h" > > @@ -2611,8 +2612,11 @@ static int nvme_suspend(struct device *dev) > { > struct pci_dev *pdev = to_pci_dev(dev); > struct nvme_dev *ndev = pci_get_drvdata(pdev); > + struct nvme_ctrl *ctrl = &ndev->ctrl; > + > + if (!(pm_suspend_via_s2idle() && (ctrl->quirks & NVME_QUIRK_NO_DISABLE))) > + nvme_dev_disable(ndev, true); > > - nvme_dev_disable(ndev, true); > return 0; > } Hi AceLan, Are there any plans to submit these changes, at least the generic part, to mainline? Thanks, Kleber > > @@ -2709,6 +2713,8 @@ static const struct pci_device_id nvme_id_table[] = { > .driver_data = NVME_QUIRK_LIGHTNVM, }, > { PCI_DEVICE(0x1d1d, 0x2807), /* CNEX WL */ > .driver_data = NVME_QUIRK_LIGHTNVM, }, > + { PCI_VDEVICE(SK_HYNIX, 0x1527), /* Sk Hynix */ > + .driver_data = NVME_QUIRK_NO_DISABLE, }, > { PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) }, > { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) }, > { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2003) },
I did, although maintainer doesn't like it much. https://lkml.org/lkml/2018/11/6/583 Kleber Souza <kleber.souza@canonical.com> 於 2018年11月7日 週三 上午12:48寫道： > > On 11/06/18 11:53, AceLan Kao wrote: > > BugLink: https://bugs.launchpad.net/bugs/1801875 > > > > Call nvme_dev_disable() function leads to the power consumption goes > > up to 2.2 Watt during suspend-to-idle, and from SK hynix FE, they > > suggest us to use its own APST feature to do the power management during > > s2idle. > > After D3 is diabled and nvme_dev_disable() is not called while > > suspending, the power consumption drops to 0.77 Watt during s2idle. > > > > Signed-off-by: AceLan Kao <acelan.kao@canonical.com> > > --- > > drivers/nvme/host/nvme.h | 5 +++++ > > drivers/nvme/host/pci.c | 8 +++++++- > > 2 files changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h > > index a9ec71cce3cc..c2f71cb23700 100644 > > --- a/drivers/nvme/host/nvme.h > > +++ b/drivers/nvme/host/nvme.h > > @@ -86,6 +86,11 @@ enum nvme_quirks { > > * Set MEDIUM priority on SQ creation > > */ > > NVME_QUIRK_MEDIUM_PRIO_SQ = (1 << 7), > > + > > + /* > > + * Do not disable nvme when suspending(s2idle) > > + */ > > + NVME_QUIRK_NO_DISABLE = (1 << 8), > > }; > > > > /* > > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > > index 3be974ff4261..5f797d595e52 100644 > > --- a/drivers/nvme/host/pci.c > > +++ b/drivers/nvme/host/pci.c > > @@ -29,6 +29,7 @@ > > #include <linux/types.h> > > #include <linux/io-64-nonatomic-lo-hi.h> > > #include <linux/sed-opal.h> > > +#include <linux/suspend.h> > > > > #include "nvme.h" > > > > @@ -2611,8 +2612,11 @@ static int nvme_suspend(struct device *dev) > > { > > struct pci_dev *pdev = to_pci_dev(dev); > > struct nvme_dev *ndev = pci_get_drvdata(pdev); > > + struct nvme_ctrl *ctrl = &ndev->ctrl; > > + > > + if (!(pm_suspend_via_s2idle() && (ctrl->quirks & NVME_QUIRK_NO_DISABLE))) > > + nvme_dev_disable(ndev, true); > > > > - nvme_dev_disable(ndev, true); > > return 0; > > } > > Hi AceLan, > > Are there any plans to submit these changes, at least the generic part, > to mainline? > > > Thanks, > Kleber > > > > > @@ -2709,6 +2713,8 @@ static const struct pci_device_id nvme_id_table[] = { > > .driver_data = NVME_QUIRK_LIGHTNVM, }, > > { PCI_DEVICE(0x1d1d, 0x2807), /* CNEX WL */ > > .driver_data = NVME_QUIRK_LIGHTNVM, }, > > + { PCI_VDEVICE(SK_HYNIX, 0x1527), /* Sk Hynix */ > > + .driver_data = NVME_QUIRK_NO_DISABLE, }, > > { PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) }, > > { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) }, > > { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2003) }, > >
On 11/07/18 03:01, AceLan Kao wrote: > I did, although maintainer doesn't like it much. > https://lkml.org/lkml/2018/11/6/583 Thank you for the clarification, AceLan. I see the point of the maintainer and it does make sense for us to keep this SAUCE patches while the firmware doesn't get fixed. Kleber > Kleber Souza <kleber.souza@canonical.com> 於 2018年11月7日 週三 上午12:48寫道： >> On 11/06/18 11:53, AceLan Kao wrote: >>> BugLink: https://bugs.launchpad.net/bugs/1801875 >>> >>> Call nvme_dev_disable() function leads to the power consumption goes >>> up to 2.2 Watt during suspend-to-idle, and from SK hynix FE, they >>> suggest us to use its own APST feature to do the power management during >>> s2idle. >>> After D3 is diabled and nvme_dev_disable() is not called while >>> suspending, the power consumption drops to 0.77 Watt during s2idle. >>> >>> Signed-off-by: AceLan Kao <acelan.kao@canonical.com> >>> --- >>> drivers/nvme/host/nvme.h | 5 +++++ >>> drivers/nvme/host/pci.c | 8 +++++++- >>> 2 files changed, 12 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h >>> index a9ec71cce3cc..c2f71cb23700 100644 >>> --- a/drivers/nvme/host/nvme.h >>> +++ b/drivers/nvme/host/nvme.h >>> @@ -86,6 +86,11 @@ enum nvme_quirks { >>> * Set MEDIUM priority on SQ creation >>> */ >>> NVME_QUIRK_MEDIUM_PRIO_SQ = (1 << 7), >>> + >>> + /* >>> + * Do not disable nvme when suspending(s2idle) >>> + */ >>> + NVME_QUIRK_NO_DISABLE = (1 << 8), >>> }; >>> >>> /* >>> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c >>> index 3be974ff4261..5f797d595e52 100644 >>> --- a/drivers/nvme/host/pci.c >>> +++ b/drivers/nvme/host/pci.c >>> @@ -29,6 +29,7 @@ >>> #include <linux/types.h> >>> #include <linux/io-64-nonatomic-lo-hi.h> >>> #include <linux/sed-opal.h> >>> +#include <linux/suspend.h> >>> >>> #include "nvme.h" >>> >>> @@ -2611,8 +2612,11 @@ static int nvme_suspend(struct device *dev) >>> { >>> struct pci_dev *pdev = to_pci_dev(dev); >>> struct nvme_dev *ndev = pci_get_drvdata(pdev); >>> + struct nvme_ctrl *ctrl = &ndev->ctrl; >>> + >>> + if (!(pm_suspend_via_s2idle() && (ctrl->quirks & NVME_QUIRK_NO_DISABLE))) >>> + nvme_dev_disable(ndev, true); >>> >>> - nvme_dev_disable(ndev, true); >>> return 0; >>> } >> Hi AceLan, >> >> Are there any plans to submit these changes, at least the generic part, >> to mainline? >> >> >> Thanks, >> Kleber >> >>> @@ -2709,6 +2713,8 @@ static const struct pci_device_id nvme_id_table[] = { >>> .driver_data = NVME_QUIRK_LIGHTNVM, }, >>> { PCI_DEVICE(0x1d1d, 0x2807), /* CNEX WL */ >>> .driver_data = NVME_QUIRK_LIGHTNVM, }, >>> + { PCI_VDEVICE(SK_HYNIX, 0x1527), /* Sk Hynix */ >>> + .driver_data = NVME_QUIRK_NO_DISABLE, }, >>> { PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) }, >>> { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) }, >>> { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2003) }, >>
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index a9ec71cce3cc..c2f71cb23700 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -86,6 +86,11 @@ enum nvme_quirks { * Set MEDIUM priority on SQ creation */ NVME_QUIRK_MEDIUM_PRIO_SQ = (1 << 7), + + /* + * Do not disable nvme when suspending(s2idle) + */ + NVME_QUIRK_NO_DISABLE = (1 << 8), }; /* diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 3be974ff4261..5f797d595e52 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -29,6 +29,7 @@ #include <linux/types.h> #include <linux/io-64-nonatomic-lo-hi.h> #include <linux/sed-opal.h> +#include <linux/suspend.h> #include "nvme.h" @@ -2611,8 +2612,11 @@ static int nvme_suspend(struct device *dev) { struct pci_dev *pdev = to_pci_dev(dev); struct nvme_dev *ndev = pci_get_drvdata(pdev); + struct nvme_ctrl *ctrl = &ndev->ctrl; + + if (!(pm_suspend_via_s2idle() && (ctrl->quirks & NVME_QUIRK_NO_DISABLE))) + nvme_dev_disable(ndev, true); - nvme_dev_disable(ndev, true); return 0; } @@ -2709,6 +2713,8 @@ static const struct pci_device_id nvme_id_table[] = { .driver_data = NVME_QUIRK_LIGHTNVM, }, { PCI_DEVICE(0x1d1d, 0x2807), /* CNEX WL */ .driver_data = NVME_QUIRK_LIGHTNVM, }, + { PCI_VDEVICE(SK_HYNIX, 0x1527), /* Sk Hynix */ + .driver_data = NVME_QUIRK_NO_DISABLE, }, { PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) }, { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) }, { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2003) },
BugLink: https://bugs.launchpad.net/bugs/1801875 Call nvme_dev_disable() function leads to the power consumption goes up to 2.2 Watt during suspend-to-idle, and from SK hynix FE, they suggest us to use its own APST feature to do the power management during s2idle. After D3 is diabled and nvme_dev_disable() is not called while suspending, the power consumption drops to 0.77 Watt during s2idle. Signed-off-by: AceLan Kao <acelan.kao@canonical.com> --- drivers/nvme/host/nvme.h | 5 +++++ drivers/nvme/host/pci.c | 8 +++++++- 2 files changed, 12 insertions(+), 1 deletion(-)