[06/10] mtd: intel-dg: wake card on operations

Commit Message

Alexander Usyskin Oct. 22, 2024, 10:41 a.m. UTC

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 <[e-mail]>
Signed-off-by: Alexander Usyskin <[e-mail]>
---
 drivers/mtd/devices/mtd-intel-dg.c | 73 +++++++++++++++++++++++++-----
 1 file changed, 61 insertions(+), 12 deletions(-)

Comments

Rodrigo Vivi Oct. 28, 2024, 2:57 p.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Alexander Usyskin <[e-mail]>
> ---
>  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.

Cc: Imre Deak <[e-mail]>
Cc: Anshuman Gupta <[e-mail]>

> +
> +	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
>

Gupta, Anshuman Oct. 28, 2024, 3:01 p.m. UTC | #2

> -----Original Message-----
> From: Vivi, Rodrigo <[e-mail]>
> Sent: Monday, October 28, 2024 8:27 PM
> To: Usyskin, Alexander <[e-mail]>; Gupta, Anshuman
> <[e-mail]>; Deak, Imre <[e-mail]>
> Cc: Miquel Raynal <[e-mail]>; Richard Weinberger
> <[e-mail]>; Vignesh Raghavendra <[e-mail]>; De Marchi,
> Lucas <[e-mail]>; Thomas Hellström
> <[e-mail]>; Maarten Lankhorst
> <[e-mail]>; Maxime Ripard <[e-mail]>;
> Thomas Zimmermann <[e-mail]>; David Airlie
> <[e-mail]>; Simona Vetter <[e-mail]>; Jani Nikula
> <[e-mail]>; Joonas Lahtinen
> <[e-mail]>; Tvrtko Ursulin <[e-mail]>;
> Weil, Oren jer <[e-mail]>; [e-mail]; dri-
> [e-mail]; [e-mail]; linux-
> [e-mail]
> 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 <[e-mail]>
> > Signed-off-by: Alexander Usyskin <[e-mail]>
> > ---
> >  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.
> 
> Cc: Imre Deak <[e-mail]>
> Cc: Anshuman Gupta <[e-mail]>
> 
> > +
> > +	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
> >

Alexander Usyskin Oct. 29, 2024, 11:24 a.m. UTC | #3

> -----Original Message-----
> From: Gupta, Anshuman <[e-mail]>
> Sent: Monday, October 28, 2024 5:02 PM
> To: Vivi, Rodrigo <[e-mail]>; Usyskin, Alexander
> <[e-mail]>; Deak, Imre <[e-mail]>
> Cc: Miquel Raynal <[e-mail]>; Richard Weinberger
> <[e-mail]>; Vignesh Raghavendra <[e-mail]>; De Marchi,
> Lucas <[e-mail]>; Thomas Hellström
> <[e-mail]>; Maarten Lankhorst
> <[e-mail]>; Maxime Ripard <[e-mail]>;
> Thomas Zimmermann <[e-mail]>; David Airlie
> <[e-mail]>; Simona Vetter <[e-mail]>; Jani Nikula
> <[e-mail]>; Joonas Lahtinen
> <[e-mail]>; Tvrtko Ursulin <[e-mail]>;
> Weil, Oren jer <[e-mail]>; [e-mail]; dri-
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: RE: [PATCH 06/10] mtd: intel-dg: wake card on operations
> 
> 
> 
> > -----Original Message-----
> > From: Vivi, Rodrigo <[e-mail]>
> > Sent: Monday, October 28, 2024 8:27 PM
> > To: Usyskin, Alexander <[e-mail]>; Gupta, Anshuman
> > <[e-mail]>; Deak, Imre <[e-mail]>
> > Cc: Miquel Raynal <[e-mail]>; Richard Weinberger
> > <[e-mail]>; Vignesh Raghavendra <[e-mail]>; De Marchi,
> > Lucas <[e-mail]>; Thomas Hellström
> > <[e-mail]>; Maarten Lankhorst
> > <[e-mail]>; Maxime Ripard
> <[e-mail]>;
> > Thomas Zimmermann <[e-mail]>; David Airlie
> > <[e-mail]>; Simona Vetter <[e-mail]>; Jani Nikula
> > <[e-mail]>; Joonas Lahtinen
> > <[e-mail]>; Tvrtko Ursulin <[e-mail]>;
> > Weil, Oren jer <[e-mail]>; [e-mail]; dri-
> > [e-mail]; [e-mail]; linux-
> > [e-mail]
> > 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 <[e-mail]>
> > > Signed-off-by: Alexander Usyskin <[e-mail]>
> > > ---
> > >  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.
Considering above, is this move is justified?
Also, MTD drivers tend to enable parent rpm, not its own one.

