Message-ID: <709d738c-3bf3-d808-4172-468d7ad947d7@roeck-us.net>
Date:   Mon, 17 Jul 2023 23:54:26 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Thierry Reding <thierry.reding@...il.com>,
        蔡承達 <billyking19920205@...il.com>
Cc:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        "jdelvare@...e.com" <jdelvare@...e.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "joel@....id.au" <joel@....id.au>,
        "andrew@...id.au" <andrew@...id.au>,
        "u.kleine-koenig@...gutronix.de" <u.kleine-koenig@...gutronix.de>,
        "corbet@....net" <corbet@....net>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "patrick@...cx.xyz" <patrick@...cx.xyz>,
        Billy Tsai <billy_tsai@...eedtech.com>
Subject: Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control
 documentation

On 7/17/23 23:39, Thierry Reding wrote:
> On Tue, Jul 18, 2023 at 08:04:24AM +0200, Krzysztof Kozlowski wrote:
>> On 18/07/2023 06:01, 蔡承達 wrote:
>>>>
>>>> On 17/07/2023 11:01, 蔡承達 wrote:
>>>>> Guenter Roeck <linux@...ck-us.net> 於 2023年7月17日 週一 上午1:00寫道：
>>>>>>
>>>>>> On 7/16/23 09:08, Krzysztof Kozlowski wrote:
>>>>>>
>>>>>> [ ... ]
>>>>>>
>>>>>>>>
>>>>>>>> This patch serial doesn't use to binding the fan control h/w. It is
>>>>>>>> used to binding the two independent h/w blocks.
>>>>>>>> One is used to provide pwm output and another is used to monitor the
>>>>>>>> speed of the input.
>>>>>>>> My patch is used to point out that the pwm and the tach is the
>>>>>>>> different function and don't need to
>>>>>>>> bind together. You can not only combine them as the fan usage but also
>>>>>>>> treat them as the individual module for
>>>>>>>> use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
>>>>>>>> tach can be used to monitor the heart beat signal.
>>>>>>>
>>>>>>> Isn't this exactly the same as in every other SoC? PWMs can be used in
>>>>>>> different ways?
>>>>>>>
>>>>>>
>>>>>> ... and in every fan controller. Not that it really makes sense because
>>>>>> normally the pwm controller part of such chips is tied to the fan input,
>>>>>> to enable automatic fan control, but it is technically possible.
>>>>>> In many cases this is also the case in SoCs, for example, in ast2500.
>>>>>> Apparently this was redesigned in ast2600 where they two blocks are
>>>>>> only lightly coupled (there are two pwm status bits in the fan status
>>>>>> register, but I have no idea what those mean). If the blocks are tightly
>>>>>> coupled, separate drivers don't really make sense.
>>>>>>
>>>>>> There are multiple ways to separate the pwm controller part from the
>>>>>> fan inputs if that is really necessary. One would be to provide a
>>>>>> sequence of address mappings, the other would be to pass the memory
>>>>>> region from an mfd driver. It is not necessary to have N instances
>>>>>> of the fan controller, even if the address space is not continuous.
>>>>>>
>>>>>
>>>>> Hi Guenter,
>>>>>
>>>>> May I ask about the meaning of the sequence of address mappings? It appears
>>>>> to consist of multiple tuples within the 'reg' property, indicating
>>>>> the usage of PWM/Tach
>>>>> registers within a single instance. After that I can use the dts like following:
>>>>>
>>>>> pwm: pwm@...10000 {
>>>>> ...
>>>>> reg = <0x1e610000 0x8
>>>>> 0x1e610010 0x8
>>>>> 0x1e610020 0x8
>>>>> 0x1e610030 0x8
>>>>> 0x1e610040 0x8
>>>>> 0x1e610050 0x8
>>>>> 0x1e610060 0x8
>>>>> 0x1e610070 0x8
>>>>> 0x1e610080 0x8
>>>>> 0x1e610090 0x8
>>>>> 0x1e6100A0 0x8
>>>>> 0x1e6100B0 0x8
>>>>> 0x1e6100C0 0x8
>>>>> 0x1e6100D0 0x8
>>>>> 0x1e6100E0 0x8
>>>>> 0x1e6100F0 0x8>;
>>>>
>>>>
>>>> Uh, no... I mean, why? We keep pointing out that this should not be done
>>>> differently than any other SoC. Open any other SoC PWM controller and
>>>> tell me why this is different? Why this cannot be one address space?
>>>
>>> Hi Krzysztof,
>>>
>>> This is because the register layout for PWM and Tach is not continuous.
>>> Each PWM/Tach instance has its own set of controller registers, and they
>>> are independent of each other.
>>
>> Register layout is not continuous in many other devices, so again - why
>> this must be different?
>>
>>>
>>> For example:
>>> PWM0 uses registers 0x0 and 0x4, while Tach0 uses registers 0x8 and 0xc.
>>> PWM1 uses registers 0x10 and 0x14, while Tach1 uses registers 0x18 and 0x1c.
>>> ...
>>>
>>> To separate the PWM controller part from the fan inputs, Guenter has
>>> provided two methods.
>>> The first method involves passing the memory region from an MFD
>>> driver, which was the
>>
>> I have no clue how can you pass memory region
>> (Documentation/devicetree/bindings/reserved-memory/) from MFD and why
>> does it make sense here.
>>
>>> initial method I intended to use. However, it seems that this method
>>> does not make sense to you.
>>>
>>> Therefore, I would like to explore the second method suggested by
>>> Guenter, which involves providing
>>> a sequence of address mappings.
> 
> At the risk of saying what others have said: given that there's a single
> reset line and a single clock line controlling all of these channels and
> given what I recall of how address demuxers work in chips, everything
> indicates that this is a single hardware block/device.
> 
> So the way that this should be described in DT is:
> 
> 	pwm@...10000 {
> 		reg = <0x1e610000 0x100>;
> 		clocks = ...;
> 		resets = ...
> 	};
> 
> That'd be the most accurate representation of this hardware in DT. It is
> then up to the driver to expose this in any way you see fit. For Linux
> it may make sense to expose this as 16 PWM channels and 16 hardware
> monitoring devices. Other operating systems using the same DT may choose

It is single chip. It should be a single hardware monitoring device with
16 channels. I don't even want to think about the mess we'd get if people
start modeling a single chip as N hardware monitoring devices, one for
each monitoring channel supported by that chip. It would be even more messy
if the driver supporting those N devices would be marked for asynchronous
probe, which would result in random hwmon device assignments.

> to expose this differently, depending on their frameworks, etc. A simple
> operating system may not expose this as separate resources at all but
> instead directly program individual registers from this block.
> 
> I'd also like to add that I think trying to split this up into multiple
> drivers in Linux is a bit overkill. In my opinion, though I know not
> everyone shares this view, it's perfectly fine for one driver to expose
> multiple types of resources. There's plenty of use-cases across the
> kernel where tightly coupled devices like this have a single driver that
> registers with multiple subsystems. Going through MFD only because this
> particular hardware doesn't split registers nicely along Linux subsystem
> boundaries.
> 
> So FWIW, I'm fine carrying hwmon code in a PWM driver and I'm equally
> fine if PWM code ends up in a hwmon driver (or any other subsystem
> really) if that makes sense for a given hardware.
> 

I am fine either way as well, as long as we are talking about a single
hwmon device and not 16 of them.

Guenter

Powered by blists - more mailing lists