Re: [RFC v3 02/13] [media] exynos5-fimc-is: Add Exynos5 FIMC-IS device tree bindings documentation
W dniu 2013-08-08 22:34, Stephen Warren pisze: On 08/05/2013 04:37 PM, Sylwester Nawrocki wrote: On 08/05/2013 06:53 PM, Stephen Warren wrote: On 08/03/2013 03:41 PM, Sylwester Nawrocki wrote: On 08/02/2013 05:02 PM, Arun Kumar K wrote: The patch adds the DT binding documentation for Samsung Exynos5 SoC series imaging subsystem (FIMC-IS). diff --git a/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt new file mode 100644 index 000..49a373a --- /dev/null +++ b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt @@ -0,0 +1,52 @@ +Samsung EXYNOS5 SoC series Imaging Subsystem (FIMC-IS) +-- + +The camera subsystem on Samsung Exynos5 SoC has some changes relative +to previous SoC versions. Exynos5 has almost similar MIPI-CSIS and +FIMC-LITE IPs but has a much improved version of FIMC-IS which can +handle sensor controls and camera post-processing operations. The +Exynos5 FIMC-IS has a dedicated ARM Cortex A5 processor, many +post-processing blocks (ISP, DRC, FD, ODC, DIS, 3DNR) and two +dedicated scalers (SCC and SCP). So there are a lot of blocks mentioned there, yet the binding doesn't seem to describe most of it. Is the binding complete? Thanks for the review Stephen. No, the binding certainly isn't complete, it doesn't describe the all available IP blocks. There are separate MMIO address regions for each ... So while we could list all the devices, we decided not to do so. Because it is not needed by the current software and we may miss some details for case where the whole subsystem is controlled by the host CPU (however such scenario is extremely unlikely AFAICT) which then would be impossible or hard to change. Yes, that's probably a good approach. I guess we should list all available devices, similarly as it's done in Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt. And then should they just be disabled through the status property if they are not needed in the Linux driver ? I guess it is more sensible than marking them as optional and then not listing them in dts at all ? If you can define complete bindings for those nodes, it might make sense to do that. If the devices are perhaps complex to represent and hence you might not be able to come up with complete bindings for them right now, it may indeed be better to simply not mention the devices you don't care about for now. We would prefer to start with a minimal binding, this would minimize possible issues in future IMHO, as the subsystem is pretty complex. Then, if detailed H/W description is required the firmware would need to be updated, which would have been backward compatible. We could probably come up with a complete binding, but there is a good chance something could be done wrong, as that couldn't be actually tested. It's not trivial to make this all work on the host CPU while most of the detailed H/W knowledge stays on the firmware teams side. +pmu subnode +--- + +Required properties: + - reg : should contain PMU physical base address and size of the memory + mapped registers. I think you need a compatible value for this. How else is the node identified? The node name probably should not be used for identification. Of course the node name is currently used for identification. There is no compatible property because this pmu node is used to get hold of only part of the Power Management Unit registers, specific to the FIMC-IS. The PMU has more registers that also other drivers would be interested in, e.g. clocks or USB. I believe the correct way to solve this is for there to be a standalone PMU node at the appropriate location in DT, and for the FIMC bindings to reference that other node by phandle. Right now, the FIMC driver SW can manually follow the phandle, look at the reg property, and map that itself. Later down the road, you could instantiate a true PMU driver, and have the FIMC driver look up that driver, and call APIs on it. This change can be made without requiring any changes to the DT binding. That way, you aren't introducing a fake PMU node into the FIMC bindings just to satisfy internal Linux driver details. That sounds reasonable. It lets us to keep the DT binding stable and at the same move forward with the FIMC-IS while the PMU part is being worked on. Thanks. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v3 02/13] [media] exynos5-fimc-is: Add Exynos5 FIMC-IS device tree bindings documentation
On 08/05/2013 04:37 PM, Sylwester Nawrocki wrote: On 08/05/2013 06:53 PM, Stephen Warren wrote: On 08/03/2013 03:41 PM, Sylwester Nawrocki wrote: On 08/02/2013 05:02 PM, Arun Kumar K wrote: The patch adds the DT binding documentation for Samsung Exynos5 SoC series imaging subsystem (FIMC-IS). diff --git a/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt new file mode 100644 index 000..49a373a --- /dev/null +++ b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt @@ -0,0 +1,52 @@ +Samsung EXYNOS5 SoC series Imaging Subsystem (FIMC-IS) +-- + +The camera subsystem on Samsung Exynos5 SoC has some changes relative +to previous SoC versions. Exynos5 has almost similar MIPI-CSIS and +FIMC-LITE IPs but has a much improved version of FIMC-IS which can +handle sensor controls and camera post-processing operations. The +Exynos5 FIMC-IS has a dedicated ARM Cortex A5 processor, many +post-processing blocks (ISP, DRC, FD, ODC, DIS, 3DNR) and two +dedicated scalers (SCC and SCP). So there are a lot of blocks mentioned there, yet the binding doesn't seem to describe most of it. Is the binding complete? Thanks for the review Stephen. No, the binding certainly isn't complete, it doesn't describe the all available IP blocks. There are separate MMIO address regions for each ... So while we could list all the devices, we decided not to do so. Because it is not needed by the current software and we may miss some details for case where the whole subsystem is controlled by the host CPU (however such scenario is extremely unlikely AFAICT) which then would be impossible or hard to change. Yes, that's probably a good approach. I guess we should list all available devices, similarly as it's done in Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt. And then should they just be disabled through the status property if they are not needed in the Linux driver ? I guess it is more sensible than marking them as optional and then not listing them in dts at all ? If you can define complete bindings for those nodes, it might make sense to do that. If the devices are perhaps complex to represent and hence you might not be able to come up with complete bindings for them right now, it may indeed be better to simply not mention the devices you don't care about for now. +pmu subnode +--- + +Required properties: + - reg : should contain PMU physical base address and size of the memory + mapped registers. I think you need a compatible value for this. How else is the node identified? The node name probably should not be used for identification. Of course the node name is currently used for identification. There is no compatible property because this pmu node is used to get hold of only part of the Power Management Unit registers, specific to the FIMC-IS. The PMU has more registers that also other drivers would be interested in, e.g. clocks or USB. I believe the correct way to solve this is for there to be a standalone PMU node at the appropriate location in DT, and for the FIMC bindings to reference that other node by phandle. Right now, the FIMC driver SW can manually follow the phandle, look at the reg property, and map that itself. Later down the road, you could instantiate a true PMU driver, and have the FIMC driver look up that driver, and call APIs on it. This change can be made without requiring any changes to the DT binding. That way, you aren't introducing a fake PMU node into the FIMC bindings just to satisfy internal Linux driver details. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v3 02/13] [media] exynos5-fimc-is: Add Exynos5 FIMC-IS device tree bindings documentation
On 08/03/2013 03:41 PM, Sylwester Nawrocki wrote: On 08/02/2013 05:02 PM, Arun Kumar K wrote: The patch adds the DT binding documentation for Samsung Exynos5 SoC series imaging subsystem (FIMC-IS). diff --git a/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt new file mode 100644 index 000..49a373a --- /dev/null +++ b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt @@ -0,0 +1,52 @@ +Samsung EXYNOS5 SoC series Imaging Subsystem (FIMC-IS) +-- + +The camera subsystem on Samsung Exynos5 SoC has some changes relative +to previous SoC versions. Exynos5 has almost similar MIPI-CSIS and +FIMC-LITE IPs but has a much improved version of FIMC-IS which can +handle sensor controls and camera post-processing operations. The +Exynos5 FIMC-IS has a dedicated ARM Cortex A5 processor, many +post-processing blocks (ISP, DRC, FD, ODC, DIS, 3DNR) and two +dedicated scalers (SCC and SCP). So there are a lot of blocks mentioned there, yet the binding doesn't seem to describe most of it. Is the binding complete? +pmu subnode +--- + +Required properties: + - reg : should contain PMU physical base address and size of the memory + mapped registers. I think you need a compatible value for this. How else is the node identified? The node name probably should not be used for identification. + +i2c-isp (ISP I2C bus controller) nodes +-- + +Required properties: + +- compatible: should be samsung,exynos4212-i2c-isp for Exynos4212, + Exynos4412 and Exynos5250 SoCs; +- reg: physical base address and length of the registers set; +- clocks: must contain gate clock specifier for this controller; +- clock-names: must contain i2c_isp entry. + +For the above nodes it is required to specify a pinctrl state named default, Is above nodes both pmu, i2c-isp? It might make sense to be more explicit re: which nodes this comment applies to. +according to the pinctrl bindings defined in ../pinctrl/pinctrl-bindings.txt. + +Device tree nodes of the image sensors' controlled directly by the FIMC-IS s/'// ? +firmware must be child nodes of their corresponding ISP I2C bus controller node. +The data link of these image sensors must be specified using the common video +interfaces bindings, defined in video-interfaces.txt. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v3 02/13] [media] exynos5-fimc-is: Add Exynos5 FIMC-IS device tree bindings documentation
On 08/05/2013 06:53 PM, Stephen Warren wrote: On 08/03/2013 03:41 PM, Sylwester Nawrocki wrote: On 08/02/2013 05:02 PM, Arun Kumar K wrote: The patch adds the DT binding documentation for Samsung Exynos5 SoC series imaging subsystem (FIMC-IS). diff --git a/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt new file mode 100644 index 000..49a373a --- /dev/null +++ b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt @@ -0,0 +1,52 @@ +Samsung EXYNOS5 SoC series Imaging Subsystem (FIMC-IS) +-- + +The camera subsystem on Samsung Exynos5 SoC has some changes relative +to previous SoC versions. Exynos5 has almost similar MIPI-CSIS and +FIMC-LITE IPs but has a much improved version of FIMC-IS which can +handle sensor controls and camera post-processing operations. The +Exynos5 FIMC-IS has a dedicated ARM Cortex A5 processor, many +post-processing blocks (ISP, DRC, FD, ODC, DIS, 3DNR) and two +dedicated scalers (SCC and SCP). So there are a lot of blocks mentioned there, yet the binding doesn't seem to describe most of it. Is the binding complete? Thanks for the review Stephen. No, the binding certainly isn't complete, it doesn't describe the all available IP blocks. There are separate MMIO address regions for each block for the main CPUs and for the Cortex-A5 which is supposed to run firmware that controls the whole subsystem. So in theory all those IP blocks should be listed as device tree nodes, with at least their compatible, reg and interrupts properties. However due to most of the sub-devices being controlled by the firmware the current Linux driver for this whole FIMC-IS subsystem doesn't need to now exact details of each internal data processing block. The is a mailbox interface used for communication between host CPU and the FIMC-IS CPU. So while we could list all the devices, we decided not to do so. Because it is not needed by the current software and we may miss some details for case where the whole subsystem is controlled by the host CPU (however such scenario is extremely unlikely AFAICT) which then would be impossible or hard to change. I guess we should list all available devices, similarly as it's done in Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt. And then should they just be disabled through the status property if they are not needed in the Linux driver ? I guess it is more sensible than marking them as optional and then not listing them in dts at all ? +pmu subnode +--- + +Required properties: + - reg : should contain PMU physical base address and size of the memory + mapped registers. I think you need a compatible value for this. How else is the node identified? The node name probably should not be used for identification. Of course the node name is currently used for identification. There is no compatible property because this pmu node is used to get hold of only part of the Power Management Unit registers, specific to the FIMC-IS. The PMU has more registers that also other drivers would be interested in, e.g. clocks or USB. I have been considering exposing the PMU registers through a syscon-like interface and having a phandle pointing to it in the relevant device nodes. Adding compatible property might not be a good approach. It would have been hard to map this to a separate device described in the SoC's datasheet. Registers specific to the FIMC-IS are not contiguous in the PMU MMIO region. + +i2c-isp (ISP I2C bus controller) nodes +-- + +Required properties: + +- compatible: should be samsung,exynos4212-i2c-isp for Exynos4212, + Exynos4412 and Exynos5250 SoCs; +- reg: physical base address and length of the registers set; +- clocks: must contain gate clock specifier for this controller; +- clock-names: must contain i2c_isp entry. + +For the above nodes it is required to specify a pinctrl state named default, Is above nodes both pmu, i2c-isp? It might make sense to be more explicit re: which nodes this comment applies to. Yeah, certainly there is room for improvement here. above nodes was supposed to refer to the i2c-isp nodes only, it should be said more precisely. +according to the pinctrl bindings defined in ../pinctrl/pinctrl-bindings.txt. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v3 02/13] [media] exynos5-fimc-is: Add Exynos5 FIMC-IS device tree bindings documentation
On 08/02/2013 05:02 PM, Arun Kumar K wrote: The patch adds the DT binding documentation for Samsung Exynos5 SoC series imaging subsystem (FIMC-IS). Signed-off-by: Arun Kumar Karun...@samsung.com --- .../devicetree/bindings/media/exynos5-fimc-is.txt | 52 1 file changed, 52 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/exynos5-fimc-is.txt diff --git a/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt new file mode 100644 index 000..49a373a --- /dev/null +++ b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt @@ -0,0 +1,52 @@ +Samsung EXYNOS5 SoC series Imaging Subsystem (FIMC-IS) +-- + +The camera subsystem on Samsung Exynos5 SoC has some changes relative +to previous SoC versions. Exynos5 has almost similar MIPI-CSIS and +FIMC-LITE IPs but has a much improved version of FIMC-IS which can +handle sensor controls and camera post-processing operations. The +Exynos5 FIMC-IS has a dedicated ARM Cortex A5 processor, many +post-processing blocks (ISP, DRC, FD, ODC, DIS, 3DNR) and two +dedicated scalers (SCC and SCP). + +fimc-is node + + +Required properties: + +- compatible: must be samsung,exynos5250-fimc-is +- reg : physical base address and size of the memory mapped + registers +- interrupt-parent : Parent interrupt controller s/Parent/parent ? +- interrupts: fimc-is interrupt to the parent combiner +- clocks: list of clock specifiers, corresponding to entries in + clock-names property; +- clock-names : must contain isp, mcu_isp, isp_div0, isp_div1, + isp_divmpwm, mcu_isp_div0, mcu_isp_div1 entries, + matching entries in the clocks property. + +pmu subnode +--- + +Required properties: + - reg : should contain PMU physical base address and size of the memory + mapped registers. + +i2c-isp (ISP I2C bus controller) nodes +-- + +Required properties: + +- compatible : should be samsung,exynos4212-i2c-isp for Exynos4212, + Exynos4412 and Exynos5250 SoCs; +- reg : physical base address and length of the registers set; +- clocks : must contain gate clock specifier for this controller; +- clock-names : must contain i2c_isp entry. + +For the above nodes it is required to specify a pinctrl state named default, +according to the pinctrl bindings defined in ../pinctrl/pinctrl-bindings.txt. + +Device tree nodes of the image sensors' controlled directly by the FIMC-IS +firmware must be child nodes of their corresponding ISP I2C bus controller node. +The data link of these image sensors must be specified using the common video +interfaces bindings, defined in video-interfaces.txt. This one looks good to me. Feel free to add Reviewed-by: Sylwester Nawrocki s.nawro...@samsung.com I'm adding the Device Tree maintainers at Cc so we can possibly get their Ack. -- Thanks, Sylwester -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v3 02/13] [media] exynos5-fimc-is: Add Exynos5 FIMC-IS device tree bindings documentation
The patch adds the DT binding documentation for Samsung Exynos5 SoC series imaging subsystem (FIMC-IS). Signed-off-by: Arun Kumar K arun...@samsung.com --- .../devicetree/bindings/media/exynos5-fimc-is.txt | 52 1 file changed, 52 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/exynos5-fimc-is.txt diff --git a/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt new file mode 100644 index 000..49a373a --- /dev/null +++ b/Documentation/devicetree/bindings/media/exynos5-fimc-is.txt @@ -0,0 +1,52 @@ +Samsung EXYNOS5 SoC series Imaging Subsystem (FIMC-IS) +-- + +The camera subsystem on Samsung Exynos5 SoC has some changes relative +to previous SoC versions. Exynos5 has almost similar MIPI-CSIS and +FIMC-LITE IPs but has a much improved version of FIMC-IS which can +handle sensor controls and camera post-processing operations. The +Exynos5 FIMC-IS has a dedicated ARM Cortex A5 processor, many +post-processing blocks (ISP, DRC, FD, ODC, DIS, 3DNR) and two +dedicated scalers (SCC and SCP). + +fimc-is node + + +Required properties: + +- compatible: must be samsung,exynos5250-fimc-is +- reg : physical base address and size of the memory mapped + registers +- interrupt-parent : Parent interrupt controller +- interrupts: fimc-is interrupt to the parent combiner +- clocks: list of clock specifiers, corresponding to entries in + clock-names property; +- clock-names : must contain isp, mcu_isp, isp_div0, isp_div1, + isp_divmpwm, mcu_isp_div0, mcu_isp_div1 entries, + matching entries in the clocks property. + +pmu subnode +--- + +Required properties: + - reg : should contain PMU physical base address and size of the memory + mapped registers. + +i2c-isp (ISP I2C bus controller) nodes +-- + +Required properties: + +- compatible : should be samsung,exynos4212-i2c-isp for Exynos4212, + Exynos4412 and Exynos5250 SoCs; +- reg : physical base address and length of the registers set; +- clocks : must contain gate clock specifier for this controller; +- clock-names : must contain i2c_isp entry. + +For the above nodes it is required to specify a pinctrl state named default, +according to the pinctrl bindings defined in ../pinctrl/pinctrl-bindings.txt. + +Device tree nodes of the image sensors' controlled directly by the FIMC-IS +firmware must be child nodes of their corresponding ISP I2C bus controller node. +The data link of these image sensors must be specified using the common video +interfaces bindings, defined in video-interfaces.txt. -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