- - 
Thanks,
Sasha



> >
> > Cc: Imre Deak <[e-mail]>
> > Cc: Anshuman Gupta <[e-mail]>
> >
> > > +
> > > +	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
> > >

Rodrigo Vivi Nov. 4, 2024, 9:16 p.m. UTC | #4

On Tue, Oct 29, 2024 at 11:24:36AM +0000, Usyskin, Alexander wrote:
> > -----Original Message-----
> > From: Gupta, Anshuman <[e-mail]>
> > Sent: Monday, October 28, 2024 5:02 PM
> > To: Vivi, Rodrigo <[e-mail]>; Usyskin, Alexander
> > <[e-mail]>; Deak, Imre <[e-mail]>
> > Cc: Miquel Raynal <[e-mail]>; Richard Weinberger
> > <[e-mail]>; Vignesh Raghavendra <[e-mail]>; De Marchi,
> > Lucas <[e-mail]>; Thomas Hellström
> > <[e-mail]>; Maarten Lankhorst
> > <[e-mail]>; Maxime Ripard <[e-mail]>;
> > Thomas Zimmermann <[e-mail]>; David Airlie
> > <[e-mail]>; Simona Vetter <[e-mail]>; Jani Nikula
> > <[e-mail]>; Joonas Lahtinen
> > <[e-mail]>; Tvrtko Ursulin <[e-mail]>;
> > Weil, Oren jer <[e-mail]>; [e-mail]; dri-
> > [e-mail]; [e-mail]; linux-
> > [e-mail]
> > Subject: RE: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Vivi, Rodrigo <[e-mail]>
> > > Sent: Monday, October 28, 2024 8:27 PM
> > > To: Usyskin, Alexander <[e-mail]>; Gupta, Anshuman
> > > <[e-mail]>; Deak, Imre <[e-mail]>
> > > Cc: Miquel Raynal <[e-mail]>; Richard Weinberger
> > > <[e-mail]>; Vignesh Raghavendra <[e-mail]>; De Marchi,
> > > Lucas <[e-mail]>; Thomas Hellström
> > > <[e-mail]>; Maarten Lankhorst
> > > <[e-mail]>; Maxime Ripard
> > <[e-mail]>;
> > > Thomas Zimmermann <[e-mail]>; David Airlie
> > > <[e-mail]>; Simona Vetter <[e-mail]>; Jani Nikula
> > > <[e-mail]>; Joonas Lahtinen
> > > <[e-mail]>; Tvrtko Ursulin <[e-mail]>;
> > > Weil, Oren jer <[e-mail]>; [e-mail]; dri-
> > > [e-mail]; [e-mail]; linux-
> > > [e-mail]
> > > 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 <[e-mail]>
> > > > Signed-off-by: Alexander Usyskin <[e-mail]>
> > > > ---
> > > >  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.

> 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?

> 
> - - 
> Thanks,
> Sasha
> 
> 
> 
> > >
> > > Cc: Imre Deak <[e-mail]>
> > > Cc: Anshuman Gupta <[e-mail]>
> > >
> > > > +
> > > > +	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
> > > >

Alexander Usyskin Nov. 5, 2024, 12:17 p.m. UTC | #5

