Re: [PATCH v2 06/11] v4l: Add support for omap4iss driver
Hi Sakari,
On Thu, Jan 26, 2012 at 3:05 PM, Sakari Ailus sakari.ai...@iki.fi wrote:
Hi Sergio,
Aguirre, Sergio wrote:
On Wed, Nov 30, 2011 at 06:14:55PM -0600, Sergio Aguirre wrote:
...
+/*
+ * iss_save_ctx - Saves ISS context.
+ * @iss: OMAP4 ISS device
+ *
+ * Routine for saving the context of each module in the ISS.
+ */
+static void iss_save_ctx(struct iss_device *iss)
+{
+ iss_save_context(iss, iss_reg_list);
+}
Do you really, really need to save context related data? Couldn't you
initialise the device the usual way when e.g. returning from suspended
state?
I'll actually have to revisit this more carefuly. I haven't really
tested Runtime PM on this.
I think I'll remove this for now, until i come up with something better.
Usually it's best to recreate the configuration the same way the driver did
it in the first place. Some registers have side effects so that restoring
them requires extra care.
+/*
+ * iss_restore_ctx - Restores ISS context.
+ * @iss: OMAP4 ISS device
+ *
+ * Routine for restoring the context of each module in the ISS.
+ */
+static void iss_restore_ctx(struct iss_device *iss)
+{
+ iss_restore_context(iss, iss_reg_list);
+}
+
...
+/*
+ * csi2_irq_ctx_set - Enables CSI2 Context IRQs.
+ * @enable: Enable/disable CSI2 Context interrupts
+ */
+static void csi2_irq_ctx_set(struct iss_csi2_device *csi2, int enable)
+{
+ u32 reg = CSI2_CTX_IRQ_FE;
+ int i;
+
+ if (csi2-use_fs_irq)
+ reg |= CSI2_CTX_IRQ_FS;
+
+ for (i = 0; i 8; i++) {
8 == number of contexts?
Yes. This loops from 0 to 7.
Are you implying that I need to add a define to this?
I think something that tells it is the number of contexts would be nicer.
...
+ writel(readl(csi2-regs1 + CSI2_SYSCONFIG) |
+ CSI2_SYSCONFIG_SOFT_RESET,
+ csi2-regs1 + CSI2_SYSCONFIG);
+
+ do {
+ reg = readl(csi2-regs1 + CSI2_SYSSTATUS)
+ CSI2_SYSSTATUS_RESET_DONE;
+ if (reg == CSI2_SYSSTATUS_RESET_DONE)
+ break;
+ soft_reset_retries++;
+ if (soft_reset_retries 5)
+ udelay(100);
How about usleep_range() here? Or omit such a long busyloop. Hardware
typically resets quite fast.
I guess i'll try to fine-tune this then.
I think it's fine to replace it with usleep_range() for now. Fine
optimisations can be left for later if really needed.
...
+static void csi2_print_status(struct iss_csi2_device *csi2)
+{
+ struct iss_device *iss = csi2-iss;
+
+ if (!csi2-available)
+ return;
+
+ dev_dbg(iss-dev, -CSI2 Register
dump-\n);
+
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, SYSCONFIG);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, SYSSTATUS);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, IRQENABLE);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, IRQSTATUS);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, CTRL);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, DBG_H);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, COMPLEXIO_CFG);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, COMPLEXIO_IRQSTATUS);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, SHORT_PACKET);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, COMPLEXIO_IRQENABLE);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, DBG_P);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, TIMING);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, CTX_CTRL1(0));
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, CTX_CTRL2(0));
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, CTX_DAT_OFST(0));
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, CTX_PING_ADDR(0));
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, CTX_PONG_ADDR(0));
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, CTX_IRQENABLE(0));
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, CTX_IRQSTATUS(0));
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, CTX_CTRL3(0));
+
+ dev_dbg(iss-dev,
\n);
_If_ this is user-triggered, you might want to consider using debugfs for
the same. Just FYI.
Ok. I'll see what can I do.
Just FYI. :-) Thinking about this agian, debugfs might not go well with this
since the prints may be triggered by the driver.
...
+ /* Skip interrupts until we reach the frame skip count. The CSI2
will be
+ * automatically disabled, as the frame skip count has been
programmed
+ * in the CSI2_CTx_CTRL1::COUNT field, so reenable it.
+ *
+ * It would have been nice to rely on the FRAME_NUMBER interrupt
instead
+ * but it turned out that the interrupt is only generated when the
CSI2
+ * writes to memory (the CSI2_CTx_CTRL1::COUNT field is decreased
+ * correctly and reaches 0 when data is forwarded to the video
port only
+ * but no interrupt arrives). Maybe a CSI2 hardware bug.
+ */
+ if (csi2-frame_skip) {
+ csi2-frame_skip--;
+ if
Re: [PATCH v2 06/11] v4l: Add support for omap4iss driver
Hi Sergio,
Aguirre, Sergio wrote:
On Wed, Nov 30, 2011 at 06:14:55PM -0600, Sergio Aguirre wrote:
...
+/*
+ * iss_save_ctx - Saves ISS context.
+ * @iss: OMAP4 ISS device
+ *
+ * Routine for saving the context of each module in the ISS.
+ */
+static void iss_save_ctx(struct iss_device *iss)
+{
+ iss_save_context(iss, iss_reg_list);
+}
Do you really, really need to save context related data? Couldn't you
initialise the device the usual way when e.g. returning from suspended
state?
I'll actually have to revisit this more carefuly. I haven't really
tested Runtime PM on this.
I think I'll remove this for now, until i come up with something better.
Usually it's best to recreate the configuration the same way the driver did
it in the first place. Some registers have side effects so that restoring
them requires extra care.
+/*
+ * iss_restore_ctx - Restores ISS context.
+ * @iss: OMAP4 ISS device
+ *
+ * Routine for restoring the context of each module in the ISS.
+ */
+static void iss_restore_ctx(struct iss_device *iss)
+{
+ iss_restore_context(iss, iss_reg_list);
+}
+
...
+/*
+ * csi2_irq_ctx_set - Enables CSI2 Context IRQs.
+ * @enable: Enable/disable CSI2 Context interrupts
+ */
+static void csi2_irq_ctx_set(struct iss_csi2_device *csi2, int enable)
+{
+ u32 reg = CSI2_CTX_IRQ_FE;
+ int i;
+
+ if (csi2-use_fs_irq)
+ reg |= CSI2_CTX_IRQ_FS;
+
+ for (i = 0; i 8; i++) {
8 == number of contexts?
Yes. This loops from 0 to 7.
Are you implying that I need to add a define to this?
I think something that tells it is the number of contexts would be nicer.
...
+ writel(readl(csi2-regs1 + CSI2_SYSCONFIG) |
+ CSI2_SYSCONFIG_SOFT_RESET,
+ csi2-regs1 + CSI2_SYSCONFIG);
+
+ do {
+ reg = readl(csi2-regs1 + CSI2_SYSSTATUS)
+ CSI2_SYSSTATUS_RESET_DONE;
+ if (reg == CSI2_SYSSTATUS_RESET_DONE)
+ break;
+ soft_reset_retries++;
+ if (soft_reset_retries 5)
+ udelay(100);
How about usleep_range() here? Or omit such a long busyloop. Hardware
typically resets quite fast.
I guess i'll try to fine-tune this then.
I think it's fine to replace it with usleep_range() for now. Fine
optimisations can be left for later if really needed.
...
+static void csi2_print_status(struct iss_csi2_device *csi2)
+{
+ struct iss_device *iss = csi2-iss;
+
+ if (!csi2-available)
+ return;
+
+ dev_dbg(iss-dev, -CSI2 Register dump-\n);
+
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, SYSCONFIG);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, SYSSTATUS);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, IRQENABLE);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, IRQSTATUS);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, CTRL);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, DBG_H);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, COMPLEXIO_CFG);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, COMPLEXIO_IRQSTATUS);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, SHORT_PACKET);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, COMPLEXIO_IRQENABLE);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, DBG_P);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, TIMING);
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, CTX_CTRL1(0));
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, CTX_CTRL2(0));
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, CTX_DAT_OFST(0));
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, CTX_PING_ADDR(0));
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, CTX_PONG_ADDR(0));
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, CTX_IRQENABLE(0));
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, CTX_IRQSTATUS(0));
+ CSI2_PRINT_REGISTER(iss, csi2-regs1, CTX_CTRL3(0));
+
+ dev_dbg(iss-dev, \n);
_If_ this is user-triggered, you might want to consider using debugfs for
the same. Just FYI.
Ok. I'll see what can I do.
Just FYI. :-) Thinking about this agian, debugfs might not go well with this
since the prints may be triggered by the driver.
...
+ /* Skip interrupts until we reach the frame skip count. The CSI2 will be
+ * automatically disabled, as the frame skip count has been programmed
+ * in the CSI2_CTx_CTRL1::COUNT field, so reenable it.
+ *
+ * It would have been nice to rely on the FRAME_NUMBER interrupt instead
+ * but it turned out that the interrupt is only generated when the CSI2
+ * writes to memory (the CSI2_CTx_CTRL1::COUNT field is decreased
+ * correctly and reaches 0 when data is forwarded to the video port only
+ * but no interrupt arrives). Maybe a CSI2 hardware bug.
+ */
+ if (csi2-frame_skip) {
+ csi2-frame_skip--;
+ if (csi2-frame_skip == 0) {
+ ctx-format_id = csi2_ctx_map_format(csi2);
+ csi2_ctx_config(csi2, ctx);
Is configuration of the context needed elsewhere than in streamon? What
changes
