RE: [PATCH] thermal: qoriq: add multiple sensors support

From: Andy Tang
Date: Mon Oct 15 2018 - 23:03:15 EST

Next message: Guo Ren: "[PATCH V9 14/21] csky: User access"
Previous message: Masahiro Yamada: "[PATCH 2/3] i2c: uniphier-f: fix occasional timeout error"
In reply to: Daniel Lezcano: "Re: [PATCH] thermal: qoriq: add multiple sensors support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Daniel,

Please see my reply inline.

> -----Original Message-----
> From: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> Sent: 2018å10æ15æ 16:56
> To: Andy Tang <andy.tang@xxxxxxx>; rui.zhang@xxxxxxxxx
> Cc: edubezval@xxxxxxxxx; linux-pm@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] thermal: qoriq: add multiple sensors support
> > >
> >>
> >>> Signed-off-by: Tang Yuantian <andy.tang@xxxxxxx>
> >>> ---
> >>> drivers/thermal/qoriq_thermal.c | 117
> >>> +++++++++++++++++++++++----------------
> >>> 1 files changed, 70 insertions(+), 47 deletions(-)
> >>>
> >>> diff --git a/drivers/thermal/qoriq_thermal.c
> >>> b/drivers/thermal/qoriq_thermal.c index c866cc1..7c1e88a 100644
> >>> --- a/drivers/thermal/qoriq_thermal.c
> >>> +++ b/drivers/thermal/qoriq_thermal.c
> >>> @@ -69,14 +69,21 @@ struct qoriq_tmu_regs {
> >>> u32 ttr3cr; /* Temperature Range 3 Control Register */
> >>> };
> >>>
> >>> +struct qoriq_tmu_data;
> >>> +
> >>> /*
> >>> * Thermal zone data
> >>> */
> >>> +struct qoriq_sensor {
> >>> + struct thermal_zone_device *tzd;
> >>> + struct qoriq_tmu_data *qdata;
> >>> + int id;
> >>> +};
> >>
> >> Can you move the qoriq_tmu_site_regs structure content inside the
> >> qoriq_sensor structure and kill the 'sites' field in the
> >> qoriq_tmu_regs structure ? Otherwise we end up with a SITES_MAX
> array
> >> in the qoriq_tmu_data structure and another one in the
> qoriq_tmu_regs
> >> structure.
> > [Andy] I am afraid I can't.
> > qoriq_tmu_site_regs structure is to define the registers. After iomap,
> TMU can be accessed.
> > qoriq_sensor structure is used for each sensor. It DONOT include the
> register defines.
> > qoriq_tmu_data structure is used for global TMU date.
> > So there is no any duplicated or redundant data here.
>
> It is not about duplicate but just code reorg.
>
> This patch changes the structure as:
>
> struct qoriq_tmu_data {
> - struct thermal_zone_device *tz;
> struct qoriq_tmu_regs __iomem *regs;
> - int sensor_id;
> bool little_endian;
> + struct qoriq_sensor *sensor[SITES_MAX];
> };
>
>
> So we have:
>
> struct qoriq_tmu_data
> => struct qoriq_sensor[SITES_MAX]
> => struct qoriq_tmu_regs
> => struct qoriq_tmu_site_regs[SITES_MAX]
>
> I'm proposing to move struct qoriq_tmu_site_regs inside the struct
> qoriq_sensor.
>
>
> We end up with:
>
> struct qoriq_sensor {
> struct thermal_zone_device *tzd;
> struct struct qoriq_tmu_site_regs *regs;
> struct qoriq_tmu_data *qdata;
> int id;
> };
[Andy] I see your point. If I add a *regs member to qoriq_sensor struct, then I can speed up the access to the register.
But I don't think it is better. Currently I can access sensor register by: qdata->regs->site[qsensor->id].
The whole point here is we can't MOVE struct qoriq_tmu_site_regs to the struct qoriq_sensor. We can only COPY
it to struct qoriq_sensor.

This is the TMU module memory map:
struct qoriq_tmu_regs {
......
u32 tscfgr; /* Sensor Configuration Register */
u8 res4[0x78];
struct qoriq_tmu_site_regs site[SITES_MAX];
u8 res5[0x9f8];
u32 ipbrr0; /* IP Block Revision Register 0 */
u32 ipbrr1; /* IP Block Revision Register 1 */
.......}
struct qoriq_tmu_site_regs site[SITES_MAX] is part of the memory map. It can't be removed or we have to define a similar struct to fill it up.

