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

From: Andy Tang
Date: Tue Dec 11 2018 - 21:45:06 EST

Next message: Richard Guy Briggs: "Re: [PATCH ghak59 V3 0/4] audit: config_change normalizations and event record gathering"
Previous message: Rob Herring: "Re: [PATCH] dt-bindings: arm: mrvl: amend Browstone compatible string"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----Original Message-----
> From: Eduardo Valentin <edubezval@xxxxxxxxx>
> Sent: 2018å11æ30æ 1:21
> To: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> Cc: Andy Tang <andy.tang@xxxxxxx>; rui.zhang@xxxxxxxxx;
> linux-pm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3] thermal: qoriq: add multiple sensors support
>
> On Wed, Nov 21, 2018 at 10:41:36AM +0100, Daniel Lezcano wrote:
> > On 21/11/2018 10:16, Andy Tang wrote:
> > > Hi Daniel,
> > >
> > > Thanks for your explanation. The problem is these two trees are not synced
> well.
> > > Let's take our driver(qoriq_thermal.c) for example.
> > >
> > > Git log on Rui's tree next branch:
> > > 2dfef65 thermal: qoriq: Switch to SPDX identifier
> > > 1a893a5 thermal: qoriq: Simplify the 'site' variable assignment
> > > f1506a6 thermal: qoriq: Use devm_thermal_zone_of_sensor_register()
> > > c30d5d5 thermal: qoriq: constify thermal_zone_of_device_ops
> > > structures
> > > 0e77488 thermal: qoriq: remove useless call for
> > > of_thermal_get_trip_points()
> > > 4352844 thermal: qoriq: Add thermal management support
> > >
> > > Git log on linux-soc-thermal tree branch next:
> > > 6017e2a thermal: qoriq: add i.mx8mq support
> > > 9b96566 thermal: Convert to using %pOFn instead of device_node.name
> > > c30d5d5 thermal: qoriq: constify thermal_zone_of_device_ops
> > > structures
> > > 0e77488 thermal: qoriq: remove useless call for
> > > of_thermal_get_trip_points()
> > > 4352844 thermal: qoriq: Add thermal management support
> > >
> > > You can see that the first 2-3 commits on these two tress are different.
> > >
> > > The strange thing is they seems sync well on Linus' tree:
> > > 0ef7791 Merge branch 'linus' of
> > > git://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-the
> > > rmal 6017e2a thermal: qoriq: add i.mx8mq support
> > > 9b96566 thermal: Convert to using %pOFn instead of device_node.name
> > > 2dfef65 thermal: qoriq: Switch to SPDX identifier
> > > 1a893a5 thermal: qoriq: Simplify the 'site' variable assignment
> > > f1506a6 thermal: qoriq: Use devm_thermal_zone_of_sensor_register()
> > > c30d5d5 thermal: qoriq: constify thermal_zone_of_device_ops
> > > structures
> > > 0e77488 thermal: qoriq: remove useless call for
> > > of_thermal_get_trip_points()
> > > 4352844 thermal: qoriq: Add thermal management support
> > >
> > > Currently my patch was created based on Run's tree, probably I should
> rebase it to soc tree.
> > > But whichever tree I use, it can't be merged to Linus' tree without conflict.
> > >
> > > Something I missed?
> >
> > No.
> >
> > Eduardo, Rui,
> >
> > why not create a 'thermal' group ala 'tip' group with a single tree
> > and two branches:
> >
> > thermal/next
> > thermal/fixes
> >
> > - Rui takes the core changes.
> > - Eduardo takes the SoC changes.
> >
> > - Both commit to thermal/next
> > - Both commit to thermal/fixes
> > - Both merge thermal/fixes into thermal/core as often as possible.
> >
> > That will help to have a more up to date branch, simplify the patch
> > submission path and reduce the latency for the merge windows.
> >
> > If you need help, I can take care of applying the fixes only and merge
> > them to thermal/next.
> >
> > That is how the tip subsystem works, Peter Ziljstra, Ingo Molnar,
> > Thomas Gleixner, have all permissions to commit in the tip tree but
> > they take care of their subsystems. If one is away for vacations or
> > whatever, someone else can take over during the absence.
> >
>
> Yeah, that is a setup people have been following. It does not necessarily mean
> it will work for all cases though.
>
> I believe regardless of process and tree setup what we are lacking here is a
> documentation of how things are being done.
>
> As I mentioned, I will work on writing something up to document at least what
> we have today before any change in process gets in place.
>
[Andy] Besides the document, what about this patch? Could it be merged before the document is ready?

