Re: [PATCH 1/2] dt-bindings: thermal: loongson,ls2k-thermal: Add compatible for Loongson-2K0300

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6946 bytes --]

On Wed, Jul 15, 2026 at 04:31:21PM +0800, Binbin Zhou wrote:
> Hi Conor:
> 
> On Wed, Jul 15, 2026 at 12:54 AM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Tue, Jul 14, 2026 at 02:53:40PM +0800, Binbin Zhou wrote:
> > > Hi Conor:
> > >
> > > Thanks for your reply.
> > >
> > > On Sat, Jul 11, 2026 at 12:21 AM Conor Dooley <conor@kernel.org> wrote:
> > > >
> > > > On Fri, Jul 10, 2026 at 04:24:59PM +0800, Binbin Zhou wrote:
> > > > > Add a new compatible string `loongson,ls2k0300-thermal` for the thermal
> > > > > sensor found on the Loongson-2K0300 SoC.
> > > > >
> > > > > The hardware differs from the existing SoCs in its register layout: it
> > > > > requires two register regions (one for the thermal sensor control and
> > > > > another for the CPU ID).
> > > > >
> > > > > Update the binding to describe this new requirement.
> > > > >
> > > > > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > > > > ---
> > > > >  .../thermal/loongson,ls2k-thermal.yaml        | 68 ++++++++++++++-----
> > > > >  1 file changed, 50 insertions(+), 18 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml b/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > > > > index 79e691b08341..b5cbfd201105 100644
> > > > > --- a/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > > > > +++ b/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > > > > @@ -10,13 +10,11 @@ maintainers:
> > > > >    - zhanghongchen <zhanghongchen@loongson.cn>
> > > > >    - Yinbo Zhu <zhuyinbo@loongson.cn>
> > > > >
> > > > > -allOf:
> > > > > -  - $ref: /schemas/thermal/thermal-sensor.yaml#
> > > > > -
> > > > >  properties:
> > > > >    compatible:
> > > > >      oneOf:
> > > > >        - enum:
> > > > > +          - loongson,ls2k0300-thermal
> > > > >            - loongson,ls2k1000-thermal
> > > > >            - loongson,ls2k2000-thermal
> > > > >        - items:
> > > > > @@ -39,23 +37,46 @@ required:
> > > > >    - reg
> > > > >    - interrupts
> > > > >
> > > > > -if:
> > > > > -  properties:
> > > > > -    compatible:
> > > > > -      contains:
> > > > > -        enum:
> > > > > -          - loongson,ls2k2000-thermal
> > > > > +allOf:
> > > > > +  - $ref: /schemas/thermal/thermal-sensor.yaml#
> > > > >
> > > > > -then:
> > > > > -  properties:
> > > > > -    reg:
> > > > > -      minItems: 2
> > > > > -      maxItems: 2
> > > > > +  - if:
> > > > > +      properties:
> > > > > +        compatible:
> > > > > +          contains:
> > > > > +            enum:
> > > > > +              - loongson,ls2k0300-thermal
> > > > > +    then:
> > > > > +      properties:
> > > > > +        reg:
> > > > > +          items:
> > > > > +            - description: Thermal base register region
> > > > > +            - description: CPU ID register region
> > > > >
> > > > > -else:
> > > > > -  properties:
> > > > > -    reg:
> > > > > -      maxItems: 1
> > > > > +  - if:
> > > > > +      properties:
> > > > > +        compatible:
> > > > > +          contains:
> > > > > +            enum:
> > > > > +              - loongson,ls2k1000-thermal
> > > > > +    then:
> > > > > +      properties:
> > > > > +        reg:
> > > > > +          items:
> > > > > +            - description: Thermal base register region
> > > > > +
> > > > > +  - if:
> > > > > +      properties:
> > > > > +        compatible:
> > > > > +          contains:
> > > > > +            enum:
> > > > > +              - loongson,ls2k2000-thermal
> > > > > +    then:
> > > > > +      properties:
> > > > > +        reg:
> > > > > +          items:
> > > > > +            - description: Thermal base register region
> > > > > +            - description: Thermal data output register region
> > > > >
> > > > >  unevaluatedProperties: false
> > > > >
> > > > > @@ -69,3 +90,14 @@ examples:
> > > > >          interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
> > > > >          #thermal-sensor-cells = <1>;
> > > > >      };
> > > > > +
> > > > > +  - |
> > > > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > > > +    thermal-sensor@16001500 {
> > > > > +       compatible = "loongson,ls2k0300-thermal";
> > > > > +       reg = <0x16001500 0x30>,
> > > > > +             <0x16003ff0 0x8>;
> > > >
> > > > Quite frankly, the address and size of this look like the second
> > > > register region here is actually a few bytes in a syscon that is being
> > > > misrepresented.
> > > > What lies at the addresses immediately before and after 0x16003ff0?
> > >
> > > Yes, it can be viewed as part of the system configuration registers,
> > > which appear somewhat disorganized. Within this section,
> > > 0x16003fe0–0x16003ffc represents eight chip ID registers.
> > >
> > > 0x16003fe0 --> the 4th chip id
> > > 0x16003fe4 --> the 5th chip id
> > > 0x16003fe8 --> the 6th chip id
> > > 0x16003fec --> the 7th chip id
> > > 0x16003ff0 --> the 0th chip id
> > > 0x16003ff4 --> the 1st chip id
> > > 0x16003ff8 --> the 2nd chip id
> > > 0x16003ffc --> the 3rd chip id
> > >
> > > Perhaps I shouldn’t have referenced `0x16003ff0` separately here. It
> > > would be more reasonable to declare the entire chip ID address space
> > > as a separate syscon and have it referenced by thermal driver.
> >
> > Probably, but even being of size 0x20 feels suspiciously small and that
> > it is likely that this is part of an even larger grouping of misc.
> > registers.
> 
> According to the manual, the chip configuration registers are not
> contiguous but are scattered throughout the memory. They are generally
> as follows:
> 
> 0x16000100 - 0x16000154:  General Chip Configuration / Chip Sampling
> Parameters / Chip Counter Registers
> 0x16000400 - 0x16000424:  PLL Clock Configuration Registers -> This is
> already used in clk driver;
> 0x16000490 - 0x160004a8:  GPIO Remapping Configuration Register ->
> This is already used in pinctrl driver;
> 0x16000500 - 0x1600050c:  USB PHY Configuration Register
> 0x16001040 - 0x160014c0:  Interrupt routing Registers -> This is
> already used in irqchip driver;
> 0x16001500 - 0x16001524:  Thermal Registers -> This is already used in
> thermal driver;
> 0x16001600 - 0x1600180c:  External Interrupt Registers -> EIO irqchip driver;
> 0x16003fe0  - 0x16003ffc:  CHIP ID Registers
> 
> Expanding the range to `0x16000100 - 0x16003ffc` might not be the best approach.
> In contrast, the chip ID register is independent. Additionally, apart
> from the first section of general-purpose chip configuration
> registers, the other registers are referenced by their respective
> modules.
> 
> So, is `reg = <0x0 0x16003fe0 0x0 0x20>;` a good choice?

Yeah, it is genuinely not a contiguous region, suspect as it may look!
This seems fine, thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]