Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller
Hi Felipe, On Monday 29 July 2013 06:05 PM, Felipe Balbi wrote: Hi, On Mon, Jul 29, 2013 at 04:45:02PM +0530, Sourav Poddar wrote: + irq = platform_get_irq(pdev, 0); + if (irq 0) { + dev_err(pdev-dev, no irq resource?\n); + return irq; + } + + spin_lock_init(qspi-lock); + + qspi-base = devm_ioremap_resource(pdev-dev, r); + if (IS_ERR(qspi-base)) { + ret = PTR_ERR(qspi-base); + goto free_master; + } + + ret = devm_request_threaded_irq(pdev-dev, irq, ti_qspi_isr, + ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT, why do you need IRQF_NO_SUSPEND ? I should get away with this. why ? Do you need or do you *not* need it ? And in either case, why ? I was thinking, this will keep the irqs up even when we are hitting suspend, we will not be prepared to handle it. ? won't be prepared in what way ? Our driver will be down, so the irq might go un-serviced. only if you wrote the driver that way. IRQ subsystem doesn't know the state of the device, all it knows is that in case an IRQ fires, it needs to call the registered IRQ handler. Now the thing is: Initially you had the flag setup, so I asked why you needed it. I expected you to tell me why you think QSPI's IRQ shouldn't be disabled during suspend and the implications of disabling it. Instead you just changed your mind and decided to remove the flag. Because you changed your mind with no explanation, that tells me you haven't fully grasped how that flag works and what it means to set (or not set) it. My question now is simply: why don't you need that flag ? What are the implications of setting that flag ? How would your driver behave if an IRQ fired while your driver was suspended ? Unless you understand what it does, how can you understand the behavior or the driver ? cheers We dont need IRQF_NO_SUSPEND flag in our qspi controller. Qspi driver need to be prevented from receiving interrupts during system wide suspend/hibernation. suspend_device_irqs in kernel/irq/pm.c marks interrupt line in use and sets IRQS_SUSPENDED for them. If we use IRQF_NO_SUSPEND, we will bypass setting this IRQS_SUSPENDED flag, which is not. desired For this, interrupt lines need to and this function is provided for this purpose. * It marks all interrupt lines in use, except for the timer ones, as disabled * and sets the IRQS_SUSPENDED flag for each of them. This flag gets used in __disable_irq api(kernel/irq/manage.c) which -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller
Hi, On Tue, Jul 30, 2013 at 01:04:28PM +0530, Sourav Poddar wrote: Hi Felipe, On Monday 29 July 2013 06:05 PM, Felipe Balbi wrote: Hi, On Mon, Jul 29, 2013 at 04:45:02PM +0530, Sourav Poddar wrote: + irq = platform_get_irq(pdev, 0); + if (irq 0) { + dev_err(pdev-dev, no irq resource?\n); + return irq; + } + + spin_lock_init(qspi-lock); + + qspi-base = devm_ioremap_resource(pdev-dev, r); + if (IS_ERR(qspi-base)) { + ret = PTR_ERR(qspi-base); + goto free_master; + } + + ret = devm_request_threaded_irq(pdev-dev, irq, ti_qspi_isr, + ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT, why do you need IRQF_NO_SUSPEND ? I should get away with this. why ? Do you need or do you *not* need it ? And in either case, why ? I was thinking, this will keep the irqs up even when we are hitting suspend, we will not be prepared to handle it. ? won't be prepared in what way ? Our driver will be down, so the irq might go un-serviced. only if you wrote the driver that way. IRQ subsystem doesn't know the state of the device, all it knows is that in case an IRQ fires, it needs to call the registered IRQ handler. Now the thing is: Initially you had the flag setup, so I asked why you needed it. I expected you to tell me why you think QSPI's IRQ shouldn't be disabled during suspend and the implications of disabling it. Instead you just changed your mind and decided to remove the flag. Because you changed your mind with no explanation, that tells me you haven't fully grasped how that flag works and what it means to set (or not set) it. My question now is simply: why don't you need that flag ? What are the implications of setting that flag ? How would your driver behave if an IRQ fired while your driver was suspended ? Unless you understand what it does, how can you understand the behavior or the driver ? cheers We dont need IRQF_NO_SUSPEND flag in our qspi controller. Qspi driver need to be prevented from receiving interrupts during system wide suspend/hibernation. suspend_device_irqs in kernel/irq/pm.c marks interrupt line in use and sets IRQS_SUSPENDED for them. If we use IRQF_NO_SUSPEND, we will bypass setting this IRQS_SUSPENDED flag, which is not. desired For this, interrupt lines need to and this function is provided for this purpose. * It marks all interrupt lines in use, except for the timer ones, as disabled * and sets the IRQS_SUSPENDED flag for each of them. This flag gets used in __disable_irq api(kernel/irq/manage.c) which some formatting issues in the mail, but good that you looked at the code. Cheers -- balbi signature.asc Description: Digital signature
Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller
Hi, On Mon, Jul 29, 2013 at 12:52:29PM +0530, Sourav Poddar wrote: diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c new file mode 100644 index 000..51fe95f --- /dev/null +++ b/drivers/spi/spi-ti-qspi.c [ snip ] +struct ti_qspi { + spinlock_t lock; /* IRQ synchronization */ + struct spi_master *master; + void __iomem*base; + struct device *dev; + struct completion transfer_complete; + struct clk *fclk; + struct ti_qspi_regs ctx_reg; + int device_type; device_type isn't used + u32 spi_max_frequency; + u32 cmd; + u32 dc; +}; you need to make a choice here ? Are you going to tabify the structure or not ? You might also want to reorganize this structure to it looks like below: struct ti_qspi { struct completion transfer_complete; /* IRQ synchronization */ spinlock_t lock; struct spi_master *master; void __iomem*base; struct clk *fclk; struct device *dev; struct ti_qspi_regs ctx_reg; u32 spi_max_frequency; u32 cmd; u32 dc; }; +/* Status */ +#define QSPI_WC (1 1) +#define QSPI_BUSY(1 0) +#define QSPI_WC_BUSY (QSPI_WC | QSPI_BUSY) WC_BUSY isn't used in this file. It looks unnecessary too. +#define QSPI_XFER_DONE QSPI_WC so transfer done is word completion ? Why do you need this ? It's not even used in this source file. +static inline void ti_qspi_read_data(struct ti_qspi *qspi, + unsigned long reg, int wlen, u8 **rxbuf) +{ + switch (wlen) { + case 8: + **rxbuf = readb(qspi-base + reg); + dev_dbg(qspi-dev, rx done, read %02x\n, *(*rxbuf-1)); you're incrementing only after printing, do you really need the -1 here? Also, I would change these into dev_vdbg(), it's quite unlikely someone needs to track all reads to the data register and since you're not printing the writes either, this looks even more like a case for dev_vdbg() + *rxbuf += 1; + break; + case 16: + **rxbuf = readw(qspi-base + reg); + dev_dbg(qspi-dev, rx done, read %02x\n, *(*rxbuf-1)); %04x and no -1 (also, if you needed to decrement, it would have to be %-2). + *rxbuf += 2; + break; + case 32: + **rxbuf = readl(qspi-base + reg); + dev_dbg(qspi-dev, rx done, read %02x\n, *(*rxbuf-1)); %04x + *rxbuf += 4; + default: + dev_dbg(qspi-dev, word lenght out of range); s/lenght/length +static int ti_qspi_setup(struct spi_device *spi) +{ + struct ti_qspi *qspi = spi_master_get_devdata(spi-master); + struct ti_qspi_regs *ctx_reg = qspi-ctx_reg; + int clk_div = 0, ret; + u32 clk_ctrl_reg, clk_rate, clk_mask; + + clk_rate = clk_get_rate(qspi-fclk); + + if (!qspi-spi_max_frequency) { + dev_err(qspi-dev, spi max frequency not defined\n); + return -EINVAL; if you're returning here... + } else { this else is unneeded + clk_div = DIV_ROUND_UP(clk_rate, qspi-spi_max_frequency) - 1; + } + + dev_dbg(qspi-dev, %s: hz: %d, clock divider %d\n, __func__, + qspi-spi_max_frequency, clk_div); + + ret = pm_runtime_get_sync(qspi-dev); + if (ret) { + dev_err(qspi-dev, pm_runtime_get_sync() failed\n); + return ret; + } after Mark's patch, is this really needed ? + clk_ctrl_reg = ti_qspi_read(qspi, QSPI_SPI_CLOCK_CNTRL_REG); + + clk_ctrl_reg = ~QSPI_CLK_EN; + + /* disable SCLK */ + if (!spi-master-busy) + ti_qspi_write(qspi, clk_ctrl_reg, QSPI_SPI_CLOCK_CNTRL_REG); + else + dev_dbg(qspi-dev, master busy doing other trasnfers\n); if master is busy, shouldn't you return -EBUSY at this point ? + + if (clk_div 0) { + dev_dbg(qspi-dev, %s: clock divider 0, using /1 divider\n, + __func__); + return -EINVAL; you do a get_sync without a put_sync() here. + } + + if (clk_div QSPI_CLK_DIV_MAX) { + dev_dbg(qspi-dev, %s: clock divider %d , using /%d divider\n, + __func__, QSPI_CLK_DIV_MAX, QSPI_CLK_DIV_MAX + 1); + return -EINVAL; no put_sync() + } + + /* enable SCLK */ + clk_mask = QSPI_CLK_EN | clk_div; + ti_qspi_write(qspi, clk_mask, QSPI_SPI_CLOCK_CNTRL_REG); + ctx_reg-clkctrl = clk_mask; + + pm_runtime_mark_last_busy(qspi-dev); + ret = pm_runtime_put_autosuspend(qspi-dev); + if (ret 0) { + dev_err(qspi-dev, pm_runtime_get_sync() failed\n); +
Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller
On Monday 29 July 2013 03:01 PM, Felipe Balbi wrote: Hi, On Mon, Jul 29, 2013 at 12:52:29PM +0530, Sourav Poddar wrote: diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c new file mode 100644 index 000..51fe95f --- /dev/null +++ b/drivers/spi/spi-ti-qspi.c [ snip ] +struct ti_qspi { + spinlock_t lock; /* IRQ synchronization */ + struct spi_master *master; + void __iomem*base; + struct device *dev; + struct completion transfer_complete; + struct clk *fclk; + struct ti_qspi_regs ctx_reg; + int device_type; device_type isn't used My bad. yes, will remove. Was added for some experiments. + u32 spi_max_frequency; + u32 cmd; + u32 dc; +}; you need to make a choice here ? Are you going to tabify the structure or not ? You might also want to reorganize this structure to it looks like below: struct ti_qspi { struct completion transfer_complete; /* IRQ synchronization */ spinlock_t lock; struct spi_master *master; void __iomem*base; struct clk *fclk; struct device *dev; struct ti_qspi_regs ctx_reg; u32 spi_max_frequency; u32 cmd; u32 dc; }; hmm..yes will do. +/* Status */ +#define QSPI_WC(1 1) +#define QSPI_BUSY (1 0) +#define QSPI_WC_BUSY (QSPI_WC | QSPI_BUSY) WC_BUSY isn't used in this file. It looks unnecessary too. Yes. +#define QSPI_XFER_DONE QSPI_WC so transfer done is word completion ? Why do you need this ? It's not even used in this source file. Yes, this was used in ealier versons for polled mode. Will remove. +static inline void ti_qspi_read_data(struct ti_qspi *qspi, + unsigned long reg, int wlen, u8 **rxbuf) +{ + switch (wlen) { + case 8: + **rxbuf = readb(qspi-base + reg); + dev_dbg(qspi-dev, rx done, read %02x\n, *(*rxbuf-1)); you're incrementing only after printing, do you really need the -1 here? Also, I would change these into dev_vdbg(), it's quite unlikely someone needs to track all reads to the data register and since you're not printing the writes either, this looks even more like a case for dev_vdbg() Ok.will change. + *rxbuf += 1; + break; + case 16: + **rxbuf = readw(qspi-base + reg); + dev_dbg(qspi-dev, rx done, read %02x\n, *(*rxbuf-1)); %04x and no -1 (also, if you needed to decrement, it would have to be %-2). Ok. + *rxbuf += 2; + break; + case 32: + **rxbuf = readl(qspi-base + reg); + dev_dbg(qspi-dev, rx done, read %02x\n, *(*rxbuf-1)); %04x OK + *rxbuf += 4; + default: + dev_dbg(qspi-dev, word lenght out of range); s/lenght/length hmm..will change. +static int ti_qspi_setup(struct spi_device *spi) +{ + struct ti_qspi *qspi = spi_master_get_devdata(spi-master); + struct ti_qspi_regs *ctx_reg =qspi-ctx_reg; + int clk_div = 0, ret; + u32 clk_ctrl_reg, clk_rate, clk_mask; + + clk_rate = clk_get_rate(qspi-fclk); + + if (!qspi-spi_max_frequency) { + dev_err(qspi-dev, spi max frequency not defined\n); + return -EINVAL; if you're returning here... + } else { this else is unneeded hmm..true. Will change. + clk_div = DIV_ROUND_UP(clk_rate, qspi-spi_max_frequency) - 1; + } + + dev_dbg(qspi-dev, %s: hz: %d, clock divider %d\n, __func__, + qspi-spi_max_frequency, clk_div); + + ret = pm_runtime_get_sync(qspi-dev); + if (ret) { + dev_err(qspi-dev, pm_runtime_get_sync() failed\n); + return ret; + } after Mark's patch, is this really needed ? + clk_ctrl_reg = ti_qspi_read(qspi, QSPI_SPI_CLOCK_CNTRL_REG); + + clk_ctrl_reg= ~QSPI_CLK_EN; + + /* disable SCLK */ + if (!spi-master-busy) + ti_qspi_write(qspi, clk_ctrl_reg, QSPI_SPI_CLOCK_CNTRL_REG); + else + dev_dbg(qspi-dev, master busy doing other trasnfers\n); if master is busy, shouldn't you return -EBUSY at this point ? Yes. will add. + + if (clk_div 0) { + dev_dbg(qspi-dev, %s: clock divider 0, using /1 divider\n, + __func__); + return -EINVAL; you do a get_sync without a put_sync() here. Hmm..will add put_sync in error path. + } + + if (clk_div QSPI_CLK_DIV_MAX) { + dev_dbg(qspi-dev, %s: clock divider%d , using /%d divider\n, + __func__, QSPI_CLK_DIV_MAX, QSPI_CLK_DIV_MAX + 1); + return -EINVAL;
Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller
Hi, On Mon, Jul 29, 2013 at 03:42:03PM +0530, Sourav Poddar wrote: + frame_length = (m-frame_length 3) / spi-bits_per_word; there's another way to optimize this. If you assume bits_per_word to always be power of two you can: frame_length = (m-frame_length 3) __ffs(spi-bits_per_word); but that would need a comment stating that you're assuming bits_per_word to always be a power of two. Not sure if this is a correct assumption. I dont think it will be a correct assumption, since our controller is flexible to handle anything from 1 to 128 as bits_per_word. right, but can the framework handle non-power-of-two bits_per_word ? + spin_unlock(qspi-lock); You should, before releasing the lock, mask your IRQs, so that you can get rid of IRQF_ONESHOT. Unmask them before returning from the IRQ thread. Sorry, did not get you here? I am reading interrupt status register before and clearing them in the threaded irq. you need to mask the IRQs, clear WC and FIRQ in IRQENABLE register... set them back at the end of the thread handler. +static int ti_qspi_probe(struct platform_device *pdev) +{ + struct ti_qspi *qspi; + struct spi_master *master; + struct resource *r; + struct device_node *np = pdev-dev.of_node; + u32 max_freq; + int ret = 0, num_cs, irq; + + master = spi_alloc_master(pdev-dev, sizeof(*qspi)); + if (!master) + return -ENOMEM; + + master-mode_bits = SPI_CPOL | SPI_CPHA; + + master-num_chipselect = 1; again with the num_chipselect = 1 ? IIRC this controller has 4 chipselects, just read 24.5.1 on your TRM. this is unnecessary. I intended to configure chip selects through dt)as done below). Will remove. which brings me to the point of: Do you review your own code before sending it out ? Doesn't look like... + irq = platform_get_irq(pdev, 0); + if (irq 0) { + dev_err(pdev-dev, no irq resource?\n); + return irq; + } + + spin_lock_init(qspi-lock); + + qspi-base = devm_ioremap_resource(pdev-dev, r); + if (IS_ERR(qspi-base)) { + ret = PTR_ERR(qspi-base); + goto free_master; + } + + ret = devm_request_threaded_irq(pdev-dev, irq, ti_qspi_isr, + ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT, why do you need IRQF_NO_SUSPEND ? I should get away with this. why ? Do you need or do you *not* need it ? And in either case, why ? -- balbi signature.asc Description: Digital signature
Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller
On Monday 29 July 2013 03:50 PM, Felipe Balbi wrote: Hi, On Mon, Jul 29, 2013 at 03:42:03PM +0530, Sourav Poddar wrote: + frame_length = (m-frame_length 3) / spi-bits_per_word; there's another way to optimize this. If you assume bits_per_word to always be power of two you can: frame_length = (m-frame_length 3) __ffs(spi-bits_per_word); but that would need a comment stating that you're assuming bits_per_word to always be a power of two. Not sure if this is a correct assumption. I dont think it will be a correct assumption, since our controller is flexible to handle anything from 1 to 128 as bits_per_word. right, but can the framework handle non-power-of-two bits_per_word ? The only limitation I see is the max bits_per_word, which is 32. Though, I did set master-bits_per_word_mask as power of 2 in my probe. + spin_unlock(qspi-lock); You should, before releasing the lock, mask your IRQs, so that you can get rid of IRQF_ONESHOT. Unmask them before returning from the IRQ thread. Sorry, did not get you here? I am reading interrupt status register before and clearing them in the threaded irq. you need to mask the IRQs, clear WC and FIRQ in IRQENABLE register... set them back at the end of the thread handler. Ok +static int ti_qspi_probe(struct platform_device *pdev) +{ + struct ti_qspi *qspi; + struct spi_master *master; + struct resource *r; + struct device_node *np = pdev-dev.of_node; + u32 max_freq; + int ret = 0, num_cs, irq; + + master = spi_alloc_master(pdev-dev, sizeof(*qspi)); + if (!master) + return -ENOMEM; + + master-mode_bits = SPI_CPOL | SPI_CPHA; + + master-num_chipselect = 1; again with the num_chipselect = 1 ? IIRC this controller has 4 chipselects, just read 24.5.1 on your TRM. this is unnecessary. I intended to configure chip selects through dt)as done below). Will remove. which brings me to the point of: Do you review your own code before sending it out ? Doesn't look like... + irq = platform_get_irq(pdev, 0); + if (irq 0) { + dev_err(pdev-dev, no irq resource?\n); + return irq; + } + + spin_lock_init(qspi-lock); + + qspi-base = devm_ioremap_resource(pdev-dev, r); + if (IS_ERR(qspi-base)) { + ret = PTR_ERR(qspi-base); + goto free_master; + } + + ret = devm_request_threaded_irq(pdev-dev, irq, ti_qspi_isr, + ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT, why do you need IRQF_NO_SUSPEND ? I should get away with this. why ? Do you need or do you *not* need it ? And in either case, why ? I was thinking, this will keep the irqs up even when we are hitting suspend, we will not be prepared to handle it. ? -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller
Hi, On Mon, Jul 29, 2013 at 04:34:41PM +0530, Sourav Poddar wrote: + irq = platform_get_irq(pdev, 0); + if (irq 0) { + dev_err(pdev-dev, no irq resource?\n); + return irq; + } + + spin_lock_init(qspi-lock); + + qspi-base = devm_ioremap_resource(pdev-dev, r); + if (IS_ERR(qspi-base)) { + ret = PTR_ERR(qspi-base); + goto free_master; + } + + ret = devm_request_threaded_irq(pdev-dev, irq, ti_qspi_isr, + ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT, why do you need IRQF_NO_SUSPEND ? I should get away with this. why ? Do you need or do you *not* need it ? And in either case, why ? I was thinking, this will keep the irqs up even when we are hitting suspend, we will not be prepared to handle it. ? won't be prepared in what way ? -- balbi signature.asc Description: Digital signature
Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller
On Monday 29 July 2013 04:39 PM, Felipe Balbi wrote: Hi, On Mon, Jul 29, 2013 at 04:34:41PM +0530, Sourav Poddar wrote: + irq = platform_get_irq(pdev, 0); + if (irq0) { + dev_err(pdev-dev, no irq resource?\n); + return irq; + } + + spin_lock_init(qspi-lock); + + qspi-base = devm_ioremap_resource(pdev-dev, r); + if (IS_ERR(qspi-base)) { + ret = PTR_ERR(qspi-base); + goto free_master; + } + + ret = devm_request_threaded_irq(pdev-dev, irq, ti_qspi_isr, + ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT, why do you need IRQF_NO_SUSPEND ? I should get away with this. why ? Do you need or do you *not* need it ? And in either case, why ? I was thinking, this will keep the irqs up even when we are hitting suspend, we will not be prepared to handle it. ? won't be prepared in what way ? Our driver will be down, so the irq might go un-serviced. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller
Hi, On Mon, Jul 29, 2013 at 04:45:02PM +0530, Sourav Poddar wrote: + irq = platform_get_irq(pdev, 0); + if (irq0) { + dev_err(pdev-dev, no irq resource?\n); + return irq; + } + + spin_lock_init(qspi-lock); + + qspi-base = devm_ioremap_resource(pdev-dev, r); + if (IS_ERR(qspi-base)) { + ret = PTR_ERR(qspi-base); + goto free_master; + } + + ret = devm_request_threaded_irq(pdev-dev, irq, ti_qspi_isr, + ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT, why do you need IRQF_NO_SUSPEND ? I should get away with this. why ? Do you need or do you *not* need it ? And in either case, why ? I was thinking, this will keep the irqs up even when we are hitting suspend, we will not be prepared to handle it. ? won't be prepared in what way ? Our driver will be down, so the irq might go un-serviced. only if you wrote the driver that way. IRQ subsystem doesn't know the state of the device, all it knows is that in case an IRQ fires, it needs to call the registered IRQ handler. Now the thing is: Initially you had the flag setup, so I asked why you needed it. I expected you to tell me why you think QSPI's IRQ shouldn't be disabled during suspend and the implications of disabling it. Instead you just changed your mind and decided to remove the flag. Because you changed your mind with no explanation, that tells me you haven't fully grasped how that flag works and what it means to set (or not set) it. My question now is simply: why don't you need that flag ? What are the implications of setting that flag ? How would your driver behave if an IRQ fired while your driver was suspended ? Unless you understand what it does, how can you understand the behavior or the driver ? cheers -- balbi signature.asc Description: Digital signature