Message ID | 20100906080838.GA6528@intel.com |
---|---|
State | RFC |
Headers | show |
On Mon, 6 Sep 2010 16:08:38 +0800 "Chuanxiao.Dong" <chuanxiao.dong@intel.com> wrote: > +/* NOW denali NAND controller in MRST has no way to cut power off > + * once it is power on, so just let these functions be empty > + * */ this isn't right; you should put the device in PCI D3 state. > + > +static struct dev_pm_ops denali_pm = { > + .runtime_suspend = denali_runtime_suspend, > + .runtime_resume = denali_runtime_resume, > + .runtime_idle = denali_runtime_idle, you don't really need a runtime_idle function > +}; > +/* support denali runtime pm */ > +static void denali_timer_add(struct denali_nand_info *denali) and this is where I get very nervous about your patch. The runtime pm infrastructure already has timers, you don't need to implement one! in fact, I think the code is rather not using the runtime PM infrastructure right. The runtime PM framework offers you a reference counter for activity, so what really ought to be done instead of this polling timer, is to take a refcount each time something is submitted to the hardware.. > + if (chip->state != FL_READY || ... and decremented each time something is completed. This will change the chip->state for you... each place chip->state is used is a candidate for this proper refcounting. basically, this timer in the driver is rather misplaced and should be replaced by using the refcount from the runtime PM infrastructure correctly...... and I suspect your patch gets a LOT simpler as well for free.
Hi See my comments below. Thanks --Chuanxiao > -----Original Message----- > From: Arjan van de Ven [mailto:arjan@infradead.org] > Sent: Tuesday, September 07, 2010 3:53 AM > To: Dong, Chuanxiao > Cc: dwmw2@infradead.org; linux-mtd@lists.infradead.org > Subject: Re: [RFC][PATCH 1/1]nand/denali: Add runtime pm support for denali > controller driver > > On Mon, 6 Sep 2010 16:08:38 +0800 > "Chuanxiao.Dong" <chuanxiao.dong@intel.com> wrote: > > > > +/* NOW denali NAND controller in MRST has no way to cut power off > > + * once it is power on, so just let these functions be empty > > + * */ > > this isn't right; you should put the device in PCI D3 state. This will be update after I got a correct runtime PM framework using. > > > + > > +static struct dev_pm_ops denali_pm = { > > + .runtime_suspend = denali_runtime_suspend, > > + .runtime_resume = denali_runtime_resume, > > + .runtime_idle = denali_runtime_idle, > > you don't really need a runtime_idle function > > > > +}; > > +/* support denali runtime pm */ > > +static void denali_timer_add(struct denali_nand_info *denali) > > and this is where I get very nervous about your patch. > > The runtime pm infrastructure already has timers, you don't need to > implement one! > > in fact, I think the code is rather not using the runtime PM > infrastructure right. > > The runtime PM framework offers you a reference counter for activity, > so what really ought to be done instead of this polling timer, is to > take a refcount each time something is submitted to the hardware.. > > > + if (chip->state != FL_READY || > > ... and decremented each time something is completed. This will change > the chip->state for you... each place chip->state is used is a > candidate for this proper refcounting. And this is what I am confusing. Each place chip->state is used is in MTD framework....no in MTD driver itself. MTD driver never know when it is opened or released. Each time before hardware has something to do chip->state is already set to be something like READING/ WRITING by MTD framework. And after finishing the hardware work, chip->state does not change. Then return back to MTD framework. If there are still something for hardware to do, chip->state will not change. Only when there is nothing to do, MTD framework will set chip->state to be READY after MTD driver finishing the last hardware work. That is the process for MTD driver working. It only maintains the hardware working part. Before or after it finishes the hardware work, the chip->state is always to be READING/WRITING. It never knows when there will have the first or the last hardware access. So that is why I add a timer for driver to detect the chip->state. And that is why I think it's better for MTD framework to do some runtime PM work. > > basically, this timer in the driver is rather misplaced and should be > replaced by using the refcount from the runtime PM infrastructure > correctly...... and I suspect your patch gets a LOT simpler as well for > free.
On Tue, 7 Sep 2010 10:12:09 +0800 "Dong, Chuanxiao" <chuanxiao.dong@intel.com> wrote: > Hi > See my comments below. > Thanks > --Chuanxiao > > > -----Original Message----- > > From: Arjan van de Ven [mailto:arjan@infradead.org] > > Sent: Tuesday, September 07, 2010 3:53 AM > > To: Dong, Chuanxiao > > Cc: dwmw2@infradead.org; linux-mtd@lists.infradead.org > > Subject: Re: [RFC][PATCH 1/1]nand/denali: Add runtime pm support > > for denali controller driver > > > > On Mon, 6 Sep 2010 16:08:38 +0800 > > "Chuanxiao.Dong" <chuanxiao.dong@intel.com> wrote: > > > > > > > +/* NOW denali NAND controller in MRST has no way to cut power off > > > + * once it is power on, so just let these functions be empty > > > + * */ > > > > this isn't right; you should put the device in PCI D3 state. > > This will be update after I got a correct runtime PM framework using. > > > > > > + > > > +static struct dev_pm_ops denali_pm = { > > > + .runtime_suspend = denali_runtime_suspend, > > > + .runtime_resume = denali_runtime_resume, > > > + .runtime_idle = denali_runtime_idle, > > > > you don't really need a runtime_idle function > > > > > > > +}; > > > +/* support denali runtime pm */ > > > +static void denali_timer_add(struct denali_nand_info *denali) > > > > and this is where I get very nervous about your patch. > > > > The runtime pm infrastructure already has timers, you don't need to > > implement one! > > > > in fact, I think the code is rather not using the runtime PM > > infrastructure right. > > > > The runtime PM framework offers you a reference counter for > > activity, so what really ought to be done instead of this polling > > timer, is to take a refcount each time something is submitted to > > the hardware.. > > > > > + if (chip->state != FL_READY || > > > > ... and decremented each time something is completed. This will > > change the chip->state for you... each place chip->state is used is > > a candidate for this proper refcounting. > > And this is what I am confusing. Each place chip->state is used is in > MTD framework....no in MTD driver itself. MTD driver never know when > it is opened or released. Each time before hardware has something to > do chip->state is already set to be something like READING/ WRITING > by MTD framework. And after finishing the hardware work, chip->state > does not change. Then return back to MTD framework. If there are > still something for hardware to do, chip->state will not change. Only > when there is nothing to do, MTD framework will set chip->state to be > READY after MTD driver finishing the last hardware work. That is the > process for MTD driver working. It only maintains the hardware > working part. Before or after it finishes the hardware work, the > chip->state is always to be READING/WRITING. It never knows when > there will have the first or the last hardware access. So that is why > I add a timer for driver to detect the chip->state. And that is why I > think it's better for MTD framework to do some runtime PM work. if you take the refcount every time you get a command, and drop a count each time a command completes.... this is all not too important.. it'll just work
> if you take the refcount every time you get a command, and drop a count > each time a command completes.... this is all not too important.. it'll > just work If I take the refcount every time I got a command and drop a count each time a command completes, the controller device will be resumed every time when it gets a command and suspended after it finishes a command. For a 4k page NAND, processing just 1M data will cause device resume/suspend 256 times....This will influence the performance....Although denali controller cannot be really resumed/suspend, to these devices who can resume/suspend, I think it is not a good way. Right now there are 6 patches for denali MTD driver pending. After these 6 patches are accepted and Denali MTD driver can be used on MRST platform, I will resubmit a runtime pm patch. Arjan, do you agree?
On Tue, 7 Sep 2010 12:03:40 +0800 "Dong, Chuanxiao" <chuanxiao.dong@intel.com> wrote: > > > if you take the refcount every time you get a command, and drop a > > count each time a command completes.... this is all not too > > important.. it'll just work > If I take the refcount every time I got a command and drop a count > each time a command completes, the controller device will be resumed > every time when it gets a command and suspended after it finishes a > command. For a 4k page NAND, processing just 1M data will cause > device resume/suspend 256 times....This will influence the > performance... so this is where you can tell the runtime PM system to use a timer, and only turn on power saving after some idle time. you don't need to do that yourself... the runtime PM system has this capability built in!
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c index 532fe07..976b6b9 100644 --- a/drivers/mtd/nand/denali.c +++ b/drivers/mtd/nand/denali.c @@ -25,6 +25,10 @@ #include <linux/pci.h> #include <linux/mtd/mtd.h> #include <linux/module.h> +#include <linux/pm.h> +#include <linux/pm_runtime.h> +#include <linux/jiffies.h> +#include <linux/param.h> #include "denali.h" @@ -119,6 +123,9 @@ static const uint32_t reset_complete[4] = {INTR_STATUS0__RST_COMP, INTR_STATUS2__RST_COMP, INTR_STATUS3__RST_COMP}; +/* record interrupt jiffies */ +static unsigned long last_intr; + /* forward declarations */ static void clear_interrupts(struct denali_nand_info *denali); static uint32_t wait_for_irq(struct denali_nand_info *denali, @@ -856,6 +863,89 @@ static int read_data_from_flash_mem(struct denali_nand_info *denali, return i*4; /* intent is to return the number of bytes read */ } +/* NOW denali NAND controller in MRST has no way to cut power off + * once it is power on, so just let these functions be empty + * */ +#ifdef CONFIG_PM_RUNTIME +static int denali_runtime_suspend(struct device *dev) +{ + dev_info(dev, "%s: disable_depth %d usage_count %d", __func__, + dev->power.disable_depth, dev->power.usage_count.counter); + dev_info(dev, "%s called", __func__); + /* Denali Controller in MRST doesn't have any capbility + * to shut power down or resume back, no register or GPIO + * can do this, so here do nothing + * */ + return 0; +} + +static int denali_runtime_resume(struct device *dev) +{ + dev_info(dev, "%s: disable_depth %d usage_count %d", __func__, + dev->power.disable_depth, dev->power.usage_count.counter); + dev_info(dev, "%s called", __func__); + /* Denali Controller in MRST doesn't have any capbility + * to shut power down or resume back, no register or GPIO + * can do this, so here do nothing + * */ + return 0; +} + +static int denali_runtime_idle(struct device *dev) +{ + dev_info(dev, "%s: disable_depth %d usage_count %d", __func__, + dev->power.disable_depth, dev->power.usage_count.counter); + dev_info(dev, "%s called", __func__); + /* denali controller will only be resume once during + * read/write/erase operation, so if call runtime idle, + * means device can be suspend + * */ + return 0; +} + +static struct dev_pm_ops denali_pm = { + .runtime_suspend = denali_runtime_suspend, + .runtime_resume = denali_runtime_resume, + .runtime_idle = denali_runtime_idle, +}; +/* support denali runtime pm */ +static void denali_timer_add(struct denali_nand_info *denali) +{ + int ret; + struct device *dev = &denali->dev->dev; + if (denali->pm_status == DENALI_OFF) { + ret = pm_runtime_get_sync(dev); + denali->pm_status = DENALI_ON; + denali->timer.expires = jiffies + 30 * HZ; + add_timer(&denali->timer); + } +} + +static void denali_timer_func(unsigned long ptr) +{ + struct denali_nand_info *denali = + (struct denali_nand_info *)ptr; + struct device *dev = &denali->dev->dev; + struct nand_chip *chip = &denali->nand; + int ret; + + if (denali->pm_status == DENALI_OFF) + BUG(); + + if (chip->state != FL_READY || + jiffies - last_intr < 1000) { + denali->timer.expires = jiffies + 30 * HZ; + add_timer(&denali->timer); + } else { + ret = pm_runtime_put_sync(dev); + denali->pm_status = DENALI_OFF; + } +} +#else +static inline void denali_timer_add(struct denali_nand_info *denali) {} +static inline void denali_timer_func(unsigned long ptr) {} +#endif + /* writes OOB data to the device */ static int write_oob_data(struct mtd_info *mtd, uint8_t *buf, int page) { @@ -865,6 +955,8 @@ static int write_oob_data(struct mtd_info *mtd, uint8_t *buf, int page) INTR_STATUS0__PROGRAM_FAIL; int status = 0; + denali_timer_add(denali); + denali->page = page; if (denali_send_pipeline_cmd(denali, false, false, SPARE_ACCESS, @@ -892,6 +984,8 @@ static void read_oob_data(struct mtd_info *mtd, uint8_t *buf, int page) uint32_t irq_mask = INTR_STATUS0__LOAD_COMP, irq_status = 0, addr = 0x0, cmd = 0x0; + denali_timer_add(denali); + denali->page = page; if (denali_send_pipeline_cmd(denali, false, true, SPARE_ACCESS, @@ -1054,6 +1148,8 @@ static void write_page(struct mtd_info *mtd, struct nand_chip *chip, uint32_t irq_mask = INTR_STATUS0__DMA_CMD_COMP | INTR_STATUS0__PROGRAM_FAIL; + denali_timer_add(denali); + /* if it is a raw xfer, we want to disable ecc, and send * the spare area. * !raw_xfer - enable ecc @@ -1149,6 +1245,8 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip, INTR_STATUS0__ECC_ERR; bool check_erased_page = false; + denali_timer_add(denali); + if (page != denali->page) { dev_err(&denali->dev->dev, "IN %s: page %d is not" " equal to denali->page %d, investigate!!", @@ -1200,6 +1298,8 @@ static int denali_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, uint32_t irq_status = 0; uint32_t irq_mask = INTR_STATUS0__DMA_CMD_COMP; + denali_timer_add(denali); + if (page != denali->page) { dev_err(&denali->dev->dev, "IN %s: page %d is not" " equal to denali->page %d, investigate!!", @@ -1263,6 +1363,8 @@ static void denali_erase(struct mtd_info *mtd, int page) uint32_t cmd = 0x0, irq_status = 0; + denali_timer_add(denali); + /* clear interrupts */ clear_interrupts(denali); @@ -1312,6 +1414,7 @@ static void denali_cmdfunc(struct mtd_info *mtd, unsigned int cmd, int col, denali->page = page; break; case NAND_CMD_RESET: + denali_timer_add(denali); reset_bank(denali); break; case NAND_CMD_READOOB: @@ -1435,6 +1538,12 @@ void denali_drv_init(struct denali_nand_info *denali) /* initialize our irq_status variable to indicate no interrupts */ denali->irq_status = 0; + + /* Initilize denali timer */ + init_timer(&denali->timer); + denali->timer.data = (unsigned long)denali; + denali->timer.function = &denali_timer_func; + denali->pm_status = DENALI_ON; } /* driver entry point */ @@ -1669,6 +1778,12 @@ static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id) ret); goto failed_req_irq; } + + /* init pm runtime */ + denali->pm_status = DENALI_OFF; + pm_runtime_allow(&dev->dev); + pm_runtime_put_noidle(&dev->dev); + return 0; failed_req_irq: @@ -1694,6 +1809,10 @@ static void denali_pci_remove(struct pci_dev *dev) { struct denali_nand_info *denali = pci_get_drvdata(dev); + del_timer(&denali->timer); + if (denali->pm_status == DENALI_ON) + pm_runtime_put_sync(&dev->dev); + nand_release(&denali->mtd); del_mtd_device(&denali->mtd); @@ -1707,11 +1826,17 @@ static void denali_pci_remove(struct pci_dev *dev) PCI_DMA_BIDIRECTIONAL); pci_set_drvdata(dev, NULL); kfree(denali); + pm_runtime_get_noresume(&dev->dev); } MODULE_DEVICE_TABLE(pci, denali_pci_ids); static struct pci_driver denali_pci_driver = { +#ifdef CONFIG_PM_RUNTIME + .driver = { + .pm = &denali_pm, + }, +#endif .name = DENALI_NAND_NAME, .id_table = denali_pci_ids, .probe = denali_pci_probe,