| Message ID | 20190604144714.2009-2-glaroque@baylibre.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add support of New Amlogic temperature sensor for G12A SoCs | expand |
Hi Guillaume, thank you for working on this! On Tue, Jun 4, 2019 at 4:47 PM Guillaume La Roque <glaroque@baylibre.com> wrote: > > This adds the devicetree binding documentation for the Temperature > Sensor found in the Amlogic Meson G12 SoCs. > Currently only the G12A SoCs are supported. so G12B is not supported (yet)? > Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> > --- > .../iio/temperature/amlogic,meson-tsensor.txt | 31 +++++++++++++++++++ > 1 file changed, 31 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt > > diff --git a/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt > new file mode 100644 > index 000000000000..d064db0e9cac > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt > @@ -0,0 +1,31 @@ > +* Amlogic Meson Temperature Sensor > + > +Required properties: > +- compatible: depending on the SoC and the position of the sensor, > + this should be one of: > + - "amlogic,meson-g12a-cpu-tsensor" for the CPU G12A SoC sensor > + - "amlogic,meson-g12a-ddr-tsensor" for the DDR G12A SoC sensor > + followed by the common : > + - "amlogic,meson-g12a-tsensor" for G12A SoC family > +- reg: the physical base address and length of the registers > +- interrupts: the interrupt indicating end of sampling > +- clocks: phandle identifier for the reference clock of temperature sensor > +- #io-channel-cells: must be 1, see ../iio-bindings.txt have you considered using the thermal framework [0] instead of the iio framework (see below)? > +- amlogic,ao-secure: phandle to the ao-secure syscon the driver has some "u_efuse_off" access. do we need to get some calibration values from the AO syscon or can we also fetch it from the eFuse? you can look at arch/arm/boot/dts/meson8.dtsi where I'm passing the temperature sensor calibration data to the SAR ADC (there's no dedicated temperature sensor IP block prior to G12A) while reading the data from the eFuse > +Optional properties: > +- amlogic,critical-temperature: temperature value in milli degrees Celsius > + to set automatic reboot on too high temperature as far as I can tell the thermal framework supports multiple trip points. I'm seeing this as a benefit because the hardware can raise interrupts at four different temperatures (defined by the driver) > +Example: > + cpu_temp: temperature-sensor@ff634800 { > + compatible = "amlogic,meson-g12a-cpu-tsensor", > + "amlogic,meson-g12a-tsensor"; > + reg = <0x0 0xff634800 0x0 0x50>; > + interrupts = <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>; > + clocks = <&clkc CLKID_TS>; > + status = "okay"; as far as I know the dt-bindings should not have a status property in the examples Martin
On Tue, 4 Jun 2019 16:47:12 +0200 Guillaume La Roque <glaroque@baylibre.com> wrote: > This adds the devicetree binding documentation for the Temperature > Sensor found in the Amlogic Meson G12 SoCs. > Currently only the G12A SoCs are supported. > > Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> Hi Guillaume, I'm afraid we decided a month or so back that all new dt bindings proposed for IIO drivers should be in yaml format. Please reformat this appropriately for v2 and make sure to run make dt_bindings_check. There are a few examples in tree already, but we are deliberately not converting existing bindings too quickly to avoid overloading reviewers. Thanks, Jonathan > --- > .../iio/temperature/amlogic,meson-tsensor.txt | 31 +++++++++++++++++++ > 1 file changed, 31 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt > > diff --git a/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt > new file mode 100644 > index 000000000000..d064db0e9cac > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt > @@ -0,0 +1,31 @@ > +* Amlogic Meson Temperature Sensor > + > +Required properties: > +- compatible: depending on the SoC and the position of the sensor, > + this should be one of: > + - "amlogic,meson-g12a-cpu-tsensor" for the CPU G12A SoC sensor > + - "amlogic,meson-g12a-ddr-tsensor" for the DDR G12A SoC sensor > + followed by the common : > + - "amlogic,meson-g12a-tsensor" for G12A SoC family > +- reg: the physical base address and length of the registers > +- interrupts: the interrupt indicating end of sampling > +- clocks: phandle identifier for the reference clock of temperature sensor > +- #io-channel-cells: must be 1, see ../iio-bindings.txt > +- amlogic,ao-secure: phandle to the ao-secure syscon > + > +Optional properties: > +- amlogic,critical-temperature: temperature value in milli degrees Celsius > + to set automatic reboot on too high temperature > + > +Example: > + cpu_temp: temperature-sensor@ff634800 { > + compatible = "amlogic,meson-g12a-cpu-tsensor", > + "amlogic,meson-g12a-tsensor"; > + reg = <0x0 0xff634800 0x0 0x50>; > + interrupts = <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>; > + clocks = <&clkc CLKID_TS>; > + status = "okay"; > + #io-channel-cells = <1>; > + amlogic,meson-ao-secure = <&sec_AO>; > + amlogic,critical-temperature = <115000>; > + };
On Thu, 6 Jun 2019 21:16:37 +0200 Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > Hi Guillaume, > > thank you for working on this! On comment from me inline. Thanks, Jonathan > > On Tue, Jun 4, 2019 at 4:47 PM Guillaume La Roque <glaroque@baylibre.com> wrote: > > > > This adds the devicetree binding documentation for the Temperature > > Sensor found in the Amlogic Meson G12 SoCs. > > Currently only the G12A SoCs are supported. > so G12B is not supported (yet)? > > > Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> > > --- > > .../iio/temperature/amlogic,meson-tsensor.txt | 31 +++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt > > > > diff --git a/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt > > new file mode 100644 > > index 000000000000..d064db0e9cac > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt > > @@ -0,0 +1,31 @@ > > +* Amlogic Meson Temperature Sensor > > + > > +Required properties: > > +- compatible: depending on the SoC and the position of the sensor, > > + this should be one of: > > + - "amlogic,meson-g12a-cpu-tsensor" for the CPU G12A SoC sensor > > + - "amlogic,meson-g12a-ddr-tsensor" for the DDR G12A SoC sensor > > + followed by the common : > > + - "amlogic,meson-g12a-tsensor" for G12A SoC family > > +- reg: the physical base address and length of the registers > > +- interrupts: the interrupt indicating end of sampling > > +- clocks: phandle identifier for the reference clock of temperature sensor > > +- #io-channel-cells: must be 1, see ../iio-bindings.txt > have you considered using the thermal framework [0] instead of the iio > framework (see below)? I had the same thought. Right now, this doesn't look generic enough for IIO to make that much sense. I'll review anyway as may give some useful pointers even if you do move it over to thermal. > > > +- amlogic,ao-secure: phandle to the ao-secure syscon > the driver has some "u_efuse_off" access. do we need to get some > calibration values from the AO syscon or can we also fetch it from the > eFuse? you can look at arch/arm/boot/dts/meson8.dtsi where I'm passing > the temperature sensor calibration data to the SAR ADC (there's no > dedicated temperature sensor IP block prior to G12A) while reading the > data from the eFuse > > > +Optional properties: > > +- amlogic,critical-temperature: temperature value in milli degrees Celsius > > + to set automatic reboot on too high temperature > as far as I can tell the thermal framework supports multiple trip > points. I'm seeing this as a benefit because the hardware can raise > interrupts at four different temperatures (defined by the driver) > > > +Example: > > + cpu_temp: temperature-sensor@ff634800 { > > + compatible = "amlogic,meson-g12a-cpu-tsensor", > > + "amlogic,meson-g12a-tsensor"; > > + reg = <0x0 0xff634800 0x0 0x50>; > > + interrupts = <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>; > > + clocks = <&clkc CLKID_TS>; > > + status = "okay"; > as far as I know the dt-bindings should not have a status property in > the examples > > > Martin
On 06/06/2019 21:16, Martin Blumenstingl wrote: > Hi Guillaume, > > thank you for working on this! > > On Tue, Jun 4, 2019 at 4:47 PM Guillaume La Roque <glaroque@baylibre.com> wrote: >> >> This adds the devicetree binding documentation for the Temperature >> Sensor found in the Amlogic Meson G12 SoCs. >> Currently only the G12A SoCs are supported. > so G12B is not supported (yet)? G12B is 95% similar as G12A, it will certainly use slighly different values. > >> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> >> --- >> .../iio/temperature/amlogic,meson-tsensor.txt | 31 +++++++++++++++++++ >> 1 file changed, 31 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt >> >> diff --git a/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt >> new file mode 100644 >> index 000000000000..d064db0e9cac >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt >> @@ -0,0 +1,31 @@ >> +* Amlogic Meson Temperature Sensor >> + >> +Required properties: >> +- compatible: depending on the SoC and the position of the sensor, >> + this should be one of: >> + - "amlogic,meson-g12a-cpu-tsensor" for the CPU G12A SoC sensor >> + - "amlogic,meson-g12a-ddr-tsensor" for the DDR G12A SoC sensor >> + followed by the common : >> + - "amlogic,meson-g12a-tsensor" for G12A SoC family >> +- reg: the physical base address and length of the registers >> +- interrupts: the interrupt indicating end of sampling >> +- clocks: phandle identifier for the reference clock of temperature sensor >> +- #io-channel-cells: must be 1, see ../iio-bindings.txt > have you considered using the thermal framework [0] instead of the iio > framework (see below)? Question: why thermal, and not hwmon ? what's the main difference ? > >> +- amlogic,ao-secure: phandle to the ao-secure syscon > the driver has some "u_efuse_off" access. do we need to get some > calibration values from the AO syscon or can we also fetch it from the > eFuse? you can look at arch/arm/boot/dts/meson8.dtsi where I'm passing > the temperature sensor calibration data to the SAR ADC (there's no > dedicated temperature sensor IP block prior to G12A) while reading the > data from the eFuse > >> +Optional properties: >> +- amlogic,critical-temperature: temperature value in milli degrees Celsius >> + to set automatic reboot on too high temperature > as far as I can tell the thermal framework supports multiple trip > points. I'm seeing this as a benefit because the hardware can raise > interrupts at four different temperatures (defined by the driver) Theoretically, but the implementation code differs a lot from the datasheet. > >> +Example: >> + cpu_temp: temperature-sensor@ff634800 { >> + compatible = "amlogic,meson-g12a-cpu-tsensor", >> + "amlogic,meson-g12a-tsensor"; >> + reg = <0x0 0xff634800 0x0 0x50>; >> + interrupts = <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>; >> + clocks = <&clkc CLKID_TS>; >> + status = "okay"; > as far as I know the dt-bindings should not have a status property in > the examples > > > Martin > > _______________________________________________ > linux-amlogic mailing list > linux-amlogic@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-amlogic >
Hi Neil, On Tue, Jun 11, 2019 at 1:01 PM Neil Armstrong <narmstrong@baylibre.com> wrote: > > On 06/06/2019 21:16, Martin Blumenstingl wrote: > > Hi Guillaume, > > > > thank you for working on this! > > > > On Tue, Jun 4, 2019 at 4:47 PM Guillaume La Roque <glaroque@baylibre.com> wrote: > >> > >> This adds the devicetree binding documentation for the Temperature > >> Sensor found in the Amlogic Meson G12 SoCs. > >> Currently only the G12A SoCs are supported. > > so G12B is not supported (yet)? > > G12B is 95% similar as G12A, it will certainly use slighly different values. OK, thank you for clarifying this as far as I can tell Guillaume's code is already prepared for that (as there's a per-instance specific struct with settings for the specific instance) which is good to know > > > >> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> > >> --- > >> .../iio/temperature/amlogic,meson-tsensor.txt | 31 +++++++++++++++++++ > >> 1 file changed, 31 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt > >> > >> diff --git a/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt > >> new file mode 100644 > >> index 000000000000..d064db0e9cac > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt > >> @@ -0,0 +1,31 @@ > >> +* Amlogic Meson Temperature Sensor > >> + > >> +Required properties: > >> +- compatible: depending on the SoC and the position of the sensor, > >> + this should be one of: > >> + - "amlogic,meson-g12a-cpu-tsensor" for the CPU G12A SoC sensor > >> + - "amlogic,meson-g12a-ddr-tsensor" for the DDR G12A SoC sensor > >> + followed by the common : > >> + - "amlogic,meson-g12a-tsensor" for G12A SoC family > >> +- reg: the physical base address and length of the registers > >> +- interrupts: the interrupt indicating end of sampling > >> +- clocks: phandle identifier for the reference clock of temperature sensor > >> +- #io-channel-cells: must be 1, see ../iio-bindings.txt > > have you considered using the thermal framework [0] instead of the iio > > framework (see below)? > > Question: why thermal, and not hwmon ? what's the main difference ? this is what came to my mind why the thermal framework fits best (at least based on my current knowledge): a) the thermal-zones (see meson-gxm-khadas-vim2.dts for example) a "thermal-sensors" property. so for active (with a fan) or passive (by limiting the maximum frequency and thus the supply voltage) cooling we need a thermal device anyways b) the thermal bindings support multiple trip points. we can map them to one of the four interrupts which the IP block can generate c) defining a temperature where the chip will power off sounds like a use-case which may be implemented by other thermal IP blocks (in other words: maybe the thermal frameworks provides some generic property to replace the "amlogic,critical-temperature" property) d) as far as I know you can tell the thermal framework to create a hwmon device with only a couple (5?) lines of code as Guillaume has already shown we can implement c) with a custom property, but that's not limited to the underlying framework (IIO, hwmon, thermal, ...) use-case d) is not a strong one because I'm using iio-hwmon to create a hwmon device on the 32-bit SoCs. however, together with case a) using an IIO driver is going to be more difficult because currently there's "only" a "generic-adc-thermal" binding (but not a "generic-iio-temperature-thermal" binding) the initial driver version doesn't have to support everything I listed above. however, I believe with the thermal framework we don't limit ourselves to one use-case and can extend the driver in the future Martin
diff --git a/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt new file mode 100644 index 000000000000..d064db0e9cac --- /dev/null +++ b/Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt @@ -0,0 +1,31 @@ +* Amlogic Meson Temperature Sensor + +Required properties: +- compatible: depending on the SoC and the position of the sensor, + this should be one of: + - "amlogic,meson-g12a-cpu-tsensor" for the CPU G12A SoC sensor + - "amlogic,meson-g12a-ddr-tsensor" for the DDR G12A SoC sensor + followed by the common : + - "amlogic,meson-g12a-tsensor" for G12A SoC family +- reg: the physical base address and length of the registers +- interrupts: the interrupt indicating end of sampling +- clocks: phandle identifier for the reference clock of temperature sensor +- #io-channel-cells: must be 1, see ../iio-bindings.txt +- amlogic,ao-secure: phandle to the ao-secure syscon + +Optional properties: +- amlogic,critical-temperature: temperature value in milli degrees Celsius + to set automatic reboot on too high temperature + +Example: + cpu_temp: temperature-sensor@ff634800 { + compatible = "amlogic,meson-g12a-cpu-tsensor", + "amlogic,meson-g12a-tsensor"; + reg = <0x0 0xff634800 0x0 0x50>; + interrupts = <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>; + clocks = <&clkc CLKID_TS>; + status = "okay"; + #io-channel-cells = <1>; + amlogic,meson-ao-secure = <&sec_AO>; + amlogic,critical-temperature = <115000>; + };
This adds the devicetree binding documentation for the Temperature Sensor found in the Amlogic Meson G12 SoCs. Currently only the G12A SoCs are supported. Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> --- .../iio/temperature/amlogic,meson-tsensor.txt | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/temperature/amlogic,meson-tsensor.txt