> -----Original Message-----
> From: Vivi, Rodrigo <[e-mail]>
> Sent: Monday, November 4, 2024 11:16 PM
> To: Usyskin, Alexander <[e-mail]>
> Cc: Gupta, Anshuman <[e-mail]>; Deak, Imre
> <[e-mail]>; Miquel Raynal <[e-mail]>;
> Richard Weinberger <[e-mail]>; Vignesh Raghavendra
> <[e-mail]>; De Marchi, Lucas <[e-mail]>; Thomas
> Hellström <[e-mail]>; Maarten Lankhorst
> <[e-mail]>; Maxime Ripard <[e-mail]>;
> Thomas Zimmermann <[e-mail]>; David Airlie
> <[e-mail]>; Simona Vetter <[e-mail]>; Jani Nikula
> <[e-mail]>; Joonas Lahtinen
> <[e-mail]>; Tvrtko Ursulin <[e-mail]>;
> Weil, Oren jer <[e-mail]>; [e-mail]; dri-
> [e-mail]; [e-mail]; linux-
> [e-mail]
> 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 <[e-mail]>
> > > Sent: Monday, October 28, 2024 5:02 PM
> > > To: Vivi, Rodrigo <[e-mail]>; Usyskin, Alexander
> > > <[e-mail]>; Deak, Imre <[e-mail]>
> > > Cc: Miquel Raynal <[e-mail]>; Richard Weinberger
> > > <[e-mail]>; Vignesh Raghavendra <[e-mail]>; De Marchi,
> > > Lucas <[e-mail]>; Thomas Hellström
> > > <[e-mail]>; Maarten Lankhorst
> > > <[e-mail]>; Maxime Ripard
> <[e-mail]>;
> > > Thomas Zimmermann <[e-mail]>; David Airlie
> > > <[e-mail]>; Simona Vetter <[e-mail]>; Jani Nikula
> > > <[e-mail]>; Joonas Lahtinen
> > > <[e-mail]>; Tvrtko Ursulin <[e-mail]>;
> > > Weil, Oren jer <[e-mail]>; [e-mail];
> dri-
> > > [e-mail]; [e-mail]; linux-
> > > [e-mail]
> > > Subject: RE: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Vivi, Rodrigo <[e-mail]>
> > > > Sent: Monday, October 28, 2024 8:27 PM
> > > > To: Usyskin, Alexander <[e-mail]>; Gupta,
> Anshuman
> > > > <[e-mail]>; Deak, Imre <[e-mail]>
> > > > Cc: Miquel Raynal <[e-mail]>; Richard Weinberger
> > > > <[e-mail]>; Vignesh Raghavendra <[e-mail]>; De Marchi,
> > > > Lucas <[e-mail]>; Thomas Hellström
> > > > <[e-mail]>; Maarten Lankhorst
> > > > <[e-mail]>; Maxime Ripard
> > > <[e-mail]>;
> > > > Thomas Zimmermann <[e-mail]>; David Airlie
> > > > <[e-mail]>; Simona Vetter <[e-mail]>; Jani Nikula
> > > > <[e-mail]>; Joonas Lahtinen
> > > > <[e-mail]>; Tvrtko Ursulin
> <[e-mail]>;
> > > > Weil, Oren jer <[e-mail]>; [e-mail];
> dri-
> > > > [e-mail]; [e-mail]; linux-
> > > > [e-mail]
> > > > 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 <[e-mail]>
> > > > > Signed-off-by: Alexander Usyskin <[e-mail]>
> > > > > ---
> > > > >  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 <[e-mail]>
> > > > Cc: Anshuman Gupta <[e-mail]>
> > > >
> > > > > +
> > > > > +	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
> > > > >

Rodrigo Vivi Nov. 7, 2024, 10:49 p.m. UTC | #6

