Re: [PATCH v14 1/2] thermal: loongson-2: add thermal management support

[zhuyinbo]

Hi Daniel,

在 2023/6/14 下午6:59, Daniel Lezcano 写道:
> Hi Yinbo,
>
>
> On 14/06/2023 10:03, zhuyinbo wrote:
> > Hi Daniel,
> >
> > Thank you very much for your feedback and suggestions. Below, I have
> > some comments, please review.
>
> [ ... ]
>
> > > > +
> > > > +    low += 100;
> > > > +    high += 100;
>
> Literals -> macros


okay, I got it.

> > > > +    reg_ctrl = low;
> > > > +    reg_ctrl |= enable ? 0x100 : 0;
> > > > +    writew(reg_ctrl, data->regs + LOONGSON2_TSENSOR_CTRL_LO + 
> > > > reg_off);
> > > > +
> > > > +    reg_ctrl = high;
> > > > +    reg_ctrl |= enable ? 0x100 : 0;
> > > > +    writew(reg_ctrl, data->regs + LOONGSON2_TSENSOR_CTRL_HI + 
> > > > reg_off);
> > >
> > > Is the 'enable' boolean really useful?
> >
> >
> > Yes, this 'enable' was to enable thermal irq.
> >
> > > Wouldn't be the sensor trip points disabled by default at reset time?
> >
> >
> > Only here will thermal irq be enabled throughout the entire driver, and
> > actual testing has shown that interrupts are valid, so this is
> > meaningful.
>
> Ok.
>
> > > If it is the case then we can get ride of this variable and make the 
> > > routine simpler
> > >
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static int loongson2_thermal_get_temp(struct thermal_zone_device 
> > > > *tz, int *temp)
> > > > +{
> > > > +    u32 reg_val;
> > > > +    struct loongson2_thermal_data *data = tz->devdata;
> > > > +
> > > > +    reg_val = readl(data->regs + LOONGSON2_TSENSOR_OUT);
> > >
> > > Seems like there is no offset for the sensor id here ?
> >
> >
> > There is no need for a sensor ID here.
> >
> > There are some things that I didn't describe clearly, which made you
> > misunderstand. Actually, the temperature sensor of 2K1000 is like this:
> >
> > There are 4 sets of temperature interrupt controllers, only one set of
> > temperature sampling registers. a sets of temperature interrupt
> > controllers was considered a sensor, which sensor include 3 register as
> > follows, where "SEL" represents which sensor is referenced, In 2k1000
> > datasheet, which "SEL" must be 0.
>
> I'm not sure to understand. Let me rephrase it and know what is wrong.
>
> 1. The thermal controller has 4 sensors. The interrupt can be set for 
> these 4 sensors.


You can think it's actually a sensor, but the thermal sensor include a
set of status register and four sets of control register in loongson-2k
series (2k1000 /2k2000), but the control register only select a group
register by set "SEL".

> 2. When reading a temperature, we have to select the sensor via the 
> 'SEL' register.


The 'SEL' was to select thermal control register set, that reading a
tempearure was to thermal status register set. thermal status register
set only one and no no other choice. The function of 'SEL' is actually
to select which temperature sensor collects the temperature as input.

> 3. The 2k1000 has one sensor with an id = 0.


Yes, the future 2k series product may still be a sensor, but the ID may
not be 0, it may be 1 or 2 or 3.

> 4. In the future, more Loongson platform can be submitted with more than 
> one sensor


From the current situation of the 2k series (2k1000/2k2000), it seems
that there will always be only one sensor, but 'SEL' can be used to
select which sensor. (0th, 1th, 2th, 3th).  If there are really special
circumstances in the future, add appropriate modifications at that time.

> If this is correct, then my comments are about the inconsistency of the 
> proposed changes. Guessing in the future Loongson board there will be 
> more than one sensor, the existing code mixes support for one and 
> multiple sensors as well as assuming id is 0.
>
> So if you add in the of_loongson2_thermal_match table a new platform 
> with several sensors, the current code will be broken because:
>
>   - the initialization loop does exit when the first thermal zone 
> registration succeed
>
>   - the interrupt handler does not figure out which sensor crossed the 
> low/high limit
>
>   - the get_temp is not selecting the right sensor


Yes,  but currently this code does not support the requirement for 
multiple sensors.

> That is my point:
>
>   - write the code to support one sensor with id=0 only
>
>     *or*
>
>   - write the code to support multiple sensors
>
> If I'm not wrong the code is closer to support multiple sensors ;)


In fact, my code is more inclined to support one sensor, but it can
determine which sensor to use based on the ID. (id = 0, 1, 2, 3)

> Let me know if these deductions are correct
>
>    -- Daniel
>
> ps : is there an English translation for the 2k1000 datasheet ?


Sorry, there seems to be no English version available at the moment.


Thanks,
Yinbo.