Attachments:

(No filename) (453.00 B)
signature.asc (849.00 B)
Download all attachments

2018-08-31 09:52:58

On Fri, Aug 31, 2018 at 02:05:59PM +0200, Philipp Rossak wrote:
>
>
> On 31.08.2018 11:09, Maxime Ripard wrote:
> > > +static int sun4i_ths_suspend(struct sun4i_gpadc_iio *info)
> > suspend is already a hook in the kernel, which hasn't the same meaning
> > than runtime_suspend (and the same applies to resume), so we'd rather
> > pick a better name. And all the functions (and the driver) use gpadc,
> > please continue to use that prefix.
>
> I agree.
> For the newer sensors (from H3) the Sensor is referenced in the datasheets
> as Thermal Sensor short THS. So I would like to use for the newer sensors
> that prefix. Is that ok?

Not really. The consistency within the driver is more important.

Maxiem

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

(No filename) (820.00 B)
signature.asc (849.00 B)
Download all attachments

2018-09-03 10:21:55

On Mon, Sep 03, 2018 at 07:01:47PM +0800, Icenowy Zheng wrote:
> 于 2018年9月3日 GMT+08:00 下午6:20:22, Maxime Ripard <[email protected]> 写到:
> >On Fri, Aug 31, 2018 at 05:51:41PM +0800, Icenowy Zheng wrote:
> >> Personally I suggest to leave out all SID or calibration related
> >> patches here.
> >>
> >> Currently we seems to be wrongly converting SID to big endian,
> >however,
> >> the orgnization of the THS calibration data on H6 shows that it's
> >> surely little endian:
> >>
> >> It consists a temperature value in 1/10 celsuis as unit, and some
> >> thermal register readout values, which are the values read out at the
> >> given temperature, and every value here (the temperature and the
> >> readout) are all half word length.
> >>
> >> Let the temperature value be AABB, the two readout values be XXYY and
> >> ZZWW, the oragnization is:
> >> BB AA YY XX WW ZZ ** ** .
> >>
> >> When converting the SID to big endian, it becomes:
> >> XX YY AA BB ** ** ZZ WW ,
> >> which is non-sense, and not able to do sub-word cell addressing.
> >>
> >> Maxime, should I drop the LE2BE conversion in SID driver? (I doubt
> >> whether it will break compatibility.)
> >
> >This is exposed to the userspace, so no.
>
> Please note the LE2BE totally breaks the SID addressing.

On which SoCs?

> Without it dropped all cells must be referenced with 4 byte word as
> unit, and half word addressing of SID is thus not possible. The
> driver will also need then to split the half words if needed.
>
> I think this is kind of hardware misbehavior, and not a simple
> UAPI change, so the UAPI stability shouldn't affect this change.

How would you feel having your files on your filesystem suddenly not
in the same endianness and having all the bytes reordered? This is
definitely about the userspace ABI.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

(No filename) (1.92 kB)
signature.asc (849.00 B)
Download all attachments

2018-09-06 07:26:48