On Tue, Nov 05, 2024 at 07:17:53AM -0500, Usyskin, Alexander wrote:
> > -----Original Message-----
> > From: Vivi, Rodrigo <[e-mail]>
> > Sent: Monday, November 4, 2024 11:16 PM
> > To: Usyskin, Alexander <[e-mail]>
> > Cc: Gupta, Anshuman <[e-mail]>; Deak, Imre
> > <[e-mail]>; Miquel Raynal <[e-mail]>;
> > Richard Weinberger <[e-mail]>; Vignesh Raghavendra
> > <[e-mail]>; De Marchi, Lucas <[e-mail]>; Thomas
> > Hellström <[e-mail]>; Maarten Lankhorst
> > <[e-mail]>; Maxime Ripard <[e-mail]>;
> > Thomas Zimmermann <[e-mail]>; David Airlie
> > <[e-mail]>; Simona Vetter <[e-mail]>; Jani Nikula
> > <[e-mail]>; Joonas Lahtinen
> > <[e-mail]>; Tvrtko Ursulin <[e-mail]>;
> > Weil, Oren jer <[e-mail]>; [e-mail]; dri-
> > [e-mail]; [e-mail]; linux-
> > [e-mail]
> > 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 <[e-mail]>
> > > > Sent: Monday, October 28, 2024 5:02 PM
> > > > To: Vivi, Rodrigo <[e-mail]>; Usyskin, Alexander
> > > > <[e-mail]>; Deak, Imre <[e-mail]>
> > > > Cc: Miquel Raynal <[e-mail]>; Richard Weinberger
> > > > <[e-mail]>; Vignesh Raghavendra <[e-mail]>; De Marchi,
> > > > Lucas <[e-mail]>; Thomas Hellström
> > > > <[e-mail]>; Maarten Lankhorst
> > > > <[e-mail]>; Maxime Ripard
> > <[e-mail]>;
> > > > Thomas Zimmermann <[e-mail]>; David Airlie
> > > > <[e-mail]>; Simona Vetter <[e-mail]>; Jani Nikula
> > > > <[e-mail]>; Joonas Lahtinen
> > > > <[e-mail]>; Tvrtko Ursulin <[e-mail]>;
> > > > Weil, Oren jer <[e-mail]>; [e-mail];
> > dri-
> > > > [e-mail]; [e-mail]; linux-
> > > > [e-mail]
> > > > Subject: RE: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Vivi, Rodrigo <[e-mail]>
> > > > > Sent: Monday, October 28, 2024 8:27 PM
> > > > > To: Usyskin, Alexander <[e-mail]>; Gupta,
> > Anshuman
> > > > > <[e-mail]>; Deak, Imre <[e-mail]>
> > > > > Cc: Miquel Raynal <[e-mail]>; Richard Weinberger
> > > > > <[e-mail]>; Vignesh Raghavendra <[e-mail]>; De Marchi,
> > > > > Lucas <[e-mail]>; Thomas Hellström
> > > > > <[e-mail]>; Maarten Lankhorst
> > > > > <[e-mail]>; Maxime Ripard
> > > > <[e-mail]>;
> > > > > Thomas Zimmermann <[e-mail]>; David Airlie
> > > > > <[e-mail]>; Simona Vetter <[e-mail]>; Jani Nikula
> > > > > <[e-mail]>; Joonas Lahtinen
> > > > > <[e-mail]>; Tvrtko Ursulin
> > <[e-mail]>;
> > > > > Weil, Oren jer <[e-mail]>; [e-mail];
> > dri-
> > > > > [e-mail]; [e-mail]; linux-
> > > > > [e-mail]
> > > > > 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 <[e-mail]>
> > > > > > Signed-off-by: Alexander Usyskin <[e-mail]>
> > > > > > ---
> > > > > >  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 <[e-mail]>
> > > > > Cc: Anshuman Gupta <[e-mail]>
> > > > >
> > > > > > +
> > > > > > +	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
> > > > > >

Miquel Raynal Nov. 8, 2024, 9:01 a.m. UTC | #7

>> > 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...

That's three drivers where there is probably room for improvement.

These differences are subtle and likely un-catch during review. Feel
free to correct the drivers if you think they are wrong and more
importantly, do the correct thing in your own.

Thanks,
Miquèl

Alexander Usyskin Nov. 10, 2024, 1:16 p.m. UTC | #8

