| Message ID | 1359745771-23684-3-git-send-email-s.nawrocki@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/01/2013 12:09 PM, Sylwester Nawrocki wrote: > This patch adds support for FIMC devices instantiated from devicetree > for S5PV210 and Exynos4 SoCs. The FIMC IP features include colorspace > conversion and scaling (mem-to-mem) and parallel/MIPI CSI2 bus video > capture interface. > > Multiple SoC revisions specific parameters are defined statically in > the driver and are used for both dt and non-dt. The driver's static > data is selected based on the compatible property. Previously the > platform device name was used to match driver data and a specific > SoC/IP version. > > Aliases are used to determine an index of the IP which is essential > for linking FIMC IP with other entities, like MIPI-CSIS (the MIPI > CSI-2 bus frontend) or FIMC-LITE and FIMC-IS ISP. > diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt > +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC) > +---------------------------------------------- > + > +The Exynos Camera subsystem comprises of multiple sub-devices that are > +represented by separate platform devices. Some of the IPs come in different "platform devices" is a rather Linux-centric term, and DT bindings should be OS-agnostic. Perhaps use "device tree nodes" here? > +variants across the SoC revisions (FIMC) and some remain mostly unchanged > +(MIPI CSIS, FIMC-LITE). > + > +All those sub-subdevices are defined as parent nodes of the common device s/parent nodes/child node/ I think? > +For every fimc node a numbered alias should be present in the aliases node. > +Aliases are of the form fimc<n>, where <n> is an integer (0...N) specifying > +the IP's instance index. Why? Isn't it up to the DT author whether they care if each fimc node is assigned a specific identification v.s. whether identification is assigned automatically? > +Optional properties > + > + - clock-frequency - maximum FIMC local clock (LCLK) frequency Again, I'd expect a clocks property here instead.
On 02/07/2013 12:40 AM, Stephen Warren wrote: >> diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt > >> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC) >> +---------------------------------------------- >> + >> +The Exynos Camera subsystem comprises of multiple sub-devices that are >> +represented by separate platform devices. Some of the IPs come in different > > "platform devices" is a rather Linux-centric term, and DT bindings > should be OS-agnostic. Perhaps use "device tree nodes" here? Indeed, thank you for the suggestion, I'll change it. >> +variants across the SoC revisions (FIMC) and some remain mostly unchanged >> +(MIPI CSIS, FIMC-LITE). >> + >> +All those sub-subdevices are defined as parent nodes of the common device > > s/parent nodes/child node/ I think? Yeah, 'parent nodes' doesn't really make sense. Thanks for catching it. >> +For every fimc node a numbered alias should be present in the aliases node. >> +Aliases are of the form fimc<n>, where<n> is an integer (0...N) specifying >> +the IP's instance index. > > Why? Isn't it up to the DT author whether they care if each fimc node is > assigned a specific identification v.s. whether identification is > assigned automatically? There are at least three different kinds of IPs that come in multiple instances in an SoC. To activate data links between them each instance needs to be clearly identified. There are also differences between instances of same device. Hence it's important these aliases don't have random values. Some more details about the SoC can be found at [1]. The aliases are also already used in the Exynos5 GScaler bindings [2] in a similar way. >> +Optional properties >> + >> + - clock-frequency - maximum FIMC local clock (LCLK) frequency > > Again, I'd expect a clocks property here instead. Please see my comment to patch 01/10. Analogously, I needed this clock frequency to ensure reliable video data pipeline operation. [1] http://tinyurl.com/anw9udm [2] http://www.spinics.net/lists/arm-kernel/msg200036.html
On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote: > On 02/07/2013 12:40 AM, Stephen Warren wrote: >>> diff --git >>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt >>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt >> >>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC) >>> +---------------------------------------------- ... >>> +For every fimc node a numbered alias should be present in the >>> aliases node. >>> +Aliases are of the form fimc<n>, where<n> is an integer (0...N) >>> specifying >>> +the IP's instance index. >> >> Why? Isn't it up to the DT author whether they care if each fimc node is >> assigned a specific identification v.s. whether identification is >> assigned automatically? > > There are at least three different kinds of IPs that come in multiple > instances in an SoC. To activate data links between them each instance > needs to be clearly identified. There are also differences between > instances of same device. Hence it's important these aliases don't have > random values. > > Some more details about the SoC can be found at [1]. The aliases are > also already used in the Exynos5 GScaler bindings [2] in a similar way. Hmmm. I'd expect explicit DT properties to represent the instance-specific "configuration", or even different compatible values. Relying on the alias ID seems rather indirect; what if in e.g. Exynos6/... the mapping from instance/alias ID to feature set changes. With explicit DT properties, that'd just be a .dts change, whereas by requiring alias IDs now, you'd need a driver change to support this.
On 02/09/2013 12:21 AM, Stephen Warren wrote: > On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote: >> On 02/07/2013 12:40 AM, Stephen Warren wrote: >>>> diff --git >>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt >>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt >>> >>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC) >>>> +---------------------------------------------- > ... >>>> +For every fimc node a numbered alias should be present in the >>>> aliases node. >>>> +Aliases are of the form fimc<n>, where<n> is an integer (0...N) >>>> specifying >>>> +the IP's instance index. >>> >>> Why? Isn't it up to the DT author whether they care if each fimc node is >>> assigned a specific identification v.s. whether identification is >>> assigned automatically? >> >> There are at least three different kinds of IPs that come in multiple >> instances in an SoC. To activate data links between them each instance >> needs to be clearly identified. There are also differences between >> instances of same device. Hence it's important these aliases don't have >> random values. >> >> Some more details about the SoC can be found at [1]. The aliases are >> also already used in the Exynos5 GScaler bindings [2] in a similar way. > > Hmmm. I'd expect explicit DT properties to represent the > instance-specific "configuration", or even different compatible values. > Relying on the alias ID seems rather indirect; what if in e.g. > Exynos6/... the mapping from instance/alias ID to feature set changes. > With explicit DT properties, that'd just be a .dts change, whereas by > requiring alias IDs now, you'd need a driver change to support this. In the initial version of this patch series I used cell-index property, but then Grant pointed out in some other mail thread it should be avoided. Hence I used the node aliases. Different compatible values might not work, when for example there are 3 IPs out of 4 of one type and the fourth one of another type. It wouldn't even by really different types, just quirks/little differences between them, e.g. no data path routed to one of other IPs. Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the MIPI-CSIS needs to be written to the FIMC.2 data input control register. Even though MIPI-CSIS.N are same in terms of hardware structure they still need to be distinguished as separate instances.
On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote: > On 02/09/2013 12:21 AM, Stephen Warren wrote: >> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote: >>> On 02/07/2013 12:40 AM, Stephen Warren wrote: >>>>> diff --git >>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt >>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt >>>> >>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC) >>>>> +---------------------------------------------- >> ... >>>>> +For every fimc node a numbered alias should be present in the >>>>> aliases node. >>>>> +Aliases are of the form fimc<n>, where<n> is an integer (0...N) >>>>> specifying >>>>> +the IP's instance index. >>>> >>>> Why? Isn't it up to the DT author whether they care if each fimc >>>> node is >>>> assigned a specific identification v.s. whether identification is >>>> assigned automatically? >>> >>> There are at least three different kinds of IPs that come in multiple >>> instances in an SoC. To activate data links between them each instance >>> needs to be clearly identified. There are also differences between >>> instances of same device. Hence it's important these aliases don't have >>> random values. >>> >>> Some more details about the SoC can be found at [1]. The aliases are >>> also already used in the Exynos5 GScaler bindings [2] in a similar way. >> >> Hmmm. I'd expect explicit DT properties to represent the >> instance-specific "configuration", or even different compatible values. >> Relying on the alias ID seems rather indirect; what if in e.g. >> Exynos6/... the mapping from instance/alias ID to feature set changes. >> With explicit DT properties, that'd just be a .dts change, whereas by >> requiring alias IDs now, you'd need a driver change to support this. > > In the initial version of this patch series I used cell-index property, > but then Grant pointed out in some other mail thread it should be > avoided. Hence I used the node aliases. To me, using cell-index is semantically equivalent to using the alias ID. > Different compatible values might not work, when for example there > are 3 IPs out of 4 of one type and the fourth one of another type. > It wouldn't even by really different types, just quirks/little > differences between them, e.g. no data path routed to one of other IPs. I was thinking of using feature-/quirk-oriented properties. For example, if there's a port on 3 of the 4 devices to connect to some other IP block, simply include a boolean property to indicate whether that port is present. It would be in 3 of the nodes but not the 4th. > Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the > MIPI-CSIS needs to be written to the FIMC.2 data input control register. > Even though MIPI-CSIS.N are same in terms of hardware structure they still > need to be distinguished as separate instances. Oh, so you're using the alias ID as the value to write into the FIMC.2 register for that. I'm not 100% familiar with aliases, but they seem like a more user-oriented naming thing to me, whereas values for hooking up intra-SoC routing are an unrelated namespace semantically, even if the values happen to line up right now. Perhaps rather than a Boolean property I mentioned above, use a custom property to indicate the ID that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this seems similar to using cell-index, my *guess* is that Grant's objection to using cell-index was more based on re-using cell-index for something other than its intended purpose than pushing you to use an alias ID rather than a property. After all, what happens in some later SoC where you have two different types of module that feed into the common module, such that type A sources have IDs 0..3 in the common module, and type B sources have IDs 4..7 in the common module - you wouldn't want to require alias ISs 4..7 for the type B DT nodes.
On 02/09/2013 01:32 AM, Stephen Warren wrote: > On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote: >> On 02/09/2013 12:21 AM, Stephen Warren wrote: >>> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote: >>>> On 02/07/2013 12:40 AM, Stephen Warren wrote: >>>>>> diff --git >>>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt >>>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt >>>>> >>>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC) >>>>>> +---------------------------------------------- >>> ... >>>>>> +For every fimc node a numbered alias should be present in the >>>>>> aliases node. >>>>>> +Aliases are of the form fimc<n>, where<n> is an integer (0...N) >>>>>> specifying >>>>>> +the IP's instance index. >>>>> >>>>> Why? Isn't it up to the DT author whether they care if each fimc >>>>> node is >>>>> assigned a specific identification v.s. whether identification is >>>>> assigned automatically? >>>> >>>> There are at least three different kinds of IPs that come in multiple >>>> instances in an SoC. To activate data links between them each instance >>>> needs to be clearly identified. There are also differences between >>>> instances of same device. Hence it's important these aliases don't have >>>> random values. >>>> >>>> Some more details about the SoC can be found at [1]. The aliases are >>>> also already used in the Exynos5 GScaler bindings [2] in a similar way. >>> >>> Hmmm. I'd expect explicit DT properties to represent the >>> instance-specific "configuration", or even different compatible values. >>> Relying on the alias ID seems rather indirect; what if in e.g. >>> Exynos6/... the mapping from instance/alias ID to feature set changes. >>> With explicit DT properties, that'd just be a .dts change, whereas by >>> requiring alias IDs now, you'd need a driver change to support this. >> >> In the initial version of this patch series I used cell-index property, >> but then Grant pointed out in some other mail thread it should be >> avoided. Hence I used the node aliases. > > To me, using cell-index is semantically equivalent to using the alias ID. I can't see significant difference either. I just switched to what seemed to be used for similar purpose. >> Different compatible values might not work, when for example there >> are 3 IPs out of 4 of one type and the fourth one of another type. >> It wouldn't even by really different types, just quirks/little >> differences between them, e.g. no data path routed to one of other IPs. > > I was thinking of using feature-/quirk-oriented properties. For example, > if there's a port on 3 of the 4 devices to connect to some other IP > block, simply include a boolean property to indicate whether that port > is present. It would be in 3 of the nodes but not the 4th. Yes, I could add several properties corresponding to all members of this [3] data structure. But still it is needed to clearly identify the IP block in a set of the hardware instances. >> Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the >> MIPI-CSIS needs to be written to the FIMC.2 data input control register. >> Even though MIPI-CSIS.N are same in terms of hardware structure they still >> need to be distinguished as separate instances. > > Oh, so you're using the alias ID as the value to write into the FIMC.2 > register for that. I'm not 100% familiar with aliases, but they seem > like a more user-oriented naming thing to me, whereas values for hooking > up intra-SoC routing are an unrelated namespace semantically, even if > the values happen to line up right now. Perhaps rather than a Boolean > property I mentioned above, use a custom property to indicate the ID > that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this seems That could be 'reg' property in the MIPI-CSIS.0 'port' subnode that links it to the image sensor node ([4], line 165). Because MIPI-CSIS IP blocks are immutably connected to the SoC camera physical MIPI CSI-2 interfaces, and the physical camera ports have fixed assignment to the MIPI-CSIS devices.. This way we could drop alias ID for the MIPI-CSIS nodes. And their instance index that is required for the top level driver which exposes topology and the routing capabilities to user space could be restored from the reg property value by subtracting a fixed offset. Similarly an instance index of FIMC-LITE could be derived from the value of reg property in a port node that links it to FIMC-IS ISP. I have been omitting these port/endpoint nodes because it seemed unnecessary to model explicitly those data paths. However it feels a bit overkill to add them just for the sake of identifying the IP block instance Still I can't really see a possibility to drop indexes for the FIMC nodes. > similar to using cell-index, my *guess* is that Grant's objection to > using cell-index was more based on re-using cell-index for something > other than its intended purpose than pushing you to use an alias ID > rather than a property. The comments to a patch for some other driver I was referring to can be found at [1]. My first patch series appeared significantly later [2]. I confused things a bit, sorry about that. I can see aliases used in bindings of multiple devices: uart, spi, sound interfaces, gpio, ... And all bindings seem to impose some rules on how their aliases are created. > After all, what happens in some later SoC where you have two different > types of module that feed into the common module, such that type A > sources have IDs 0..3 in the common module, and type B sources have IDs > 4..7 in the common module - you wouldn't want to require alias ISs 4..7 > for the type B DT nodes. There is no need to write alias ID directly into registers, and actually it doesn't really happen. But we need to know that, for example camera A is connected to physical MIPI CSI-2 channel 0 and to capture video with DMA engine of FIMC.2 we need to set FIMC.2 input register to link it to MIPI-CSIS 0. The system registers, where a sort of camera/display glue logic is configured also refer to FIMC devices explicitly by ID. So the driver as a client of the system registers block/glue logic interface needs to pass an information which FIMC H/W instance it wants to be e.g. attached/detached to/from some data pipeline. I still need to design some API for the camera system registers (glue logic) block. This would be shared by the V4L2 and the DRM FIMC driver. I thought about a couple of function calls specific to Exynos platform that would be called directly by related drivers. [1] https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-January/012128.html [2] http://www.spinics.net/lists/linux-media/msg48341.html [3] http://lxr.linux.no/#linux+v3.7.6/drivers/media/platform/s5p-fimc/fimc-core.h#L365 [4] http://tinyurl.com/arczzuo
On 02/09/2013 11:29 PM, Sylwester Nawrocki wrote: > >> After all, what happens in some later SoC where you have two different >> types of module that feed into the common module, such that type A >> sources have IDs 0..3 in the common module, and type B sources have IDs >> 4..7 in the common module - you wouldn't want to require alias ISs 4..7 >> for the type B DT nodes. I forgot to add, any ID remapping could happen in the common module, if it requires it. Type A and type B sources could have indexes 0...3 and the common module could derive its configuration from the source ID *and* the source type. The idea behind aliases was to identify each instance, rather than providing an exact configuration data that the common module could use.
On 02/09/2013 03:29 PM, Sylwester Nawrocki wrote: > On 02/09/2013 01:32 AM, Stephen Warren wrote: >> On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote: >>> On 02/09/2013 12:21 AM, Stephen Warren wrote: >>>> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote: >>>>> On 02/07/2013 12:40 AM, Stephen Warren wrote: >>>>>>> diff --git >>>>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt >>>>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt >>>>>> >>>>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC) >>>>>>> +---------------------------------------------- >>>> ... >>>>>>> +For every fimc node a numbered alias should be present in the >>>>>>> aliases node. >>>>>>> +Aliases are of the form fimc<n>, where<n> is an integer (0...N) >>>>>>> specifying >>>>>>> +the IP's instance index. ... >>> Different compatible values might not work, when for example there >>> are 3 IPs out of 4 of one type and the fourth one of another type. >>> It wouldn't even by really different types, just quirks/little >>> differences between them, e.g. no data path routed to one of other IPs. >> >> I was thinking of using feature-/quirk-oriented properties. For example, >> if there's a port on 3 of the 4 devices to connect to some other IP >> block, simply include a boolean property to indicate whether that port >> is present. It would be in 3 of the nodes but not the 4th. > > Yes, I could add several properties corresponding to all members of this > [3] data structure. But still it is needed to clearly identify the IP > block in a set of the hardware instances. Why? What decisions will be made based on the identify of the IP block instance that wouldn't be covered by DT properties that describe which features/bugs/... the IP block instance has? >>> Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the >>> MIPI-CSIS needs to be written to the FIMC.2 data input control register. >>> Even though MIPI-CSIS.N are same in terms of hardware structure they >>> still >>> need to be distinguished as separate instances. >> >> Oh, so you're using the alias ID as the value to write into the FIMC.2 >> register for that. I'm not 100% familiar with aliases, but they seem >> like a more user-oriented naming thing to me, whereas values for hooking >> up intra-SoC routing are an unrelated namespace semantically, even if >> the values happen to line up right now. Perhaps rather than a Boolean >> property I mentioned above, use a custom property to indicate the ID >> that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this seems > > That could be 'reg' property in the MIPI-CSIS.0 'port' subnode that > links it to the image sensor node ([4], line 165). Because MIPI-CSIS IP > blocks are immutably connected to the SoC camera physical MIPI CSI-2 > interfaces, and the physical camera ports have fixed assignment to the > MIPI-CSIS devices.. This way we could drop alias ID for the MIPI-CSIS > nodes. And their instance index that is required for the top level > driver which exposes topology and the routing capabilities to user space > could be restored from the reg property value by subtracting a fixed > offset. I suppose that would work. It feels a little indirect, and I think means that the driver needs to go find some child node defining its end of some link, then find the node representing the other end of the link, then read properties out of that other node to find the value. That seems a little unusual, but I guess it would work. I'm not sure of the long-term implications of doing that kind of thing. You'd want to run the idea past some DT maintainers/experts. ... > I can see aliases used in bindings of multiple devices: uart, spi, sound > interfaces, gpio, ... And all bindings seem to impose some rules on how > their aliases are created. Do you have specific examples? I really don't think the bindings should be dictating the alias values. >> After all, what happens in some later SoC where you have two different >> types of module that feed into the common module, such that type A >> sources have IDs 0..3 in the common module, and type B sources have IDs >> 4..7 in the common module - you wouldn't want to require alias ISs 4..7 >> for the type B DT nodes. > > There is no need to write alias ID directly into registers, and actually > it doesn't really happen. But we need to know that, for example camera A > is connected to physical MIPI CSI-2 channel 0 and to capture video with > DMA engine of FIMC.2 we need to set FIMC.2 input register to link it to > MIPI-CSIS 0. OK, so the IDs are selecting which register to write, or which mux settings to access. That's pretty much semantically the same thing.
On 02/11/2013 10:50 PM, Stephen Warren wrote: > On 02/09/2013 03:29 PM, Sylwester Nawrocki wrote: >> On 02/09/2013 01:32 AM, Stephen Warren wrote: >>> On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote: >>>> On 02/09/2013 12:21 AM, Stephen Warren wrote: >>>>> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote: >>>>>> On 02/07/2013 12:40 AM, Stephen Warren wrote: >>>>>>>> diff --git >>>>>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt >>>>>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt >>>>>>> >>>>>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC) >>>>>>>> +---------------------------------------------- >>>>> ... >>>>>>>> +For every fimc node a numbered alias should be present in the >>>>>>>> aliases node. >>>>>>>> +Aliases are of the form fimc<n>, where<n> is an integer (0...N) >>>>>>>> specifying >>>>>>>> +the IP's instance index. > ... >>>> Different compatible values might not work, when for example there >>>> are 3 IPs out of 4 of one type and the fourth one of another type. >>>> It wouldn't even by really different types, just quirks/little >>>> differences between them, e.g. no data path routed to one of other IPs. >>> >>> I was thinking of using feature-/quirk-oriented properties. For example, >>> if there's a port on 3 of the 4 devices to connect to some other IP >>> block, simply include a boolean property to indicate whether that port >>> is present. It would be in 3 of the nodes but not the 4th. >> >> Yes, I could add several properties corresponding to all members of this >> [3] data structure. But still it is needed to clearly identify the IP >> block in a set of the hardware instances. > > Why? What decisions will be made based on the identify of the IP block > instance that wouldn't be covered by DT properties that describe which > features/bugs/... the IP block instance has? The whole subsystem topology is exposed to user space through the Media Controller API. Although the user space libraries/applications using this driver are not much concerned how the hardware is represented internally in the kernel, some properties of the media entities seen in user space are derived from the hardware details, e.g. the media entity names contain index of a corresponding IP block. Please see [1] for an example of a topology exposed by the driver. Since different H/W instances have different capabilities, user space libraries/plugins can be coded to e.g. use one instance for video playback post-processing and another for camera capture. The capabilities could be also discovered with the V4L2 API to some level of detail. But still assigning random entity names to the IP blocks has a potential of breaking user space or causing some malfunctions. Perhaps I should just use a custom properties like "samsung,fimc-id" ? I tried to represent some intra-soc data routing details with our common video interfaces bindings and it really looked like a lot of unnecessary nodes, with 11 camera sub-device nodes required to cover a front a rear facing camera. Some details can be just coded in the driver, especially that newer SoCs will get a new driver anyway, since there are huge differences between the media subsystem architecture across subsequent SoC revisions. [1] http://www.spinics.net/lists/linux-media/attachments/psPhA96YX70U.ps >>>> Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the >>>> MIPI-CSIS needs to be written to the FIMC.2 data input control register. >>>> Even though MIPI-CSIS.N are same in terms of hardware structure they >>>> still >>>> need to be distinguished as separate instances. >>> >>> Oh, so you're using the alias ID as the value to write into the FIMC.2 >>> register for that. I'm not 100% familiar with aliases, but they seem >>> like a more user-oriented naming thing to me, whereas values for hooking >>> up intra-SoC routing are an unrelated namespace semantically, even if >>> the values happen to line up right now. Perhaps rather than a Boolean >>> property I mentioned above, use a custom property to indicate the ID >>> that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this seems >> >> That could be 'reg' property in the MIPI-CSIS.0 'port' subnode that >> links it to the image sensor node ([4], line 165). Because MIPI-CSIS IP >> blocks are immutably connected to the SoC camera physical MIPI CSI-2 >> interfaces, and the physical camera ports have fixed assignment to the >> MIPI-CSIS devices.. This way we could drop alias ID for the MIPI-CSIS >> nodes. And their instance index that is required for the top level >> driver which exposes topology and the routing capabilities to user space >> could be restored from the reg property value by subtracting a fixed >> offset. > > I suppose that would work. It feels a little indirect, and I think means > that the driver needs to go find some child node defining its end of > some link, then find the node representing the other end of the link, > then read properties out of that other node to find the value. That > seems a little unusual, but I guess it would work. I'm not sure of the > long-term implications of doing that kind of thing. You'd want to run > the idea past some DT maintainers/experts. It's a bit simpler than that. We would need only to look for the reg property in a local port subnode. MIPI-CSIS correspond to physical MIPI CSI-2 bus interface of an SoC, hence it has to have specific reg values that identify each camera input interface. csis { /* MIPI CSI-2 Slave */ ... port { reg = <1>; /* e.g. MIPI-CSIS.1 */ csis_ep: endpoint { remote-endpoint = <&img_sensor_ep>; } }; }; i2c-controller { ... image-sensor { /* MIPI CSI-2 Master */ ... port { img_sensor_ep: endpoint { remote-endpoint = <&csis_ep>; } }; }; }; Naturally the image-sensor node represents a device external to an SoC. > ... >> I can see aliases used in bindings of multiple devices: uart, spi, sound >> interfaces, gpio, ... And all bindings seem to impose some rules on how >> their aliases are created. > > Do you have specific examples? I really don't think the bindings should > be dictating the alias values. I just grepped through the existing bindings documentation: gpio/gpio-mxs.txt:Note: Each GPIO port should have an alias correctly numbered in "aliases" gpio/gpio-mxs.txt-node. serial/fsl-imx-uart.txt:Note: Each uart controller should have an alias correctly numbered serial/fsl-imx-uart.txt:in "aliases" node. mmc/synopsis-dw-mshc.txt:- All the MSHC controller nodes should be represented in the aliases node using mmc/synopsis-dw-mshc.txt: the following format 'mshc{n}' where n is a unique number for the alias. sound/mxs-saif.txt:Note: Each SAIF controller should have an alias correctly numbered sound/mxs-saif.txt:in "aliases" node. spi/spi-samsung.txt:- All the SPI controller nodes should be represented in the aliases node using spi/spi-samsung.txt: the following format 'spi{n}' where n is a unique number for the alias. tty/serial/fsl-mxs-auart.txt:Note: Each auart port should have an alias correctly numbered in "aliases" tty/serial/fsl-mxs-auart.txt-node. powerpc/fsl/mpic-msgr.txt: An alias should be created for every message register block. They are not powerpc/fsl/mpic-msgr.txt- required, though. However, a particular implementation of this binding powerpc/fsl/mpic-msgr.txt: may require aliases to be present. Aliases are of the form powerpc/fsl/mpic-msgr.txt- 'mpic-msgr-block<n>', where <n> is an integer specifying the block's number. powerpc/fsl/mpic-msgr.txt- Numbers shall start at 0. I think "correctly numbered" in the above statements means there are some specific rules on how the aliases are created, however those seem not clearly communicated. And there is a new patch series that allows I2C bus controller enumeration by means of the aliases: http://www.spinics.net/lists/arm-kernel/msg224162.html >>> After all, what happens in some later SoC where you have two different >>> types of module that feed into the common module, such that type A >>> sources have IDs 0..3 in the common module, and type B sources have IDs >>> 4..7 in the common module - you wouldn't want to require alias ISs 4..7 >>> for the type B DT nodes. >> >> There is no need to write alias ID directly into registers, and actually >> it doesn't really happen. But we need to know that, for example camera A >> is connected to physical MIPI CSI-2 channel 0 and to capture video with >> DMA engine of FIMC.2 we need to set FIMC.2 input register to link it to >> MIPI-CSIS 0. > > OK, so the IDs are selecting which register to write, or which mux > settings to access. That's pretty much semantically the same thing.
On 02/12/2013 03:39 PM, Sylwester Nawrocki wrote: > On 02/11/2013 10:50 PM, Stephen Warren wrote: >> On 02/09/2013 03:29 PM, Sylwester Nawrocki wrote: >>> On 02/09/2013 01:32 AM, Stephen Warren wrote: >>>> On 02/08/2013 05:05 PM, Sylwester Nawrocki wrote: >>>>> On 02/09/2013 12:21 AM, Stephen Warren wrote: >>>>>> On 02/08/2013 04:16 PM, Sylwester Nawrocki wrote: >>>>>>> On 02/07/2013 12:40 AM, Stephen Warren wrote: >>>>>>>>> diff --git >>>>>>>>> a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt >>>>>>>>> b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt >>>>>>>> >>>>>>>>> +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC) >>>>>>>>> +---------------------------------------------- >>>>>> ... >>>>>>>>> +For every fimc node a numbered alias should be present in the >>>>>>>>> aliases node. >>>>>>>>> +Aliases are of the form fimc<n>, where<n> is an integer >>>>>>>>> (0...N) >>>>>>>>> specifying >>>>>>>>> +the IP's instance index. >> ... >>>>> Different compatible values might not work, when for example there >>>>> are 3 IPs out of 4 of one type and the fourth one of another type. >>>>> It wouldn't even by really different types, just quirks/little >>>>> differences between them, e.g. no data path routed to one of other >>>>> IPs. >>>> >>>> I was thinking of using feature-/quirk-oriented properties. For >>>> example, >>>> if there's a port on 3 of the 4 devices to connect to some other IP >>>> block, simply include a boolean property to indicate whether that port >>>> is present. It would be in 3 of the nodes but not the 4th. >>> >>> Yes, I could add several properties corresponding to all members of this >>> [3] data structure. But still it is needed to clearly identify the IP >>> block in a set of the hardware instances. >> >> Why? What decisions will be made based on the identify of the IP block >> instance that wouldn't be covered by DT properties that describe which >> features/bugs/... the IP block instance has? > > The whole subsystem topology is exposed to user space through the Media > Controller API. OK, stable user-visible names are a reasonable use for device tree. I still don't think you should use those user-visible IDs for making any other kind of decision though. >>>>> Then to connect e.g. MIPI-CSIS.0 to FIMC.2 at run time an index of the >>>>> MIPI-CSIS needs to be written to the FIMC.2 data input control >>>>> register. >>>>> Even though MIPI-CSIS.N are same in terms of hardware structure they >>>>> still >>>>> need to be distinguished as separate instances. >>>> >>>> Oh, so you're using the alias ID as the value to write into the FIMC.2 >>>> register for that. I'm not 100% familiar with aliases, but they seem >>>> like a more user-oriented naming thing to me, whereas values for >>>> hooking >>>> up intra-SoC routing are an unrelated namespace semantically, even if >>>> the values happen to line up right now. Perhaps rather than a Boolean >>>> property I mentioned above, use a custom property to indicate the ID >>>> that the FIMC.2 object knows the MIPI-CSIS.0 object as? While this >>>> seems >>> >>> That could be 'reg' property in the MIPI-CSIS.0 'port' subnode that >>> links it to the image sensor node ([4], line 165). Because MIPI-CSIS IP >>> blocks are immutably connected to the SoC camera physical MIPI CSI-2 >>> interfaces, and the physical camera ports have fixed assignment to the >>> MIPI-CSIS devices.. This way we could drop alias ID for the MIPI-CSIS >>> nodes. And their instance index that is required for the top level >>> driver which exposes topology and the routing capabilities to user space >>> could be restored from the reg property value by subtracting a fixed >>> offset. >> >> I suppose that would work. It feels a little indirect, and I think means >> that the driver needs to go find some child node defining its end of >> some link, then find the node representing the other end of the link, >> then read properties out of that other node to find the value. That >> seems a little unusual, but I guess it would work. I'm not sure of the >> long-term implications of doing that kind of thing. You'd want to run >> the idea past some DT maintainers/experts. > > It's a bit simpler than that. We would need only to look for the reg > property in a local port subnode. MIPI-CSIS correspond to physical MIPI > CSI-2 bus interface of an SoC, hence it has to have specific reg values > that identify each camera input interface. Oh I see. I guess if a device is using its own node to determine its own identify, that's reasonable. I thought you were talking about a situation like: FIMC <--> XXX where FIMC wanted to determine what ID XXX knew that particular FIMC as. >>> I can see aliases used in bindings of multiple devices: uart, spi, sound >>> interfaces, gpio, ... And all bindings seem to impose some rules on how >>> their aliases are created. >> >> Do you have specific examples? I really don't think the bindings should >> be dictating the alias values. > > I just grepped through the existing bindings documentation: ... > I think "correctly numbered" in the above statements means there are some > specific rules on how the aliases are created, however those seem not > clearly communicated. A binding specifying that an alias must (or even should) exist for each node seems odd to me. In the absence of an explicit rule for how to determine the alias IDs to use, I think the rule would simply be that the aliases must be unique? > And there is a new patch series that allows I2C bus controller enumeration > by means of the aliases: > > http://www.spinics.net/lists/arm-kernel/msg224162.html That's not enumerating controllers by alias (they're still enumerated by scanning the DT nodes for buses in the normal way). The change simply assigns the bus ID of each controller from an alias; exactly what aliases are for.
On 02/13/2013 09:42 PM, Stephen Warren wrote: > On 02/12/2013 03:39 PM, Sylwester Nawrocki wrote: [...] >> The whole subsystem topology is exposed to user space through the Media >> Controller API. > > OK, stable user-visible names are a reasonable use for device tree. I > still don't think you should use those user-visible IDs for making any > other kind of decision though. OK, I will update the bindings so all variant details are placed in the device tree. Then the routing information would mostly be coming from the device specific dt properties/the common media bindings and the state of links between the media entities, set by the user. >> It's a bit simpler than that. We would need only to look for the reg >> property in a local port subnode. MIPI-CSIS correspond to physical MIPI >> CSI-2 bus interface of an SoC, hence it has to have specific reg values >> that identify each camera input interface. > > Oh I see. I guess if a device is using its own node to determine its own > identify, that's reasonable. OK, I'm going to post an updated patch series in a week or two. > I thought you were talking about a situation like: > > FIMC <--> XXX > > where FIMC wanted to determine what ID XXX knew that particular FIMC as. Ah, no. Sorry for the poor explanation. FIMC are on a sort if interconnect bus and they can be attached to a single data source, even in parallel, and the data source entity don't even need to be fully aware of it. >>>> I can see aliases used in bindings of multiple devices: uart, spi, sound >>>> interfaces, gpio, ... And all bindings seem to impose some rules on how >>>> their aliases are created. >>> >>> Do you have specific examples? I really don't think the bindings should >>> be dictating the alias values. >> >> I just grepped through the existing bindings documentation: > ... >> I think "correctly numbered" in the above statements means there are some >> specific rules on how the aliases are created, however those seem not >> clearly communicated. > > A binding specifying that an alias must (or even should) exist for each > node seems odd to me. In the absence of an explicit rule for how to > determine the alias IDs to use, I think the rule would simply be that > the aliases must be unique? I guess so. Inspecting of_alias_get_id() call sites tells us that most drivers just fail when alias is not present and only rarely it is not treated as an error condition. >> And there is a new patch series that allows I2C bus controller enumeration >> by means of the aliases: >> >> http://www.spinics.net/lists/arm-kernel/msg224162.html > > That's not enumerating controllers by alias (they're still enumerated by > scanning the DT nodes for buses in the normal way). The change simply > assigns the bus ID of each controller from an alias; exactly what > aliases are for. OK, that clarifies a bit my understanding of the aliases. Thanks, Sylwester
diff --git a/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt new file mode 100644 index 0000000..916b2c3 --- /dev/null +++ b/Documentation/devicetree/bindings/media/soc/samsung-fimc.txt @@ -0,0 +1,72 @@ +Samsung S5P/EXYNOS SoC Camera Subsystem (FIMC) +---------------------------------------------- + +The Exynos Camera subsystem comprises of multiple sub-devices that are +represented by separate platform devices. Some of the IPs come in different +variants across the SoC revisions (FIMC) and some remain mostly unchanged +(MIPI CSIS, FIMC-LITE). + +All those sub-subdevices are defined as parent nodes of the common device +node, which also includes common properties of the whole subsystem not really +specific to any single sub-device, like common camera port pins or external +clocks for image sensors attached to the SoC. + +Common 'camera' node +-------------------- + +Required properties: + +- compatible : must be "samsung,fimc", "simple-bus" + +The 'camera' node must include at least one 'fimc' child node. + + +'fimc' device nodes +------------------- + +Required properties: + +- compatible : "samsung,s5pv210-fimc" for S5PV210, + "samsung,exynos4210-fimc" for Exynos4210, + "samsung,exynos4212-fimc" for Exynos4212/4412 SoCs; +- reg : physical base address and size of the device memory mapped + registers; +- interrupts : FIMC interrupt to the CPU should be described here; + +For every fimc node a numbered alias should be present in the aliases node. +Aliases are of the form fimc<n>, where <n> is an integer (0...N) specifying +the IP's instance index. + +Optional properties + + - clock-frequency - maximum FIMC local clock (LCLK) frequency + +Example: + + aliases { + csis0 = &csis_0; + fimc0 = &fimc_0; + }; + + camera { + compatible = "samsung,fimc", "simple-bus"; + #address-cells = <1>; + #size-cells = <1>; + status = "okay"; + + fimc_0: fimc@11800000 { + compatible = "samsung,exynos4210-fimc"; + reg = <0x11800000 0x1000>; + interrupts = <0 85 0>; + status = "okay"; + }; + + csis_0: csis@11880000 { + compatible = "samsung,exynos4210-csis"; + reg = <0x11880000 0x1000>; + interrupts = <0 78 0>; + max-data-lanes = <4>; + }; + }; + +[1] Documentation/devicetree/bindings/media/soc/samsung-mipi-csis.txt diff --git a/drivers/media/platform/s5p-fimc/fimc-capture.c b/drivers/media/platform/s5p-fimc/fimc-capture.c index f822b8e..bc84301 100644 --- a/drivers/media/platform/s5p-fimc/fimc-capture.c +++ b/drivers/media/platform/s5p-fimc/fimc-capture.c @@ -1883,7 +1883,7 @@ int fimc_initialize_capture_subdev(struct fimc_dev *fimc) v4l2_subdev_init(sd, &fimc_subdev_ops); sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE; - snprintf(sd->name, sizeof(sd->name), "FIMC.%d", fimc->pdev->id); + snprintf(sd->name, sizeof(sd->name), "FIMC.%d", fimc->id); fimc->vid_cap.sd_pads[FIMC_SD_PAD_SINK].flags = MEDIA_PAD_FL_SINK; fimc->vid_cap.sd_pads[FIMC_SD_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE; diff --git a/drivers/media/platform/s5p-fimc/fimc-core.c b/drivers/media/platform/s5p-fimc/fimc-core.c index 29f7bb7..07ca0e0 100644 --- a/drivers/media/platform/s5p-fimc/fimc-core.c +++ b/drivers/media/platform/s5p-fimc/fimc-core.c @@ -21,6 +21,8 @@ #include <linux/pm_runtime.h> #include <linux/list.h> #include <linux/io.h> +#include <linux/of.h> +#include <linux/of_device.h> #include <linux/slab.h> #include <linux/clk.h> #include <media/v4l2-ioctl.h> @@ -863,45 +865,57 @@ static int fimc_m2m_resume(struct fimc_dev *fimc) return 0; } +static const struct of_device_id fimc_of_match[]; + static int fimc_probe(struct platform_device *pdev) { - const struct fimc_drvdata *drv_data = fimc_get_drvdata(pdev); - struct s5p_platform_fimc *pdata; + struct fimc_drvdata *drv_data = NULL; + struct device *dev = &pdev->dev; + const struct of_device_id *of_id; + u32 lclk_freq = 0; struct fimc_dev *fimc; struct resource *res; int ret = 0; - if (pdev->id >= drv_data->num_entities) { - dev_err(&pdev->dev, "Invalid platform device id: %d\n", - pdev->id); - return -EINVAL; - } - - fimc = devm_kzalloc(&pdev->dev, sizeof(*fimc), GFP_KERNEL); + fimc = devm_kzalloc(dev, sizeof(*fimc), GFP_KERNEL); if (!fimc) return -ENOMEM; - fimc->id = pdev->id; + if (dev->of_node) { + of_id = of_match_node(fimc_of_match, dev->of_node); + if (of_id) + drv_data = (struct fimc_drvdata *)of_id->data; + fimc->id = of_alias_get_id(dev->of_node, "fimc"); + + of_property_read_u32(dev->of_node, "clock-frequency", + &lclk_freq); + } else { + drv_data = fimc_get_drvdata(pdev); + fimc->id = pdev->id; + } + + if (!drv_data || fimc->id < 0 || fimc->id >= drv_data->num_entities) { + dev_err(dev, "Invalid driver data or device index (%d)\n", + fimc->id); + return -EINVAL; + } fimc->variant = drv_data->variant[fimc->id]; fimc->pdev = pdev; - pdata = pdev->dev.platform_data; - fimc->pdata = pdata; - init_waitqueue_head(&fimc->irq_queue); spin_lock_init(&fimc->slock); mutex_init(&fimc->lock); res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - fimc->regs = devm_request_and_ioremap(&pdev->dev, res); + fimc->regs = devm_request_and_ioremap(dev, res); if (fimc->regs == NULL) { - dev_err(&pdev->dev, "Failed to obtain io memory\n"); + dev_err(dev, "Failed to obtain io memory\n"); return -ENOENT; } res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); if (res == NULL) { - dev_err(&pdev->dev, "Failed to get IRQ resource\n"); + dev_err(dev, "Failed to get IRQ resource\n"); return -ENXIO; } @@ -909,7 +923,10 @@ static int fimc_probe(struct platform_device *pdev) if (ret) return ret; - ret = clk_set_rate(fimc->clock[CLK_BUS], drv_data->lclk_frequency); + if (lclk_freq == 0) + lclk_freq = drv_data->lclk_frequency; + + ret = clk_set_rate(fimc->clock[CLK_BUS], lclk_freq); if (ret < 0) return ret; @@ -917,10 +934,10 @@ static int fimc_probe(struct platform_device *pdev) if (ret < 0) return ret; - ret = devm_request_irq(&pdev->dev, res->start, fimc_irq_handler, - 0, dev_name(&pdev->dev), fimc); + ret = devm_request_irq(dev, res->start, fimc_irq_handler, + 0, dev_name(dev), fimc); if (ret) { - dev_err(&pdev->dev, "failed to install irq (%d)\n", ret); + dev_err(dev, "failed to install irq (%d)\n", ret); goto err_clk; } @@ -929,27 +946,26 @@ static int fimc_probe(struct platform_device *pdev) goto err_clk; platform_set_drvdata(pdev, fimc); - pm_runtime_enable(&pdev->dev); - ret = pm_runtime_get_sync(&pdev->dev); + pm_runtime_enable(dev); + ret = pm_runtime_get_sync(dev); if (ret < 0) goto err_sd; /* Initialize contiguous memory allocator */ - fimc->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev); + fimc->alloc_ctx = vb2_dma_contig_init_ctx(dev); if (IS_ERR(fimc->alloc_ctx)) { ret = PTR_ERR(fimc->alloc_ctx); goto err_pm; } - dev_dbg(&pdev->dev, "FIMC.%d registered successfully\n", fimc->id); + dev_dbg(dev, "FIMC.%d registered successfully\n", fimc->id); - pm_runtime_put(&pdev->dev); + pm_runtime_put(dev); return 0; err_pm: - pm_runtime_put(&pdev->dev); + pm_runtime_put(dev); err_sd: fimc_unregister_capture_subdev(fimc); err_clk: - clk_disable(fimc->clock[CLK_BUS]); fimc_clk_put(fimc); return ret; } @@ -1258,10 +1274,24 @@ static const struct platform_device_id fimc_driver_ids[] = { .name = "exynos4x12-fimc", .driver_data = (unsigned long)&fimc_drvdata_exynos4x12, }, - {}, + { }, }; MODULE_DEVICE_TABLE(platform, fimc_driver_ids); +static const struct of_device_id fimc_of_match[] = { + { + .compatible = "samsung,s5pv210-fimc", + .data = &fimc_drvdata_s5pv210, + }, { + .compatible = "samsung,exynos4210-fimc", + .data = &fimc_drvdata_exynos4210, + }, { + .compatible = "samsung,exynos4212-fimc", + .data = &fimc_drvdata_exynos4x12, + }, + { /* sentinel */ }, +}; + static const struct dev_pm_ops fimc_pm_ops = { SET_SYSTEM_SLEEP_PM_OPS(fimc_suspend, fimc_resume) SET_RUNTIME_PM_OPS(fimc_runtime_suspend, fimc_runtime_resume, NULL) @@ -1272,9 +1302,10 @@ static struct platform_driver fimc_driver = { .remove = fimc_remove, .id_table = fimc_driver_ids, .driver = { - .name = FIMC_MODULE_NAME, - .owner = THIS_MODULE, - .pm = &fimc_pm_ops, + .of_match_table = fimc_of_match, + .name = FIMC_MODULE_NAME, + .owner = THIS_MODULE, + .pm = &fimc_pm_ops, } };