| Message ID | 20180830154518.29507-28-embed3d@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IIO-based thermal sensor driver for Allwinner H3 and A83T SoC | expand |
Hi Philipp, On Thu, 30 Aug 2018 17:45:15 +0200 Philipp Rossak <embed3d@gmail.com> wrote: > The H3 SID is supported by the kernel so we can add a NVMEM Data cell, > that contains the calibration data. > > On the H3 the eFuses are located at the offset 0x200. The thermal data > itself has an offset of 0x34 from the eFuse base. So we end on an offset > of 0x234. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > --- > arch/arm/boot/dts/sun8i-h3.dtsi | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi > index 1866aec69ec1..0fc447f0c02a 100644 > --- a/arch/arm/boot/dts/sun8i-h3.dtsi > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi > @@ -106,8 +106,15 @@ > > soc { > sid: eeprom@1c14000 { > + #address-cells = <1>; > + #size-cells = <1>; > compatible = "allwinner,sun8i-h3-sid"; > reg = <0x01c14000 0x400>; > + > + /* Data cells */ > + thermal_calibration: calib@234 { > + reg = <0x234 0x8>; > + }; You are declaring 8 bytes of calibration data but to my knowledge it's only 2 bytes per sensor, so 2 bytes for H3. Am I missing something ? Thanks, > }; > }; > > @@ -227,4 +234,6 @@ > &ths { > compatible = "allwinner,sun8i-h3-ths"; > #thermal-sensor-cells = <0>; > + nvmem-cells = <&thermal_calibration>; > + nvmem-cell-names = "calibration"; > }; > -- > 2.11.0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 04.09.2018 18:46, Emmanuel Vadot wrote: >> + /* Data cells */ >> + thermal_calibration: calib@234 { >> + reg = <0x234 0x8>; >> + }; > You are declaring 8 bytes of calibration data but to my knowledge it's > only 2 bytes per sensor, so 2 bytes for H3. > Am I missing something ? > > Thanks, Emmanuel you are right, it is 2 bytes per Sensor and should be 2 bytes for H3, but the thermal calibration data field is on all chips 64 bit wide - so 8 bytes. So I'm reading here the complete calibration data field. Philipp
On Thu, Sep 06, 2018 at 01:47:47PM +0200, Philipp Rossak wrote: > On 04.09.2018 18:46, Emmanuel Vadot wrote: > > > + /* Data cells */ > > > + thermal_calibration: calib@234 { > > > + reg = <0x234 0x8>; > > > + }; > > You are declaring 8 bytes of calibration data but to my knowledge it's > > only 2 bytes per sensor, so 2 bytes for H3. > > Am I missing something ? > > > > Thanks, > > Emmanuel you are right, it is 2 bytes per Sensor and should be 2 bytes for > H3, but the thermal calibration data field is on all chips 64 bit wide - so > 8 bytes. So I'm reading here the complete calibration data field. Having one cell per channel would make more sense I guess. Maxime
于 2018年9月6日 GMT+08:00 下午7:51:15, Maxime Ripard <maxime.ripard@bootlin.com> 写到: >On Thu, Sep 06, 2018 at 01:47:47PM +0200, Philipp Rossak wrote: >> On 04.09.2018 18:46, Emmanuel Vadot wrote: >> > > + /* Data cells */ >> > > + thermal_calibration: calib@234 { >> > > + reg = <0x234 0x8>; >> > > + }; >> > You are declaring 8 bytes of calibration data but to my knowledge >it's >> > only 2 bytes per sensor, so 2 bytes for H3. >> > Am I missing something ? >> > >> > Thanks, >> >> Emmanuel you are right, it is 2 bytes per Sensor and should be 2 >bytes for >> H3, but the thermal calibration data field is on all chips 64 bit >wide - so >> 8 bytes. So I'm reading here the complete calibration data field. > >Having one cell per channel would make more sense I guess. I have mentioned that this is impossible because of wrong addressing caused by LE2BE in SID driver. > >Maxime
On 06.09.2018 14:04, Icenowy Zheng wrote: > > > 于 2018年9月6日 GMT+08:00 下午7:51:15, Maxime Ripard <maxime.ripard@bootlin.com> 写到: >> On Thu, Sep 06, 2018 at 01:47:47PM +0200, Philipp Rossak wrote: >>> On 04.09.2018 18:46, Emmanuel Vadot wrote: >>>>> + /* Data cells */ >>>>> + thermal_calibration: calib@234 { >>>>> + reg = <0x234 0x8>; >>>>> + }; >>>> You are declaring 8 bytes of calibration data but to my knowledge >> it's >>>> only 2 bytes per sensor, so 2 bytes for H3. >>>> Am I missing something ? >>>> >>>> Thanks, >>> >>> Emmanuel you are right, it is 2 bytes per Sensor and should be 2 >> bytes for >>> H3, but the thermal calibration data field is on all chips 64 bit >> wide - so >>> 8 bytes. So I'm reading here the complete calibration data field. >> >> Having one cell per channel would make more sense I guess. Ok I will change this. > > I have mentioned that this is impossible because of wrong > addressing caused by LE2BE in SID driver. > I know! But I would like to prepare patches for it, that they can be merged when this is fixed. Philipp
Sorry for resurrecting an old discussion, but since someone posted patches for H5 and H6, I thought we should resolve this. I'm working on patches to fix / replace the big-endian issue. On Thu, Sep 6, 2018 at 7:51 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Thu, Sep 06, 2018 at 01:47:47PM +0200, Philipp Rossak wrote: > > On 04.09.2018 18:46, Emmanuel Vadot wrote: > > > > + /* Data cells */ > > > > + thermal_calibration: calib@234 { > > > > + reg = <0x234 0x8>; > > > > + }; > > > You are declaring 8 bytes of calibration data but to my knowledge it's > > > only 2 bytes per sensor, so 2 bytes for H3. > > > Am I missing something ? > > > > > > Thanks, > > > > Emmanuel you are right, it is 2 bytes per Sensor and should be 2 bytes for > > H3, but the thermal calibration data field is on all chips 64 bit wide - so > > 8 bytes. So I'm reading here the complete calibration data field. > > Having one cell per channel would make more sense I guess. Would it? The 2 32-bit words directly map onto the registers 0x74 / 0x78 in the THS. As far as the SID is concerned, their is just one consumer for this data, the thermal sensor. How the thermal sensor uses that data is really not its concern. And the thermal sensor is really just copying the data from the e-fuses into its registers. Nothing more. Furthermore, with the register access interface, the e-fuses are read/write 32 bits at a time. Seems to me it would make more sense to enforce a 32-bit word size, so cells should be multiples of 32 bits. Regards ChenYu
On Tue, Feb 19, 2019 at 03:54:00PM +0800, Chen-Yu Tsai wrote: > Sorry for resurrecting an old discussion, but since someone posted patches > for H5 and H6, I thought we should resolve this. I'm working on patches to > fix / replace the big-endian issue. > > On Thu, Sep 6, 2018 at 7:51 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > On Thu, Sep 06, 2018 at 01:47:47PM +0200, Philipp Rossak wrote: > > > On 04.09.2018 18:46, Emmanuel Vadot wrote: > > > > > + /* Data cells */ > > > > > + thermal_calibration: calib@234 { > > > > > + reg = <0x234 0x8>; > > > > > + }; > > > > You are declaring 8 bytes of calibration data but to my knowledge it's > > > > only 2 bytes per sensor, so 2 bytes for H3. > > > > Am I missing something ? > > > > > > > > Thanks, > > > > > > Emmanuel you are right, it is 2 bytes per Sensor and should be 2 bytes for > > > H3, but the thermal calibration data field is on all chips 64 bit wide - so > > > 8 bytes. So I'm reading here the complete calibration data field. > > > > Having one cell per channel would make more sense I guess. > > Would it? The 2 32-bit words directly map onto the registers 0x74 / 0x78 in > the THS. As far as the SID is concerned, their is just one consumer for this > data, the thermal sensor. How the thermal sensor uses that data is really not > its concern. And the thermal sensor is really just copying the data from the > e-fuses into its registers. Nothing more. > > Furthermore, with the register access interface, the e-fuses are read/write > 32 bits at a time. Seems to me it would make more sense to enforce a 32-bit > word size, so cells should be multiples of 32 bits. I guess you convinced me :) Maxime
On Tue, 19 Feb 2019 15:54:00 +0800 Chen-Yu Tsai <wens@csie.org> wrote: > Sorry for resurrecting an old discussion, but since someone posted patches > for H5 and H6, I thought we should resolve this. I'm working on patches to > fix / replace the big-endian issue. > > On Thu, Sep 6, 2018 at 7:51 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > On Thu, Sep 06, 2018 at 01:47:47PM +0200, Philipp Rossak wrote: > > > On 04.09.2018 18:46, Emmanuel Vadot wrote: > > > > > + /* Data cells */ > > > > > + thermal_calibration: calib@234 { > > > > > + reg = <0x234 0x8>; > > > > > + }; > > > > You are declaring 8 bytes of calibration data but to my knowledge it's > > > > only 2 bytes per sensor, so 2 bytes for H3. > > > > Am I missing something ? > > > > > > > > Thanks, > > > > > > Emmanuel you are right, it is 2 bytes per Sensor and should be 2 bytes for > > > H3, but the thermal calibration data field is on all chips 64 bit wide - so > > > 8 bytes. So I'm reading here the complete calibration data field. > > > > Having one cell per channel would make more sense I guess. > > Would it? The 2 32-bit words directly map onto the registers 0x74 / 0x78 in > the THS. As far as the SID is concerned, their is just one consumer for this > data, the thermal sensor. How the thermal sensor uses that data is really not > its concern. And the thermal sensor is really just copying the data from the > e-fuses into its registers. Nothing more. If you see this at the controller lever and not at the sensor level it might make sense to use the full 32bits data even some bits are ignored I guess. > Furthermore, with the register access interface, the e-fuses are read/write > 32 bits at a time. Seems to me it would make more sense to enforce a 32-bit > word size, so cells should be multiples of 32 bits. Ok, honestly at this point I don't really care what choice is done as long as the dts is updated, I'll deal with the diff for FreeBSD and ping the others BSD (Net and Open who also have a driver) for this change. > Regards > ChenYu Cheers,
On 19.02.19 08:54, Chen-Yu Tsai wrote: > Sorry for resurrecting an old discussion, but since someone posted patches > for H5 and H6, I thought we should resolve this. I'm working on patches to > fix / replace the big-endian issue. > > On Thu, Sep 6, 2018 at 7:51 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: >> >> On Thu, Sep 06, 2018 at 01:47:47PM +0200, Philipp Rossak wrote: >>> On 04.09.2018 18:46, Emmanuel Vadot wrote: >>>>> + /* Data cells */ >>>>> + thermal_calibration: calib@234 { >>>>> + reg = <0x234 0x8>; >>>>> + }; >>>> You are declaring 8 bytes of calibration data but to my knowledge it's >>>> only 2 bytes per sensor, so 2 bytes for H3. >>>> Am I missing something ? >>>> >>>> Thanks, >>> >>> Emmanuel you are right, it is 2 bytes per Sensor and should be 2 bytes for >>> H3, but the thermal calibration data field is on all chips 64 bit wide - so >>> 8 bytes. So I'm reading here the complete calibration data field. >> >> Having one cell per channel would make more sense I guess. > > Would it? The 2 32-bit words directly map onto the registers 0x74 / 0x78 in > the THS. As far as the SID is concerned, their is just one consumer for this > data, the thermal sensor. How the thermal sensor uses that data is really not > its concern. And the thermal sensor is really just copying the data from the > e-fuses into its registers. Nothing more. Using 2 32-bit words for the THS would be also ok (from my perspective). > > Furthermore, with the register access interface, the e-fuses are read/write > 32 bits at a time. Seems to me it would make more sense to enforce a 32-bit > word size, so cells should be multiples of 32 bits. > For THS I'm ok with that. > Regards > ChenYu > Regards, Philipp
diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi index 1866aec69ec1..0fc447f0c02a 100644 --- a/arch/arm/boot/dts/sun8i-h3.dtsi +++ b/arch/arm/boot/dts/sun8i-h3.dtsi @@ -106,8 +106,15 @@ soc { sid: eeprom@1c14000 { + #address-cells = <1>; + #size-cells = <1>; compatible = "allwinner,sun8i-h3-sid"; reg = <0x01c14000 0x400>; + + /* Data cells */ + thermal_calibration: calib@234 { + reg = <0x234 0x8>; + }; }; }; @@ -227,4 +234,6 @@ &ths { compatible = "allwinner,sun8i-h3-ths"; #thermal-sensor-cells = <0>; + nvmem-cells = <&thermal_calibration>; + nvmem-cell-names = "calibration"; };
The H3 SID is supported by the kernel so we can add a NVMEM Data cell, that contains the calibration data. On the H3 the eFuses are located at the offset 0x200. The thermal data itself has an offset of 0x34 from the eFuse base. So we end on an offset of 0x234. Signed-off-by: Philipp Rossak <embed3d@gmail.com> --- arch/arm/boot/dts/sun8i-h3.dtsi | 9 +++++++++ 1 file changed, 9 insertions(+)