> -----Original Message-----
> From: Vivi, Rodrigo <[e-mail]>
> Sent: Friday, November 8, 2024 12:50 AM
> To: Usyskin, Alexander <[e-mail]>
> Cc: Gupta, Anshuman <[e-mail]>; Deak, Imre
> <[e-mail]>; Miquel Raynal <[e-mail]>;
> Richard Weinberger <[e-mail]>; Vignesh Raghavendra
> <[e-mail]>; De Marchi, Lucas <[e-mail]>; Thomas
> Hellström <[e-mail]>; Maarten Lankhorst
> <[e-mail]>; Maxime Ripard <[e-mail]>;
> Thomas Zimmermann <[e-mail]>; David Airlie
> <[e-mail]>; Simona Vetter <[e-mail]>; Jani Nikula
> <[e-mail]>; Joonas Lahtinen
> <[e-mail]>; Tvrtko Ursulin <[e-mail]>;
> Weil, Oren jer <[e-mail]>; [e-mail]; dri-
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: Re: [PATCH 06/10] mtd: intel-dg: wake card on operations
> 
> On Tue, Nov 05, 2024 at 07:17:53AM -0500, Usyskin, Alexander wrote:
> > > -----Original Message-----
> > > From: Vivi, Rodrigo <[e-mail]>
> > > Sent: Monday, November 4, 2024 11:16 PM
> > > To: Usyskin, Alexander <[e-mail]>
> > > Cc: Gupta, Anshuman <[e-mail]>; Deak, Imre
> > > <[e-mail]>; Miquel Raynal <[e-mail]>;
> > > Richard Weinberger <[e-mail]>; Vignesh Raghavendra
> > > <[e-mail]>; De Marchi, Lucas <[e-mail]>;
> Thomas
> > > Hellström <[e-mail]>; Maarten Lankhorst
> > > <[e-mail]>; Maxime Ripard
> <[e-mail]>;
> > > Thomas Zimmermann <[e-mail]>; David Airlie
> > > <[e-mail]>; Simona Vetter <[e-mail]>; Jani Nikula
> > > <[e-mail]>; Joonas Lahtinen
> > > <[e-mail]>; Tvrtko Ursulin <[e-mail]>;
> > > Weil, Oren jer <[e-mail]>; [e-mail];
> dri-
> > > [e-mail]; [e-mail]; linux-
> > > [e-mail]
> > > 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 <[e-mail]>
> > > > > Sent: Monday, October 28, 2024 5:02 PM
> > > > > To: Vivi, Rodrigo <[e-mail]>; Usyskin, Alexander
> > > > > <[e-mail]>; Deak, Imre <[e-mail]>
> > > > > Cc: Miquel Raynal <[e-mail]>; Richard Weinberger
> > > > > <[e-mail]>; Vignesh Raghavendra <[e-mail]>; De
> Marchi,
> > > > > Lucas <[e-mail]>; Thomas Hellström
> > > > > <[e-mail]>; Maarten Lankhorst
> > > > > <[e-mail]>; Maxime Ripard
> > > <[e-mail]>;
> > > > > Thomas Zimmermann <[e-mail]>; David Airlie
> > > > > <[e-mail]>; Simona Vetter <[e-mail]>; Jani Nikula
> > > > > <[e-mail]>; Joonas Lahtinen
> > > > > <[e-mail]>; Tvrtko Ursulin
> <[e-mail]>;
> > > > > Weil, Oren jer <[e-mail]>; linux-
> [e-mail];
> > > dri-
> > > > > [e-mail]; [e-mail]; linux-
> > > > > [e-mail]
> > > > > Subject: RE: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Vivi, Rodrigo <[e-mail]>
> > > > > > Sent: Monday, October 28, 2024 8:27 PM
> > > > > > To: Usyskin, Alexander <[e-mail]>; Gupta,
> > > Anshuman
> > > > > > <[e-mail]>; Deak, Imre <[e-mail]>
> > > > > > Cc: Miquel Raynal <[e-mail]>; Richard Weinberger
> > > > > > <[e-mail]>; Vignesh Raghavendra <[e-mail]>; De
> Marchi,
> > > > > > Lucas <[e-mail]>; Thomas Hellström
> > > > > > <[e-mail]>; Maarten Lankhorst
> > > > > > <[e-mail]>; Maxime Ripard
> > > > > <[e-mail]>;
> > > > > > Thomas Zimmermann <[e-mail]>; David Airlie
> > > > > > <[e-mail]>; Simona Vetter <[e-mail]>; Jani Nikula
> > > > > > <[e-mail]>; Joonas Lahtinen
> > > > > > <[e-mail]>; Tvrtko Ursulin
> > > <[e-mail]>;
> > > > > > Weil, Oren jer <[e-mail]>; linux-
> [e-mail];
> > > dri-
> > > > > > [e-mail]; [e-mail]; linux-
> > > > > > [e-mail]
> > > > > > 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 <[e-mail]>
> > > > > > > Signed-off-by: Alexander Usyskin <[e-mail]>
> > > > > > > ---
> > > > > > >  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.
> 

At the probe time I do not have dev->mtd, but now I see you point here.
I'll separate power management:
- probe before dev->mtd creation will use aux_dev->dev (that will be mtd->dev.parent later)
- mtd functions will use mtd->dev

Is this that you have in mind?

> >
> > > > 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 <[e-mail]>
> > > > > > Cc: Anshuman Gupta <[e-mail]>
> > > > > >
> > > > > > > +
> > > > > > > +	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
> > > > > > >

Alexander Usyskin Nov. 11, 2024, 11:29 a.m. UTC | #9