>
>
> >>> - if (sensor_specs.args_count >= 1) {
> >>> - id = sensor_specs.args[0];
> >>> - WARN(sensor_specs.args_count > 1,
> >>> - "%s: too many cells in sensor specifier %d\n",
> >>> - sensor_specs.np->name,
> sensor_specs.args_count);
> >>> - } else {
> >>> - id = 0;
> >>> + if (id > SITES_MAX)
> >>> + return -EINVAL;
> >>> +
> >>> + qdata->sensor[id] = devm_kzalloc(&pdev->dev,
> >>> + sizeof(struct qoriq_sensor), GFP_KERNEL);
> >>> + if (!qdata->sensor[id])
> >>> + return -ENOMEM;
> >>> +
> >>> + qdata->sensor[id]->id = id;
> >>> + qdata->sensor[id]->qdata = qdata;
> >>> +
> >>> + qdata->sensor[id]->tzd =
> >> devm_thermal_zone_of_sensor_register(
> >>> + &pdev->dev, id, qdata->sensor[id], &tmu_tz_ops);
> >>> +
> >>> + if (IS_ERR(qdata->sensor[id]->tzd)) {
> >>> + ret = PTR_ERR(qdata->sensor[id]->tzd);
> >>> + dev_err(&pdev->dev,
> >>> + "Failed to register thermal zone device.\n");
> >>> + return -ENODEV;
> >>> + }
> >>> +
> >>> + sites |= 0x1 << (15 - id);
> >>
> >> The current code is reading the DT in order to get the sensor id and
> >> initialize it. IOW, the DT gives the sensors to use.
> >>
> >> IMO, it would be more self contained if the driver initializes all
> >> the sensors without taking care of the DT and let the of- code to do
> >> the binding when the thermal zone, no ?
> > [Andy] could you please explain more about this way? I am not sure how
> to implement it.
> > But one thing is for sure: we must get the sensor IDs explicitly so
> > that we can enable them by the following command: tmu_write(qdata,
> > sites | TMR_ME | TMR_ALPF, &qdata->regs->tmr);
>
> What I meant is about code separation between the driver itself and the
> of-thermal code.
>
> The code above re-inspect the DT to find out the sensor ids in order to
> enable them and somehow this is not wrong but breaks the self
> encapsulation of the driver. I was suggesting if it isn't possible to enable all
> the sensors without taking care of digging into the DT.

[Andy] I don't want to re-parse the DT here too. But I have to.
This driver will be used by all our SOCs with different sensor IDs and number.
For example: there are 2 sensors on ls1088a platform with ID 0 and 1.
While on ls1043a there are 6 sensors with ID 0, 1, 2, 3, 4, 5.
If we don't scan the DT we would not know how many sensors it is and what are the sensor's IDs,
unless we hardcode it in driver.

BR,
Andy

>
>
>
>
> --
>
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> www.linaro.org%2F&data=02%7C01%7Candy.tang%40nxp.com%7C38
> 9338eb5bc44d6c268308d6327bfc1c%7C686ea1d3bc2b4c6fa92cd99c5c
> 301635%7C0%7C0%7C636751905429537336&sdata=IKzO0HNquNm
> BPrvxikDiIBFchvdtg8HpqJiuYkqo1%2F4%3D&reserved=0> Linaro.org
> â Open source software for ARM SoCs
>
> Follow Linaro:
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> www.facebook.com%2Fpages%2FLinaro&data=02%7C01%7Candy.tan
> g%40nxp.com%7C389338eb5bc44d6c268308d6327bfc1c%7C686ea1d3b
> c2b4c6fa92cd99c5c301635%7C0%7C0%7C636751905429537336&s
> data=vdR7lC1Q%2FbHUgJXlwlLaoZUdfawrACOQUM94yhPfWVA%3D&
> reserved=0> Facebook |
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ft
> witter.com%2F%23!%2Flinaroorg&data=02%7C01%7Candy.tang%40n
> xp.com%7C389338eb5bc44d6c268308d6327bfc1c%7C686ea1d3bc2b4c
> 6fa92cd99c5c301635%7C0%7C0%7C636751905429547345&sdata=
> k2QUt15gv8NI4fyqPYFoFVN%2FiM7XKOzh45eax%2F2e2Hk%3D&rese
> rved=0> Twitter |
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> www.linaro.org%2Flinaro-blog%2F&data=02%7C01%7Candy.tang%40
> nxp.com%7C389338eb5bc44d6c268308d6327bfc1c%7C686ea1d3bc2b4
> c6fa92cd99c5c301635%7C0%7C0%7C636751905429547345&sdata
> =cbTIEuaYP4l%2FLZR45M253QRfqgXbZPw87%2Bo5EfwTzys%3D&res
> erved=0> Blog

Next message: Guo Ren: "[PATCH V9 14/21] csky: User access"
Previous message: Masahiro Yamada: "[PATCH 2/3] i2c: uniphier-f: fix occasional timeout error"
In reply to: Daniel Lezcano: "Re: [PATCH] thermal: qoriq: add multiple sensors support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]