[PATCH 06/10] mtd: intel-dg: wake card on operations
Usyskin, Alexander
alexander.usyskin at intel.com
Tue Nov 5 04:17:53 PST 2024
> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Sent: Monday, November 4, 2024 11:16 PM
> To: Usyskin, Alexander <alexander.usyskin at intel.com>
> Cc: Gupta, Anshuman <anshuman.gupta at intel.com>; Deak, Imre
> <imre.deak at intel.com>; Miquel Raynal <miquel.raynal at bootlin.com>;
> Richard Weinberger <richard at nod.at>; Vignesh Raghavendra
> <vigneshr at ti.com>; De Marchi, Lucas <lucas.demarchi at intel.com>; Thomas
> Hellström <thomas.hellstrom at linux.intel.com>; Maarten Lankhorst
> <maarten.lankhorst at linux.intel.com>; Maxime Ripard <mripard at kernel.org>;
> Thomas Zimmermann <tzimmermann at suse.de>; David Airlie
> <airlied at gmail.com>; Simona Vetter <simona at ffwll.ch>; Jani Nikula
> <jani.nikula at linux.intel.com>; Joonas Lahtinen
> <joonas.lahtinen at linux.intel.com>; Tvrtko Ursulin <tursulin at ursulin.net>;
> Weil, Oren jer <oren.jer.weil at intel.com>; linux-mtd at lists.infradead.org; dri-
> devel at lists.freedesktop.org; intel-gfx at lists.freedesktop.org; linux-
> kernel at vger.kernel.org
> Subject: Re: [PATCH 06/10] mtd: intel-dg: wake card on operations
>
> On Tue, Oct 29, 2024 at 11:24:36AM +0000, Usyskin, Alexander wrote:
> > > -----Original Message-----
> > > From: Gupta, Anshuman <anshuman.gupta at intel.com>
> > > Sent: Monday, October 28, 2024 5:02 PM
> > > To: Vivi, Rodrigo <rodrigo.vivi at intel.com>; Usyskin, Alexander
> > > <alexander.usyskin at intel.com>; Deak, Imre <imre.deak at intel.com>
> > > Cc: Miquel Raynal <miquel.raynal at bootlin.com>; Richard Weinberger
> > > <richard at nod.at>; Vignesh Raghavendra <vigneshr at ti.com>; De Marchi,
> > > Lucas <lucas.demarchi at intel.com>; Thomas Hellström
> > > <thomas.hellstrom at linux.intel.com>; Maarten Lankhorst
> > > <maarten.lankhorst at linux.intel.com>; Maxime Ripard
> <mripard at kernel.org>;
> > > Thomas Zimmermann <tzimmermann at suse.de>; David Airlie
> > > <airlied at gmail.com>; Simona Vetter <simona at ffwll.ch>; Jani Nikula
> > > <jani.nikula at linux.intel.com>; Joonas Lahtinen
> > > <joonas.lahtinen at linux.intel.com>; Tvrtko Ursulin <tursulin at ursulin.net>;
> > > Weil, Oren jer <oren.jer.weil at intel.com>; linux-mtd at lists.infradead.org;
> dri-
> > > devel at lists.freedesktop.org; intel-gfx at lists.freedesktop.org; linux-
> > > kernel at vger.kernel.org
> > > Subject: RE: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> > > > Sent: Monday, October 28, 2024 8:27 PM
> > > > To: Usyskin, Alexander <alexander.usyskin at intel.com>; Gupta,
> Anshuman
> > > > <anshuman.gupta at intel.com>; Deak, Imre <imre.deak at intel.com>
> > > > Cc: Miquel Raynal <miquel.raynal at bootlin.com>; Richard Weinberger
> > > > <richard at nod.at>; Vignesh Raghavendra <vigneshr at ti.com>; De Marchi,
> > > > Lucas <lucas.demarchi at intel.com>; Thomas Hellström
> > > > <thomas.hellstrom at linux.intel.com>; Maarten Lankhorst
> > > > <maarten.lankhorst at linux.intel.com>; Maxime Ripard
> > > <mripard at kernel.org>;
> > > > Thomas Zimmermann <tzimmermann at suse.de>; David Airlie
> > > > <airlied at gmail.com>; Simona Vetter <simona at ffwll.ch>; Jani Nikula
> > > > <jani.nikula at linux.intel.com>; Joonas Lahtinen
> > > > <joonas.lahtinen at linux.intel.com>; Tvrtko Ursulin
> <tursulin at ursulin.net>;
> > > > Weil, Oren jer <oren.jer.weil at intel.com>; linux-mtd at lists.infradead.org;
> dri-
> > > > devel at lists.freedesktop.org; intel-gfx at lists.freedesktop.org; linux-
> > > > kernel at vger.kernel.org
> > > > Subject: Re: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > > >
> > > > On Tue, Oct 22, 2024 at 01:41:15PM +0300, Alexander Usyskin wrote:
> > > > > Enable runtime PM in mtd driver to notify graphics driver that whole
> > > > > card should be kept awake while nvm operations are performed
> through
> > > > > this driver.
> > > > >
> > > > > CC: Lucas De Marchi <lucas.demarchi at intel.com>
> > > > > Signed-off-by: Alexander Usyskin <alexander.usyskin at intel.com>
> > > > > ---
> > > > > drivers/mtd/devices/mtd-intel-dg.c | 73
> > > > > +++++++++++++++++++++++++-----
> > > > > 1 file changed, 61 insertions(+), 12 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mtd/devices/mtd-intel-dg.c
> > > > > b/drivers/mtd/devices/mtd-intel-dg.c
> > > > > index 4951438e8faf..3d53f35478e8 100644
> > > > > --- a/drivers/mtd/devices/mtd-intel-dg.c
> > > > > +++ b/drivers/mtd/devices/mtd-intel-dg.c
> > > > > @@ -15,11 +15,14 @@
> > > > > #include <linux/module.h>
> > > > > #include <linux/mtd/mtd.h>
> > > > > #include <linux/mtd/partitions.h>
> > > > > +#include <linux/pm_runtime.h>
> > > > > #include <linux/string.h>
> > > > > #include <linux/slab.h>
> > > > > #include <linux/sizes.h>
> > > > > #include <linux/types.h>
> > > > >
> > > > > +#define INTEL_DG_NVM_RPM_TIMEOUT 500
> > > > > +
> > > > > struct intel_dg_nvm {
> > > > > struct kref refcnt;
> > > > > struct mtd_info mtd;
> > > > > @@ -462,6 +465,7 @@ static int intel_dg_mtd_erase(struct mtd_info
> > > *mtd,
> > > > struct erase_info *info)
> > > > > loff_t from;
> > > > > size_t len;
> > > > > size_t total_len;
> > > > > + int ret = 0;
> > > > >
> > > > > if (WARN_ON(!nvm))
> > > > > return -EINVAL;
> > > > > @@ -476,20 +480,28 @@ static int intel_dg_mtd_erase(struct
> mtd_info
> > > > *mtd, struct erase_info *info)
> > > > > total_len = info->len;
> > > > > addr = info->addr;
> > > > >
> > > > > + ret = pm_runtime_resume_and_get(mtd->dev.parent);
> > > > > + if (ret < 0) {
> > > > > + dev_err(&mtd->dev, "rpm: get failed %d\n", ret);
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > guard(mutex)(&nvm->lock);
> > > > >
> > > > > while (total_len > 0) {
> > > > > if (!IS_ALIGNED(addr, SZ_4K) || !IS_ALIGNED(total_len,
> > > > SZ_4K)) {
> > > > > dev_err(&mtd->dev, "unaligned erase %llx %zx\n",
> > > > addr, total_len);
> > > > > info->fail_addr = addr;
> > > > > - return -ERANGE;
> > > > > + ret = -ERANGE;
> > > > > + goto out;
> > > > > }
> > > > >
> > > > > idx = idg_nvm_get_region(nvm, addr);
> > > > > if (idx >= nvm->nregions) {
> > > > > dev_err(&mtd->dev, "out of range");
> > > > > info->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
> > > > > - return -ERANGE;
> > > > > + ret = -ERANGE;
> > > > > + goto out;
> > > > > }
> > > > >
> > > > > from = addr - nvm->regions[idx].offset; @@ -505,14 +517,18
> > > > @@
> > > > > static int intel_dg_mtd_erase(struct mtd_info *mtd, struct erase_info
> > > *info)
> > > > > if (bytes < 0) {
> > > > > dev_dbg(&mtd->dev, "erase failed with %zd\n",
> > > > bytes);
> > > > > info->fail_addr += nvm->regions[idx].offset;
> > > > > - return bytes;
> > > > > + ret = bytes;
> > > > > + goto out;
> > > > > }
> > > > >
> > > > > addr += len;
> > > > > total_len -= len;
> > > > > }
> > > > >
> > > > > - return 0;
> > > > > +out:
> > > > > + pm_runtime_mark_last_busy(mtd->dev.parent);
> > > > > + pm_runtime_put_autosuspend(mtd->dev.parent);
> > > > > + return ret;
> > > > > }
> > > > >
> > > > > static int intel_dg_mtd_read(struct mtd_info *mtd, loff_t from,
> > > > > size_t len, @@ -541,17 +557,25 @@ static int
> intel_dg_mtd_read(struct
> > > > mtd_info *mtd, loff_t from, size_t len,
> > > > > if (len > nvm->regions[idx].size - from)
> > > > > len = nvm->regions[idx].size - from;
> > > > >
> > > > > + ret = pm_runtime_resume_and_get(mtd->dev.parent);
> > > > > + if (ret < 0) {
> > > > > + dev_err(&mtd->dev, "rpm: get failed %zd\n", ret);
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > guard(mutex)(&nvm->lock);
> > > > >
> > > > > ret = idg_read(nvm, region, from, len, buf);
> > > > > if (ret < 0) {
> > > > > dev_dbg(&mtd->dev, "read failed with %zd\n", ret);
> > > > > - return ret;
> > > > > + } else {
> > > > > + *retlen = ret;
> > > > > + ret = 0;
> > > > > }
> > > > >
> > > > > - *retlen = ret;
> > > > > -
> > > > > - return 0;
> > > > > + pm_runtime_mark_last_busy(mtd->dev.parent);
> > > > > + pm_runtime_put_autosuspend(mtd->dev.parent);
> > > > > + return ret;
> > > > > }
> > > > >
> > > > > static int intel_dg_mtd_write(struct mtd_info *mtd, loff_t to, size_t
> > > > > len, @@ -580,17 +604,25 @@ static int intel_dg_mtd_write(struct
> > > mtd_info
> > > > *mtd, loff_t to, size_t len,
> > > > > if (len > nvm->regions[idx].size - to)
> > > > > len = nvm->regions[idx].size - to;
> > > > >
> > > > > + ret = pm_runtime_resume_and_get(mtd->dev.parent);
> > > > > + if (ret < 0) {
> > > > > + dev_err(&mtd->dev, "rpm: get failed %zd\n", ret);
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > guard(mutex)(&nvm->lock);
> > > > >
> > > > > ret = idg_write(nvm, region, to, len, buf);
> > > > > if (ret < 0) {
> > > > > dev_dbg(&mtd->dev, "write failed with %zd\n", ret);
> > > > > - return ret;
> > > > > + } else {
> > > > > + *retlen = ret;
> > > > > + ret = 0;
> > > > > }
> > > > >
> > > > > - *retlen = ret;
> > > > > -
> > > > > - return 0;
> > > > > + pm_runtime_mark_last_busy(mtd->dev.parent);
> > > > > + pm_runtime_put_autosuspend(mtd->dev.parent);
> > > > > + return ret;
> > > > > }
> > > > >
> > > > > static void intel_dg_nvm_release(struct kref *kref) @@ -722,6
> +754,17
> > > > > @@ static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
> > > > > n++;
> > > > > }
> > > > >
> > > > > + pm_runtime_enable(device);
> > > > > +
> > > > > + pm_runtime_set_autosuspend_delay(device,
> > > > INTEL_DG_NVM_RPM_TIMEOUT);
> > > > > + pm_runtime_use_autosuspend(device);
> > > >
> > > > something looks strange here...
> > > > on the functions above you get and put references for the parent device
> > > > directly.
> > > > But here you enable the rpm for this device.
> > > >
> > > > If I remember correctly from some old experiments, the rpm is smart
> enough
> > > > and wake up the parent before waking up the child. So I'm wondering if
> we
> > > > should only do the child without the parent.
> > > Agree parent can't runtime suspend ind if it has active child.
> > > Let this driver have it's own get/put routine instead of parent.
> > > Thanks,
> > > Anshuman Gupta.
> >
> > I need to wake DG card before the MTD device is established to scan the
> partition table on flash.
> > Thus, if I move rpm down to MTD device I should enable and take parent
> (auxiliary device) rpm anyway.
>
> That's the part that I'm not sure if I agree. if I remember from some
> experiments in the past,
> when you call to wake up the child, the parent will wakeup first anyway.
>
The child (mtd device) does not exist at this point of time.
To create MTD device, the partition table should be provided
and it read directly from flash that should be powered to do read.
> > Considering above, is this move is justified?
> > Also, MTD drivers tend to enable parent rpm, not its own one.
>
> What other drivers are you looking for reference?
drivers/mtd/nand/raw/tegra_nand.c
drivers/mtd/nand/raw/renesas-nand-controller.c
drivers/mtd/maps/physmap-core.c
>
> >
> > - -
> > Thanks,
> > Sasha
> >
> >
> >
> > > >
> > > > Cc: Imre Deak <imre.deak at intel.com>
> > > > Cc: Anshuman Gupta <anshuman.gupta at intel.com>
> > > >
> > > > > +
> > > > > + ret = pm_runtime_resume_and_get(device);
> > > > > + if (ret < 0) {
> > > > > + dev_err(device, "rpm: get failed %d\n", ret);
> > > > > + goto err_norpm;
> > > > > + }
> > > > > +
> > > > > nvm->base = devm_ioremap_resource(device, &invm->bar);
> > > > > if (IS_ERR(nvm->base)) {
> > > > > dev_err(device, "mmio not mapped\n"); @@ -744,9 +787,13
> > > > @@ static
> > > > > int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
> > > > >
> > > > > dev_set_drvdata(&aux_dev->dev, nvm);
> > > > >
> > > > > + pm_runtime_put(device);
> > > > > return 0;
> > > > >
> > > > > err:
> > > > > + pm_runtime_put(device);
> > > > > +err_norpm:
> > > > > + pm_runtime_disable(device);
> > > > > kref_put(&nvm->refcnt, intel_dg_nvm_release);
> > > > > return ret;
> > > > > }
> > > > > @@ -758,6 +805,8 @@ static void intel_dg_mtd_remove(struct
> > > > auxiliary_device *aux_dev)
> > > > > if (!nvm)
> > > > > return;
> > > > >
> > > > > + pm_runtime_disable(&aux_dev->dev);
> > > > > +
> > > > > mtd_device_unregister(&nvm->mtd);
> > > > >
> > > > > dev_set_drvdata(&aux_dev->dev, NULL);
> > > > > --
> > > > > 2.43.0
> > > > >
More information about the linux-mtd
mailing list