LinuxLists.cc - [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

Hi,

On Tue, Mar 31, 2020 at 03:33:44PM +0200, Robert Foss wrote:
> From: Dongchun Zhu <[email protected]>
>
> This patch adds documentation of device tree in YAML schema for the
> OV8856 CMOS image sensor.
>
> Signed-off-by: Dongchun Zhu <[email protected]>
> Signed-off-by: Robert Foss <[email protected]>
> ---
>
> - 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 <[email protected]>
> + - Dongchun Zhu <[email protected]>
> +
> +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

Attachments:

(No filename) (3.73 kB)
signature.asc (235.00 B)
Download all attachments

2020-04-02 09:58:10

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

'Hey Marco,

On Tue, 31 Mar 2020 at 17:13, Marco Felsch <[email protected]> wrote:
>
> Hi Robert,
>
> On 20-03-31 15:33, Robert Foss wrote:
> > From: Dongchun Zhu <[email protected]>
> >
> > This patch adds documentation of device tree in YAML schema for the
> > OV8856 CMOS image sensor.
> >
> > Signed-off-by: Dongchun Zhu <[email protected]>
> > Signed-off-by: Robert Foss <[email protected]>
> > ---
> >
> > - 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 <[email protected]>
> > + - Dongchun Zhu <[email protected]>
> > +
> > +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: [email protected]
> > 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 <[email protected]>
> > --
> > 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 |

2020-04-02 10:11:37

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

Hey Maxime,

On Wed, 1 Apr 2020 at 10:07, Maxime Ripard <[email protected]> wrote:
>
> Hi,
>
> On Tue, Mar 31, 2020 at 03:33:44PM +0200, Robert Foss wrote:
> > From: Dongchun Zhu <[email protected]>
> >
> > This patch adds documentation of device tree in YAML schema for the
> > OV8856 CMOS image sensor.
> >
> > Signed-off-by: Dongchun Zhu <[email protected]>
> > Signed-off-by: Robert Foss <[email protected]>
> > ---
> >
> > - 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 <[email protected]>
> > + - Dongchun Zhu <[email protected]>
> > +
> > +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

2020-04-03 19:42:11

by Marco Felsch

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

Hi Robert,

On 20-04-02 11:57, Robert Foss wrote:
> 'Hey Marco,
>
> On Tue, 31 Mar 2020 at 17:13, Marco Felsch <[email protected]> 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: [email protected]
> > > 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 <[email protected]>
> > > --
> > > 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 |
>

--
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 |

2020-04-03 23:29:14

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

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 <[email protected]> wrote:
> >
> > Hi,
> >
> > On Tue, Mar 31, 2020 at 03:33:44PM +0200, Robert Foss wrote:
> > > From: Dongchun Zhu <[email protected]>
> > >
> > > This patch adds documentation of device tree in YAML schema for the
> > > OV8856 CMOS image sensor.
> > >
> > > Signed-off-by: Dongchun Zhu <[email protected]>
> > > Signed-off-by: Robert Foss <[email protected]>
> > > ---
> > >
> > > - 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 <[email protected]>
> > > + - Dongchun Zhu <[email protected]>
> > > +
> > > +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.

--
Regards,

Sakari Ailus

2020-04-04 09:24:54

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

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 <[email protected]> wrote:
> > On Tue, Mar 31, 2020 at 03:33:44PM +0200, Robert Foss wrote:
> > > From: Dongchun Zhu <[email protected]>
> > >
> > > This patch adds documentation of device tree in YAML schema for the
> > > OV8856 CMOS image sensor.
> > >
> > > Signed-off-by: Dongchun Zhu <[email protected]>
> > > Signed-off-by: Robert Foss <[email protected]>
> > > ---
> > >
> > > - 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 <[email protected]>
> > > + - Dongchun Zhu <[email protected]>
> > > +
> > > +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

Attachments:

(No filename) (4.60 kB)
signature.asc (235.00 B)
Download all attachments

2020-04-04 09:35:29

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

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 <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Mar 31, 2020 at 03:33:44PM +0200, Robert Foss wrote:
> > > > From: Dongchun Zhu <[email protected]>
> > > >
> > > > This patch adds documentation of device tree in YAML schema for the
> > > > OV8856 CMOS image sensor.
> > > >
> > > > Signed-off-by: Dongchun Zhu <[email protected]>
> > > > Signed-off-by: Robert Foss <[email protected]>
> > > > ---
> > > >
> > > > - 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 <[email protected]>
> > > > + - Dongchun Zhu <[email protected]>
> > > > +
> > > > +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

Attachments:

(No filename) (6.27 kB)
signature.asc (235.00 B)
Download all attachments

2020-04-06 08:27:02

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

Hey Maxime,

On Sat, 4 Apr 2020 at 11:34, Maxime Ripard <[email protected]> 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 <[email protected]> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tue, Mar 31, 2020 at 03:33:44PM +0200, Robert Foss wrote:
> > > > > From: Dongchun Zhu <[email protected]>
> > > > >
> > > > > This patch adds documentation of device tree in YAML schema for the
> > > > > OV8856 CMOS image sensor.
> > > > >
> > > > > Signed-off-by: Dongchun Zhu <[email protected]>
> > > > > Signed-off-by: Robert Foss <[email protected]>
> > > > > ---
> > > > >
> > > > > - 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 <[email protected]>
> > > > > + - Dongchun Zhu <[email protected]>
> > > > > +
> > > > > +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.

2020-04-06 08:36:54

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

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 <[email protected]> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tue, Mar 31, 2020 at 03:33:44PM +0200, Robert Foss wrote:
> > > > > From: Dongchun Zhu <[email protected]>
> > > > >
> > > > > This patch adds documentation of device tree in YAML schema for the
> > > > > OV8856 CMOS image sensor.
> > > > >
> > > > > Signed-off-by: Dongchun Zhu <[email protected]>
> > > > > Signed-off-by: Robert Foss <[email protected]>
> > > > > ---
> > > > >
> > > > > - 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 <[email protected]>
> > > > > + - Dongchun Zhu <[email protected]>
> > > > > +
> > > > > +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.

--
Regards,

Sakari Ailus

2020-04-07 08:37:34

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

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

Attachments:

(No filename) (1.66 kB)
signature.asc (235.00 B)
Download all attachments

2020-04-07 11:30:19

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

Hey Maixme & Sakari,

On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <[email protected]> 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

2020-04-07 12:34:33

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

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 <[email protected]> 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/[email protected]/

> 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

Attachments:

(No filename) (2.51 kB)
signature.asc (235.00 B)
Download all attachments

2020-04-07 15:48:51

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <[email protected]> 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 <[email protected]> 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/[email protected]/

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.

2020-04-07 16:23:12

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

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 <[email protected]> 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/[email protected]/
>
> > 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.

--
Sakari Ailus

2020-04-07 16:41:24

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <[email protected]> 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 <[email protected]> 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/[email protected]/
>
> 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.

--
Regards,

Sakari Ailus

2020-04-07 16:48:11

by Tomasz Figa

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

On Tue, Apr 7, 2020 at 6:40 PM Sakari Ailus <[email protected]> wrote:
>
> On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <[email protected]> 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 <[email protected]> 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/[email protected]/
> >
> > 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?

2020-04-07 17:22:47

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

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 <[email protected]> wrote:
> >
> > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <[email protected]> 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 <[email protected]> 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/[email protected]/
> > >
> > > 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.

--
Sakari Ailus

2020-04-08 13:47:56

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

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 <[email protected]> wrote:
> > >
> > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <[email protected]> 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 <[email protected]> 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/[email protected]/
> > > >
> > > > 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

Attachments:

(No filename) (5.09 kB)
signature.asc (235.00 B)
Download all attachments

2020-04-08 13:54:57

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

On Wed, Apr 08, 2020 at 02:35:28PM +0200, Tomasz Figa wrote:
> On Wed, Apr 8, 2020 at 2:21 PM Maxime Ripard <[email protected]> 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 <[email protected]> wrote:
> > > > >
> > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <[email protected]> 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 <[email protected]> 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/[email protected]/
> > > > > >
> > > > > > 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

Attachments:

(No filename) (5.64 kB)
signature.asc (235.00 B)
Download all attachments

2020-04-08 14:23:44

by Tomasz Figa

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

On Wed, Apr 8, 2020 at 2:21 PM Maxime Ripard <[email protected]> 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 <[email protected]> wrote:
> > > >
> > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <[email protected]> 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 <[email protected]> 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/[email protected]/
> > > > >
> > > > > 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

2020-04-08 17:25:54

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

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 <[email protected]> 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 <[email protected]> wrote:
> > > > > >
> > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <[email protected]> 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 <[email protected]> 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/[email protected]/
> > > > > > >
> > > > > > > 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

2020-04-08 17:33:50

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

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 <[email protected]> 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 <[email protected]> wrote:
> > > > > > >
> > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <[email protected]> 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 <[email protected]> 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/[email protected]/
> > > > > > > >
> > > > > > > > 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

--
Sakari Ailus

2020-04-08 18:30:45

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

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.

--
With Best Regards,
Andy Shevchenko

2020-04-09 08:33:53

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

Hi Maxime,

On Wed, 8 Apr 2020 at 17:30, Sakari Ailus <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
> > > > > > >
> > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <[email protected]> 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 <[email protected]> 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/[email protected]/
> > > > > > > >
> > > > > > > > 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

2020-04-15 22:56:36

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

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 <[email protected]> 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 <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <[email protected]> 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 <[email protected]> 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/[email protected]/
> > > > > > > > >
> > > > > > > > > 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

Attachments:

(No filename) (7.72 kB)
signature.asc (235.00 B)
Download all attachments

2020-04-15 23:01:03

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

On Wed, 15 Apr 2020 at 12:18, Maxime Ripard <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > > > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <[email protected]> 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 <[email protected]> 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/[email protected]/
> > > > > > > > > >
> > > > > > > > > > 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

2020-04-16 00:19:22

[permalink] [raw]

Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings

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 <[email protected]> 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 <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> > > > > > > > > > On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <[email protected]> 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 <[email protected]> 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/[email protected]/
> > > > > > > > > >
> > > > > > > > > > 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.

--
Regards,

Sakari Ailus

2020-04-20 15:20:18