Re: [PATCH] lib/genalloc.c: add check gen_pool_dma_alloc() if dma pointer is not NULL
On Thu, 23 Jan 2014 15:51:31 +0530 Prabhakar Lad prabhakar.cse...@gmail.com wrote: Hi Sachin, On Thu, Jan 23, 2014 at 3:38 PM, Sachin Kamat sachin.ka...@linaro.org wrote: Hi Prabhakar, On 23 January 2014 15:26, Prabhakar Lad prabhakar.cse...@gmail.com wrote: From: Lad, Prabhakar prabhakar.cse...@gmail.com In the gen_pool_dma_alloc() the dma pointer can be NULL and while assigning gen_pool_virt_to_phys(pool, vaddr) to dma caused the following crash on da850 evm, [snip] Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com --- lib/genalloc.c |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/genalloc.c b/lib/genalloc.c index dda3116..f48163f 100644 --- a/lib/genalloc.c +++ b/lib/genalloc.c @@ -334,7 +334,8 @@ void *gen_pool_dma_alloc(struct gen_pool *pool, size_t size, dma_addr_t *dma) if (!vaddr) return NULL; - *dma = gen_pool_virt_to_phys(pool, vaddr); + if (dma) + *dma = gen_pool_virt_to_phys(pool, vaddr); Wouldn't it be better to return (with error/message) if dma is NULL rather than silently ignore it? I am not sure if returning here with error is OK, may be just adding a warning message could be OK ? The patch look OK as-is to me. `dma' is a second return value from gen_pool_dma_alloc() and this patch extends the gen_pool_dma_alloc() interface by making that return value optional. That's good for callers who don't want the physical address, and they can call gen_pool_virt_to_phys() at a later time to get the physical address anyway. From my reading, 3.13.x kernels will need this patch. I suppose we should document the API change: --- a/lib/genalloc.c~lib-genallocc-add-check-gen_pool_dma_alloc-if-dma-pointer-is-not-null-fix +++ a/lib/genalloc.c @@ -316,7 +316,7 @@ EXPORT_SYMBOL(gen_pool_alloc); * gen_pool_dma_alloc - allocate special memory from the pool for DMA usage * @pool: pool to allocate from * @size: number of bytes to allocate from the pool - * @dma: dma-view physical address + * @dma: dma-view physical address return value. Use NULL if unneeded. * * Allocate the requested number of bytes from the specified pool. * Uses the pool allocation function (with first-fit algorithm by default). _ ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3 00/12] tnetv107x ssp driver stack
On Thu, 21 Oct 2010 15:26:04 -0700 Kevin Hilman khil...@deeprootsystems.com wrote: Cyril Chemparathy cy...@ti.com writes: TI's sequencer serial port (TI-SSP) is a jack-of-all-trades type of serial port device. It has a built-in programmable execution engine that can be programmed to operate as almost any serial bus (I2C, SPI, EasyScale, and others). Andrew, looking for some advice here... This is a piece of davinci hardware, but introduces drivers in various subsystems. I'm willing to merge this series via the davinci tree after getting acks from the various subsystem maintainers. Is this an OK approach? It seems best to me to merge this all together. Yes, that's the best approach. Some of the desired acks may not be forthcoming. In which case the best you can do is to review the code yourself, make the maintainer(s) aware of what's going on and just go ahead and merge it. If people are too busy to look at the code then so be it - we can't really permit that problem to permanently block new drivers. We already have acks for the regulator and gpio driver parts, and the backlight driver has a clear owner in MAINTAINERS. However, who should be doing the final review/ack of the drivers/misc and drivers/gpio changes is less clear to me. I'll go and have a look at drivers/misc/ti_ssp.c. Thanks, Kevin arch/arm/mach-davinci/board-tnetv107x-evm.c| 199 +++ arch/arm/mach-davinci/devices-tnetv107x.c | 25 + arch/arm/mach-davinci/include/mach/ti_ssp.h| 98 arch/arm/mach-davinci/include/mach/tnetv107x.h |2 + arch/arm/mach-davinci/tnetv107x.c |2 +- drivers/gpio/Kconfig | 10 + drivers/gpio/Makefile |1 + drivers/gpio/ti-ssp-gpio.c | 200 +++ drivers/misc/Kconfig | 11 + drivers/misc/Makefile |1 + drivers/misc/ti_ssp.c | 436 +++ drivers/regulator/Kconfig | 10 + drivers/regulator/Makefile |1 + drivers/regulator/tps6524x-regulator.c | 692 drivers/spi/Kconfig|7 + drivers/spi/Makefile |1 + drivers/spi/spi_ti_ssp.c | 397 ++ drivers/video/backlight/Kconfig|7 + drivers/video/backlight/Makefile |2 +- drivers/video/backlight/tps6116x.c | 340 ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3 01/12] misc: add driver for sequencer serial port
On Thu, 21 Oct 2010 17:01:02 -0400
Cyril Chemparathy cy...@ti.com wrote:
TI's sequencer serial port (TI-SSP) is a jack-of-all-trades type of serial
port
device. It has a built-in programmable execution engine that can be
programmed
to operate as almost any serial bus (I2C, SPI, EasyScale, and others).
This patch adds a driver for this controller device. The driver does not
expose a user-land interface. Protocol drivers built on top of this layer are
expected to remain in-kernel.
...
+struct ti_ssp_dev_data {
+ const char *dev_name;
+ unsigned long iosel; /* see note below */
+ unsigned long config;
+ const void *pdata;
+ int pdata_sz;
I suppose this really should have type size_t. Also a better name is
pdata_size - we prefer to avoid this random omission of vowels from
kernel identifiers. Just spell it out; it makes it easier to remember.
+};
+
+struct ti_ssp_data {
+ unsigned long out_clock;
+ struct ti_ssp_dev_data dev_data[2];
+};
+
...
+config TI_SSP
+ depends on ARCH_DAVINCI_TNETV107X
+ tristate Sequencer Serial Port support
+ default y
Was `y' a good choice?
+ ---help---
+ Say Y here if you want support for the Sequencer Serial Port
+ in a Texas Instruments TNETV107X SoC.
+
+ To compile this driver as a module, choose M here: the
+ module will be called ti_ssp.
...
+#define dev2ssp(dev) dev_get_drvdata(dev-parent)
+#define dev2port(dev)(to_platform_device(dev)-id)
These could be implemented as C funtions. That's superior because of
the typechecking. At present dev2ssp() will happily compile and fail
at runtime if passed anystructure which has a 'const struct device
*parent'.
+/* Register Access Helpers */
+static inline u32 ssp_read(struct ti_ssp *ssp, int reg)
+{
+ return __raw_readl(ssp-regs + reg);
+}
+
+static inline void ssp_write(struct ti_ssp *ssp, int reg, u32 val)
+{
+ __raw_writel(val, ssp-regs + reg);
+}
Why are the __raw functions used here?
+static inline void ssp_rmw(struct ti_ssp *ssp, int reg, u32 mask, u32 bits)
+{
+ u32 val = ssp_read(ssp, reg);
+ val = ~mask;
+ val |= bits;
+ ssp_write(ssp, reg, val);
+}
Locking? Perhaps this function must be called under ssp-lock? If so,
that should be documented here and it appears that not all callsites
actually do that correctly.
...
+static int __set_iosel(struct ti_ssp *ssp, int port, u32 iosel)
+{
+ unsigned val;
+
+ /* IOSEL1 gets the least significant 16 bits */
+ val = ssp_read(ssp, REG_IOSEL_1);
+ val = 0x (port ? 0 : 16);
+ val |= (iosel 0x) (port ? 16 : 0);
+ ssp_write(ssp, REG_IOSEL_1, val);
+
+ /* IOSEL2 gets the most significant 16 bits */
+ val = ssp_read(ssp, REG_IOSEL_2);
+ val = 0x0007 (port ? 0 : 16);
+ val |= (iosel 0x0007) (port ? 0 : 16);
+ ssp_write(ssp, REG_IOSEL_2, val);
More rmw's which need locking. It should be documented please. Both
callers get it right this time.
+ return 0;
+}
+
...
+int ti_ssp_run(struct device *dev, u32 pc, u32 input, u32 *output)
+{
+ struct ti_ssp *ssp = dev2ssp(dev);
+ int port = dev2port(dev);
+ int count;
+
+ if (pc ~(0x3f))
+ return -EINVAL;
+
+ ssp_port_write(ssp, port, PORT_ADDR, input 16);
+ ssp_port_write(ssp, port, PORT_DATA, input 0x);
+ ssp_port_rmw(ssp, port, PORT_CFG_1, 0x3f, pc);
+
+ ssp_port_set_bits(ssp, port, PORT_CFG_1, SSP_START);
+
+ for (count = 1; count; count--) {
+ if ((ssp_port_read(ssp, port, PORT_CFG_1) SSP_BUSY) == 0)
+ break;
+ udelay(1);
+ }
+
+ if (output) {
+ *(output) = (ssp_port_read(ssp, port, PORT_ADDR) 16) |
+ (ssp_port_read(ssp, port, PORT_DATA) 0x);
+ }
+
+ if (!count) {
+ dev_err(ssp-dev, timed out waiting for SSP operation\n);
+ return -EIO;
+ }
There doesn't seem much point in writing to *output if the port_read()
timed out?
...
That's all fairly minor stuff. It looks Good Enough For Linux to me.
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2] mtd-nand: davinci: correct 4-bit error correction
On Thu, 15 Jul 2010 17:33:03 +0530 Sudhakar Rajashekhara sudhakar@ti.com wrote: On Thu, Jul 15, 2010 at 17:11:32, Sudhakar Rajashekhara wrote: Hi, On Thu, Jul 15, 2010 at 16:31:19, Jon Povey wrote: Sudhakar Rajashekhara wrote: On TI's DA830/OMAP-L137, DA850/OMAP-L138 and DM365, after setting the 4BITECC_ADD_CALC_START bit in the NAND Flash control register to 1 and before waiting for the NAND Flash status register to be equal to 1, 2 or 3, we have to wait till the ECC HW goes to correction state. Without this wait, ECC correction calculations will not be proper. This has been tested on DA830/OMAP-L137, DA850/OMAP-L138, DM355 and DM365 EVMs. Signed-off-by: Sudhakar Rajashekhara sudhakar@ti.com Acked-by: Sneha Narnakaje nsnehapra...@ti.com Cc: David Woodhouse dw...@infradead.org Signed-off-by: Andrew Morton a...@linux-foundation.org Have these people acked and signed off this new version of the patch? No. Andrew Morton has not signed off this version. I'll remove Signed-off-by: Andrew Morton a...@linux-foundation.org Andrew Morton had signed off an earlier version of this patch and it was present in -mm tree for a long time. He has not yet commented on v2 version of this patch. But I thought I can carry forward the Sign-offs from previous version to the next version. What's the common practice? I've lost the plot on this patch and I think I'll drop my copy. Please update and resend, cc'ing everyone. I've been sitting on this thing since November last year! It fixes a bug! Where the heck are the MTD maintainers and why aren't they running around with hair on fire getting this thing finalised and merged?!?! ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH] mtd-nand: davinci: correct 4-bit error correction
On Tue, 13 Jul 2010 15:02:59 +0530 Sudhakar Rajashekhara
sudhakar@ti.com wrote:
Hi,
On Mon, Jul 12, 2010 at 11:58:18, Sudhakar Rajashekhara wrote:
On Sat, Jul 10, 2010 at 04:09:32, Andrew Morton wrote:
On Fri, 9 Jul 2010 10:59:49 +0530
Sudhakar Rajashekhara sudhakar@ti.com wrote:
+
+ /*
+* ECC_STATE field reads 0x3 (Error correction complete)
immediately
+* after setting the 4BITECC_ADD_CALC_START bit. So if you
immediately
+* begin trying to poll for the state, you may fall right out
of your
+* loop without any of the correction calculations having taken
place.
+* The recommendation from the hardware team is to wait till
ECC_STATE
+* reads less than 4, which means ECC HW has entered correction
state.
+*/
+ do {
+ ecc_state = (davinci_nand_readl(info,
+ NANDFSR_OFFSET) 8) 0x0f;
+ cpu_relax();
+ } while ((ecc_state 4) time_before(jiffies, timeo));
An up-to-100-milliseond busy wait is pretty bad. For how long do you
expect this to spin in practice?
On the hardware, I have never seen this taking 100 msec to come out of
the loop. I'll check with the hardware folks on the maximum time to wait
for, before the ECC engine is ready.
I checked this with the hardware team but no one is sure about the exact
time one should wait before the ECC engine becomes ready. But everyone is
of the opinion that 100 loop cycles should be enough. To be on the safer
side, I'll be changing the timeout to 10 milliseconds in the next version
of this patch.
Going from 100ms down to 10ms sounds a bit risky. It'd be better to
retain the 100ms and to make the kernel spend most of that time
sleeping, rather than busywaiting, IMO.
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH] mtd-nand: davinci: correct 4-bit error correction
On Fri, 9 Jul 2010 10:59:49 +0530
Sudhakar Rajashekhara sudhakar@ti.com wrote:
+
+ /*
+ * ECC_STATE field reads 0x3 (Error correction complete) immediately
+ * after setting the 4BITECC_ADD_CALC_START bit. So if you immediately
+ * begin trying to poll for the state, you may fall right out of your
+ * loop without any of the correction calculations having taken place.
+ * The recommendation from the hardware team is to wait till ECC_STATE
+ * reads less than 4, which means ECC HW has entered correction state.
+ */
+ do {
+ ecc_state = (davinci_nand_readl(info,
+ NANDFSR_OFFSET) 8) 0x0f;
+ cpu_relax();
+ } while ((ecc_state 4) time_before(jiffies, timeo));
An up-to-100-milliseond busy wait is pretty bad. For how long do you
expect this to spin in practice?
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2] DA8XX/OMAP-L1XX: FB: Implement double buffering
On Wed, 31 Mar 2010 20:43:29 -0500 Ambrose, Martin mar...@ti.com wrote: + /* +* Set flag to 0 and wait for isr to set to 1. It would seem there is a +* race condition here where the ISR could have occured just before or +* just after this set. But since we are just coarsely waiting for +* a frame to complete then that's OK. i.e. if the frame completed +* just before this code executed then we have to wait another full +* frame time but there is no way to avoid such a situation. On the +* other hand if the frame completed just after then we don't need +* to wait long at all. Either way we are guaranteed to return to the +* user immediately after a frame completion which is all that is +* required. +*/ + par-vsync_flag = 0; + ret = wait_event_interruptible_timeout(par-vsync_wait, + par-vsync_flag != 0, + par-vsync_timeout); If the calling process has signal_pending() (say, someone hit ^C) then wait_event_interruptible_timeout() will fall straight through with -ERESTARTSYS. Will this cause the driver to malfunction at all? I don't think so since the driver doesn't make use of this information in any way. This is just status to the caller that the current frame has finished DMA'ing out of the framebuffer. Could you maybe propose a scenario/use case where you think it is problematic? I could then either reason why it should be OK or I'll create a test harness and see how the driver can/should be modified. Gee, I dunno - I don't understand the driver to that level. If you're OK with this wait being interrupted by a signal and the driver handles it OK then fine, that's a feature. To test it I suppose you should give your test app a signal handler and blast kills at it from another process. ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2] DA8XX/OMAP-L1XX: FB: Implement double buffering
On Mon, 29 Mar 2010 08:16:51 -0500
Ambrose, Martin mar...@ti.com wrote:
+/*
+ * Function to wait for vertical sync which for this LCD peripheral
+ * translates into waiting for the current raster frame to complete.
+ */
+static int fb_wait_for_vsync(struct fb_info *info)
+{
+ struct da8xx_fb_par *par = info-par;
+ wait_queue_t wq;
+ int ret;
+
+ init_waitqueue_entry(wq, current);
DECLARE_WAITQUEUE() would be more conventional.
+ /*
+* Set flag to 0 and wait for isr to set to 1. It would seem there is
a
+* race condition here where the ISR could have occured just before or
+* just after this set. But since we are just coarsely waiting for
+* a frame to complete then that's OK. i.e. if the frame completed
+* just before this code executed then we have to wait another full
+* frame time but there is no way to avoid such a situation. On the
+* other hand if the frame completed just after then we don't need
+* to wait long at all. Either way we are guaranteed to return to the
+* user immediately after a frame completion which is all that is
+* required.
+*/
+ par-vsync_flag = 0;
+ ret = wait_event_interruptible_timeout(par-vsync_wait,
+ par-vsync_flag != 0,
+ par-vsync_timeout);
If the calling process has signal_pending() (say, someone hit ^C) then
wait_event_interruptible_timeout() will fall straight through with
-ERESTARTSYS. Will this cause the driver to malfunction at all?
+ if (ret 0)
+ return ret;
+ if (ret == 0)
+ return -ETIMEDOUT;
+
+ return 0;
+}
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2] davinci: MMC: Pass number of SG segments as platform data
On Fri, 12 Mar 2010 18:04:09 +0530 Sudhakar Rajashekhara sudhakar@ti.com wrote: On some platforms like DM355, the number of EDMA parameter slots available for EDMA_SLOT_ANY usage are few. In such cases, if MMC/SD uses 16 slots for each instance of MMC controller, then the number of slots available for other modules will be very few. By passing the number of EDMA slots to be used in MMC driver from platform data, EDMA slots available for other purposes can be controlled. It's unclear what the runtime effects of this change are. I assumed (without justification) that they're minor and I scheduled the patch for 2.6.35. If that was the wrong decision then blame the changelog ;) ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