> -----Original Message-----
> From: Usyskin, Alexander
> Sent: Sunday, November 10, 2024 3:17 PM
> To: Vivi, Rodrigo <[e-mail]>
> Cc: Gupta, Anshuman <[e-mail]>; Deak, Imre
> <[e-mail]>; Miquel Raynal <[e-mail]>;
> Richard Weinberger <[e-mail]>; Vignesh Raghavendra
> <[e-mail]>; De Marchi, Lucas <[e-mail]>; Thomas
> Hellström <[e-mail]>; Maarten Lankhorst
> <[e-mail]>; Maxime Ripard <[e-mail]>;
> Thomas Zimmermann <[e-mail]>; David Airlie
> <[e-mail]>; Simona Vetter <[e-mail]>; Jani Nikula
> <[e-mail]>; Joonas Lahtinen
> <[e-mail]>; Tvrtko Ursulin <[e-mail]>;
> Weil, Oren jer <[e-mail]>; [e-mail]; dri-
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: RE: [PATCH 06/10] mtd: intel-dg: wake card on operations
> 
> > -----Original Message-----
> > From: Vivi, Rodrigo <[e-mail]>
> > Sent: Friday, November 8, 2024 12:50 AM
> > To: Usyskin, Alexander <[e-mail]>
> > Cc: Gupta, Anshuman <[e-mail]>; Deak, Imre
> > <[e-mail]>; Miquel Raynal <[e-mail]>;
> > Richard Weinberger <[e-mail]>; Vignesh Raghavendra
> > <[e-mail]>; De Marchi, Lucas <[e-mail]>; Thomas
> > Hellström <[e-mail]>; Maarten Lankhorst
> > <[e-mail]>; Maxime Ripard
> <[e-mail]>;
> > Thomas Zimmermann <[e-mail]>; David Airlie
> > <[e-mail]>; Simona Vetter <[e-mail]>; Jani Nikula
> > <[e-mail]>; Joonas Lahtinen
> > <[e-mail]>; Tvrtko Ursulin <[e-mail]>;
> > Weil, Oren jer <[e-mail]>; [e-mail]; dri-
> > [e-mail]; [e-mail]; linux-
> > [e-mail]
> > Subject: Re: [PATCH 06/10] mtd: intel-dg: wake card on operations
> >
> > On Tue, Nov 05, 2024 at 07:17:53AM -0500, Usyskin, Alexander wrote:
> > > > -----Original Message-----
> > > > From: Vivi, Rodrigo <[e-mail]>
> > > > Sent: Monday, November 4, 2024 11:16 PM
> > > > To: Usyskin, Alexander <[e-mail]>
> > > > Cc: Gupta, Anshuman <[e-mail]>; Deak, Imre
> > > > <[e-mail]>; Miquel Raynal <[e-mail]>;
> > > > Richard Weinberger <[e-mail]>; Vignesh Raghavendra
> > > > <[e-mail]>; De Marchi, Lucas <[e-mail]>;
> > Thomas
> > > > Hellström <[e-mail]>; Maarten Lankhorst
> > > > <[e-mail]>; Maxime Ripard
> > <[e-mail]>;
> > > > Thomas Zimmermann <[e-mail]>; David Airlie
> > > > <[e-mail]>; Simona Vetter <[e-mail]>; Jani Nikula
> > > > <[e-mail]>; Joonas Lahtinen
> > > > <[e-mail]>; Tvrtko Ursulin
> <[e-mail]>;
> > > > Weil, Oren jer <[e-mail]>; [e-mail];
> > dri-
> > > > [e-mail]; [e-mail]; linux-
> > > > [e-mail]
> > > > 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 <[e-mail]>
> > > > > > Sent: Monday, October 28, 2024 5:02 PM
> > > > > > To: Vivi, Rodrigo <[e-mail]>; Usyskin, Alexander
> > > > > > <[e-mail]>; Deak, Imre <[e-mail]>
> > > > > > Cc: Miquel Raynal <[e-mail]>; Richard Weinberger
> > > > > > <[e-mail]>; Vignesh Raghavendra <[e-mail]>; De
> > Marchi,
> > > > > > Lucas <[e-mail]>; Thomas Hellström
> > > > > > <[e-mail]>; Maarten Lankhorst
> > > > > > <[e-mail]>; Maxime Ripard
> > > > <[e-mail]>;
> > > > > > Thomas Zimmermann <[e-mail]>; David Airlie
> > > > > > <[e-mail]>; Simona Vetter <[e-mail]>; Jani Nikula
> > > > > > <[e-mail]>; Joonas Lahtinen
> > > > > > <[e-mail]>; Tvrtko Ursulin
> > <[e-mail]>;
> > > > > > Weil, Oren jer <[e-mail]>; linux-
> > [e-mail];
> > > > dri-
> > > > > > [e-mail]; [e-mail]; linux-
> > > > > > [e-mail]
> > > > > > Subject: RE: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Vivi, Rodrigo <[e-mail]>
> > > > > > > Sent: Monday, October 28, 2024 8:27 PM
> > > > > > > To: Usyskin, Alexander <[e-mail]>; Gupta,
> > > > Anshuman
> > > > > > > <[e-mail]>; Deak, Imre <[e-mail]>
> > > > > > > Cc: Miquel Raynal <[e-mail]>; Richard
> Weinberger
> > > > > > > <[e-mail]>; Vignesh Raghavendra <[e-mail]>; De
> > Marchi,
> > > > > > > Lucas <[e-mail]>; Thomas Hellström
> > > > > > > <[e-mail]>; Maarten Lankhorst
> > > > > > > <[e-mail]>; Maxime Ripard
> > > > > > <[e-mail]>;
> > > > > > > Thomas Zimmermann <[e-mail]>; David Airlie
> > > > > > > <[e-mail]>; Simona Vetter <[e-mail]>; Jani Nikula
> > > > > > > <[e-mail]>; Joonas Lahtinen
> > > > > > > <[e-mail]>; Tvrtko Ursulin
> > > > <[e-mail]>;
> > > > > > > Weil, Oren jer <[e-mail]>; linux-
> > [e-mail];
> > > > dri-
> > > > > > > [e-mail]; [e-mail]; linux-
> > > > > > > [e-mail]
> > > > > > > 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 <[e-mail]>
> > > > > > > > Signed-off-by: Alexander Usyskin <[e-mail]>
> > > > > > > > ---
> > > > > > > >  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.
> >
> 
> At the probe time I do not have dev->mtd, but now I see you point here.
> I'll separate power management:
> - probe before dev->mtd creation will use aux_dev->dev (that will be mtd-
> >dev.parent later)
> - mtd functions will use mtd->dev
> 
> Is this that you have in mind?