On Thu, Sep 06, 2018 at 01:39:43PM +0200, Philipp Rossak wrote:
> On 06.09.2018 09:24, Quentin Schulz wrote:
> > Hi Philipp,
> >
> > On Thu, Aug 30, 2018 at 05:45:18PM +0200, Philipp Rossak wrote:
> > > Since we have now thermal trotteling enabeled we can now add the full
> > > range of the OPP table.
> > >
> > That's not the reason why they were not added.
> >
> > Please see commit 2db639d8c1663d7543c9ab5323383d94c8a76c63[1].
> >
> > Basically, you only want the OPPs which can work below or at the default
> > voltage of the CPU supply, because the CPU supply is specific to each
> > board.
> >
> > If you set your CPU to work at a given frequency and the voltage isn't
> > updated (saying opp-microvolt = <x>; in DT isn't enough, you need
> > cpu-supply to be provided and functional), the CPU might just crash.
> >
> > Without cpu-supply property, underclocking isn't effective in term of
> > thermal cooling or power saving. Overclocking is very, very, very likely
> > to make the CPU crash.
> >
> > It's not a very difficult thing to do to test if a given frequency work
> > well but it needs a specific test environment and it's a lengthy test,
> > you can have a look at those tools here[3] if you like. It's not because
> > it works in a given test case that'll work on the long term under heavy
> > load and constant frequency changes.
> >
> > For A83T, I already did it and the outcome is the patch in [1]. Same for
> > A33.
> >
> > So, if you want to use these three higher OPPs, you need to define them
> > in your board DTS and add the cpu-supply property. See what's done for
> > the A33 and more specifically the Sinlinx SinA33[2] as an example.
> >
> > [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2db639d8c1663d7543c9ab5323383d94c8a76c63
> > [2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts
> > [3]http://linux-sunxi.org/Hardware_Reliability_Tests#CPU
> >
> > Quentin
> >
>
> Hey Quentin,
>
> thanks for your feedback!
>
> Sounds like we will never be able to run the A83T on its maximum frequency
> in mainline.
>
> I will do some testing, during the next weeks/months when I have time.
> With the old Allwinner kernel I was able to run the A83T with its maximum
> frequency without any problems since my board is very good cooled.

You definitely can, but I think Quentin's point was to do it on a
per-board basis, not for all of the A83t boards at once.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

(No filename) (2.60 kB)
signature.asc (849.00 B)
Download all attachments

2018-09-06 12:06:16

by Icenowy Zheng

[permalink] [raw]

Subject: Re: [linux-sunxi] Re: [PATCH v3 27/30] ARM: dts: sun8i: h3: use calibration for ths

于 2018年9月6日 GMT+08:00 下午7:51:15, Maxime Ripard <[email protected]> 写到:
>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

2018-09-06 12:07:55

by Quentin Schulz

[permalink] [raw]

Subject: Re: [PATCH v3 30/30] ARM: sun8i: a83t: full range OPP tables and CPUfreq

Hi Maxime, Philipp,

On Thu, Sep 06, 2018 at 01:42:41PM +0200, Maxime Ripard wrote:
> On Thu, Sep 06, 2018 at 01:39:43PM +0200, Philipp Rossak wrote:
> > On 06.09.2018 09:24, Quentin Schulz wrote:
> > > Hi Philipp,
> > >
> > > On Thu, Aug 30, 2018 at 05:45:18PM +0200, Philipp Rossak wrote:
> > > > Since we have now thermal trotteling enabeled we can now add the full
> > > > range of the OPP table.
> > > >
> > > That's not the reason why they were not added.
> > >
> > > Please see commit 2db639d8c1663d7543c9ab5323383d94c8a76c63[1].
> > >
> > > Basically, you only want the OPPs which can work below or at the default
> > > voltage of the CPU supply, because the CPU supply is specific to each
> > > board.
> > >
> > > If you set your CPU to work at a given frequency and the voltage isn't
> > > updated (saying opp-microvolt = <x>; in DT isn't enough, you need
> > > cpu-supply to be provided and functional), the CPU might just crash.
> > >
> > > Without cpu-supply property, underclocking isn't effective in term of
> > > thermal cooling or power saving. Overclocking is very, very, very likely
> > > to make the CPU crash.
> > >
> > > It's not a very difficult thing to do to test if a given frequency work
> > > well but it needs a specific test environment and it's a lengthy test,
> > > you can have a look at those tools here[3] if you like. It's not because
> > > it works in a given test case that'll work on the long term under heavy
> > > load and constant frequency changes.
> > >
> > > For A83T, I already did it and the outcome is the patch in [1]. Same for
> > > A33.
> > >
> > > So, if you want to use these three higher OPPs, you need to define them
> > > in your board DTS and add the cpu-supply property. See what's done for
> > > the A33 and more specifically the Sinlinx SinA33[2] as an example.
> > >
> > > [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2db639d8c1663d7543c9ab5323383d94c8a76c63
> > > [2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts
> > > [3]http://linux-sunxi.org/Hardware_Reliability_Tests#CPU
> > >
> > > Quentin
> > >
> >
> > Hey Quentin,
> >
> > thanks for your feedback!
> >
> > Sounds like we will never be able to run the A83T on its maximum frequency
> > in mainline.
> >
> > I will do some testing, during the next weeks/months when I have time.
> > With the old Allwinner kernel I was able to run the A83T with its maximum
> > frequency without any problems since my board is very good cooled.
>
> You definitely can, but I think Quentin's point was to do it on a
> per-board basis, not for all of the A83t boards at once.
>

Yes, exactly. That's what we did for the Sinlinx SinA33. Just take
inspiration from it.

You can run the CPU at max frequency with the old Allwinner kernel
because it most likely updates the CPU voltage. Let's say overheating is
not an issue, you still need to have a way to overclock AND overvolt to
the values in the OPP you're targetting. Your CPU won't be stable with
just overclocking without overvolting it, trust me, I've been there.

It can even be possible that the Allwinner BSP is only stable in your
use case.

The CPU voltage is specific to every board (cpu-supply property). So we
do not declare the CPU frequencies that require overvolting since we
cannot be sure a board will define the cpu-supply property and thus, be
able to overvolt the CPU.

So, though thermal throttling is an important feature because
overheating the CPU too much will shut it down on the hardware level
(among other consequences), it does not make the higher OPPs magically
work without a valid and working cpu-supply.

The link I gave you is an important piece of software to test the
stability of a system under heavy load and a lot of frequency changes.
It's important to know that because it works for you in your use case
doesn't make it work in any use case. Testing with these pieces of
software helps to cover more hardcore use cases but isn't perfect of
course. That's a start though.

Quentin

Attachments:

(No filename) (4.09 kB)
signature.asc (849.00 B)
Download all attachments

2018-09-06 17:53:51

On Mon, Sep 10, 2018 at 02:44:24PM -0500, Rob Herring wrote:
> On Fri, Aug 31, 2018 at 10:48:54AM +0200, Maxime Ripard wrote:
> > On Thu, Aug 30, 2018 at 05:45:02PM +0200, Philipp Rossak wrote:
> > > Allwinner H3 features a thermal sensor like the one in A33, but has its
> > > register re-arranged, the clock divider moved to CCU (originally the
> > > clock divider is in ADC) and added a pair of bus clock and reset.
> > >
> > > Allwinner A83T features a thermal sensor similar to the H3, the ths clock,
> > > the bus clock and the reset was removed from the CCU. The THS in A83T
> > > has a clock that is directly connected and runs with 24 MHz.
> > >
> > > Update the binding document to cover H3 and A83T.
> > >
> > > Signed-off-by: Philipp Rossak <[email protected]>
> >
> > You probably want to have a look at:
> > https://www.spinics.net/lists/arm-kernel/msg670167.html
>
> Well, which is it? An ADC or thermal sensor?

It's both actually. This IP used to be called GPADC, and had a thermal
sensor + some ADC channels. The design evolved across several
generations of SoCs to drop the ADC channels and be used only to have
thermal sensors.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

(No filename) (1.25 kB)
signature.asc (849.00 B)
Download all attachments

2019-02-19 07:54:49

by Chen-Yu Tsai

[permalink] [raw]

Subject: Allwinner SID THS calibration data cell representation?

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

2019-02-20 14:56:51

On Tue, Mar 19, 2019 at 12:30:25PM +0000, M?ns Rullg?rd wrote:
> Philipp Rossak <[email protected]> writes:
>
> > Allwiner H3 and A83T SoCs have a thermal sensor, which is a large refactored
> > version of the old Allwinner "GPADC" (although it have already only
> > thermal part left in A33).
> >
> > This patch tried to add support for the sensor in H3 and A83T based on
> >
> > This Patchtseries was in the beginning based on Icenowy Zengs v4 patchseries [1]. Since we decided to merge the mfd driver into the GPADC this changed. So only one patch could be reused.
> >
> > Patches that adds support for H5, A64, A80 and H6 SoCs are allready prepared,
> > and will be upstreamed if this patchseries is applied and the testing is done.
> >
> > Sorry for delaying this.
> >
> > Regards,
> > Philipp
> >
> > changes since v2:
> > * mfd driver is now merged into the gpadc driver
> > * complete rework
> >
> > changes since v1:
> > * collecting all acks
> > * rewording commits/fix typos
> > * move code in place where it is used
> > * fix naming conventions of defines
> > * clarify commits
> > * update documentation to cover the new nvmem calibraion
> > * change nvmem calibration
> >
> > Icenowy Zheng (1):
> > iio: adc: sun4i-gpadc-iio: rename A33-specified registers to contain
> > A33
> >
> > Philipp Rossak (29):
> > mfd: Makefile: Remove build option for MFD:sun4i-gpadc
> > mfd: Kconfig: Remove MFD_SUN4I_GPADC config option
> > iio: adc: Remove ID table
> > iio: adc: Kconfig: Update Kconfig to new build options
> > iio: adc: move SUN4I_GPADC_CHANNEL define to header file
> > iio: adc: remove ofnode options
> > iio: adc: remove mfd_probe & sunwi_irq_init function
> > iio: adc: remove hwmon structure
> > iio: adc: Threat A33 as thermal sensor and remove non thermal sun4i
> > channel
> > iio: adc: rework irq and adc_channel handling
> > iio: adc: add new compatibles
> > mfd: Remove old mfd driver & Move sun4i-gpadc.h to iio/adc/
> > arm: config: Enable SUN4I_GPADC in defconfig
> > dt-bindings: update the Allwinner GPADC device tree binding for H3 &
> > A83T
> > iio: adc: sun4i-gpadc-iio: rework: readout temp_data
> > iio: adc: sun4i-gpadc-iio: rework: support clocks and reset
> > iio: adc: sun4i-gpadc-iio: rework: support multiple sensors
> > iio: adc: sun4i-gpadc-iio: rework: support nvmem calibration data
> > iio: adc: sun4i-gpadc-iio: rework: device specific suspend & resume
> > iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor
> > iio: adc: sun4i-gpadc-iio: add support for A83T thermal sensor
> > ARM: dts: sunxi-h3-h5: add support for the thermal sensor in H3 and H5
> > ARM: dts: sun8i: h3: add support for the thermal sensor in H3
> > ARM: dts: sun8i: h3: add thermal zone to H3
> > ARM: dts: sun8i: h3: enable H3 sid controller
> > ARM: dts: sun8i: h3: use calibration for ths
> > ARM: dts: sun8i: a83t: add support for the thermal sensor in A83T
> > ARM: dts: sun8i: a83t: add thermal zone to A83T
> > ARM: sun8i: a83t: full range OPP tables and CPUfreq
> >
> > .../devicetree/bindings/iio/adc/sun4i-gpadc.txt | 41 +-
> > arch/arm/boot/dts/sun8i-a83t.dtsi | 143 +++++
> > arch/arm/boot/dts/sun8i-h3.dtsi | 52 ++
> > arch/arm/boot/dts/sunxi-h3-h5.dtsi | 10 +
> > arch/arm/configs/sunxi_defconfig | 1 +
> > drivers/iio/adc/Kconfig | 11 +-
> > drivers/iio/adc/sun4i-gpadc-iio.c | 617 +++++++++++++--------
> > drivers/mfd/Kconfig | 17 -
> > drivers/mfd/Makefile | 1 -
> > drivers/mfd/sun4i-gpadc.c | 181 ------
> > include/linux/{mfd => iio/adc}/sun4i-gpadc.h | 47 +-
> > 11 files changed, 681 insertions(+), 440 deletions(-)
> > delete mode 100644 drivers/mfd/sun4i-gpadc.c
> > rename include/linux/{mfd => iio/adc}/sun4i-gpadc.h (72%)
>
> What became of these patches? I can't seem to find any follow-ups. Was
> the approach abandoned in favour of something else?

It didn't, I don't think we ever got any follow-up to that series so
that's the main reason it's been held back.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

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

2019-03-19 13:06:19

by Chen-Yu Tsai

[permalink] [raw]

Subject: Re: [linux-sunxi] Re: [PATCH v3 00/30] IIO-based thermal sensor driver for Allwinner H3 and A83T SoC

On Tue, Mar 19, 2019 at 8:37 PM Maxime Ripard <[email protected]> wrote:
>
> On Tue, Mar 19, 2019 at 12:30:25PM +0000, Måns Rullgård wrote:
> > Philipp Rossak <[email protected]> writes:
> >
> > > Allwiner H3 and A83T SoCs have a thermal sensor, which is a large refactored
> > > version of the old Allwinner "GPADC" (although it have already only
> > > thermal part left in A33).
> > >
> > > This patch tried to add support for the sensor in H3 and A83T based on
> > >
> > > This Patchtseries was in the beginning based on Icenowy Zengs v4 patchseries [1]. Since we decided to merge the mfd driver into the GPADC this changed. So only one patch could be reused.
> > >
> > > Patches that adds support for H5, A64, A80 and H6 SoCs are allready prepared,
> > > and will be upstreamed if this patchseries is applied and the testing is done.
> > >
> > > Sorry for delaying this.
> > >
> > > Regards,
> > > Philipp
> > >
> > > changes since v2:
> > > * mfd driver is now merged into the gpadc driver
> > > * complete rework
> > >
> > > changes since v1:
> > > * collecting all acks
> > > * rewording commits/fix typos
> > > * move code in place where it is used
> > > * fix naming conventions of defines
> > > * clarify commits
> > > * update documentation to cover the new nvmem calibraion
> > > * change nvmem calibration
> > >
> > > Icenowy Zheng (1):
> > > iio: adc: sun4i-gpadc-iio: rename A33-specified registers to contain
> > > A33
> > >
> > > Philipp Rossak (29):
> > > mfd: Makefile: Remove build option for MFD:sun4i-gpadc
> > > mfd: Kconfig: Remove MFD_SUN4I_GPADC config option
> > > iio: adc: Remove ID table
> > > iio: adc: Kconfig: Update Kconfig to new build options
> > > iio: adc: move SUN4I_GPADC_CHANNEL define to header file
> > > iio: adc: remove ofnode options
> > > iio: adc: remove mfd_probe & sunwi_irq_init function
> > > iio: adc: remove hwmon structure
> > > iio: adc: Threat A33 as thermal sensor and remove non thermal sun4i
> > > channel
> > > iio: adc: rework irq and adc_channel handling
> > > iio: adc: add new compatibles
> > > mfd: Remove old mfd driver & Move sun4i-gpadc.h to iio/adc/
> > > arm: config: Enable SUN4I_GPADC in defconfig
> > > dt-bindings: update the Allwinner GPADC device tree binding for H3 &
> > > A83T
> > > iio: adc: sun4i-gpadc-iio: rework: readout temp_data
> > > iio: adc: sun4i-gpadc-iio: rework: support clocks and reset
> > > iio: adc: sun4i-gpadc-iio: rework: support multiple sensors
> > > iio: adc: sun4i-gpadc-iio: rework: support nvmem calibration data
> > > iio: adc: sun4i-gpadc-iio: rework: device specific suspend & resume
> > > iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor
> > > iio: adc: sun4i-gpadc-iio: add support for A83T thermal sensor
> > > ARM: dts: sunxi-h3-h5: add support for the thermal sensor in H3 and H5
> > > ARM: dts: sun8i: h3: add support for the thermal sensor in H3
> > > ARM: dts: sun8i: h3: add thermal zone to H3
> > > ARM: dts: sun8i: h3: enable H3 sid controller
> > > ARM: dts: sun8i: h3: use calibration for ths
> > > ARM: dts: sun8i: a83t: add support for the thermal sensor in A83T
> > > ARM: dts: sun8i: a83t: add thermal zone to A83T
> > > ARM: sun8i: a83t: full range OPP tables and CPUfreq
> > >
> > > .../devicetree/bindings/iio/adc/sun4i-gpadc.txt | 41 +-
> > > arch/arm/boot/dts/sun8i-a83t.dtsi | 143 +++++
> > > arch/arm/boot/dts/sun8i-h3.dtsi | 52 ++
> > > arch/arm/boot/dts/sunxi-h3-h5.dtsi | 10 +
> > > arch/arm/configs/sunxi_defconfig | 1 +
> > > drivers/iio/adc/Kconfig | 11 +-
> > > drivers/iio/adc/sun4i-gpadc-iio.c | 617 +++++++++++++--------
> > > drivers/mfd/Kconfig | 17 -
> > > drivers/mfd/Makefile | 1 -
> > > drivers/mfd/sun4i-gpadc.c | 181 ------
> > > include/linux/{mfd => iio/adc}/sun4i-gpadc.h | 47 +-
> > > 11 files changed, 681 insertions(+), 440 deletions(-)
> > > delete mode 100644 drivers/mfd/sun4i-gpadc.c
> > > rename include/linux/{mfd => iio/adc}/sun4i-gpadc.h (72%)
> >
> > What became of these patches? I can't seem to find any follow-ups. Was
> > the approach abandoned in favour of something else?
>
> It didn't, I don't think we ever got any follow-up to that series so
> that's the main reason it's been held back.

IIRC it was blocked by a disagreement on how to handle the calibration data
in the SID. Hopefully this is now resolved.

ChenYu