BR,
Andy
> >
> >
> >
> >
> > >> -----Original Message-----
> > >> From: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> > >> Sent: 2018å11æ21æ 16:44
> > >> To: Andy Tang <andy.tang@xxxxxxx>; rui.zhang@xxxxxxxxx;
> > >> edubezval@xxxxxxxxx
> > >> Cc: linux-pm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > >> Subject: Re: [PATCH v3] thermal: qoriq: add multiple sensors
> > >> support
> > >>
> > >> On 21/11/2018 02:34, Andy Tang wrote:
> > >>> Hi all,
> > >>>
> > >>> Do you have any comments on this patch?
> > >>>
> > >>> I found for our thermal driver(qoriq_thermal.c) there are
> > >>> different
> > >> between the following two git trees:
> > >>> git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux.git
> > >>> branch: next
> > >>>
> > >> git://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-th
> > >> ermal.gi
> > >> t.
> > >>> branch: next
> > >>>
> > >>> Could you please clarify which git tree/branch should I use?
> > >>
> > >> SoC changes are submitted against linux-soc-thermal.git.
> > >>
> > >> Generic thermal framework are sent against Zhang Rui's tree but it
> > >> happens sometimes Eduardo pick them also when the changes are
> > >> related to SoC behavior.
> > >>
> > >> However, I agree that can be confusing :)
> > >>
> > >> Eduardo, Rui,
> > >>
> > >> how about to add a section in the maintainer handbook for the
> > >> thermal to clarify the expectations and the flow?
> > >>
> > >>>> -----Original Message-----
> > >>>> From: Andy Tang
> > >>>> Sent: 2018å11æ14æ 15:29
> > >>>> To: rui.zhang@xxxxxxxxx; daniel.lezcano@xxxxxxxxxx
> > >>>> Cc: edubezval@xxxxxxxxx; linux-pm@xxxxxxxxxxxxxxx;
> > >>>> linux-kernel@xxxxxxxxxxxxxxx
> > >>>> Subject: RE: [PATCH v3] thermal: qoriq: add multiple sensors
> > >>>> support
> > >>>>
> > >>>> PING.
> > >>>>
> > >>>> BR,
> > >>>> Andy
> > >>>>
> > >>>>> -----Original Message-----
> > >>>>> From: andy.tang@xxxxxxx <andy.tang@xxxxxxx>
> > >>>>> Sent: 2018å10æ30æ 9:00
> > >>>>> To: rui.zhang@xxxxxxxxx; daniel.lezcano@xxxxxxxxxx
> > >>>>> Cc: edubezval@xxxxxxxxx; linux-pm@xxxxxxxxxxxxxxx;
> > >>>>> linux-kernel@xxxxxxxxxxxxxxx; Andy Tang <andy.tang@xxxxxxx>
> > >>>>> Subject: [PATCH v3] thermal: qoriq: add multiple sensors support
> > >>>>>
> > >>>>> From: Yuantian Tang <andy.tang@xxxxxxx>
> > >>>>>
> > >>>>> The QorIQ Layerscape SoC has several thermal sensors but the
> > >> current
> > >>>>> driver only supports one.
> > >>>>>
> > >>>>> Massage the code to be sensor oriented and allow the support for
> > >>>>> multiple sensors.
> > >>>>>
> > >>>>> Signed-off-by: Yuantian Tang <andy.tang@xxxxxxx>
> > >>>>> Reviewed-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> > >>>>> ---
> > >>>>> v3:
> > >>>>> - add Reviewed-by
> > >>>>> v2:
> > >>>>> - update the commit message
> > >>>>> - refine the qoriq_tmu_register_tmu_zone()
> > >>>>>
> > >>>>> drivers/thermal/qoriq_thermal.c | 100
> > >>>>> ++++++++++++++++++---------------------
> > >>>>> 1 files changed, 46 insertions(+), 54 deletions(-)
> > >>>>>
> > >>>>> diff --git a/drivers/thermal/qoriq_thermal.c
> > >>>>> b/drivers/thermal/qoriq_thermal.c index 450ed66..8beb344 100644
> > >>>>> --- a/drivers/thermal/qoriq_thermal.c
> > >>>>> +++ b/drivers/thermal/qoriq_thermal.c
> > >>>>> @@ -59,14 +59,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;
> > >>>>> +};
> > >>>>> +
> > >>>>> 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];
> > >>>>> };
> > >>>>>
> > >>>>> static void tmu_write(struct qoriq_tmu_data *p, u32 val, void
> > >>>>> __iomem
> > >>>>> *addr) @@ -87,48 +94,51 @@ static u32 tmu_read(struct
> > >>>> qoriq_tmu_data
> > >>>>> *p, void __iomem *addr)
> > >>>>>
> > >>>>> static int tmu_get_temp(void *p, int *temp) {
> > >>>>> + struct qoriq_sensor *qsensor = p;
> > >>>>> + struct qoriq_tmu_data *qdata = qsensor->qdata;
> > >>>>> u32 val;
> > >>>>> - struct qoriq_tmu_data *data = p;
> > >>>>>
> > >>>>> - val = tmu_read(data, &data->regs->site[data->sensor_id].tritsr);
> > >>>>> + val = tmu_read(qdata, &qdata->regs->site[qsensor->id].tritsr);
> > >>>>> *temp = (val & 0xff) * 1000;
> > >>>>>
> > >>>>> return 0;
> > >>>>> }
> > >>>>>
> > >>>>> -static int qoriq_tmu_get_sensor_id(void)
> > >>>>> +static const struct thermal_zone_of_device_ops tmu_tz_ops = {
> > >>>>> + .get_temp = tmu_get_temp,
> > >>>>> +};
> > >>>>> +
> > >>>>> +static int qoriq_tmu_register_tmu_zone(struct platform_device
> > >>>>> +*pdev)
> > >>>>> {
> > >>>>> - int ret, id;
> > >>>>> - struct of_phandle_args sensor_specs;
> > >>>>> - struct device_node *np, *sensor_np;
> > >>>>> + struct qoriq_tmu_data *qdata = platform_get_drvdata(pdev);
> > >>>>> + int id, sites = 0;
> > >>>>>
> > >>>>> - np = of_find_node_by_name(NULL, "thermal-zones");
> > >>>>> - if (!np)
> > >>>>> - return -ENODEV;
> > >>>>> + for (id = 0; id < SITES_MAX; id++) {
> > >>>>> + qdata->sensor[id] = devm_kzalloc(&pdev->dev,
> > >>>>> + sizeof(struct qoriq_sensor), GFP_KERNEL);
> > >>>>> + if (!qdata->sensor[id])
> > >>>>> + return -ENOMEM;
> > >>>>>
> > >>>>> - sensor_np = of_get_next_child(np, NULL);
> > >>>>> - ret = of_parse_phandle_with_args(sensor_np,
> > >> "thermal-sensors",
> > >>>>> - "#thermal-sensor-cells",
> > >>>>> - 0, &sensor_specs);
> > >>>>> - if (ret) {
> > >>>>> - of_node_put(np);
> > >>>>> - of_node_put(sensor_np);
> > >>>>> - return ret;
> > >>>>> - }
> > >>>>> + qdata->sensor[id]->id = id;
> > >>>>> + qdata->sensor[id]->qdata = qdata;
> > >>>>>
> > >>>>> - 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;
> > >>>>> - }
> > >>>>> + 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)) {
> > >>>>> + if (PTR_ERR(qdata->sensor[id]->tzd) == -ENODEV)
> > >>>>> + continue;
> > >>>>> + else
> > >>>>> + return PTR_ERR(qdata->sensor[id]->tzd);
> > >>>>>
> > >>>>> - of_node_put(np);
> > >>>>> - of_node_put(sensor_np);
> > >>>>> + }
> > >>>>> +
> > >>>>> + sites |= 0x1 << (15 - id);
> > >>>>> + }
> > >>>>> + /* Enable monitoring */
> > >>>>> + if (sites != 0)
> > >>>>> + tmu_write(qdata, sites | TMR_ME | TMR_ALPF,
> > >>>>> &qdata->regs->tmr);
> > >>>>>
> > >>>>> - return id;
> > >>>>> + return 0;
> > >>>>> }
> > >>>>>
> > >>>>> static int qoriq_tmu_calibration(struct platform_device *pdev)
> > >>>>> @@
> > >>>>> -178,16 +188,11 @@ static void qoriq_tmu_init_device(struct
> > >>>>> qoriq_tmu_data *data)
> > >>>>> tmu_write(data, TMR_DISABLE, &data->regs->tmr); }
> > >>>>>
> > >>>>> -static const struct thermal_zone_of_device_ops tmu_tz_ops = {
> > >>>>> - .get_temp = tmu_get_temp,
> > >>>>> -};
> > >>>>> -
> > >>>>> static int qoriq_tmu_probe(struct platform_device *pdev) {
> > >>>>> int ret;
> > >>>>> struct qoriq_tmu_data *data;
> > >>>>> struct device_node *np = pdev->dev.of_node;
> > >>>>> - u32 site;
> > >>>>>
> > >>>>> if (!np) {
> > >>>>> dev_err(&pdev->dev, "Device OF-Node is NULL"); @@
> > >> -203,13
> > >>>>> +208,6 @@ static int qoriq_tmu_probe(struct platform_device
> > >> *pdev)
> > >>>>>
> > >>>>> data->little_endian = of_property_read_bool(np,
> > >>>>> "little-endian");
> > >>>>>
> > >>>>> - data->sensor_id = qoriq_tmu_get_sensor_id();
> > >>>>> - if (data->sensor_id < 0) {
> > >>>>> - dev_err(&pdev->dev, "Failed to get sensor id\n");
> > >>>>> - ret = -ENODEV;
> > >>>>> - goto err_iomap;
> > >>>>> - }
> > >>>>> -
> > >>>>> data->regs = of_iomap(np, 0);
> > >>>>> if (!data->regs) {
> > >>>>> dev_err(&pdev->dev, "Failed to get memory region\n");
> > >> @@
> > >>>>> -223,19 +221,13 @@ static int qoriq_tmu_probe(struct
> > >> platform_device
> > >>>>> *pdev)
> > >>>>> if (ret < 0)
> > >>>>> goto err_tmu;
> > >>>>>
> > >>>>> - data->tz = devm_thermal_zone_of_sensor_register(&pdev->dev,
> > >>>>> - data->sensor_id,
> > >>>>> - data, &tmu_tz_ops);
> > >>>>> - if (IS_ERR(data->tz)) {
> > >>>>> - ret = PTR_ERR(data->tz);
> > >>>>> - dev_err(&pdev->dev,
> > >>>>> - "Failed to register thermal zone device %d\n", ret);
> > >>>>> - goto err_tmu;
> > >>>>> + ret = qoriq_tmu_register_tmu_zone(pdev);
> > >>>>> + if (ret < 0) {
> > >>>>> + dev_err(&pdev->dev, "Failed to register sensors\n");
> > >>>>> + ret = -ENODEV;
> > >>>>> + goto err_iomap;
> > >>>>> }
> > >>>>>
> > >>>>> - /* Enable monitoring */
> > >>>>> - site = 0x1 << (15 - data->sensor_id);
> > >>>>> - tmu_write(data, site | TMR_ME | TMR_ALPF, &data->regs->tmr);
> > >>>>>
> > >>>>> return 0;
> > >>>>>
> > >>>>> --
> > >>>>> 1.7.1
> > >>>
> > >>
> > >>
> > >> --
> > >>
> > >> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> > >> https://emea01.safelinks.protection.outlook.com/?url=www.linaro.org
> > >>
> &data=02%7C01%7Candy.tang%40nxp.com%7C523c8a2809a34f57700808d
> 65
> > >>
> 61f0575%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636791088572
> 03
> > >>
> 0840&sdata=XV0I9%2B3FctUSF%2BDAqRVEWGk8ITzshd%2FtQfpRZJguYZc
> %3D
> > >>
> &reserved=0%2F&data=02%7C01%7Candy.tang%40nxp.com%7Ca0
> > >> d265f95a424d33256b08d64f8d74c8%7C686ea1d3bc2b4c6fa92cd99c5c
> > >> 301635%7C0%7C0%7C636783866299399290&sdata=AVw3XdjmiRO
> > >> XP7Xz7MTNMVgzI8hYG9aWsR02opMZqjM%3D&reserved=0>
> > >> Linaro.org â Open source software for ARM SoCs
> > >>
> > >> Follow Linaro:
> > >> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> > >> https://emea01.safelinks.protection.outlook.com/?url=www.facebook.c
> > >>
> om&data=02%7C01%7Candy.tang%40nxp.com%7C523c8a2809a34f577008
> 08d
> > >>
> 6561f0575%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6367910885
> 72
> > >>
> 030840&sdata=bbsFbY4CoFGFyduTSXlR9oK42hpFGAXTmZMAswNchkU%3
> D&amp
> > >> ;reserved=0%2Fpages%2FLinaro&data=02%7C01%7Candy.tan
> > >> g%40nxp.com%7Ca0d265f95a424d33256b08d64f8d74c8%7C686ea1d3b
> > >> c2b4c6fa92cd99c5c301635%7C0%7C0%7C636783866299399290&s
> > >>
> data=NM8wm4ri%2F2kdBW0FdCSXrL5ogg6owPfoRGacqm%2BKsEw%3D&a
> > >> mp;reserved=0> Facebook |
> > >> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> > >> t
> witter.com%2F%23!%2Flinaroorg&data=02%7C01%7Candy.tang%40n
> > >> xp.com%7Ca0d265f95a424d33256b08d64f8d74c8%7C686ea1d3bc2b4c6
> > >> fa92cd99c5c301635%7C0%7C0%7C636783866299399290&sdata=
> > >> Wplqwpisgmqrf3yLhTzdo5O6TvN2Jfu5IjBTvS4xOWk%3D&reserved=0>
> > >> Twitter |
> > >> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> > >> https://emea01.safelinks.protection.outlook.com/?url=www.linaro.org
> > >>
> &data=02%7C01%7Candy.tang%40nxp.com%7C523c8a2809a34f57700808d
> 65
> > >>
> 61f0575%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636791088572
> 03
> > >>
> 0840&sdata=XV0I9%2B3FctUSF%2BDAqRVEWGk8ITzshd%2FtQfpRZJguYZc
> %3D
> > >>
> &reserved=0%2Flinaro-blog%2F&data=02%7C01%7Candy.tang%40
> > >> nxp.com%7Ca0d265f95a424d33256b08d64f8d74c8%7C686ea1d3bc2b4c
> > >> 6fa92cd99c5c301635%7C0%7C0%7C636783866299399290&sdata=I
> > >> b6KFIP3LNPGuDDb1dpnOllrqAAsc%2BZNCUuJZUPso6k%3D&reserve
> > >> d=0> Blog
> > >
> >
> >
> > --
> >
> >
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww
> > .linaro.org%2F&data=02%7C01%7Candy.tang%40nxp.com%7C523c8a280
> 9a34f
> >
> 57700808d6561f0575%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
> 36791
> >
> 088572030840&sdata=JdPh7CGtYKckvXZbaXyYh1YIzLIWuINxGuc8%2Fp5qb
> 7w%3
> > D&reserved=0> Linaro.org â Open source software for ARM SoCs
> >
> > Follow Linaro:
> >
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww
> > .facebook.com%2Fpages%2FLinaro&data=02%7C01%7Candy.tang%40nx
> p.com%
> >
> 7C523c8a2809a34f57700808d6561f0575%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%
> >
> 7C0%7C0%7C636791088572030840&sdata=120jlpoyO%2FErgYCdiJkQ5PIu1
> lqIO
> > 976W%2BBlifjq9zw%3D&reserved=0> Facebook |
> > <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwi
> >
> tter.com%2F%23!%2Flinaroorg&data=02%7C01%7Candy.tang%40nxp.com
> %7C5
> >
> 23c8a2809a34f57700808d6561f0575%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0
> > %7C0%7C636791088572030840&sdata=hqRnoNwph7It3VtREb9PMnktZn
> 6IWqPVht
> > RqXL8qyKA%3D&reserved=0> Twitter |
> >
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww
> > .linaro.org%2Flinaro-blog%2F&data=02%7C01%7Candy.tang%40nxp.com
> %7C
> >
> 523c8a2809a34f57700808d6561f0575%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C
> >
> 0%7C0%7C636791088572030840&sdata=GYax5%2FZzQtx6omIyuMMw0klh
> rFUNsnP
> > LFZ%2Fsg4OY%2BYI%3D&reserved=0> Blog
> >

Next message: Richard Guy Briggs: "Re: [PATCH ghak59 V3 0/4] audit: config_change normalizations and event record gathering"
Previous message: Rob Herring: "Re: [PATCH] dt-bindings: arm: mrvl: amend Browstone compatible string"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]