I've tried it and found out that mtd->dev is not initialized if partitions are present [1].
Miquel - this may be the reason why other mtd drivers use pci or platform
devices to manage runtime pm.
Or I have missed something?

[1] https://elixir.bootlin.com/linux/v6.12-rc6/source/drivers/mtd/mtdcore.c#L1078
> 
> > >
> > > > > 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 <[e-mail]>
> > > > > > > Cc: Anshuman Gupta <[e-mail]>
> > > > > > >
> > > > > > > > +
> > > > > > > > +	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
> > > > > > > >

Miquel Raynal Nov. 11, 2024, 8:27 p.m. UTC | #10

Hi Alexander,

Please reduce the context when answering, otherwise it's hard to find
all places where you commented.

>> > > > 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.
>> >
>> 
>> At the probe time I do not have dev->mtd, but now I see you point here.
>> I'll separate power management:
>> - probe before dev->mtd creation will use aux_dev->dev (that will be mtd-
>> >dev.parent later)
>> - mtd functions will use mtd->dev
>> 
>> Is this that you have in mind?
>
> I've tried it and found out that mtd->dev is not initialized if partitions are present [1].
> Miquel - this may be the reason why other mtd drivers use pci or platform
> devices to manage runtime pm.
> Or I have missed something?

Please keep in mind there is _a lot_ of history behind mtd, and
sometimes choices from the past cannot be simply "fixed" without
breaking userspace. The problem with mtd is that the "mtd" structure
defines nothing with precision. It may be a controller, a chip, a
partition, or whatever mix of those. In this particular case, I believe
you are mentioning the KEEP_PARTITIONED_MASTER configuration, which by
default is unset, which means you'll loose the "top level" mtd device?

However in general I believe the "framework" struct device is maybe less
relevant than the "bus" struct device when it comes to runtime PM, so
actually I would eventually expect this device to be used?

> [1] https://elixir.bootlin.com/linux/v6.12-rc6/source/drivers/mtd/mtdcore.c#L1078

Thanks,
Miquèl

Patch

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);
+
+	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);