video_omap4 and control module access (was ... from opensuse-arm list)
Full thread in opensuse mailing list:
http://lists.opensuse.org/archive/opensuse-arm/2014-06/msg00004.html
Moving this thread out of opensuse to kernel public lists +CC of maintainers relevant to the control module/clk.
Quoted 89 lines · Nishanth Menon
Quoted 46 lines · Laurent Pinchart
Hi Nishanth,
Quoted 43 lines · On Thursday 05 June 2014 08:37:27 Nishanth Menon wrote:
Quoted 37 lines · Takashi Iwai
Quoted 4 lines · Alexander Graf
Quoted 3 lines · Matwey V. Kornilov
Quoted 1 line · Alexander Graf
Quoted 1 line
note: expected '
uint32_t*' but argument is of type 'dma_addr_t*'Quoted 8 lines
Quoted 4 lines
I've fixed that one, but can not figure out what is wrong now:
https://build.opensuse.org/package/live_build_log/home:matwey:pcm051:13.
2/kernel-default/standard/armv7l>>If I had to guess I'd say someone forgot to put a few
EXPORT_SYMBOLsinto the code and never tested whether compiling his v4l / video driver actually works when it's compiled as a module.The problem is
CONFIG_VIDEO_OMAP4=ywhile the whole V4L stuff is built as modules. You have to build V4L into kernel, too.
That said, it's aKconfigdependency issue.Looking at the code, though, omap4-iss driver itself is written to be built also as a module. But its
Kconfigis bool, so the problem happens. Maybe a patch like below works?Takashi
---
(quoted diff) +0
-0b/drivers/staging/media/omap4iss/Kconfig index
78b0fba7047e..0c3e3c1acd4f100644drivers/staging/media/omap4iss/Kconfig +1
-1@@ -1,5 +1,5 @@ config VIDEO_OMAP4 - bool "OMAP 4 Camera support" + tristate "OMAP 4 Camera support" depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && I2C && ARCH_OMAP4 select VIDEOBUF2_DMA_CONTIGQuoted 1 line
---help---
+Sakari and Laurent. Full thread:
http://lists.opensuse.org/archive/opensuse-arm/2014-06/msg00004.htmlQuoted 9 lines
Quoted 1 line
I agree, I see no reason for these to be bool.
There's no good reason for the option to be a boolean, but there's a bad reason :-/ The OMAP4 ISS driver calls the
omap4_ctrl_pad_readl()andomap4_ctrl_pad_writel()functions, which are not exported. The right way to fix this would be to implement a control module driver for the OMAP4, but that's not a straightforward task, and I don't have time to do so at the moment.a) control module:
from:drivers/staging/media/omap4iss/iss_csiphy.c
/*
* SCM.CONTROL_CAMERA_RX
* - bit [31] : CSIPHY2 lane 2 enable (4460+ only)
* - bit [30:29] : CSIPHY2 per-lane enable (1 to 0)
* - bit [28:24] : CSIPHY1 per-lane enable (4 to 0)
* - bit [21] : CSIPHY2 CTRLCLK enable
* - bit [20:19] : CSIPHY2 config: 00 d-phy, 01/10 ccp2
* - bit [18] : CSIPHY1 CTRLCLK enable
* - bit [17:16] : CSIPHY1 config: 00 d-phy, 01/10 ccp2 */
cam_rx_ctrl= omap4_ctrl_pad_readl(OMAP4_CTRL_MODULE_PAD_CORE_CONTROL_CAMERA_RX);Is'nt that what pinctrl does? And should be rather trivial to do, no?
c) if there is something else that these bits do that I cant figure out, example: for specific stuff like control module bit for clock (which the above code kinda sounds similar to), like how we had for display recently - model it with dts clock[1]
b) if you cannot use existing frameworks OR use pinctrl, last ditch way to do it in pdata-quirks in mach-omap2 with fops being send over.
We did debate putting entire control module as a
syscon_driver, the current split (prior to syscon) just makes it impractical to switch over to it at this point in time, maybe once all dt-fication is done, it might be possible to switch over to that.
Signature · 2 lines
Regards, Nishanth Menon
2014-06-05 20:47 GMT+04:00 Nishanth Menon <[email protected]>:
Quoted 6 lines
Full thread in opensuse mailing list:
http://lists.opensuse.org/archive/opensuse-arm/2014-06/msg00004.html
Moving this thread out of opensuse to kernel public lists +CC of maintainers relevant to the control module/clk.
Full build log (600KB) and config are available here:
http://yadi.sk/d/sVEkp5CjSQDF8
http://yadi.sk/d/c-9N18f8SQDEs
Signature · 4 lines
With best regards, Matwey V. Kornilov http://blog.matwey.name xmpp:[email protected]
Hi Nishanth,
Quoted 83 lines · On Thursday 05 June 2014 11:47:19 Nishanth Menon wrote:
Full thread in opensuse mailing list:
http://lists.opensuse.org/archive/opensuse-arm/2014-06/msg00004.html
Moving this thread out of opensuse to kernel public lists +CC of maintainers relevant to the control module/clk.
Quoted 75 lines · Nishanth Menon
Quoted 45 lines · Laurent Pinchart
Quoted 44 lines · On Thursday 05 June 2014 08:37:27 Nishanth Menon wrote:
Quoted 38 lines · Takashi Iwai
Quoted 4 lines · Alexander Graf
Quoted 3 lines · Matwey V. Kornilov
Quoted 1 line · Alexander Graf
Quoted 1 line
note: expected '
uint32_t*' but argument is of type 'dma_addr_t*'Quoted 8 lines
Quoted 4 lines
I've fixed that one, but can not figure out what is wrong now:
https://build.opensuse.org/package/live_build_log/home:matwey:pcm051:
13.2/kernel-default/standard/armv7l>>If I had to guess I'd say someone forgot to put a few
EXPORT_SYMBOLsinto the code and never tested whether compiling his v4l / video driver actually works when it's compiled as a module.The problem is
CONFIG_VIDEO_OMAP4=ywhile the whole V4L stuff is built as modules. You have to build V4L into kernel, too.
That said, it's aKconfigdependency issue.Looking at the code, though, omap4-iss driver itself is written to be built also as a module. But its
Kconfigis bool, so the problem happens. Maybe a patch like below works?Takashi
---
(quoted diff) +0
-0b/drivers/staging/media/omap4iss/Kconfig index
78b0fba7047e..0c3e3c1acd4f
100644drivers/staging/media/omap4iss/Kconfig +1
-1@@ -1,5 +1,5 @@ config VIDEO_OMAP4 - bool "OMAP 4 Camera support" + tristate "OMAP 4 Camera support" depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && I2C && ARCH_OMAP4 select VIDEOBUF2_DMA_CONTIGQuoted 1 line
---help---
+Sakari and Laurent. Full thread:
http://lists.opensuse.org/archive/opensuse-arm/2014-06/msg00004.htmlQuoted 8 lines
Quoted 1 line
I agree, I see no reason for these to be bool.
There's no good reason for the option to be a boolean, but there's a bad reason :-/ The OMAP4 ISS driver calls the
omap4_ctrl_pad_readl()andomap4_ctrl_pad_writel()functions, which are not exported. The right way to fix this would be to implement a control module driver for the OMAP4, but that's not a straightforward task, and I don't have time to do so at the moment.a) control module:
from:drivers/staging/media/omap4iss/iss_csiphy.c/*
* SCM.CONTROL_CAMERA_RX
* - bit [31] : CSIPHY2 lane 2 enable (4460+ only)
* - bit [30:29] : CSIPHY2 per-lane enable (1 to 0)
* - bit [28:24] : CSIPHY1 per-lane enable (4 to 0)
* - bit [21] : CSIPHY2 CTRLCLK enable
* - bit [20:19] : CSIPHY2 config: 00 d-phy, 01/10 ccp2
* - bit [18] : CSIPHY1 CTRLCLK enable
* - bit [17:16] : CSIPHY1 config: 00 d-phy, 01/10 ccp2 */
cam_rx_ctrl= omap4_ctrl_pad_readl(
OMAP4_CTRL_MODULE_PAD_CORE_CONTROL_CAMERA_RX);Is'nt that what pinctrl does? And should be rather trivial to do, no?
It's a bit of a grey area. While enabling/disabling CSI lanes could be considered as falling into the scope of pinctrl (but even there I have some doubts, as this is one level beyond pin muxing in my opinion), pushing clock enabling/disabling to pinctrl would require a really big shoehorn :-) The register also controls the CSI2 PHY mode of operation (CCP2, CSI1 or CSI2), which doesn't really look like pinctrl territory to me.
Let's also keep in mind that the configuration currently hardcoded by the driver is a shortcut. We want to be able to control each field in the register dynamically and independently. We can't apply partial pinctrl configurations today, so we would need to declare one configuration for every field combination, which really doesn't scale.
Quoted 4 lines
Quoted 4 lines
c) if there is something else that these bits do that I cant figure out, example: for specific stuff like control module bit for clock (which the above code kinda sounds similar to), like how we had for display recently - model it with dts clock[1]
The TRM ISS section states that
"(vii) A dedicated internal clock gate control is present for each PHY. Enable or disable the internal CTRLCLK from the CAMERARX_CSI22_CTRLCLKE[21] CAMERARX_CSI22_CTRLCLKE bit or the [18] CAMERARX_CSI21_CTRLCLKE bit."
Those two of the bits could probably be exposed through CCF, and [1] seems to go in the right direction for that. We will still need a solution for the other bits though, as we will need to handle all accesses to the register from a single driver in order to implement proper locking (although we could consider that the caller should handle access serialization, but that's a bit messy I believe).
Quoted 2 lines
Quoted 2 lines
b) if you cannot use existing frameworks OR use pinctrl, last ditch way to do it in pdata-quirks in mach-omap2 with fops being send over.
I'm working on DT support for the OMAP4 ISS driver, so that's not an option I'm afraid.
Quoted 6 lines
Quoted 6 lines
We did debate putting entire control module as a
syscon_driver, the current split (prior to syscon) just makes it impractical to switch over to it at this point in time, maybe once all dt-fication is done, it might be possible to switch over to that.
Signature · 3 lines
Regards, Laurent Pinchart