On Tue, Nov 05, 2024 at 07:17:53AM -0500, Usyskin, Alexander wrote:
> > -----Original Message-----
> > From: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>
> > Sent: Monday, November 4, 2024 11:16 PM
> > To: Usyskin, Alexander <alexander.usyskin@xxxxxxxxx>
> > Cc: Gupta, Anshuman <anshuman.gupta@xxxxxxxxx>; Deak, Imre
> > <imre.deak@xxxxxxxxx>; Miquel Raynal <miquel.raynal@xxxxxxxxxxx>;
> > Richard Weinberger <richard@xxxxxx>; Vignesh Raghavendra
> > <vigneshr@xxxxxx>; De Marchi, Lucas <lucas.demarchi@xxxxxxxxx>; Thomas
> > Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>; Maarten Lankhorst
> > <maarten.lankhorst@xxxxxxxxxxxxxxx>; Maxime Ripard <mripard@xxxxxxxxxx>;
> > Thomas Zimmermann <tzimmermann@xxxxxxx>; David Airlie
> > <airlied@xxxxxxxxx>; Simona Vetter <simona@xxxxxxxx>; Jani Nikula
> > <jani.nikula@xxxxxxxxxxxxxxx>; Joonas Lahtinen
> > <joonas.lahtinen@xxxxxxxxxxxxxxx>; Tvrtko Ursulin <tursulin@xxxxxxxxxxx>;
> > Weil, Oren jer <oren.jer.weil@xxxxxxxxx>; linux-mtd@xxxxxxxxxxxxxxxxxxx; dri-
> > devel@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxx>
> > > > Sent: Monday, October 28, 2024 5:02 PM
> > > > To: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>; Usyskin, Alexander
> > > > <alexander.usyskin@xxxxxxxxx>; Deak, Imre <imre.deak@xxxxxxxxx>
> > > > Cc: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>; Richard Weinberger
> > > > <richard@xxxxxx>; Vignesh Raghavendra <vigneshr@xxxxxx>; De Marchi,
> > > > Lucas <lucas.demarchi@xxxxxxxxx>; Thomas Hellström
> > > > <thomas.hellstrom@xxxxxxxxxxxxxxx>; Maarten Lankhorst
> > > > <maarten.lankhorst@xxxxxxxxxxxxxxx>; Maxime Ripard
> > <mripard@xxxxxxxxxx>;
> > > > Thomas Zimmermann <tzimmermann@xxxxxxx>; David Airlie
> > > > <airlied@xxxxxxxxx>; Simona Vetter <simona@xxxxxxxx>; Jani Nikula
> > > > <jani.nikula@xxxxxxxxxxxxxxx>; Joonas Lahtinen
> > > > <joonas.lahtinen@xxxxxxxxxxxxxxx>; Tvrtko Ursulin <tursulin@xxxxxxxxxxx>;
> > > > Weil, Oren jer <oren.jer.weil@xxxxxxxxx>; linux-mtd@xxxxxxxxxxxxxxxxxxx;
> > dri-
> > > > devel@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; linux-
> > > > kernel@xxxxxxxxxxxxxxx
> > > > Subject: RE: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>
> > > > > Sent: Monday, October 28, 2024 8:27 PM
> > > > > To: Usyskin, Alexander <alexander.usyskin@xxxxxxxxx>; Gupta,
> > Anshuman
> > > > > <anshuman.gupta@xxxxxxxxx>; Deak, Imre <imre.deak@xxxxxxxxx>
> > > > > Cc: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>; Richard Weinberger
> > > > > <richard@xxxxxx>; Vignesh Raghavendra <vigneshr@xxxxxx>; De Marchi,
> > > > > Lucas <lucas.demarchi@xxxxxxxxx>; Thomas Hellström
> > > > > <thomas.hellstrom@xxxxxxxxxxxxxxx>; Maarten Lankhorst
> > > > > <maarten.lankhorst@xxxxxxxxxxxxxxx>; Maxime Ripard
> > > > <mripard@xxxxxxxxxx>;
> > > > > Thomas Zimmermann <tzimmermann@xxxxxxx>; David Airlie
> > > > > <airlied@xxxxxxxxx>; Simona Vetter <simona@xxxxxxxx>; Jani Nikula
> > > > > <jani.nikula@xxxxxxxxxxxxxxx>; Joonas Lahtinen
> > > > > <joonas.lahtinen@xxxxxxxxxxxxxxx>; Tvrtko Ursulin
> > <tursulin@xxxxxxxxxxx>;
> > > > > Weil, Oren jer <oren.jer.weil@xxxxxxxxx>; linux-mtd@xxxxxxxxxxxxxxxxxxx;
> > dri-
> > > > > devel@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; linux-
> > > > > kernel@xxxxxxxxxxxxxxx
> > > > > 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@xxxxxxxxx>
> > > > > > Signed-off-by: Alexander Usyskin <alexander.usyskin@xxxxxxxxx>
> > > > > > ---
> > > > > > 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.
I don't understand... you have the mtd->dev at this point... this is
the one you should be touching, not the mtd->dev.parent... even at the
probe, but moreover on everywhere else as well.
>
> > > 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
I see they getting pdev->dev... not the parent...
>
> >
> > >
> > > - -
> > > Thanks,
> > > Sasha
> > >
> > >
> > >
> > > > >
> > > > > Cc: Imre Deak <imre.deak@xxxxxxxxx>
> > > > > Cc: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
> > > > >
> > > > > > +
> > > > > > + 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
> > > > > >
