| Message ID | 20200331133346.372517-2-robert.foss@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: ov8856: Add devicetree support | expand |
Hi Robert, On 20-03-31 15:33, Robert Foss wrote: > From: Dongchun Zhu <dongchun.zhu@mediatek.com> > > This patch adds documentation of device tree in YAML schema for the > OV8856 CMOS image sensor. > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > Signed-off-by: Robert Foss <robert.foss@linaro.org> > --- > > - Changes since v5: > * Add assigned-clocks and assigned-clock-rates > * robher: dt-schema errors > > - Changes since v4: > * Fabio: Change reset-gpio to GPIO_ACTIVE_LOW, explain in description > * Add clock-lanes property to example > * robher: Fix syntax error in devicetree example > > - Changes since v3: > * robher: Fix syntax error > * robher: Removed maxItems > * Fixes yaml 'make dt-binding-check' errors > > - Changes since v2: > Fixes comments from from Andy, Tomasz, Sakari, Rob. > * Convert text documentation to YAML schema. > > - Changes since v1: > Fixes comments from Sakari, Tomasz > * Add clock-frequency and link-frequencies in DT > > .../devicetree/bindings/media/i2c/ov8856.yaml | 150 ++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 151 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov8856.yaml b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml > new file mode 100644 > index 000000000000..beeddfbb8709 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml > @@ -0,0 +1,150 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +# Copyright (c) 2019 MediaTek Inc. > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/i2c/ov8856.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Omnivision OV8856 CMOS Sensor Device Tree Bindings > + > +maintainers: > + - Ben Kao <ben.kao@intel.com> > + - Dongchun Zhu <dongchun.zhu@mediatek.com> > + > +description: |- > + The Omnivision OV8856 is a high performance, 1/4-inch, 8 megapixel, CMOS > + image sensor that delivers 3264x2448 at 30fps. It provides full-frame, > + sub-sampled, and windowed 10-bit MIPI images in various formats via the > + Serial Camera Control Bus (SCCB) interface. This chip is programmable > + through I2C and two-wire SCCB. The sensor output is available via CSI-2 > + serial data output (up to 4-lane). > + > +properties: > + compatible: > + const: ovti,ov8856 > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + clock-names: > + description: > + Input clock for the sensor. > + items: > + - const: xvclk > + > + clock-frequency: > + description: > + Frequency of the xvclk clock in Hertz. Why do we need this here? > + assigned-clocks: > + description: > + Input clock for the sensor. > + > + assigned-clock-rates: > + description: > + Frequency of the xvclk clock in Hertz. Also this isn't related to the chip. You need this because you are using a qcom platform which provides the clock. IMHO you only need to specify the clock. You can get the frequency with the clk_get_rate() function. > + dovdd-supply: > + description: > + Definition of the regulator used as interface power supply. Phandle to the interface power supply regulator? > + > + avdd-supply: > + description: > + Definition of the regulator used as analog power supply. > + > + dvdd-supply: > + description: > + Definition of the regulator used as digital power supply. > + > + reset-gpios: > + description: > + The phandle and specifier for the GPIO that controls sensor reset. > + This corresponds to the hardware pin XSHUTDOWN which is physically > + active low. > + > + port: > + type: object > + additionalProperties: false > + description: > + A node containing input and output port nodes with endpoint definitions > + as documented in > + Documentation/devicetree/bindings/media/video-interfaces.txt > + > + properties: > + endpoint: > + type: object > + > + properties: > + clock-lanes: > + maxItems: 1 > + > + data-lanes: > + maxItems: 1 > + > + remote-endpoint: true > + > + required: > + - clock-lanes > + - data-lanes > + - remote-endpoint > + > + required: > + - endpoint > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + - clock-frequency > + - assigned-clocks > + - assigned-clock-rates > + - dovdd-supply > + - avdd-supply > + - dvdd-supply > + - reset-gpios > + - port > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + #include <dt-bindings/clock/qcom,camcc-sdm845.h> IMHO we should avoid examples with hardware specific includes. > + > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + ov8856: camera@10 { > + compatible = "ovti,ov8856"; > + reg = <0x10>; > + > + reset-gpios = <&pio 111 GPIO_ACTIVE_LOW>; > + pinctrl-names = "default"; > + pinctrl-0 = <&clk_24m_cam>; > + > + clocks = <&clock_camcc CAM_CC_MCLK0_CLK>; > + clock-names = "xvclk"; > + clock-frequency = <19200000>; > + assigned-clocks = <&clock_camcc CAM_CC_MCLK0_CLK>; > + assigned-clock-rates = <19200000>; > + > + avdd-supply = <&mt6358_vcama2_reg>; > + dvdd-supply = <&mt6358_vcamd_reg>; > + dovdd-supply = <&mt6358_vcamio_reg>; > + > + port { > + wcam_out: endpoint { > + remote-endpoint = <&mipi_in_wcam>; > + clock-lanes = <0>; > + data-lanes = <1 2 3 4>; > + link-frequencies = /bits/ 64 <360000000 180000000>; Should we add the link-frequencies as optional param? Regards, Marco > + }; > + }; > + }; > + }; > +... > \ No newline at end of file > diff --git a/MAINTAINERS b/MAINTAINERS > index a6fbdf354d34..0f99e863978a 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -12355,6 +12355,7 @@ L: linux-media@vger.kernel.org > T: git git://linuxtv.org/media_tree.git > S: Maintained > F: drivers/media/i2c/ov8856.c > +F: Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > OMNIVISION OV9650 SENSOR DRIVER > M: Sakari Ailus <sakari.ailus@linux.intel.com> > -- > 2.25.1 > >
Hi, On Tue, Mar 31, 2020 at 03:33:44PM +0200, Robert Foss wrote: > From: Dongchun Zhu <dongchun.zhu@mediatek.com> > > This patch adds documentation of device tree in YAML schema for the > OV8856 CMOS image sensor. > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > Signed-off-by: Robert Foss <robert.foss@linaro.org> > --- > > - Changes since v5: > * Add assigned-clocks and assigned-clock-rates > * robher: dt-schema errors > > - Changes since v4: > * Fabio: Change reset-gpio to GPIO_ACTIVE_LOW, explain in description > * Add clock-lanes property to example > * robher: Fix syntax error in devicetree example > > - Changes since v3: > * robher: Fix syntax error > * robher: Removed maxItems > * Fixes yaml 'make dt-binding-check' errors > > - Changes since v2: > Fixes comments from from Andy, Tomasz, Sakari, Rob. > * Convert text documentation to YAML schema. > > - Changes since v1: > Fixes comments from Sakari, Tomasz > * Add clock-frequency and link-frequencies in DT > > .../devicetree/bindings/media/i2c/ov8856.yaml | 150 ++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 151 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov8856.yaml b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml > new file mode 100644 > index 000000000000..beeddfbb8709 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml > @@ -0,0 +1,150 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +# Copyright (c) 2019 MediaTek Inc. > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/i2c/ov8856.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Omnivision OV8856 CMOS Sensor Device Tree Bindings > + > +maintainers: > + - Ben Kao <ben.kao@intel.com> > + - Dongchun Zhu <dongchun.zhu@mediatek.com> > + > +description: |- > + The Omnivision OV8856 is a high performance, 1/4-inch, 8 megapixel, CMOS > + image sensor that delivers 3264x2448 at 30fps. It provides full-frame, > + sub-sampled, and windowed 10-bit MIPI images in various formats via the > + Serial Camera Control Bus (SCCB) interface. This chip is programmable > + through I2C and two-wire SCCB. The sensor output is available via CSI-2 > + serial data output (up to 4-lane). > + > +properties: > + compatible: > + const: ovti,ov8856 > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + clock-names: > + description: > + Input clock for the sensor. > + items: > + - const: xvclk > + > + clock-frequency: > + description: > + Frequency of the xvclk clock in Hertz. We also had that discussion recently for another omnivision sensor (ov5645 iirc), but what is clock-frequency useful for? It seems that the sensor is passed in clocks, so if you need to retrieve the clock rate you should use the clock API instead. Looking at the driver, it looks like it first retrieves the clock, set it to clock-frequency, and then checks that this is OV8856_XVCLK_19_2 (19.2 MHz). The datasheet says that the sensor can have any frequency in the 6 - 27 MHz range, so this is a driver limitation and should be set in the driver using the clock API, and you can always bail out if it doesn't provide a rate that is not acceptable for the drivers assumption. In any case, you don't need clock-frequency here... > + assigned-clocks: > + description: > + Input clock for the sensor. > + > + assigned-clock-rates: > + description: > + Frequency of the xvclk clock in Hertz. And you don't need assigned-clock-rates either. Maxime
'Hey Marco, On Tue, 31 Mar 2020 at 17:13, Marco Felsch <m.felsch@pengutronix.de> wrote: > > Hi Robert, > > On 20-03-31 15:33, Robert Foss wrote: > > From: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > This patch adds documentation of device tree in YAML schema for the > > OV8856 CMOS image sensor. > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > Signed-off-by: Robert Foss <robert.foss@linaro.org> > > --- > > > > - Changes since v5: > > * Add assigned-clocks and assigned-clock-rates > > * robher: dt-schema errors > > > > - Changes since v4: > > * Fabio: Change reset-gpio to GPIO_ACTIVE_LOW, explain in description > > * Add clock-lanes property to example > > * robher: Fix syntax error in devicetree example > > > > - Changes since v3: > > * robher: Fix syntax error > > * robher: Removed maxItems > > * Fixes yaml 'make dt-binding-check' errors > > > > - Changes since v2: > > Fixes comments from from Andy, Tomasz, Sakari, Rob. > > * Convert text documentation to YAML schema. > > > > - Changes since v1: > > Fixes comments from Sakari, Tomasz > > * Add clock-frequency and link-frequencies in DT > > > > .../devicetree/bindings/media/i2c/ov8856.yaml | 150 ++++++++++++++++++ > > MAINTAINERS | 1 + > > 2 files changed, 151 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov8856.yaml b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > new file mode 100644 > > index 000000000000..beeddfbb8709 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > @@ -0,0 +1,150 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +# Copyright (c) 2019 MediaTek Inc. > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/i2c/ov8856.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Omnivision OV8856 CMOS Sensor Device Tree Bindings > > + > > +maintainers: > > + - Ben Kao <ben.kao@intel.com> > > + - Dongchun Zhu <dongchun.zhu@mediatek.com> > > + > > +description: |- > > + The Omnivision OV8856 is a high performance, 1/4-inch, 8 megapixel, CMOS > > + image sensor that delivers 3264x2448 at 30fps. It provides full-frame, > > + sub-sampled, and windowed 10-bit MIPI images in various formats via the > > + Serial Camera Control Bus (SCCB) interface. This chip is programmable > > + through I2C and two-wire SCCB. The sensor output is available via CSI-2 > > + serial data output (up to 4-lane). > > + > > +properties: > > + compatible: > > + const: ovti,ov8856 > > + > > + reg: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > + > > + clock-names: > > + description: > > + Input clock for the sensor. > > + items: > > + - const: xvclk > > + > > + clock-frequency: > > + description: > > + Frequency of the xvclk clock in Hertz. > > Why do we need this here? > > > + assigned-clocks: > > + description: > > + Input clock for the sensor. > > + > > + assigned-clock-rates: > > + description: > > + Frequency of the xvclk clock in Hertz. > > Also this isn't related to the chip. You need this because you are using > a qcom platform which provides the clock. > > IMHO you only need to specify the clock. You can get the frequency with > the clk_get_rate() function. The way I understood this, was that clk_get_rate() would fetch the clock rate as defined by the 'assigned-clock-rates' Is this not the case? And if so, what rate would cllk_get_rate() actually retrieve? > > > + dovdd-supply: > > + description: > > + Definition of the regulator used as interface power supply. > > Phandle to the interface power supply regulator? > > > + > > + avdd-supply: > > + description: > > + Definition of the regulator used as analog power supply. > > + > > + dvdd-supply: > > + description: > > + Definition of the regulator used as digital power supply. > > + > > + reset-gpios: > > + description: > > + The phandle and specifier for the GPIO that controls sensor reset. > > + This corresponds to the hardware pin XSHUTDOWN which is physically > > + active low. > > + > > + port: > > + type: object > > + additionalProperties: false > > + description: > > + A node containing input and output port nodes with endpoint definitions > > + as documented in > > + Documentation/devicetree/bindings/media/video-interfaces.txt > > + > > + properties: > > + endpoint: > > + type: object > > + > > + properties: > > + clock-lanes: > > + maxItems: 1 > > + > > + data-lanes: > > + maxItems: 1 > > + > > + remote-endpoint: true > > + > > + required: > > + - clock-lanes > > + - data-lanes > > + - remote-endpoint > > + > > + required: > > + - endpoint > > + > > +required: > > + - compatible > > + - reg > > + - clocks > > + - clock-names > > + - clock-frequency > > + - assigned-clocks > > + - assigned-clock-rates > > + - dovdd-supply > > + - avdd-supply > > + - dvdd-supply > > + - reset-gpios > > + - port > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/gpio/gpio.h> > > + #include <dt-bindings/clock/qcom,camcc-sdm845.h> > > IMHO we should avoid examples with hardware specific includes. The HW specific include is used for the clocks property. clocks = <&clock_camcc CAM_CC_MCLK0_CLK>; Is there a non hw specific clock that would be better to use for examples? > > > + > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + ov8856: camera@10 { > > + compatible = "ovti,ov8856"; > > + reg = <0x10>; > > + > > + reset-gpios = <&pio 111 GPIO_ACTIVE_LOW>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&clk_24m_cam>; > > + > > + clocks = <&clock_camcc CAM_CC_MCLK0_CLK>; > > + clock-names = "xvclk"; > > + clock-frequency = <19200000>; > > + assigned-clocks = <&clock_camcc CAM_CC_MCLK0_CLK>; > > + assigned-clock-rates = <19200000>; > > + > > + avdd-supply = <&mt6358_vcama2_reg>; > > + dvdd-supply = <&mt6358_vcamd_reg>; > > + dovdd-supply = <&mt6358_vcamio_reg>; > > + > > + port { > > + wcam_out: endpoint { > > + remote-endpoint = <&mipi_in_wcam>; > > + clock-lanes = <0>; > > + data-lanes = <1 2 3 4>; > > + link-frequencies = /bits/ 64 <360000000 180000000>; > > Should we add the link-frequencies as optional param? > > Regards, > Marco > > > + }; > > + }; > > + }; > > + }; > > +... > > \ No newline at end of file > > diff --git a/MAINTAINERS b/MAINTAINERS > > index a6fbdf354d34..0f99e863978a 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -12355,6 +12355,7 @@ L: linux-media@vger.kernel.org > > T: git git://linuxtv.org/media_tree.git > > S: Maintained > > F: drivers/media/i2c/ov8856.c > > +F: Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > > > OMNIVISION OV9650 SENSOR DRIVER > > M: Sakari Ailus <sakari.ailus@linux.intel.com> > > -- > > 2.25.1 > > > > > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hey Maxime, On Wed, 1 Apr 2020 at 10:07, Maxime Ripard <maxime@cerno.tech> wrote: > > Hi, > > On Tue, Mar 31, 2020 at 03:33:44PM +0200, Robert Foss wrote: > > From: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > This patch adds documentation of device tree in YAML schema for the > > OV8856 CMOS image sensor. > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > Signed-off-by: Robert Foss <robert.foss@linaro.org> > > --- > > > > - Changes since v5: > > * Add assigned-clocks and assigned-clock-rates > > * robher: dt-schema errors > > > > - Changes since v4: > > * Fabio: Change reset-gpio to GPIO_ACTIVE_LOW, explain in description > > * Add clock-lanes property to example > > * robher: Fix syntax error in devicetree example > > > > - Changes since v3: > > * robher: Fix syntax error > > * robher: Removed maxItems > > * Fixes yaml 'make dt-binding-check' errors > > > > - Changes since v2: > > Fixes comments from from Andy, Tomasz, Sakari, Rob. > > * Convert text documentation to YAML schema. > > > > - Changes since v1: > > Fixes comments from Sakari, Tomasz > > * Add clock-frequency and link-frequencies in DT > > > > .../devicetree/bindings/media/i2c/ov8856.yaml | 150 ++++++++++++++++++ > > MAINTAINERS | 1 + > > 2 files changed, 151 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov8856.yaml b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > new file mode 100644 > > index 000000000000..beeddfbb8709 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > @@ -0,0 +1,150 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +# Copyright (c) 2019 MediaTek Inc. > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/i2c/ov8856.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Omnivision OV8856 CMOS Sensor Device Tree Bindings > > + > > +maintainers: > > + - Ben Kao <ben.kao@intel.com> > > + - Dongchun Zhu <dongchun.zhu@mediatek.com> > > + > > +description: |- > > + The Omnivision OV8856 is a high performance, 1/4-inch, 8 megapixel, CMOS > > + image sensor that delivers 3264x2448 at 30fps. It provides full-frame, > > + sub-sampled, and windowed 10-bit MIPI images in various formats via the > > + Serial Camera Control Bus (SCCB) interface. This chip is programmable > > + through I2C and two-wire SCCB. The sensor output is available via CSI-2 > > + serial data output (up to 4-lane). > > + > > +properties: > > + compatible: > > + const: ovti,ov8856 > > + > > + reg: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > + > > + clock-names: > > + description: > > + Input clock for the sensor. > > + items: > > + - const: xvclk > > + > > + clock-frequency: > > + description: > > + Frequency of the xvclk clock in Hertz. > > We also had that discussion recently for another omnivision sensor > (ov5645 iirc), but what is clock-frequency useful for? > > It seems that the sensor is passed in clocks, so if you need to > retrieve the clock rate you should use the clock API instead. > > Looking at the driver, it looks like it first retrieves the clock, set > it to clock-frequency, and then checks that this is OV8856_XVCLK_19_2 > (19.2 MHz). As far as I understand it, 19.2MHz is requirement for the sensor mode that currently defaults to. Some modes require higher clock speeds than this however. > > The datasheet says that the sensor can have any frequency in the 6 - > 27 MHz range, so this is a driver limitation and should be set in the > driver using the clock API, and you can always bail out if it doesn't > provide a rate that is not acceptable for the drivers assumption. > > In any case, you don't need clock-frequency here... So your suggestion is that we remove all clocks-rate properties, and replace the clk_get_rate() calls in the driver with clk_set_rate() calls for the desired frequencies? > > > + assigned-clocks: > > + description: > > + Input clock for the sensor. > > + > > + assigned-clock-rates: > > + description: > > + Frequency of the xvclk clock in Hertz. > > And you don't need assigned-clock-rates either. Ack. > > Maxime
Hi Robert, On 20-04-02 11:57, Robert Foss wrote: > 'Hey Marco, > > On Tue, 31 Mar 2020 at 17:13, Marco Felsch <m.felsch@pengutronix.de> wrote: ... > > > + assigned-clocks: > > > + description: > > > + Input clock for the sensor. > > > + > > > + assigned-clock-rates: > > > + description: > > > + Frequency of the xvclk clock in Hertz. > > > > Also this isn't related to the chip. You need this because you are using > > a qcom platform which provides the clock. > > > > IMHO you only need to specify the clock. You can get the frequency with > > the clk_get_rate() function. > > The way I understood this, was that clk_get_rate() would fetch the > clock rate as defined by the 'assigned-clock-rates' > Is this not the case? And if so, what rate would cllk_get_rate() > actually retrieve? > You're right clk_get_rate() should be used to get the current clk rate but assigned-cock-rates only applies to your setup where the host supplies the clk. This is not the case ff we connect a simple external osc which provides a static (not adjustable) ckl. ... > > > +examples: > > > + - | > > > + #include <dt-bindings/gpio/gpio.h> > > > + #include <dt-bindings/clock/qcom,camcc-sdm845.h> > > > > IMHO we should avoid examples with hardware specific includes. > > The HW specific include is used for the clocks property. > clocks = <&clock_camcc CAM_CC_MCLK0_CLK>; > > Is there a non hw specific clock that would be better to use for examples? Yes, just use: clocks = <&cam_osc>; The dt-validation provides dummy hooks for such phandles. Regards, Marco > > > > > > + > > > + i2c { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + ov8856: camera@10 { > > > + compatible = "ovti,ov8856"; > > > + reg = <0x10>; > > > + > > > + reset-gpios = <&pio 111 GPIO_ACTIVE_LOW>; > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&clk_24m_cam>; > > > + > > > + clocks = <&clock_camcc CAM_CC_MCLK0_CLK>; > > > + clock-names = "xvclk"; > > > + clock-frequency = <19200000>; > > > + assigned-clocks = <&clock_camcc CAM_CC_MCLK0_CLK>; > > > + assigned-clock-rates = <19200000>; > > > + > > > + avdd-supply = <&mt6358_vcama2_reg>; > > > + dvdd-supply = <&mt6358_vcamd_reg>; > > > + dovdd-supply = <&mt6358_vcamio_reg>; > > > + > > > + port { > > > + wcam_out: endpoint { > > > + remote-endpoint = <&mipi_in_wcam>; > > > + clock-lanes = <0>; > > > + data-lanes = <1 2 3 4>; > > > + link-frequencies = /bits/ 64 <360000000 180000000>; > > > > Should we add the link-frequencies as optional param? > > > > Regards, > > Marco > > > > > + }; > > > + }; > > > + }; > > > + }; > > > +... > > > \ No newline at end of file > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > index a6fbdf354d34..0f99e863978a 100644 > > > --- a/MAINTAINERS > > > +++ b/MAINTAINERS > > > @@ -12355,6 +12355,7 @@ L: linux-media@vger.kernel.org > > > T: git git://linuxtv.org/media_tree.git > > > S: Maintained > > > F: drivers/media/i2c/ov8856.c > > > +F: Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > > > > > OMNIVISION OV9650 SENSOR DRIVER > > > M: Sakari Ailus <sakari.ailus@linux.intel.com> > > > -- > > > 2.25.1 > > > > > > > > > > -- > > Pengutronix e.K. | | > > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | >
Hi Robert, On Thu, Apr 02, 2020 at 12:10:00PM +0200, Robert Foss wrote: > Hey Maxime, > > On Wed, 1 Apr 2020 at 10:07, Maxime Ripard <maxime@cerno.tech> wrote: > > > > Hi, > > > > On Tue, Mar 31, 2020 at 03:33:44PM +0200, Robert Foss wrote: > > > From: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > > > This patch adds documentation of device tree in YAML schema for the > > > OV8856 CMOS image sensor. > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > Signed-off-by: Robert Foss <robert.foss@linaro.org> > > > --- > > > > > > - Changes since v5: > > > * Add assigned-clocks and assigned-clock-rates > > > * robher: dt-schema errors > > > > > > - Changes since v4: > > > * Fabio: Change reset-gpio to GPIO_ACTIVE_LOW, explain in description > > > * Add clock-lanes property to example > > > * robher: Fix syntax error in devicetree example > > > > > > - Changes since v3: > > > * robher: Fix syntax error > > > * robher: Removed maxItems > > > * Fixes yaml 'make dt-binding-check' errors > > > > > > - Changes since v2: > > > Fixes comments from from Andy, Tomasz, Sakari, Rob. > > > * Convert text documentation to YAML schema. > > > > > > - Changes since v1: > > > Fixes comments from Sakari, Tomasz > > > * Add clock-frequency and link-frequencies in DT > > > > > > .../devicetree/bindings/media/i2c/ov8856.yaml | 150 ++++++++++++++++++ > > > MAINTAINERS | 1 + > > > 2 files changed, 151 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov8856.yaml b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > > new file mode 100644 > > > index 000000000000..beeddfbb8709 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > > @@ -0,0 +1,150 @@ > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > > +# Copyright (c) 2019 MediaTek Inc. > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/media/i2c/ov8856.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Omnivision OV8856 CMOS Sensor Device Tree Bindings > > > + > > > +maintainers: > > > + - Ben Kao <ben.kao@intel.com> > > > + - Dongchun Zhu <dongchun.zhu@mediatek.com> > > > + > > > +description: |- > > > + The Omnivision OV8856 is a high performance, 1/4-inch, 8 megapixel, CMOS > > > + image sensor that delivers 3264x2448 at 30fps. It provides full-frame, > > > + sub-sampled, and windowed 10-bit MIPI images in various formats via the > > > + Serial Camera Control Bus (SCCB) interface. This chip is programmable > > > + through I2C and two-wire SCCB. The sensor output is available via CSI-2 > > > + serial data output (up to 4-lane). > > > + > > > +properties: > > > + compatible: > > > + const: ovti,ov8856 > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + clocks: > > > + maxItems: 1 > > > + > > > + clock-names: > > > + description: > > > + Input clock for the sensor. > > > + items: > > > + - const: xvclk > > > + > > > + clock-frequency: > > > + description: > > > + Frequency of the xvclk clock in Hertz. > > > > We also had that discussion recently for another omnivision sensor > > (ov5645 iirc), but what is clock-frequency useful for? > > > > It seems that the sensor is passed in clocks, so if you need to > > retrieve the clock rate you should use the clock API instead. > > > > Looking at the driver, it looks like it first retrieves the clock, set > > it to clock-frequency, and then checks that this is OV8856_XVCLK_19_2 > > (19.2 MHz). > > As far as I understand it, 19.2MHz is requirement for the sensor mode > that currently defaults to. Some modes require higher clock speeds > than this however. It's very system specific. Either way, bindings should not assume a particular driver implementation. > > > > > The datasheet says that the sensor can have any frequency in the 6 - > > 27 MHz range, so this is a driver limitation and should be set in the > > driver using the clock API, and you can always bail out if it doesn't > > provide a rate that is not acceptable for the drivers assumption. > > > > In any case, you don't need clock-frequency here... > > So your suggestion is that we remove all clocks-rate properties, and > replace the clk_get_rate() calls in the driver with clk_set_rate() > calls for the desired frequencies? The driver shouldn't set the rate here unless it gets it from DT (but that was not the intention). So the driver should get the frequency instead.
Hi Robert, On Thu, Apr 02, 2020 at 12:10:00PM +0200, Robert Foss wrote: > On Wed, 1 Apr 2020 at 10:07, Maxime Ripard <maxime@cerno.tech> wrote: > > On Tue, Mar 31, 2020 at 03:33:44PM +0200, Robert Foss wrote: > > > From: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > > > This patch adds documentation of device tree in YAML schema for the > > > OV8856 CMOS image sensor. > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > Signed-off-by: Robert Foss <robert.foss@linaro.org> > > > --- > > > > > > - Changes since v5: > > > * Add assigned-clocks and assigned-clock-rates > > > * robher: dt-schema errors > > > > > > - Changes since v4: > > > * Fabio: Change reset-gpio to GPIO_ACTIVE_LOW, explain in description > > > * Add clock-lanes property to example > > > * robher: Fix syntax error in devicetree example > > > > > > - Changes since v3: > > > * robher: Fix syntax error > > > * robher: Removed maxItems > > > * Fixes yaml 'make dt-binding-check' errors > > > > > > - Changes since v2: > > > Fixes comments from from Andy, Tomasz, Sakari, Rob. > > > * Convert text documentation to YAML schema. > > > > > > - Changes since v1: > > > Fixes comments from Sakari, Tomasz > > > * Add clock-frequency and link-frequencies in DT > > > > > > .../devicetree/bindings/media/i2c/ov8856.yaml | 150 ++++++++++++++++++ > > > MAINTAINERS | 1 + > > > 2 files changed, 151 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov8856.yaml b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > > new file mode 100644 > > > index 000000000000..beeddfbb8709 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > > @@ -0,0 +1,150 @@ > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > > +# Copyright (c) 2019 MediaTek Inc. > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/media/i2c/ov8856.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Omnivision OV8856 CMOS Sensor Device Tree Bindings > > > + > > > +maintainers: > > > + - Ben Kao <ben.kao@intel.com> > > > + - Dongchun Zhu <dongchun.zhu@mediatek.com> > > > + > > > +description: |- > > > + The Omnivision OV8856 is a high performance, 1/4-inch, 8 megapixel, CMOS > > > + image sensor that delivers 3264x2448 at 30fps. It provides full-frame, > > > + sub-sampled, and windowed 10-bit MIPI images in various formats via the > > > + Serial Camera Control Bus (SCCB) interface. This chip is programmable > > > + through I2C and two-wire SCCB. The sensor output is available via CSI-2 > > > + serial data output (up to 4-lane). > > > + > > > +properties: > > > + compatible: > > > + const: ovti,ov8856 > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + clocks: > > > + maxItems: 1 > > > + > > > + clock-names: > > > + description: > > > + Input clock for the sensor. > > > + items: > > > + - const: xvclk > > > + > > > + clock-frequency: > > > + description: > > > + Frequency of the xvclk clock in Hertz. > > > > We also had that discussion recently for another omnivision sensor > > (ov5645 iirc), but what is clock-frequency useful for? > > > > It seems that the sensor is passed in clocks, so if you need to > > retrieve the clock rate you should use the clock API instead. > > > > Looking at the driver, it looks like it first retrieves the clock, set > > it to clock-frequency, and then checks that this is OV8856_XVCLK_19_2 > > (19.2 MHz). > > As far as I understand it, 19.2MHz is requirement for the sensor mode > that currently defaults to. Some modes require higher clock speeds > than this however. > > > > > The datasheet says that the sensor can have any frequency in the 6 - > > 27 MHz range, so this is a driver limitation and should be set in the > > driver using the clock API, and you can always bail out if it doesn't > > provide a rate that is not acceptable for the drivers assumption. > > > > In any case, you don't need clock-frequency here... > > So your suggestion is that we remove all clocks-rate properties, and > replace the clk_get_rate() calls in the driver with clk_set_rate() > calls for the desired frequencies? Yep, and if you need to make sure that the frequency that your provider rounded to is matching 19.2MHz like you were doing, then you can throw a clk_get_rate after it. It seems overly-restrictive to me, but the device might be picky Maxime
Hi, On Sat, Apr 04, 2020 at 02:27:36AM +0300, Sakari Ailus wrote: > Hi Robert, > > On Thu, Apr 02, 2020 at 12:10:00PM +0200, Robert Foss wrote: > > Hey Maxime, > > > > On Wed, 1 Apr 2020 at 10:07, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > Hi, > > > > > > On Tue, Mar 31, 2020 at 03:33:44PM +0200, Robert Foss wrote: > > > > From: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > > > > > This patch adds documentation of device tree in YAML schema for the > > > > OV8856 CMOS image sensor. > > > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > Signed-off-by: Robert Foss <robert.foss@linaro.org> > > > > --- > > > > > > > > - Changes since v5: > > > > * Add assigned-clocks and assigned-clock-rates > > > > * robher: dt-schema errors > > > > > > > > - Changes since v4: > > > > * Fabio: Change reset-gpio to GPIO_ACTIVE_LOW, explain in description > > > > * Add clock-lanes property to example > > > > * robher: Fix syntax error in devicetree example > > > > > > > > - Changes since v3: > > > > * robher: Fix syntax error > > > > * robher: Removed maxItems > > > > * Fixes yaml 'make dt-binding-check' errors > > > > > > > > - Changes since v2: > > > > Fixes comments from from Andy, Tomasz, Sakari, Rob. > > > > * Convert text documentation to YAML schema. > > > > > > > > - Changes since v1: > > > > Fixes comments from Sakari, Tomasz > > > > * Add clock-frequency and link-frequencies in DT > > > > > > > > .../devicetree/bindings/media/i2c/ov8856.yaml | 150 ++++++++++++++++++ > > > > MAINTAINERS | 1 + > > > > 2 files changed, 151 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov8856.yaml b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > > > new file mode 100644 > > > > index 000000000000..beeddfbb8709 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > > > @@ -0,0 +1,150 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > > > +# Copyright (c) 2019 MediaTek Inc. > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/media/i2c/ov8856.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Omnivision OV8856 CMOS Sensor Device Tree Bindings > > > > + > > > > +maintainers: > > > > + - Ben Kao <ben.kao@intel.com> > > > > + - Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > + > > > > +description: |- > > > > + The Omnivision OV8856 is a high performance, 1/4-inch, 8 megapixel, CMOS > > > > + image sensor that delivers 3264x2448 at 30fps. It provides full-frame, > > > > + sub-sampled, and windowed 10-bit MIPI images in various formats via the > > > > + Serial Camera Control Bus (SCCB) interface. This chip is programmable > > > > + through I2C and two-wire SCCB. The sensor output is available via CSI-2 > > > > + serial data output (up to 4-lane). > > > > + > > > > +properties: > > > > + compatible: > > > > + const: ovti,ov8856 > > > > + > > > > + reg: > > > > + maxItems: 1 > > > > + > > > > + clocks: > > > > + maxItems: 1 > > > > + > > > > + clock-names: > > > > + description: > > > > + Input clock for the sensor. > > > > + items: > > > > + - const: xvclk > > > > + > > > > + clock-frequency: > > > > + description: > > > > + Frequency of the xvclk clock in Hertz. > > > > > > We also had that discussion recently for another omnivision sensor > > > (ov5645 iirc), but what is clock-frequency useful for? > > > > > > It seems that the sensor is passed in clocks, so if you need to > > > retrieve the clock rate you should use the clock API instead. > > > > > > Looking at the driver, it looks like it first retrieves the clock, set > > > it to clock-frequency, and then checks that this is OV8856_XVCLK_19_2 > > > (19.2 MHz). > > > > As far as I understand it, 19.2MHz is requirement for the sensor mode > > that currently defaults to. Some modes require higher clock speeds > > than this however. > > It's very system specific. Either way, bindings should not assume a > particular driver implementation. > > > > > > > > > The datasheet says that the sensor can have any frequency in the 6 - > > > 27 MHz range, so this is a driver limitation and should be set in the > > > driver using the clock API, and you can always bail out if it doesn't > > > provide a rate that is not acceptable for the drivers assumption. > > > > > > In any case, you don't need clock-frequency here... > > > > So your suggestion is that we remove all clocks-rate properties, and > > replace the clk_get_rate() calls in the driver with clk_set_rate() > > calls for the desired frequencies? > > The driver shouldn't set the rate here unless it gets it from DT (but that > was not the intention). So the driver should get the frequency instead. I'm actually saying the opposite :) Like you were saying, the binding (or DT, for that matter) shouldn't assume a particular driver implementation. So one corollary is that if the driver has some restrictions in Linux, it shouldn't be part of the binding, right? This binding uses multiple clock properties but as far as I can see, the driver retrieves a clock using clocks and makes sure that its rate match its limitation of 19.2MHz using clock-frequency (which is redundant on a clk_get_rate on the clocks provided earlier). I'm suspecting that the parent clock on multiple SoCs can be configured and is not a fixed rate crystal, so assigned-clocks-rate is here just to make sure we set the frequency at the one being checked in the driver's probe so that it all works. But that 19.2MHz is not a limitation of the device itself, it's a limitation of our implementation, so we can instead implement something equivalent in Linux using a clk_set_rate to 19.2MHz (to make sure that our parent clock is configured at the right rate) and the clk_get_rate and compare that to 19.2MHz (to make sure that it's not been rounded too far apart from the frequency we expect). This is doing exactly the same thing, except that we don't encode our implementation limitations in the DT, but in the driver instead. Maxime
Hey Maxime, On Sat, 4 Apr 2020 at 11:34, Maxime Ripard <maxime@cerno.tech> wrote: > > Hi, > > On Sat, Apr 04, 2020 at 02:27:36AM +0300, Sakari Ailus wrote: > > Hi Robert, > > > > On Thu, Apr 02, 2020 at 12:10:00PM +0200, Robert Foss wrote: > > > Hey Maxime, > > > > > > On Wed, 1 Apr 2020 at 10:07, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > Hi, > > > > > > > > On Tue, Mar 31, 2020 at 03:33:44PM +0200, Robert Foss wrote: > > > > > From: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > > > > > > > This patch adds documentation of device tree in YAML schema for the > > > > > OV8856 CMOS image sensor. > > > > > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > > Signed-off-by: Robert Foss <robert.foss@linaro.org> > > > > > --- > > > > > > > > > > - Changes since v5: > > > > > * Add assigned-clocks and assigned-clock-rates > > > > > * robher: dt-schema errors > > > > > > > > > > - Changes since v4: > > > > > * Fabio: Change reset-gpio to GPIO_ACTIVE_LOW, explain in description > > > > > * Add clock-lanes property to example > > > > > * robher: Fix syntax error in devicetree example > > > > > > > > > > - Changes since v3: > > > > > * robher: Fix syntax error > > > > > * robher: Removed maxItems > > > > > * Fixes yaml 'make dt-binding-check' errors > > > > > > > > > > - Changes since v2: > > > > > Fixes comments from from Andy, Tomasz, Sakari, Rob. > > > > > * Convert text documentation to YAML schema. > > > > > > > > > > - Changes since v1: > > > > > Fixes comments from Sakari, Tomasz > > > > > * Add clock-frequency and link-frequencies in DT > > > > > > > > > > .../devicetree/bindings/media/i2c/ov8856.yaml | 150 ++++++++++++++++++ > > > > > MAINTAINERS | 1 + > > > > > 2 files changed, 151 insertions(+) > > > > > create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov8856.yaml b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > > > > new file mode 100644 > > > > > index 000000000000..beeddfbb8709 > > > > > --- /dev/null > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > > > > @@ -0,0 +1,150 @@ > > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > > > > +# Copyright (c) 2019 MediaTek Inc. > > > > > +%YAML 1.2 > > > > > +--- > > > > > +$id: http://devicetree.org/schemas/media/i2c/ov8856.yaml# > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > > + > > > > > +title: Omnivision OV8856 CMOS Sensor Device Tree Bindings > > > > > + > > > > > +maintainers: > > > > > + - Ben Kao <ben.kao@intel.com> > > > > > + - Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > > + > > > > > +description: |- > > > > > + The Omnivision OV8856 is a high performance, 1/4-inch, 8 megapixel, CMOS > > > > > + image sensor that delivers 3264x2448 at 30fps. It provides full-frame, > > > > > + sub-sampled, and windowed 10-bit MIPI images in various formats via the > > > > > + Serial Camera Control Bus (SCCB) interface. This chip is programmable > > > > > + through I2C and two-wire SCCB. The sensor output is available via CSI-2 > > > > > + serial data output (up to 4-lane). > > > > > + > > > > > +properties: > > > > > + compatible: > > > > > + const: ovti,ov8856 > > > > > + > > > > > + reg: > > > > > + maxItems: 1 > > > > > + > > > > > + clocks: > > > > > + maxItems: 1 > > > > > + > > > > > + clock-names: > > > > > + description: > > > > > + Input clock for the sensor. > > > > > + items: > > > > > + - const: xvclk > > > > > + > > > > > + clock-frequency: > > > > > + description: > > > > > + Frequency of the xvclk clock in Hertz. > > > > > > > > We also had that discussion recently for another omnivision sensor > > > > (ov5645 iirc), but what is clock-frequency useful for? > > > > > > > > It seems that the sensor is passed in clocks, so if you need to > > > > retrieve the clock rate you should use the clock API instead. > > > > > > > > Looking at the driver, it looks like it first retrieves the clock, set > > > > it to clock-frequency, and then checks that this is OV8856_XVCLK_19_2 > > > > (19.2 MHz). > > > > > > As far as I understand it, 19.2MHz is requirement for the sensor mode > > > that currently defaults to. Some modes require higher clock speeds > > > than this however. > > > > It's very system specific. Either way, bindings should not assume a > > particular driver implementation. > > > > > > > > > > > > > The datasheet says that the sensor can have any frequency in the 6 - > > > > 27 MHz range, so this is a driver limitation and should be set in the > > > > driver using the clock API, and you can always bail out if it doesn't > > > > provide a rate that is not acceptable for the drivers assumption. > > > > > > > > In any case, you don't need clock-frequency here... > > > > > > So your suggestion is that we remove all clocks-rate properties, and > > > replace the clk_get_rate() calls in the driver with clk_set_rate() > > > calls for the desired frequencies? > > > > The driver shouldn't set the rate here unless it gets it from DT (but that > > was not the intention). So the driver should get the frequency instead. > > I'm actually saying the opposite :) > > Like you were saying, the binding (or DT, for that matter) shouldn't > assume a particular driver implementation. > > So one corollary is that if the driver has some restrictions in Linux, > it shouldn't be part of the binding, right? > > This binding uses multiple clock properties but as far as I can see, > the driver retrieves a clock using clocks and makes sure that its rate > match its limitation of 19.2MHz using clock-frequency (which is > redundant on a clk_get_rate on the clocks provided earlier). > > I'm suspecting that the parent clock on multiple SoCs can be > configured and is not a fixed rate crystal, so assigned-clocks-rate is > here just to make sure we set the frequency at the one being checked > in the driver's probe so that it all works. > > But that 19.2MHz is not a limitation of the device itself, it's a > limitation of our implementation, so we can instead implement > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make > sure that our parent clock is configured at the right rate) and the > clk_get_rate and compare that to 19.2MHz (to make sure that it's not > been rounded too far apart from the frequency we expect). > > This is doing exactly the same thing, except that we don't encode our > implementation limitations in the DT, but in the driver instead. > Thanks for taking the time to explain this. I'll spin a new revision that moves the clock rate handling into the driver.
Hi Maxime, On Sat, Apr 04, 2020 at 11:34:46AM +0200, Maxime Ripard wrote: > Hi, > > On Sat, Apr 04, 2020 at 02:27:36AM +0300, Sakari Ailus wrote: > > Hi Robert, > > > > On Thu, Apr 02, 2020 at 12:10:00PM +0200, Robert Foss wrote: > > > Hey Maxime, > > > > > > On Wed, 1 Apr 2020 at 10:07, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > Hi, > > > > > > > > On Tue, Mar 31, 2020 at 03:33:44PM +0200, Robert Foss wrote: > > > > > From: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > > > > > > > This patch adds documentation of device tree in YAML schema for the > > > > > OV8856 CMOS image sensor. > > > > > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > > Signed-off-by: Robert Foss <robert.foss@linaro.org> > > > > > --- > > > > > > > > > > - Changes since v5: > > > > > * Add assigned-clocks and assigned-clock-rates > > > > > * robher: dt-schema errors > > > > > > > > > > - Changes since v4: > > > > > * Fabio: Change reset-gpio to GPIO_ACTIVE_LOW, explain in description > > > > > * Add clock-lanes property to example > > > > > * robher: Fix syntax error in devicetree example > > > > > > > > > > - Changes since v3: > > > > > * robher: Fix syntax error > > > > > * robher: Removed maxItems > > > > > * Fixes yaml 'make dt-binding-check' errors > > > > > > > > > > - Changes since v2: > > > > > Fixes comments from from Andy, Tomasz, Sakari, Rob. > > > > > * Convert text documentation to YAML schema. > > > > > > > > > > - Changes since v1: > > > > > Fixes comments from Sakari, Tomasz > > > > > * Add clock-frequency and link-frequencies in DT > > > > > > > > > > .../devicetree/bindings/media/i2c/ov8856.yaml | 150 ++++++++++++++++++ > > > > > MAINTAINERS | 1 + > > > > > 2 files changed, 151 insertions(+) > > > > > create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov8856.yaml b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > > > > new file mode 100644 > > > > > index 000000000000..beeddfbb8709 > > > > > --- /dev/null > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml > > > > > @@ -0,0 +1,150 @@ > > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > > > > +# Copyright (c) 2019 MediaTek Inc. > > > > > +%YAML 1.2 > > > > > +--- > > > > > +$id: http://devicetree.org/schemas/media/i2c/ov8856.yaml# > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > > + > > > > > +title: Omnivision OV8856 CMOS Sensor Device Tree Bindings > > > > > + > > > > > +maintainers: > > > > > + - Ben Kao <ben.kao@intel.com> > > > > > + - Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > > + > > > > > +description: |- > > > > > + The Omnivision OV8856 is a high performance, 1/4-inch, 8 megapixel, CMOS > > > > > + image sensor that delivers 3264x2448 at 30fps. It provides full-frame, > > > > > + sub-sampled, and windowed 10-bit MIPI images in various formats via the > > > > > + Serial Camera Control Bus (SCCB) interface. This chip is programmable > > > > > + through I2C and two-wire SCCB. The sensor output is available via CSI-2 > > > > > + serial data output (up to 4-lane). > > > > > + > > > > > +properties: > > > > > + compatible: > > > > > + const: ovti,ov8856 > > > > > + > > > > > + reg: > > > > > + maxItems: 1 > > > > > + > > > > > + clocks: > > > > > + maxItems: 1 > > > > > + > > > > > + clock-names: > > > > > + description: > > > > > + Input clock for the sensor. > > > > > + items: > > > > > + - const: xvclk > > > > > + > > > > > + clock-frequency: > > > > > + description: > > > > > + Frequency of the xvclk clock in Hertz. > > > > > > > > We also had that discussion recently for another omnivision sensor > > > > (ov5645 iirc), but what is clock-frequency useful for? > > > > > > > > It seems that the sensor is passed in clocks, so if you need to > > > > retrieve the clock rate you should use the clock API instead. > > > > > > > > Looking at the driver, it looks like it first retrieves the clock, set > > > > it to clock-frequency, and then checks that this is OV8856_XVCLK_19_2 > > > > (19.2 MHz). > > > > > > As far as I understand it, 19.2MHz is requirement for the sensor mode > > > that currently defaults to. Some modes require higher clock speeds > > > than this however. > > > > It's very system specific. Either way, bindings should not assume a > > particular driver implementation. > > > > > > > > > > > > > The datasheet says that the sensor can have any frequency in the 6 - > > > > 27 MHz range, so this is a driver limitation and should be set in the > > > > driver using the clock API, and you can always bail out if it doesn't > > > > provide a rate that is not acceptable for the drivers assumption. > > > > > > > > In any case, you don't need clock-frequency here... > > > > > > So your suggestion is that we remove all clocks-rate properties, and > > > replace the clk_get_rate() calls in the driver with clk_set_rate() > > > calls for the desired frequencies? > > > > The driver shouldn't set the rate here unless it gets it from DT (but that > > was not the intention). So the driver should get the frequency instead. > > I'm actually saying the opposite :) > > Like you were saying, the binding (or DT, for that matter) shouldn't > assume a particular driver implementation. > > So one corollary is that if the driver has some restrictions in Linux, > it shouldn't be part of the binding, right? Correct. > > This binding uses multiple clock properties but as far as I can see, > the driver retrieves a clock using clocks and makes sure that its rate > match its limitation of 19.2MHz using clock-frequency (which is > redundant on a clk_get_rate on the clocks provided earlier). > > I'm suspecting that the parent clock on multiple SoCs can be > configured and is not a fixed rate crystal, so assigned-clocks-rate is > here just to make sure we set the frequency at the one being checked > in the driver's probe so that it all works. Agreed. > > But that 19.2MHz is not a limitation of the device itself, it's a > limitation of our implementation, so we can instead implement > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make > sure that our parent clock is configured at the right rate) and the > clk_get_rate and compare that to 19.2MHz (to make sure that it's not > been rounded too far apart from the frequency we expect). > > This is doing exactly the same thing, except that we don't encode our > implementation limitations in the DT, but in the driver instead. What I really wanted to say that a driver that doesn't get the clock frequency from DT but still sets that frequency is broken. This frequency is highly system specific, and in many cases only a certain frequency is usable, for a few reasons: On many SoCs, not all common frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others are being used as well), and then that frequency affects the usable CSI-2 bus frequencies directly --- and of those, only safe, known-good ones should be used. IOW, getting the external clock frequency wrong typically has an effect that that none of the known-good CSI-2 bus clock frequencies are available.
Hi Sakari, On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote: > > But that 19.2MHz is not a limitation of the device itself, it's a > > limitation of our implementation, so we can instead implement > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make > > sure that our parent clock is configured at the right rate) and the > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not > > been rounded too far apart from the frequency we expect). > > > > This is doing exactly the same thing, except that we don't encode our > > implementation limitations in the DT, but in the driver instead. > > What I really wanted to say that a driver that doesn't get the clock > frequency from DT but still sets that frequency is broken. > > This frequency is highly system specific, and in many cases only a certain > frequency is usable, for a few reasons: On many SoCs, not all common > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others > are being used as well), and then that frequency affects the usable CSI-2 > bus frequencies directly --- and of those, only safe, known-good ones > should be used. IOW, getting the external clock frequency wrong typically > has an effect that that none of the known-good CSI-2 bus clock frequencies > are available. So clock-frequency is not about the "Frequency of the xvclk clock in Hertz", but the frequency at which that clock must run on this particular SoC / board to be functional? If so, then yeah, we should definitely keep it, but the documentation of the binding should be made clearer as well. assigned-clock-rates should still go away though. Maxime
Hey Maixme & Sakari, On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote: > > Hi Sakari, > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote: > > > But that 19.2MHz is not a limitation of the device itself, it's a > > > limitation of our implementation, so we can instead implement > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make > > > sure that our parent clock is configured at the right rate) and the > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not > > > been rounded too far apart from the frequency we expect). > > > > > > This is doing exactly the same thing, except that we don't encode our > > > implementation limitations in the DT, but in the driver instead. > > > > What I really wanted to say that a driver that doesn't get the clock > > frequency from DT but still sets that frequency is broken. > > > > This frequency is highly system specific, and in many cases only a certain > > frequency is usable, for a few reasons: On many SoCs, not all common > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others > > are being used as well), and then that frequency affects the usable CSI-2 > > bus frequencies directly --- and of those, only safe, known-good ones > > should be used. IOW, getting the external clock frequency wrong typically > > has an effect that that none of the known-good CSI-2 bus clock frequencies > > are available. > > So clock-frequency is not about the "Frequency of the xvclk clock in > Hertz", but the frequency at which that clock must run on this > particular SoC / board to be functional? > > If so, then yeah, we should definitely keep it, but the documentation > of the binding should be made clearer as well. > Alright so, let me summarise the desired approach then. ACPI: - Fetch the "clock-frequency" property - Verify it to be 19.2Mhz DT: - Fetch the "clock-frequency" property - Verify it to be 19.2Mhz - Get xvclk clock - Get xvclk clock rate - Verify xvclk clock rate to be 19.2Mhz Since the xvclk clock isn't available under ACPI, this is how the two cases would be distinguished between. Does this sound about right? > assigned-clock-rates should still go away though. Ack. > > Maxime
Hi Robert, On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote: > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote: > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote: > > > > But that 19.2MHz is not a limitation of the device itself, it's a > > > > limitation of our implementation, so we can instead implement > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make > > > > sure that our parent clock is configured at the right rate) and the > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not > > > > been rounded too far apart from the frequency we expect). > > > > > > > > This is doing exactly the same thing, except that we don't encode our > > > > implementation limitations in the DT, but in the driver instead. > > > > > > What I really wanted to say that a driver that doesn't get the clock > > > frequency from DT but still sets that frequency is broken. > > > > > > This frequency is highly system specific, and in many cases only a certain > > > frequency is usable, for a few reasons: On many SoCs, not all common > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others > > > are being used as well), and then that frequency affects the usable CSI-2 > > > bus frequencies directly --- and of those, only safe, known-good ones > > > should be used. IOW, getting the external clock frequency wrong typically > > > has an effect that that none of the known-good CSI-2 bus clock frequencies > > > are available. > > > > So clock-frequency is not about the "Frequency of the xvclk clock in > > Hertz", but the frequency at which that clock must run on this > > particular SoC / board to be functional? > > > > If so, then yeah, we should definitely keep it, but the documentation > > of the binding should be made clearer as well. > > Alright so, let me summarise the desired approach then. There's a separate discussion on the same topic here: https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/ > ACPI: > - Fetch the "clock-frequency" property > - Verify it to be 19.2Mhz > > DT: > - Fetch the "clock-frequency" property > - Verify it to be 19.2Mhz > - Get xvclk clock > - Get xvclk clock rate > - Verify xvclk clock rate to be 19.2Mhz The current status is that you should 's/clock-frequency/link-frequencies/', and in order to replace assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz between steps 3 and 4 Maxime
On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote: > > Hi Robert, > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote: > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote: > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote: > > > > > But that 19.2MHz is not a limitation of the device itself, it's a > > > > > limitation of our implementation, so we can instead implement > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make > > > > > sure that our parent clock is configured at the right rate) and the > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not > > > > > been rounded too far apart from the frequency we expect). > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our > > > > > implementation limitations in the DT, but in the driver instead. > > > > > > > > What I really wanted to say that a driver that doesn't get the clock > > > > frequency from DT but still sets that frequency is broken. > > > > > > > > This frequency is highly system specific, and in many cases only a certain > > > > frequency is usable, for a few reasons: On many SoCs, not all common > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others > > > > are being used as well), and then that frequency affects the usable CSI-2 > > > > bus frequencies directly --- and of those, only safe, known-good ones > > > > should be used. IOW, getting the external clock frequency wrong typically > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies > > > > are available. > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in > > > Hertz", but the frequency at which that clock must run on this > > > particular SoC / board to be functional? > > > > > > If so, then yeah, we should definitely keep it, but the documentation > > > of the binding should be made clearer as well. > > > > Alright so, let me summarise the desired approach then. > > There's a separate discussion on the same topic here: > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/ Thanks for the link. > > > ACPI: > > - Fetch the "clock-frequency" property > > - Verify it to be 19.2Mhz > > > > DT: > > - Fetch the "clock-frequency" property > > - Verify it to be 19.2Mhz > > - Get xvclk clock > > - Get xvclk clock rate > > - Verify xvclk clock rate to be 19.2Mhz > > The current status is that you should > 's/clock-frequency/link-frequencies/', and in order to replace > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz > between steps 3 and 4 Would we want to 's/clock-frequency/link-frequencies/' for ACPI too? I imagine that would cause some breakage.
Hi Maxime, On Tue, Apr 07, 2020 at 02:32:32PM +0200, Maxime Ripard wrote: > Hi Robert, > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote: > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote: > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote: > > > > > But that 19.2MHz is not a limitation of the device itself, it's a > > > > > limitation of our implementation, so we can instead implement > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make > > > > > sure that our parent clock is configured at the right rate) and the > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not > > > > > been rounded too far apart from the frequency we expect). > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our > > > > > implementation limitations in the DT, but in the driver instead. > > > > > > > > What I really wanted to say that a driver that doesn't get the clock > > > > frequency from DT but still sets that frequency is broken. > > > > > > > > This frequency is highly system specific, and in many cases only a certain > > > > frequency is usable, for a few reasons: On many SoCs, not all common > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others > > > > are being used as well), and then that frequency affects the usable CSI-2 > > > > bus frequencies directly --- and of those, only safe, known-good ones > > > > should be used. IOW, getting the external clock frequency wrong typically > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies > > > > are available. > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in > > > Hertz", but the frequency at which that clock must run on this > > > particular SoC / board to be functional? > > > > > > If so, then yeah, we should definitely keep it, but the documentation > > > of the binding should be made clearer as well. > > > > Alright so, let me summarise the desired approach then. > > There's a separate discussion on the same topic here: > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/ > > > ACPI: > > - Fetch the "clock-frequency" property > > - Verify it to be 19.2Mhz > > > > DT: > > - Fetch the "clock-frequency" property > > - Verify it to be 19.2Mhz > > - Get xvclk clock > > - Get xvclk clock rate > > - Verify xvclk clock rate to be 19.2Mhz > > The current status is that you should > 's/clock-frequency/link-frequencies/', and in order to replace > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz > between steps 3 and 4 The (CSI-2) link frequency is specified in the endpoint, and is already being read by the V4L2 fwnode framework.
On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote: > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote: > > > > Hi Robert, > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote: > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote: > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a > > > > > > limitation of our implementation, so we can instead implement > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make > > > > > > sure that our parent clock is configured at the right rate) and the > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not > > > > > > been rounded too far apart from the frequency we expect). > > > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our > > > > > > implementation limitations in the DT, but in the driver instead. > > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock > > > > > frequency from DT but still sets that frequency is broken. > > > > > > > > > > This frequency is highly system specific, and in many cases only a certain > > > > > frequency is usable, for a few reasons: On many SoCs, not all common > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others > > > > > are being used as well), and then that frequency affects the usable CSI-2 > > > > > bus frequencies directly --- and of those, only safe, known-good ones > > > > > should be used. IOW, getting the external clock frequency wrong typically > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies > > > > > are available. > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in > > > > Hertz", but the frequency at which that clock must run on this > > > > particular SoC / board to be functional? > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation > > > > of the binding should be made clearer as well. > > > > > > Alright so, let me summarise the desired approach then. > > > > There's a separate discussion on the same topic here: > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/ > > Thanks for the link. > > > > > > ACPI: > > > - Fetch the "clock-frequency" property > > > - Verify it to be 19.2Mhz > > > > > > DT: > > > - Fetch the "clock-frequency" property > > > - Verify it to be 19.2Mhz > > > - Get xvclk clock > > > - Get xvclk clock rate > > > - Verify xvclk clock rate to be 19.2Mhz > > > > The current status is that you should > > 's/clock-frequency/link-frequencies/', and in order to replace > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz > > between steps 3 and 4 > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too? > I imagine that would cause some breakage. It would, yes, and it would be no more correct on DT either. There are basically two possibilities here; either use the clock-frequency property and set the frequency, or rely on assigned-clock-rates, and get the frequency instead. The latter, while I understand it is generally preferred, comes with having to figure out the register list set that closest matches the frequency obtained. The former generally gets around this silently by the clock driver setting the closest frequency it can support.
On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote: > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote: > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > Hi Robert, > > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote: > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote: > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a > > > > > > > limitation of our implementation, so we can instead implement > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make > > > > > > > sure that our parent clock is configured at the right rate) and the > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not > > > > > > > been rounded too far apart from the frequency we expect). > > > > > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our > > > > > > > implementation limitations in the DT, but in the driver instead. > > > > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock > > > > > > frequency from DT but still sets that frequency is broken. > > > > > > > > > > > > This frequency is highly system specific, and in many cases only a certain > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others > > > > > > are being used as well), and then that frequency affects the usable CSI-2 > > > > > > bus frequencies directly --- and of those, only safe, known-good ones > > > > > > should be used. IOW, getting the external clock frequency wrong typically > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies > > > > > > are available. > > > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in > > > > > Hertz", but the frequency at which that clock must run on this > > > > > particular SoC / board to be functional? > > > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation > > > > > of the binding should be made clearer as well. > > > > > > > > Alright so, let me summarise the desired approach then. > > > > > > There's a separate discussion on the same topic here: > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/ > > > > Thanks for the link. > > > > > > > > > ACPI: > > > > - Fetch the "clock-frequency" property > > > > - Verify it to be 19.2Mhz > > > > > > > > DT: > > > > - Fetch the "clock-frequency" property > > > > - Verify it to be 19.2Mhz > > > > - Get xvclk clock > > > > - Get xvclk clock rate > > > > - Verify xvclk clock rate to be 19.2Mhz > > > > > > The current status is that you should > > > 's/clock-frequency/link-frequencies/', and in order to replace > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz > > > between steps 3 and 4 > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too? > > I imagine that would cause some breakage. > > It would, yes, and it would be no more correct on DT either. > > There are basically two possibilities here; either use the clock-frequency > property and set the frequency, or rely on assigned-clock-rates, and get > the frequency instead. > > The latter, while I understand it is generally preferred, comes with having > to figure out the register list set that closest matches the frequency > obtained. The former generally gets around this silently by the clock > driver setting the closest frequency it can support. Wouldn't the former actually cause problems, because the closest frequency the clock driver can support could be pretty far from the one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the resulting frequency anyway. Perhaps a simplified approach of rounding the result of clk_get_rate() to a multiple of 100 KHz and comparing it to the hardcoded value would be enough in practice?
Hi Tomasz, On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote: > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote: > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > Hi Robert, > > > > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote: > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote: > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a > > > > > > > > limitation of our implementation, so we can instead implement > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make > > > > > > > > sure that our parent clock is configured at the right rate) and the > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not > > > > > > > > been rounded too far apart from the frequency we expect). > > > > > > > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our > > > > > > > > implementation limitations in the DT, but in the driver instead. > > > > > > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock > > > > > > > frequency from DT but still sets that frequency is broken. > > > > > > > > > > > > > > This frequency is highly system specific, and in many cases only a certain > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others > > > > > > > are being used as well), and then that frequency affects the usable CSI-2 > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones > > > > > > > should be used. IOW, getting the external clock frequency wrong typically > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies > > > > > > > are available. > > > > > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in > > > > > > Hertz", but the frequency at which that clock must run on this > > > > > > particular SoC / board to be functional? > > > > > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation > > > > > > of the binding should be made clearer as well. > > > > > > > > > > Alright so, let me summarise the desired approach then. > > > > > > > > There's a separate discussion on the same topic here: > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/ > > > > > > Thanks for the link. > > > > > > > > > > > > ACPI: > > > > > - Fetch the "clock-frequency" property > > > > > - Verify it to be 19.2Mhz > > > > > > > > > > DT: > > > > > - Fetch the "clock-frequency" property > > > > > - Verify it to be 19.2Mhz > > > > > - Get xvclk clock > > > > > - Get xvclk clock rate > > > > > - Verify xvclk clock rate to be 19.2Mhz > > > > > > > > The current status is that you should > > > > 's/clock-frequency/link-frequencies/', and in order to replace > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz > > > > between steps 3 and 4 > > > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too? > > > I imagine that would cause some breakage. > > > > It would, yes, and it would be no more correct on DT either. > > > > There are basically two possibilities here; either use the clock-frequency > > property and set the frequency, or rely on assigned-clock-rates, and get > > the frequency instead. > > > > The latter, while I understand it is generally preferred, comes with having > > to figure out the register list set that closest matches the frequency > > obtained. The former generally gets around this silently by the clock > > driver setting the closest frequency it can support. > > Wouldn't the former actually cause problems, because the closest > frequency the clock driver can support could be pretty far from the > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the > resulting frequency anyway. That's possible, yes; in this case there wouldn't be a guarantee the frequency wouldn't be far off. > > Perhaps a simplified approach of rounding the result of clk_get_rate() > to a multiple of 100 KHz and comparing it to the hardcoded value would > be enough in practice? Then the question is: how much deviation from the expected value is allowed? I think there was another case where such range was checked and because the clock was just slightly more off, the probe failed because of that. I'd in that case check some fairly wide range, and print a warning if the frequency isn't in that range, but still proceed. It's generally impossible to set a precise limit on the tolerance.
On Tue, Apr 07, 2020 at 08:20:35PM +0300, Sakari Ailus wrote: > On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote: > > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote: > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > Hi Robert, > > > > > > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote: > > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote: > > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a > > > > > > > > > limitation of our implementation, so we can instead implement > > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make > > > > > > > > > sure that our parent clock is configured at the right rate) and the > > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not > > > > > > > > > been rounded too far apart from the frequency we expect). > > > > > > > > > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our > > > > > > > > > implementation limitations in the DT, but in the driver instead. > > > > > > > > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock > > > > > > > > frequency from DT but still sets that frequency is broken. > > > > > > > > > > > > > > > > This frequency is highly system specific, and in many cases only a certain > > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common > > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others > > > > > > > > are being used as well), and then that frequency affects the usable CSI-2 > > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones > > > > > > > > should be used. IOW, getting the external clock frequency wrong typically > > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies > > > > > > > > are available. > > > > > > > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in > > > > > > > Hertz", but the frequency at which that clock must run on this > > > > > > > particular SoC / board to be functional? > > > > > > > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation > > > > > > > of the binding should be made clearer as well. > > > > > > > > > > > > Alright so, let me summarise the desired approach then. > > > > > > > > > > There's a separate discussion on the same topic here: > > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/ > > > > > > > > Thanks for the link. > > > > > > > > > > > > > > > ACPI: > > > > > > - Fetch the "clock-frequency" property > > > > > > - Verify it to be 19.2Mhz > > > > > > > > > > > > DT: > > > > > > - Fetch the "clock-frequency" property > > > > > > - Verify it to be 19.2Mhz > > > > > > - Get xvclk clock > > > > > > - Get xvclk clock rate > > > > > > - Verify xvclk clock rate to be 19.2Mhz > > > > > > > > > > The current status is that you should > > > > > 's/clock-frequency/link-frequencies/', and in order to replace > > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz > > > > > between steps 3 and 4 > > > > > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too? > > > > I imagine that would cause some breakage. > > > > > > It would, yes, and it would be no more correct on DT either. > > > > > > There are basically two possibilities here; either use the clock-frequency > > > property and set the frequency, or rely on assigned-clock-rates, and get > > > the frequency instead. > > > > > > The latter, while I understand it is generally preferred, comes with having > > > to figure out the register list set that closest matches the frequency > > > obtained. The former generally gets around this silently by the clock > > > driver setting the closest frequency it can support. > > > > Wouldn't the former actually cause problems, because the closest > > frequency the clock driver can support could be pretty far from the > > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the > > resulting frequency anyway. > > That's possible, yes; in this case there wouldn't be a guarantee the > frequency wouldn't be far off. assigned-clock-rates is really fragile... There's zero guarantee on how far the actual rate is going to be from the asked one, but more importantly you have zero guarantee on the time frame that rate is going to be enforced for. It's simply going to change the rate as a one-off thing, and if there's the next millisecond someone else is going to change its rate one way or another, it's going to do so and you won't have any notification. And even semantically, they do not share the same meaning at all, so we should really stop using assigned-clock-rates if we can, instead of pushing it. Maxime
On Wed, Apr 8, 2020 at 2:21 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Tue, Apr 07, 2020 at 08:20:35PM +0300, Sakari Ailus wrote: > > On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote: > > > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote: > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > > > Hi Robert, > > > > > > > > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote: > > > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote: > > > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a > > > > > > > > > > limitation of our implementation, so we can instead implement > > > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make > > > > > > > > > > sure that our parent clock is configured at the right rate) and the > > > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not > > > > > > > > > > been rounded too far apart from the frequency we expect). > > > > > > > > > > > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our > > > > > > > > > > implementation limitations in the DT, but in the driver instead. > > > > > > > > > > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock > > > > > > > > > frequency from DT but still sets that frequency is broken. > > > > > > > > > > > > > > > > > > This frequency is highly system specific, and in many cases only a certain > > > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common > > > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others > > > > > > > > > are being used as well), and then that frequency affects the usable CSI-2 > > > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones > > > > > > > > > should be used. IOW, getting the external clock frequency wrong typically > > > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies > > > > > > > > > are available. > > > > > > > > > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in > > > > > > > > Hertz", but the frequency at which that clock must run on this > > > > > > > > particular SoC / board to be functional? > > > > > > > > > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation > > > > > > > > of the binding should be made clearer as well. > > > > > > > > > > > > > > Alright so, let me summarise the desired approach then. > > > > > > > > > > > > There's a separate discussion on the same topic here: > > > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/ > > > > > > > > > > Thanks for the link. > > > > > > > > > > > > > > > > > > ACPI: > > > > > > > - Fetch the "clock-frequency" property > > > > > > > - Verify it to be 19.2Mhz > > > > > > > > > > > > > > DT: > > > > > > > - Fetch the "clock-frequency" property > > > > > > > - Verify it to be 19.2Mhz > > > > > > > - Get xvclk clock > > > > > > > - Get xvclk clock rate > > > > > > > - Verify xvclk clock rate to be 19.2Mhz > > > > > > > > > > > > The current status is that you should > > > > > > 's/clock-frequency/link-frequencies/', and in order to replace > > > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz > > > > > > between steps 3 and 4 > > > > > > > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too? > > > > > I imagine that would cause some breakage. > > > > > > > > It would, yes, and it would be no more correct on DT either. > > > > > > > > There are basically two possibilities here; either use the clock-frequency > > > > property and set the frequency, or rely on assigned-clock-rates, and get > > > > the frequency instead. > > > > > > > > The latter, while I understand it is generally preferred, comes with having > > > > to figure out the register list set that closest matches the frequency > > > > obtained. The former generally gets around this silently by the clock > > > > driver setting the closest frequency it can support. > > > > > > Wouldn't the former actually cause problems, because the closest > > > frequency the clock driver can support could be pretty far from the > > > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the > > > resulting frequency anyway. > > > > That's possible, yes; in this case there wouldn't be a guarantee the > > frequency wouldn't be far off. > > assigned-clock-rates is really fragile... There's zero guarantee on > how far the actual rate is going to be from the asked one, but more > importantly you have zero guarantee on the time frame that rate is > going to be enforced for. > Is there such a guarantee if clk_set_rate() is called? > It's simply going to change the rate as a one-off thing, and if > there's the next millisecond someone else is going to change its rate > one way or another, it's going to do so and you won't have any > notification. > > And even semantically, they do not share the same meaning at all, so > we should really stop using assigned-clock-rates if we can, instead of > pushing it. > > Maxime
On Wed, Apr 08, 2020 at 02:35:28PM +0200, Tomasz Figa wrote: > On Wed, Apr 8, 2020 at 2:21 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Tue, Apr 07, 2020 at 08:20:35PM +0300, Sakari Ailus wrote: > > > On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote: > > > > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote: > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > > > > > Hi Robert, > > > > > > > > > > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote: > > > > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote: > > > > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a > > > > > > > > > > > limitation of our implementation, so we can instead implement > > > > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make > > > > > > > > > > > sure that our parent clock is configured at the right rate) and the > > > > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not > > > > > > > > > > > been rounded too far apart from the frequency we expect). > > > > > > > > > > > > > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our > > > > > > > > > > > implementation limitations in the DT, but in the driver instead. > > > > > > > > > > > > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock > > > > > > > > > > frequency from DT but still sets that frequency is broken. > > > > > > > > > > > > > > > > > > > > This frequency is highly system specific, and in many cases only a certain > > > > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common > > > > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others > > > > > > > > > > are being used as well), and then that frequency affects the usable CSI-2 > > > > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones > > > > > > > > > > should be used. IOW, getting the external clock frequency wrong typically > > > > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies > > > > > > > > > > are available. > > > > > > > > > > > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in > > > > > > > > > Hertz", but the frequency at which that clock must run on this > > > > > > > > > particular SoC / board to be functional? > > > > > > > > > > > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation > > > > > > > > > of the binding should be made clearer as well. > > > > > > > > > > > > > > > > Alright so, let me summarise the desired approach then. > > > > > > > > > > > > > > There's a separate discussion on the same topic here: > > > > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/ > > > > > > > > > > > > Thanks for the link. > > > > > > > > > > > > > > > > > > > > > ACPI: > > > > > > > > - Fetch the "clock-frequency" property > > > > > > > > - Verify it to be 19.2Mhz > > > > > > > > > > > > > > > > DT: > > > > > > > > - Fetch the "clock-frequency" property > > > > > > > > - Verify it to be 19.2Mhz > > > > > > > > - Get xvclk clock > > > > > > > > - Get xvclk clock rate > > > > > > > > - Verify xvclk clock rate to be 19.2Mhz > > > > > > > > > > > > > > The current status is that you should > > > > > > > 's/clock-frequency/link-frequencies/', and in order to replace > > > > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz > > > > > > > between steps 3 and 4 > > > > > > > > > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too? > > > > > > I imagine that would cause some breakage. > > > > > > > > > > It would, yes, and it would be no more correct on DT either. > > > > > > > > > > There are basically two possibilities here; either use the clock-frequency > > > > > property and set the frequency, or rely on assigned-clock-rates, and get > > > > > the frequency instead. > > > > > > > > > > The latter, while I understand it is generally preferred, comes with having > > > > > to figure out the register list set that closest matches the frequency > > > > > obtained. The former generally gets around this silently by the clock > > > > > driver setting the closest frequency it can support. > > > > > > > > Wouldn't the former actually cause problems, because the closest > > > > frequency the clock driver can support could be pretty far from the > > > > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the > > > > resulting frequency anyway. > > > > > > That's possible, yes; in this case there wouldn't be a guarantee the > > > frequency wouldn't be far off. > > > > assigned-clock-rates is really fragile... There's zero guarantee on > > how far the actual rate is going to be from the asked one, but more > > importantly you have zero guarantee on the time frame that rate is > > going to be enforced for. > > Is there such a guarantee if clk_set_rate() is called? with clk_set_rate itself, no, but... > > It's simply going to change the rate as a one-off thing, and if > > there's the next millisecond someone else is going to change its rate > > one way or another, it's going to do so and you won't have any > > notification. You can get notified, and you can use clk_set_rate_exclusive if you *really* want to enforce it. Maxime
Hi Maxime, On Wed, Apr 08, 2020 at 03:43:15PM +0200, Maxime Ripard wrote: > On Wed, Apr 08, 2020 at 02:35:28PM +0200, Tomasz Figa wrote: > > On Wed, Apr 8, 2020 at 2:21 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > On Tue, Apr 07, 2020 at 08:20:35PM +0300, Sakari Ailus wrote: > > > > On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote: > > > > > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > > > > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote: > > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > > > > > > > Hi Robert, > > > > > > > > > > > > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote: > > > > > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote: > > > > > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a > > > > > > > > > > > > limitation of our implementation, so we can instead implement > > > > > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make > > > > > > > > > > > > sure that our parent clock is configured at the right rate) and the > > > > > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not > > > > > > > > > > > > been rounded too far apart from the frequency we expect). > > > > > > > > > > > > > > > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our > > > > > > > > > > > > implementation limitations in the DT, but in the driver instead. > > > > > > > > > > > > > > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock > > > > > > > > > > > frequency from DT but still sets that frequency is broken. > > > > > > > > > > > > > > > > > > > > > > This frequency is highly system specific, and in many cases only a certain > > > > > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common > > > > > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others > > > > > > > > > > > are being used as well), and then that frequency affects the usable CSI-2 > > > > > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones > > > > > > > > > > > should be used. IOW, getting the external clock frequency wrong typically > > > > > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies > > > > > > > > > > > are available. > > > > > > > > > > > > > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in > > > > > > > > > > Hertz", but the frequency at which that clock must run on this > > > > > > > > > > particular SoC / board to be functional? > > > > > > > > > > > > > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation > > > > > > > > > > of the binding should be made clearer as well. > > > > > > > > > > > > > > > > > > Alright so, let me summarise the desired approach then. > > > > > > > > > > > > > > > > There's a separate discussion on the same topic here: > > > > > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/ > > > > > > > > > > > > > > Thanks for the link. > > > > > > > > > > > > > > > > > > > > > > > > ACPI: > > > > > > > > > - Fetch the "clock-frequency" property > > > > > > > > > - Verify it to be 19.2Mhz > > > > > > > > > > > > > > > > > > DT: > > > > > > > > > - Fetch the "clock-frequency" property > > > > > > > > > - Verify it to be 19.2Mhz > > > > > > > > > - Get xvclk clock > > > > > > > > > - Get xvclk clock rate > > > > > > > > > - Verify xvclk clock rate to be 19.2Mhz > > > > > > > > > > > > > > > > The current status is that you should > > > > > > > > 's/clock-frequency/link-frequencies/', and in order to replace > > > > > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz > > > > > > > > between steps 3 and 4 > > > > > > > > > > > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too? > > > > > > > I imagine that would cause some breakage. > > > > > > > > > > > > It would, yes, and it would be no more correct on DT either. > > > > > > > > > > > > There are basically two possibilities here; either use the clock-frequency > > > > > > property and set the frequency, or rely on assigned-clock-rates, and get > > > > > > the frequency instead. > > > > > > > > > > > > The latter, while I understand it is generally preferred, comes with having > > > > > > to figure out the register list set that closest matches the frequency > > > > > > obtained. The former generally gets around this silently by the clock > > > > > > driver setting the closest frequency it can support. > > > > > > > > > > Wouldn't the former actually cause problems, because the closest > > > > > frequency the clock driver can support could be pretty far from the > > > > > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the > > > > > resulting frequency anyway. > > > > > > > > That's possible, yes; in this case there wouldn't be a guarantee the > > > > frequency wouldn't be far off. > > > > > > assigned-clock-rates is really fragile... There's zero guarantee on > > > how far the actual rate is going to be from the asked one, but more > > > importantly you have zero guarantee on the time frame that rate is > > > going to be enforced for. > > > > Is there such a guarantee if clk_set_rate() is called? > > with clk_set_rate itself, no, but... > > > > It's simply going to change the rate as a one-off thing, and if > > > there's the next millisecond someone else is going to change its rate > > > one way or another, it's going to do so and you won't have any > > > notification. > > You can get notified, and you can use clk_set_rate_exclusive if you > *really* want to enforce it. Is the conclusion then we should go back to relying on the clock-frequency property? This has been discussed multiple times over the years, and I don't really disagree with the above. The frequency is typically indeed hand-picked for the hardware, and no other frequency should be used in any circumstances. No sensor driver I've seen has used clk_set_rate_exclusive() but I guess they should. The absence of practical problems has been probably because of two factors; firstly, these are typically clocks dedicated to the sensors and secondly, good luck.
Cc'ing Laurent as well. On Wed, Apr 08, 2020 at 06:28:57PM +0300, Sakari Ailus wrote: > Hi Maxime, > > On Wed, Apr 08, 2020 at 03:43:15PM +0200, Maxime Ripard wrote: > > On Wed, Apr 08, 2020 at 02:35:28PM +0200, Tomasz Figa wrote: > > > On Wed, Apr 8, 2020 at 2:21 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Tue, Apr 07, 2020 at 08:20:35PM +0300, Sakari Ailus wrote: > > > > > On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote: > > > > > > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > > > > > > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote: > > > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > > > > > > > > > Hi Robert, > > > > > > > > > > > > > > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote: > > > > > > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote: > > > > > > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a > > > > > > > > > > > > > limitation of our implementation, so we can instead implement > > > > > > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make > > > > > > > > > > > > > sure that our parent clock is configured at the right rate) and the > > > > > > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not > > > > > > > > > > > > > been rounded too far apart from the frequency we expect). > > > > > > > > > > > > > > > > > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our > > > > > > > > > > > > > implementation limitations in the DT, but in the driver instead. > > > > > > > > > > > > > > > > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock > > > > > > > > > > > > frequency from DT but still sets that frequency is broken. > > > > > > > > > > > > > > > > > > > > > > > > This frequency is highly system specific, and in many cases only a certain > > > > > > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common > > > > > > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others > > > > > > > > > > > > are being used as well), and then that frequency affects the usable CSI-2 > > > > > > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones > > > > > > > > > > > > should be used. IOW, getting the external clock frequency wrong typically > > > > > > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies > > > > > > > > > > > > are available. > > > > > > > > > > > > > > > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in > > > > > > > > > > > Hertz", but the frequency at which that clock must run on this > > > > > > > > > > > particular SoC / board to be functional? > > > > > > > > > > > > > > > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation > > > > > > > > > > > of the binding should be made clearer as well. > > > > > > > > > > > > > > > > > > > > Alright so, let me summarise the desired approach then. > > > > > > > > > > > > > > > > > > There's a separate discussion on the same topic here: > > > > > > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/ > > > > > > > > > > > > > > > > Thanks for the link. > > > > > > > > > > > > > > > > > > > > > > > > > > > ACPI: > > > > > > > > > > - Fetch the "clock-frequency" property > > > > > > > > > > - Verify it to be 19.2Mhz > > > > > > > > > > > > > > > > > > > > DT: > > > > > > > > > > - Fetch the "clock-frequency" property > > > > > > > > > > - Verify it to be 19.2Mhz > > > > > > > > > > - Get xvclk clock > > > > > > > > > > - Get xvclk clock rate > > > > > > > > > > - Verify xvclk clock rate to be 19.2Mhz > > > > > > > > > > > > > > > > > > The current status is that you should > > > > > > > > > 's/clock-frequency/link-frequencies/', and in order to replace > > > > > > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz > > > > > > > > > between steps 3 and 4 > > > > > > > > > > > > > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too? > > > > > > > > I imagine that would cause some breakage. > > > > > > > > > > > > > > It would, yes, and it would be no more correct on DT either. > > > > > > > > > > > > > > There are basically two possibilities here; either use the clock-frequency > > > > > > > property and set the frequency, or rely on assigned-clock-rates, and get > > > > > > > the frequency instead. > > > > > > > > > > > > > > The latter, while I understand it is generally preferred, comes with having > > > > > > > to figure out the register list set that closest matches the frequency > > > > > > > obtained. The former generally gets around this silently by the clock > > > > > > > driver setting the closest frequency it can support. > > > > > > > > > > > > Wouldn't the former actually cause problems, because the closest > > > > > > frequency the clock driver can support could be pretty far from the > > > > > > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the > > > > > > resulting frequency anyway. > > > > > > > > > > That's possible, yes; in this case there wouldn't be a guarantee the > > > > > frequency wouldn't be far off. > > > > > > > > assigned-clock-rates is really fragile... There's zero guarantee on > > > > how far the actual rate is going to be from the asked one, but more > > > > importantly you have zero guarantee on the time frame that rate is > > > > going to be enforced for. > > > > > > Is there such a guarantee if clk_set_rate() is called? > > > > with clk_set_rate itself, no, but... > > > > > > It's simply going to change the rate as a one-off thing, and if > > > > there's the next millisecond someone else is going to change its rate > > > > one way or another, it's going to do so and you won't have any > > > > notification. > > > > You can get notified, and you can use clk_set_rate_exclusive if you > > *really* want to enforce it. > > Is the conclusion then we should go back to relying on the clock-frequency > property? > > This has been discussed multiple times over the years, and I don't really > disagree with the above. The frequency is typically indeed hand-picked for > the hardware, and no other frequency should be used in any circumstances. > > No sensor driver I've seen has used clk_set_rate_exclusive() but I guess > they should. The absence of practical problems has been probably because of > two factors; firstly, these are typically clocks dedicated to the sensors > and secondly, good luck. > > -- > Regards, > > Sakari Ailus
On Wed, Apr 08, 2020 at 06:30:51PM +0300, Sakari Ailus wrote: > On Wed, Apr 08, 2020 at 06:28:57PM +0300, Sakari Ailus wrote: > > On Wed, Apr 08, 2020 at 03:43:15PM +0200, Maxime Ripard wrote: ... > > No sensor driver I've seen has used clk_set_rate_exclusive() but I guess > > they should. The absence of practical problems has been probably because of > > two factors; firstly, these are typically clocks dedicated to the sensors > > and secondly, good luck. As I heard in another thread clk_*_exclusive() is quite a big hammer with a lot of side effects and if it can be avoided, it must be avoided.
Hi Maxime, On Wed, 8 Apr 2020 at 17:30, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > Hi Maxime, > > On Wed, Apr 08, 2020 at 03:43:15PM +0200, Maxime Ripard wrote: > > On Wed, Apr 08, 2020 at 02:35:28PM +0200, Tomasz Figa wrote: > > > On Wed, Apr 8, 2020 at 2:21 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Tue, Apr 07, 2020 at 08:20:35PM +0300, Sakari Ailus wrote: > > > > > On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote: > > > > > > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > > > > > > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote: > > > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > > > > > > > > > Hi Robert, > > > > > > > > > > > > > > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote: > > > > > > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote: > > > > > > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a > > > > > > > > > > > > > limitation of our implementation, so we can instead implement > > > > > > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make > > > > > > > > > > > > > sure that our parent clock is configured at the right rate) and the > > > > > > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not > > > > > > > > > > > > > been rounded too far apart from the frequency we expect). > > > > > > > > > > > > > > > > > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our > > > > > > > > > > > > > implementation limitations in the DT, but in the driver instead. > > > > > > > > > > > > > > > > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock > > > > > > > > > > > > frequency from DT but still sets that frequency is broken. > > > > > > > > > > > > > > > > > > > > > > > > This frequency is highly system specific, and in many cases only a certain > > > > > > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common > > > > > > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others > > > > > > > > > > > > are being used as well), and then that frequency affects the usable CSI-2 > > > > > > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones > > > > > > > > > > > > should be used. IOW, getting the external clock frequency wrong typically > > > > > > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies > > > > > > > > > > > > are available. > > > > > > > > > > > > > > > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in > > > > > > > > > > > Hertz", but the frequency at which that clock must run on this > > > > > > > > > > > particular SoC / board to be functional? > > > > > > > > > > > > > > > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation > > > > > > > > > > > of the binding should be made clearer as well. > > > > > > > > > > > > > > > > > > > > Alright so, let me summarise the desired approach then. > > > > > > > > > > > > > > > > > > There's a separate discussion on the same topic here: > > > > > > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/ > > > > > > > > > > > > > > > > Thanks for the link. > > > > > > > > > > > > > > > > > > > > > > > > > > > ACPI: > > > > > > > > > > - Fetch the "clock-frequency" property > > > > > > > > > > - Verify it to be 19.2Mhz > > > > > > > > > > > > > > > > > > > > DT: > > > > > > > > > > - Fetch the "clock-frequency" property > > > > > > > > > > - Verify it to be 19.2Mhz > > > > > > > > > > - Get xvclk clock > > > > > > > > > > - Get xvclk clock rate > > > > > > > > > > - Verify xvclk clock rate to be 19.2Mhz > > > > > > > > > > > > > > > > > > The current status is that you should > > > > > > > > > 's/clock-frequency/link-frequencies/', and in order to replace > > > > > > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz > > > > > > > > > between steps 3 and 4 > > > > > > > > > > > > > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too? > > > > > > > > I imagine that would cause some breakage. > > > > > > > > > > > > > > It would, yes, and it would be no more correct on DT either. > > > > > > > > > > > > > > There are basically two possibilities here; either use the clock-frequency > > > > > > > property and set the frequency, or rely on assigned-clock-rates, and get > > > > > > > the frequency instead. > > > > > > > > > > > > > > The latter, while I understand it is generally preferred, comes with having > > > > > > > to figure out the register list set that closest matches the frequency > > > > > > > obtained. The former generally gets around this silently by the clock > > > > > > > driver setting the closest frequency it can support. > > > > > > > > > > > > Wouldn't the former actually cause problems, because the closest > > > > > > frequency the clock driver can support could be pretty far from the > > > > > > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the > > > > > > resulting frequency anyway. > > > > > > > > > > That's possible, yes; in this case there wouldn't be a guarantee the > > > > > frequency wouldn't be far off. > > > > > > > > assigned-clock-rates is really fragile... There's zero guarantee on > > > > how far the actual rate is going to be from the asked one, but more > > > > importantly you have zero guarantee on the time frame that rate is > > > > going to be enforced for. > > > > > > Is there such a guarantee if clk_set_rate() is called? > > > > with clk_set_rate itself, no, but... > > > > > > It's simply going to change the rate as a one-off thing, and if > > > > there's the next millisecond someone else is going to change its rate > > > > one way or another, it's going to do so and you won't have any > > > > notification. > > > > You can get notified, and you can use clk_set_rate_exclusive if you > > *really* want to enforce it. > > Is the conclusion then we should go back to relying on the clock-frequency > property? I too am curious about the conclusion, if we have arrived at one yet. I sent out v4 of this series yesterday, which used the 'assigned-clock-rates' approach but I would like update the series with whatever is deemed the best approach. > > This has been discussed multiple times over the years, and I don't really > disagree with the above. The frequency is typically indeed hand-picked for > the hardware, and no other frequency should be used in any circumstances. > > No sensor driver I've seen has used clk_set_rate_exclusive() but I guess > they should. The absence of practical problems has been probably because of > two factors; firstly, these are typically clocks dedicated to the sensors > and secondly, good luck. > > -- > Regards, > > Sakari Ailus
On Wed, Apr 08, 2020 at 06:30:51PM +0300, Sakari Ailus wrote: > Cc'ing Laurent as well. > > On Wed, Apr 08, 2020 at 06:28:57PM +0300, Sakari Ailus wrote: > > Hi Maxime, > > > > On Wed, Apr 08, 2020 at 03:43:15PM +0200, Maxime Ripard wrote: > > > On Wed, Apr 08, 2020 at 02:35:28PM +0200, Tomasz Figa wrote: > > > > On Wed, Apr 8, 2020 at 2:21 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > On Tue, Apr 07, 2020 at 08:20:35PM +0300, Sakari Ailus wrote: > > > > > > On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote: > > > > > > > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > > > > > > > > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote: > > > > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > > > > > > > > > > > Hi Robert, > > > > > > > > > > > > > > > > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote: > > > > > > > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote: > > > > > > > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a > > > > > > > > > > > > > > limitation of our implementation, so we can instead implement > > > > > > > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make > > > > > > > > > > > > > > sure that our parent clock is configured at the right rate) and the > > > > > > > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not > > > > > > > > > > > > > > been rounded too far apart from the frequency we expect). > > > > > > > > > > > > > > > > > > > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our > > > > > > > > > > > > > > implementation limitations in the DT, but in the driver instead. > > > > > > > > > > > > > > > > > > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock > > > > > > > > > > > > > frequency from DT but still sets that frequency is broken. > > > > > > > > > > > > > > > > > > > > > > > > > > This frequency is highly system specific, and in many cases only a certain > > > > > > > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common > > > > > > > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others > > > > > > > > > > > > > are being used as well), and then that frequency affects the usable CSI-2 > > > > > > > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones > > > > > > > > > > > > > should be used. IOW, getting the external clock frequency wrong typically > > > > > > > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies > > > > > > > > > > > > > are available. > > > > > > > > > > > > > > > > > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in > > > > > > > > > > > > Hertz", but the frequency at which that clock must run on this > > > > > > > > > > > > particular SoC / board to be functional? > > > > > > > > > > > > > > > > > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation > > > > > > > > > > > > of the binding should be made clearer as well. > > > > > > > > > > > > > > > > > > > > > > Alright so, let me summarise the desired approach then. > > > > > > > > > > > > > > > > > > > > There's a separate discussion on the same topic here: > > > > > > > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/ > > > > > > > > > > > > > > > > > > Thanks for the link. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ACPI: > > > > > > > > > > > - Fetch the "clock-frequency" property > > > > > > > > > > > - Verify it to be 19.2Mhz > > > > > > > > > > > > > > > > > > > > > > DT: > > > > > > > > > > > - Fetch the "clock-frequency" property > > > > > > > > > > > - Verify it to be 19.2Mhz > > > > > > > > > > > - Get xvclk clock > > > > > > > > > > > - Get xvclk clock rate > > > > > > > > > > > - Verify xvclk clock rate to be 19.2Mhz > > > > > > > > > > > > > > > > > > > > The current status is that you should > > > > > > > > > > 's/clock-frequency/link-frequencies/', and in order to replace > > > > > > > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz > > > > > > > > > > between steps 3 and 4 > > > > > > > > > > > > > > > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too? > > > > > > > > > I imagine that would cause some breakage. > > > > > > > > > > > > > > > > It would, yes, and it would be no more correct on DT either. > > > > > > > > > > > > > > > > There are basically two possibilities here; either use the clock-frequency > > > > > > > > property and set the frequency, or rely on assigned-clock-rates, and get > > > > > > > > the frequency instead. > > > > > > > > > > > > > > > > The latter, while I understand it is generally preferred, comes with having > > > > > > > > to figure out the register list set that closest matches the frequency > > > > > > > > obtained. The former generally gets around this silently by the clock > > > > > > > > driver setting the closest frequency it can support. > > > > > > > > > > > > > > Wouldn't the former actually cause problems, because the closest > > > > > > > frequency the clock driver can support could be pretty far from the > > > > > > > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the > > > > > > > resulting frequency anyway. > > > > > > > > > > > > That's possible, yes; in this case there wouldn't be a guarantee the > > > > > > frequency wouldn't be far off. > > > > > > > > > > assigned-clock-rates is really fragile... There's zero guarantee on > > > > > how far the actual rate is going to be from the asked one, but more > > > > > importantly you have zero guarantee on the time frame that rate is > > > > > going to be enforced for. > > > > > > > > Is there such a guarantee if clk_set_rate() is called? > > > > > > with clk_set_rate itself, no, but... > > > > > > > > It's simply going to change the rate as a one-off thing, and if > > > > > there's the next millisecond someone else is going to change its rate > > > > > one way or another, it's going to do so and you won't have any > > > > > notification. > > > > > > You can get notified, and you can use clk_set_rate_exclusive if you > > > *really* want to enforce it. > > > > Is the conclusion then we should go back to relying on the clock-frequency > > property? clock-frequency or link-frequencies. link-frequencies seems to be a better fit here, but we don't really have the choice for older bindings. > > This has been discussed multiple times over the years, and I don't really > > disagree with the above. The frequency is typically indeed hand-picked for > > the hardware, and no other frequency should be used in any circumstances. > > > > No sensor driver I've seen has used clk_set_rate_exclusive() but I guess > > they should. The absence of practical problems has been probably because of > > two factors; firstly, these are typically clocks dedicated to the sensors > > and secondly, good luck. My point was that at least with handling the clock rate within the driver (as opposed to assigned-clock-rates) you have multiple options in dealing with changing colck rates / parents (Modelling the sensor clock as a clock itself, using clk_set_rate_exclusive, using a notifier, etc).. Some are more intrusive to the rest of the system than others (especially clk_set_rate_exclusive), so I'm not really advocating for any here, but we should make sure we have them in the first place. Maxime
On Wed, 15 Apr 2020 at 12:18, Maxime Ripard <maxime@cerno.tech> wrote: > > On Wed, Apr 08, 2020 at 06:30:51PM +0300, Sakari Ailus wrote: > > Cc'ing Laurent as well. > > > > On Wed, Apr 08, 2020 at 06:28:57PM +0300, Sakari Ailus wrote: > > > Hi Maxime, > > > > > > On Wed, Apr 08, 2020 at 03:43:15PM +0200, Maxime Ripard wrote: > > > > On Wed, Apr 08, 2020 at 02:35:28PM +0200, Tomasz Figa wrote: > > > > > On Wed, Apr 8, 2020 at 2:21 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > On Tue, Apr 07, 2020 at 08:20:35PM +0300, Sakari Ailus wrote: > > > > > > > On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote: > > > > > > > > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > > > > > > > > > > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote: > > > > > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > > > > > > > > > > > > > Hi Robert, > > > > > > > > > > > > > > > > > > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote: > > > > > > > > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote: > > > > > > > > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a > > > > > > > > > > > > > > > limitation of our implementation, so we can instead implement > > > > > > > > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make > > > > > > > > > > > > > > > sure that our parent clock is configured at the right rate) and the > > > > > > > > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not > > > > > > > > > > > > > > > been rounded too far apart from the frequency we expect). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our > > > > > > > > > > > > > > > implementation limitations in the DT, but in the driver instead. > > > > > > > > > > > > > > > > > > > > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock > > > > > > > > > > > > > > frequency from DT but still sets that frequency is broken. > > > > > > > > > > > > > > > > > > > > > > > > > > > > This frequency is highly system specific, and in many cases only a certain > > > > > > > > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common > > > > > > > > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others > > > > > > > > > > > > > > are being used as well), and then that frequency affects the usable CSI-2 > > > > > > > > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones > > > > > > > > > > > > > > should be used. IOW, getting the external clock frequency wrong typically > > > > > > > > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies > > > > > > > > > > > > > > are available. > > > > > > > > > > > > > > > > > > > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in > > > > > > > > > > > > > Hertz", but the frequency at which that clock must run on this > > > > > > > > > > > > > particular SoC / board to be functional? > > > > > > > > > > > > > > > > > > > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation > > > > > > > > > > > > > of the binding should be made clearer as well. > > > > > > > > > > > > > > > > > > > > > > > > Alright so, let me summarise the desired approach then. > > > > > > > > > > > > > > > > > > > > > > There's a separate discussion on the same topic here: > > > > > > > > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/ > > > > > > > > > > > > > > > > > > > > Thanks for the link. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ACPI: > > > > > > > > > > > > - Fetch the "clock-frequency" property > > > > > > > > > > > > - Verify it to be 19.2Mhz > > > > > > > > > > > > > > > > > > > > > > > > DT: > > > > > > > > > > > > - Fetch the "clock-frequency" property > > > > > > > > > > > > - Verify it to be 19.2Mhz > > > > > > > > > > > > - Get xvclk clock > > > > > > > > > > > > - Get xvclk clock rate > > > > > > > > > > > > - Verify xvclk clock rate to be 19.2Mhz > > > > > > > > > > > > > > > > > > > > > > The current status is that you should > > > > > > > > > > > 's/clock-frequency/link-frequencies/', and in order to replace > > > > > > > > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz > > > > > > > > > > > between steps 3 and 4 > > > > > > > > > > > > > > > > > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too? > > > > > > > > > > I imagine that would cause some breakage. > > > > > > > > > > > > > > > > > > It would, yes, and it would be no more correct on DT either. > > > > > > > > > > > > > > > > > > There are basically two possibilities here; either use the clock-frequency > > > > > > > > > property and set the frequency, or rely on assigned-clock-rates, and get > > > > > > > > > the frequency instead. > > > > > > > > > > > > > > > > > > The latter, while I understand it is generally preferred, comes with having > > > > > > > > > to figure out the register list set that closest matches the frequency > > > > > > > > > obtained. The former generally gets around this silently by the clock > > > > > > > > > driver setting the closest frequency it can support. > > > > > > > > > > > > > > > > Wouldn't the former actually cause problems, because the closest > > > > > > > > frequency the clock driver can support could be pretty far from the > > > > > > > > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the > > > > > > > > resulting frequency anyway. > > > > > > > > > > > > > > That's possible, yes; in this case there wouldn't be a guarantee the > > > > > > > frequency wouldn't be far off. > > > > > > > > > > > > assigned-clock-rates is really fragile... There's zero guarantee on > > > > > > how far the actual rate is going to be from the asked one, but more > > > > > > importantly you have zero guarantee on the time frame that rate is > > > > > > going to be enforced for. > > > > > > > > > > Is there such a guarantee if clk_set_rate() is called? > > > > > > > > with clk_set_rate itself, no, but... > > > > > > > > > > It's simply going to change the rate as a one-off thing, and if > > > > > > there's the next millisecond someone else is going to change its rate > > > > > > one way or another, it's going to do so and you won't have any > > > > > > notification. > > > > > > > > You can get notified, and you can use clk_set_rate_exclusive if you > > > > *really* want to enforce it. > > > > > > Is the conclusion then we should go back to relying on the clock-frequency > > > property? > > clock-frequency or link-frequencies. link-frequencies seems to be a > better fit here, but we don't really have the choice for older > bindings. Alright, so since this is a new binding, let's aim for link-frequencies then. I don't think I understand what they look like on the driver side. Do you know an example of what a link-frequencies based driver would look like? > > > > This has been discussed multiple times over the years, and I don't really > > > disagree with the above. The frequency is typically indeed hand-picked for > > > the hardware, and no other frequency should be used in any circumstances. > > > > > > No sensor driver I've seen has used clk_set_rate_exclusive() but I guess > > > they should. The absence of practical problems has been probably because of > > > two factors; firstly, these are typically clocks dedicated to the sensors > > > and secondly, good luck. > > My point was that at least with handling the clock rate within the > driver (as opposed to assigned-clock-rates) you have multiple options > in dealing with changing colck rates / parents (Modelling the sensor > clock as a clock itself, using clk_set_rate_exclusive, using a > notifier, etc).. Some are more intrusive to the rest of the system > than others (especially clk_set_rate_exclusive), so I'm not really > advocating for any here, but we should make sure we have them in the > first place. > > Maxime
On Wed, Apr 15, 2020 at 12:18:27PM +0200, Maxime Ripard wrote: > On Wed, Apr 08, 2020 at 06:30:51PM +0300, Sakari Ailus wrote: > > Cc'ing Laurent as well. > > > > On Wed, Apr 08, 2020 at 06:28:57PM +0300, Sakari Ailus wrote: > > > Hi Maxime, > > > > > > On Wed, Apr 08, 2020 at 03:43:15PM +0200, Maxime Ripard wrote: > > > > On Wed, Apr 08, 2020 at 02:35:28PM +0200, Tomasz Figa wrote: > > > > > On Wed, Apr 8, 2020 at 2:21 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > On Tue, Apr 07, 2020 at 08:20:35PM +0300, Sakari Ailus wrote: > > > > > > > On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote: > > > > > > > > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > > > > > > > > > > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote: > > > > > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > > > > > > > > > > > > > Hi Robert, > > > > > > > > > > > > > > > > > > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote: > > > > > > > > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote: > > > > > > > > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a > > > > > > > > > > > > > > > limitation of our implementation, so we can instead implement > > > > > > > > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make > > > > > > > > > > > > > > > sure that our parent clock is configured at the right rate) and the > > > > > > > > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not > > > > > > > > > > > > > > > been rounded too far apart from the frequency we expect). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our > > > > > > > > > > > > > > > implementation limitations in the DT, but in the driver instead. > > > > > > > > > > > > > > > > > > > > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock > > > > > > > > > > > > > > frequency from DT but still sets that frequency is broken. > > > > > > > > > > > > > > > > > > > > > > > > > > > > This frequency is highly system specific, and in many cases only a certain > > > > > > > > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common > > > > > > > > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others > > > > > > > > > > > > > > are being used as well), and then that frequency affects the usable CSI-2 > > > > > > > > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones > > > > > > > > > > > > > > should be used. IOW, getting the external clock frequency wrong typically > > > > > > > > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies > > > > > > > > > > > > > > are available. > > > > > > > > > > > > > > > > > > > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in > > > > > > > > > > > > > Hertz", but the frequency at which that clock must run on this > > > > > > > > > > > > > particular SoC / board to be functional? > > > > > > > > > > > > > > > > > > > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation > > > > > > > > > > > > > of the binding should be made clearer as well. > > > > > > > > > > > > > > > > > > > > > > > > Alright so, let me summarise the desired approach then. > > > > > > > > > > > > > > > > > > > > > > There's a separate discussion on the same topic here: > > > > > > > > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/ > > > > > > > > > > > > > > > > > > > > Thanks for the link. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ACPI: > > > > > > > > > > > > - Fetch the "clock-frequency" property > > > > > > > > > > > > - Verify it to be 19.2Mhz > > > > > > > > > > > > > > > > > > > > > > > > DT: > > > > > > > > > > > > - Fetch the "clock-frequency" property > > > > > > > > > > > > - Verify it to be 19.2Mhz > > > > > > > > > > > > - Get xvclk clock > > > > > > > > > > > > - Get xvclk clock rate > > > > > > > > > > > > - Verify xvclk clock rate to be 19.2Mhz > > > > > > > > > > > > > > > > > > > > > > The current status is that you should > > > > > > > > > > > 's/clock-frequency/link-frequencies/', and in order to replace > > > > > > > > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz > > > > > > > > > > > between steps 3 and 4 > > > > > > > > > > > > > > > > > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too? > > > > > > > > > > I imagine that would cause some breakage. > > > > > > > > > > > > > > > > > > It would, yes, and it would be no more correct on DT either. > > > > > > > > > > > > > > > > > > There are basically two possibilities here; either use the clock-frequency > > > > > > > > > property and set the frequency, or rely on assigned-clock-rates, and get > > > > > > > > > the frequency instead. > > > > > > > > > > > > > > > > > > The latter, while I understand it is generally preferred, comes with having > > > > > > > > > to figure out the register list set that closest matches the frequency > > > > > > > > > obtained. The former generally gets around this silently by the clock > > > > > > > > > driver setting the closest frequency it can support. > > > > > > > > > > > > > > > > Wouldn't the former actually cause problems, because the closest > > > > > > > > frequency the clock driver can support could be pretty far from the > > > > > > > > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the > > > > > > > > resulting frequency anyway. > > > > > > > > > > > > > > That's possible, yes; in this case there wouldn't be a guarantee the > > > > > > > frequency wouldn't be far off. > > > > > > > > > > > > assigned-clock-rates is really fragile... There's zero guarantee on > > > > > > how far the actual rate is going to be from the asked one, but more > > > > > > importantly you have zero guarantee on the time frame that rate is > > > > > > going to be enforced for. > > > > > > > > > > Is there such a guarantee if clk_set_rate() is called? > > > > > > > > with clk_set_rate itself, no, but... > > > > > > > > > > It's simply going to change the rate as a one-off thing, and if > > > > > > there's the next millisecond someone else is going to change its rate > > > > > > one way or another, it's going to do so and you won't have any > > > > > > notification. > > > > > > > > You can get notified, and you can use clk_set_rate_exclusive if you > > > > *really* want to enforce it. > > > > > > Is the conclusion then we should go back to relying on the clock-frequency > > > property? > > clock-frequency or link-frequencies. link-frequencies seems to be a > better fit here, but we don't really have the choice for older > bindings. You can't replace one with the other as the two are different things. The clock-frequency refers to the external clock frequency whereas the link-frequencies refers to the frequencies allowed on the CSI-2 bus. > > > > This has been discussed multiple times over the years, and I don't really > > > disagree with the above. The frequency is typically indeed hand-picked for > > > the hardware, and no other frequency should be used in any circumstances. > > > > > > No sensor driver I've seen has used clk_set_rate_exclusive() but I guess > > > they should. The absence of practical problems has been probably because of > > > two factors; firstly, these are typically clocks dedicated to the sensors > > > and secondly, good luck. > > My point was that at least with handling the clock rate within the > driver (as opposed to assigned-clock-rates) you have multiple options > in dealing with changing colck rates / parents (Modelling the sensor > clock as a clock itself, using clk_set_rate_exclusive, using a > notifier, etc).. Some are more intrusive to the rest of the system > than others (especially clk_set_rate_exclusive), so I'm not really > advocating for any here, but we should make sure we have them in the > first place. Using a different frequency really should not be allowed. It may be possible on a development system, hobbyist platform, but never in production. Therefore the exclusive variant sounds like the right one to me.
On Wed, Apr 15, 2020 at 07:16:16PM +0300, Sakari Ailus wrote: > On Wed, Apr 15, 2020 at 12:18:27PM +0200, Maxime Ripard wrote: > > On Wed, Apr 08, 2020 at 06:30:51PM +0300, Sakari Ailus wrote: > > > Cc'ing Laurent as well. > > > > > > On Wed, Apr 08, 2020 at 06:28:57PM +0300, Sakari Ailus wrote: > > > > Hi Maxime, > > > > > > > > On Wed, Apr 08, 2020 at 03:43:15PM +0200, Maxime Ripard wrote: > > > > > On Wed, Apr 08, 2020 at 02:35:28PM +0200, Tomasz Figa wrote: > > > > > > On Wed, Apr 8, 2020 at 2:21 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > On Tue, Apr 07, 2020 at 08:20:35PM +0300, Sakari Ailus wrote: > > > > > > > > On Tue, Apr 07, 2020 at 06:46:06PM +0200, Tomasz Figa wrote: > > > > > > > > > On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > > > > > > > > > > > > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote: > > > > > > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > > > > > > > > > > > > > > > Hi Robert, > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote: > > > > > > > > > > > > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > > > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote: > > > > > > > > > > > > > > > > But that 19.2MHz is not a limitation of the device itself, it's a > > > > > > > > > > > > > > > > limitation of our implementation, so we can instead implement > > > > > > > > > > > > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make > > > > > > > > > > > > > > > > sure that our parent clock is configured at the right rate) and the > > > > > > > > > > > > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not > > > > > > > > > > > > > > > > been rounded too far apart from the frequency we expect). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This is doing exactly the same thing, except that we don't encode our > > > > > > > > > > > > > > > > implementation limitations in the DT, but in the driver instead. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > What I really wanted to say that a driver that doesn't get the clock > > > > > > > > > > > > > > > frequency from DT but still sets that frequency is broken. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This frequency is highly system specific, and in many cases only a certain > > > > > > > > > > > > > > > frequency is usable, for a few reasons: On many SoCs, not all common > > > > > > > > > > > > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others > > > > > > > > > > > > > > > are being used as well), and then that frequency affects the usable CSI-2 > > > > > > > > > > > > > > > bus frequencies directly --- and of those, only safe, known-good ones > > > > > > > > > > > > > > > should be used. IOW, getting the external clock frequency wrong typically > > > > > > > > > > > > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies > > > > > > > > > > > > > > > are available. > > > > > > > > > > > > > > > > > > > > > > > > > > > > So clock-frequency is not about the "Frequency of the xvclk clock in > > > > > > > > > > > > > > Hertz", but the frequency at which that clock must run on this > > > > > > > > > > > > > > particular SoC / board to be functional? > > > > > > > > > > > > > > > > > > > > > > > > > > > > If so, then yeah, we should definitely keep it, but the documentation > > > > > > > > > > > > > > of the binding should be made clearer as well. > > > > > > > > > > > > > > > > > > > > > > > > > > Alright so, let me summarise the desired approach then. > > > > > > > > > > > > > > > > > > > > > > > > There's a separate discussion on the same topic here: > > > > > > > > > > > > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/ > > > > > > > > > > > > > > > > > > > > > > Thanks for the link. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ACPI: > > > > > > > > > > > > > - Fetch the "clock-frequency" property > > > > > > > > > > > > > - Verify it to be 19.2Mhz > > > > > > > > > > > > > > > > > > > > > > > > > > DT: > > > > > > > > > > > > > - Fetch the "clock-frequency" property > > > > > > > > > > > > > - Verify it to be 19.2Mhz > > > > > > > > > > > > > - Get xvclk clock > > > > > > > > > > > > > - Get xvclk clock rate > > > > > > > > > > > > > - Verify xvclk clock rate to be 19.2Mhz > > > > > > > > > > > > > > > > > > > > > > > > The current status is that you should > > > > > > > > > > > > 's/clock-frequency/link-frequencies/', and in order to replace > > > > > > > > > > > > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz > > > > > > > > > > > > between steps 3 and 4 > > > > > > > > > > > > > > > > > > > > > > Would we want to 's/clock-frequency/link-frequencies/' for ACPI too? > > > > > > > > > > > I imagine that would cause some breakage. > > > > > > > > > > > > > > > > > > > > It would, yes, and it would be no more correct on DT either. > > > > > > > > > > > > > > > > > > > > There are basically two possibilities here; either use the clock-frequency > > > > > > > > > > property and set the frequency, or rely on assigned-clock-rates, and get > > > > > > > > > > the frequency instead. > > > > > > > > > > > > > > > > > > > > The latter, while I understand it is generally preferred, comes with having > > > > > > > > > > to figure out the register list set that closest matches the frequency > > > > > > > > > > obtained. The former generally gets around this silently by the clock > > > > > > > > > > driver setting the closest frequency it can support. > > > > > > > > > > > > > > > > > > Wouldn't the former actually cause problems, because the closest > > > > > > > > > frequency the clock driver can support could be pretty far from the > > > > > > > > > one requested? (E.g. 19.2 MHz vs 24 MHz) The driver needs to check the > > > > > > > > > resulting frequency anyway. > > > > > > > > > > > > > > > > That's possible, yes; in this case there wouldn't be a guarantee the > > > > > > > > frequency wouldn't be far off. > > > > > > > > > > > > > > assigned-clock-rates is really fragile... There's zero guarantee on > > > > > > > how far the actual rate is going to be from the asked one, but more > > > > > > > importantly you have zero guarantee on the time frame that rate is > > > > > > > going to be enforced for. > > > > > > > > > > > > Is there such a guarantee if clk_set_rate() is called? > > > > > > > > > > with clk_set_rate itself, no, but... > > > > > > > > > > > > It's simply going to change the rate as a one-off thing, and if > > > > > > > there's the next millisecond someone else is going to change its rate > > > > > > > one way or another, it's going to do so and you won't have any > > > > > > > notification. > > > > > > > > > > You can get notified, and you can use clk_set_rate_exclusive if you > > > > > *really* want to enforce it. > > > > > > > > Is the conclusion then we should go back to relying on the clock-frequency > > > > property? > > > > clock-frequency or link-frequencies. link-frequencies seems to be a > > better fit here, but we don't really have the choice for older > > bindings. > > You can't replace one with the other as the two are different things. The > clock-frequency refers to the external clock frequency whereas the > link-frequencies refers to the frequencies allowed on the CSI-2 bus. Ack. > > > > This has been discussed multiple times over the years, and I don't really > > > > disagree with the above. The frequency is typically indeed hand-picked for > > > > the hardware, and no other frequency should be used in any circumstances. > > > > > > > > No sensor driver I've seen has used clk_set_rate_exclusive() but I guess > > > > they should. The absence of practical problems has been probably because of > > > > two factors; firstly, these are typically clocks dedicated to the sensors > > > > and secondly, good luck. > > > > My point was that at least with handling the clock rate within the > > driver (as opposed to assigned-clock-rates) you have multiple options > > in dealing with changing colck rates / parents (Modelling the sensor > > clock as a clock itself, using clk_set_rate_exclusive, using a > > notifier, etc).. Some are more intrusive to the rest of the system > > than others (especially clk_set_rate_exclusive), so I'm not really > > advocating for any here, but we should make sure we have them in the > > first place. > > Using a different frequency really should not be allowed. It may be > possible on a development system, hobbyist platform, but never in > production. Therefore the exclusive variant sounds like the right one to > me. In all of those cases you would not allow a different frequency. The only difference is whether you react to a rate change in your parent clock, or prevent it from happening in the first place. The latter is easier to do, but has a wider impact on the rest of the system than the former. Maxime
diff --git a/Documentation/devicetree/bindings/media/i2c/ov8856.yaml b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml new file mode 100644 index 000000000000..beeddfbb8709 --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/ov8856.yaml @@ -0,0 +1,150 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +# Copyright (c) 2019 MediaTek Inc. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/ov8856.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Omnivision OV8856 CMOS Sensor Device Tree Bindings + +maintainers: + - Ben Kao <ben.kao@intel.com> + - Dongchun Zhu <dongchun.zhu@mediatek.com> + +description: |- + The Omnivision OV8856 is a high performance, 1/4-inch, 8 megapixel, CMOS + image sensor that delivers 3264x2448 at 30fps. It provides full-frame, + sub-sampled, and windowed 10-bit MIPI images in various formats via the + Serial Camera Control Bus (SCCB) interface. This chip is programmable + through I2C and two-wire SCCB. The sensor output is available via CSI-2 + serial data output (up to 4-lane). + +properties: + compatible: + const: ovti,ov8856 + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + clock-names: + description: + Input clock for the sensor. + items: + - const: xvclk + + clock-frequency: + description: + Frequency of the xvclk clock in Hertz. + + assigned-clocks: + description: + Input clock for the sensor. + + assigned-clock-rates: + description: + Frequency of the xvclk clock in Hertz. + + dovdd-supply: + description: + Definition of the regulator used as interface power supply. + + avdd-supply: + description: + Definition of the regulator used as analog power supply. + + dvdd-supply: + description: + Definition of the regulator used as digital power supply. + + reset-gpios: + description: + The phandle and specifier for the GPIO that controls sensor reset. + This corresponds to the hardware pin XSHUTDOWN which is physically + active low. + + port: + type: object + additionalProperties: false + description: + A node containing input and output port nodes with endpoint definitions + as documented in + Documentation/devicetree/bindings/media/video-interfaces.txt + + properties: + endpoint: + type: object + + properties: + clock-lanes: + maxItems: 1 + + data-lanes: + maxItems: 1 + + remote-endpoint: true + + required: + - clock-lanes + - data-lanes + - remote-endpoint + + required: + - endpoint + +required: + - compatible + - reg + - clocks + - clock-names + - clock-frequency + - assigned-clocks + - assigned-clock-rates + - dovdd-supply + - avdd-supply + - dvdd-supply + - reset-gpios + - port + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/clock/qcom,camcc-sdm845.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + ov8856: camera@10 { + compatible = "ovti,ov8856"; + reg = <0x10>; + + reset-gpios = <&pio 111 GPIO_ACTIVE_LOW>; + pinctrl-names = "default"; + pinctrl-0 = <&clk_24m_cam>; + + clocks = <&clock_camcc CAM_CC_MCLK0_CLK>; + clock-names = "xvclk"; + clock-frequency = <19200000>; + assigned-clocks = <&clock_camcc CAM_CC_MCLK0_CLK>; + assigned-clock-rates = <19200000>; + + avdd-supply = <&mt6358_vcama2_reg>; + dvdd-supply = <&mt6358_vcamd_reg>; + dovdd-supply = <&mt6358_vcamio_reg>; + + port { + wcam_out: endpoint { + remote-endpoint = <&mipi_in_wcam>; + clock-lanes = <0>; + data-lanes = <1 2 3 4>; + link-frequencies = /bits/ 64 <360000000 180000000>; + }; + }; + }; + }; +... \ No newline at end of file diff --git a/MAINTAINERS b/MAINTAINERS index a6fbdf354d34..0f99e863978a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12355,6 +12355,7 @@ L: linux-media@vger.kernel.org T: git git://linuxtv.org/media_tree.git S: Maintained F: drivers/media/i2c/ov8856.c +F: Documentation/devicetree/bindings/media/i2c/ov8856.yaml OMNIVISION OV9650 SENSOR DRIVER M: Sakari Ailus <sakari.ailus@linux.intel.com>