From: Guenter Roeck <linux@roeck-us.net>
To: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Cc: Rob Herring <robh+dt@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Lee Jones <lee.jones@linaro.org>,
Jean Delvare <jdelvare@suse.com>,
Mark Rutland <mark.rutland@arm.com>,
Joel Stanley <joel@jms.id.au>, Andrew Jeffery <andrew@aj.id.au>,
Jonathan Corbet <corbet@lwn.net>,
Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
Kishon Vijay Abraham I <kishon@ti.com>,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
"Darrick J . Wong" <darrick.wong@oracle.com>,
Eric Sandeen <sandeen@redhat.com>, Arnd Bergmann <arnd@arndb.de>,
Wu Hao <hao.wu@intel.com>,
Tomohiro Kusumi <kusumi.tomohiro@gmail.com>,
"Bryant G . Ly" <bryantly@linux.vnet.ibm.com>,
Frederic Barrat <fbarrat@linux.vnet.ibm.com>,
"David S . Miller" <davem@davemloft.net>,
Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
Randy Dunlap <rdunlap@infradead.org>,
Philippe Ombredanne <pombredanne@nexb.com>,
Vinod Koul <vkoul@kernel.org>,
Stephen Boyd <sboyd@codeaurora.org>,
David Kershner <david.kershner@unisys.com>,
Uwe Kleine-Konig <u.kleine-koenig@pengutronix.de>,
Sagar Dharia <sdharia@codeaurora.org>,
Johan Hovold <johan@kernel.org>,
Thomas Gleixner <tglx@linutronix.de>,
Juergen Gross <jgross@suse.com>,
Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>,
Tomer Maimon <tmaimon77@gmail.com>,
linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org,
openbmc@lists.ozlabs.org, Alan Cox <alan@linux.intel.com>,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Jason M Biils <jason.m.bills@linux.intel.com>,
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>,
Andrew Lunn <andrew@lunn.ch>,
Stef van Os <stef.van.os@prodrive-technologies.com>
Subject: Re: [PATCH v11 14/14] hwmon: Add PECI dimmtemp driver
Date: Mon, 16 Dec 2019 15:27:20 -0800 [thread overview]
Message-ID: <20191216232720.GA17398@roeck-us.net> (raw)
In-Reply-To: <c6ccb0ff-c0b4-86b2-1768-ba63713034a4@linux.intel.com>
On Mon, Dec 16, 2019 at 02:17:34PM -0800, Jae Hyun Yoo wrote:
> [...]
>
> > > > > +static int get_dimm_temp(struct peci_dimmtemp *priv, int dimm_no)
> > > > > +{
> > > > > + int dimm_order = dimm_no % priv->gen_info->dimm_idx_max;
> > > > > + int chan_rank = dimm_no / priv->gen_info->dimm_idx_max;
> > > > > + struct peci_rd_pci_cfg_local_msg rp_msg;
> > > > > + u8 cfg_data[4];
> > > > > + int ret;
> > > > > +
> > > > > + if (!peci_sensor_need_update(&priv->temp[dimm_no]))
> > > > > + return 0;
> > > > > +
> > > > > + ret = read_ddr_dimm_temp_config(priv, chan_rank, cfg_data);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + priv->temp[dimm_no].value = cfg_data[dimm_order] * 1000;
> > > > > +
> > > > > + switch (priv->gen_info->model) {
> > > > > + case INTEL_FAM6_SKYLAKE_X:
> > > > > + rp_msg.addr = priv->mgr->client->addr;
> > > > > + rp_msg.bus = 2;
> > > > > + /*
> > > > > + * Device 10, Function 2: IMC 0 channel 0 -> rank 0
> > > > > + * Device 10, Function 6: IMC 0 channel 1 -> rank 1
> > > > > + * Device 11, Function 2: IMC 0 channel 2 -> rank 2
> > > > > + * Device 12, Function 2: IMC 1 channel 0 -> rank 3
> > > > > + * Device 12, Function 6: IMC 1 channel 1 -> rank 4
> > > > > + * Device 13, Function 2: IMC 1 channel 2 -> rank 5
> > > > > + */
> > > > > + rp_msg.device = 10 + chan_rank / 3 * 2 +
> > > > > + (chan_rank % 3 == 2 ? 1 : 0);
> > > > > + rp_msg.function = chan_rank % 3 == 1 ? 6 : 2;
> > > > > + rp_msg.reg = 0x120 + dimm_order * 4;
> > > > > + rp_msg.rx_len = 4;
> > > > > +
> > > > > + ret = peci_command(priv->mgr->client->adapter,
> > > > > + PECI_CMD_RD_PCI_CFG_LOCAL, &rp_msg);
> > > > > + if (rp_msg.cc != PECI_DEV_CC_SUCCESS)
> > > > > + ret = -EAGAIN;
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + priv->temp_max[dimm_no] = rp_msg.pci_config[1] * 1000;
> > > > > + priv->temp_crit[dimm_no] = rp_msg.pci_config[2] * 1000;
> > > > > + break;
> > > > > + case INTEL_FAM6_SKYLAKE_XD:
> > > > > + rp_msg.addr = priv->mgr->client->addr;
> > > > > + rp_msg.bus = 2;
> > > > > + /*
> > > > > + * Device 10, Function 2: IMC 0 channel 0 -> rank 0
> > > > > + * Device 10, Function 6: IMC 0 channel 1 -> rank 1
> > > > > + * Device 12, Function 2: IMC 1 channel 0 -> rank 2
> > > > > + * Device 12, Function 6: IMC 1 channel 1 -> rank 3
> > > > > + */
> > > > > + rp_msg.device = 10 + chan_rank / 2 * 2;
> > > > > + rp_msg.function = (chan_rank % 2) ? 6 : 2;
> > > > > + rp_msg.reg = 0x120 + dimm_order * 4;
> > > > > + rp_msg.rx_len = 4;
> > > > > +
> > > > > + ret = peci_command(priv->mgr->client->adapter,
> > > > > + PECI_CMD_RD_PCI_CFG_LOCAL, &rp_msg);
> > > > > + if (rp_msg.cc != PECI_DEV_CC_SUCCESS)
> > > > > + ret = -EAGAIN;
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + priv->temp_max[dimm_no] = rp_msg.pci_config[1] * 1000;
> > > > > + priv->temp_crit[dimm_no] = rp_msg.pci_config[2] * 1000;
> > > > > + break;
> > > > > + case INTEL_FAM6_HASWELL_X:
> > > > > + case INTEL_FAM6_BROADWELL_X:
> > > > > + rp_msg.addr = priv->mgr->client->addr;
> > > > > + rp_msg.bus = 1;
> > > > > + /*
> > > > > + * Device 20, Function 0: IMC 0 channel 0 -> rank 0
> > > > > + * Device 20, Function 1: IMC 0 channel 1 -> rank 1
> > > > > + * Device 21, Function 0: IMC 0 channel 2 -> rank 2
> > > > > + * Device 21, Function 1: IMC 0 channel 3 -> rank 3
> > > > > + * Device 23, Function 0: IMC 1 channel 0 -> rank 4
> > > > > + * Device 23, Function 1: IMC 1 channel 1 -> rank 5
> > > > > + * Device 24, Function 0: IMC 1 channel 2 -> rank 6
> > > > > + * Device 24, Function 1: IMC 1 channel 3 -> rank 7
> > > > > + */
> > > > > + rp_msg.device = 20 + chan_rank / 2 + chan_rank / 4;
> > > > > + rp_msg.function = chan_rank % 2;
> > > > > + rp_msg.reg = 0x120 + dimm_order * 4;
> > > > > + rp_msg.rx_len = 4;
> > > > > +
> > > > > + ret = peci_command(priv->mgr->client->adapter,
> > > > > + PECI_CMD_RD_PCI_CFG_LOCAL, &rp_msg);
> > > > > + if (rp_msg.cc != PECI_DEV_CC_SUCCESS)
> > > > > + ret = -EAGAIN;
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + priv->temp_max[dimm_no] = rp_msg.pci_config[1] * 1000;
> > > > > + priv->temp_crit[dimm_no] = rp_msg.pci_config[2] * 1000;
> > > > > + break;
> > > > > + default:
> > > > > + return -EOPNOTSUPP;
> > > >
> > > > It looks like the sensors are created even on unsupported platforms,
> > > > which would generate error messages whenever someone tries to read
> > > > the attributes.
> > > >
> > > > There should be some code early on checking this, and the driver
> > > > should not even instantiate if the CPU model is not supported.
> > >
> > > Actually, this 'default' case will not be happened because this driver
> > > will be registered only when the CPU model is supported. The CPU model
> > > checking code is in 'intel-peci-client.c' which is [11/14] of this
> > > patch set.
> > >
> >
> > That again assumes that both drivers will be modified in sync in the future.
> > We can not make that assumption.
>
> As you said, both drivers must be modified in sync in the future because
> each Intel CPU family uses different way of reading DIMM temperature.
> In case if supported CPU checking code updated without making sync with
> it, this driver will return the error.
>
... and in that situation the driver should not instantiate in the
first place. Its probe function should return -ENODEV.
Guenter
next prev parent reply other threads:[~2019-12-16 23:27 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-11 19:46 [PATCH v11 00/14] PECI device driver introduction Jae Hyun Yoo
2019-12-11 19:46 ` [PATCH v11 01/14] dt-bindings: Add PECI subsystem document Jae Hyun Yoo
2019-12-18 2:52 ` Rob Herring
2019-12-18 23:12 ` Jae Hyun Yoo
2019-12-11 19:46 ` [PATCH v11 02/14] Documentation: ioctl: Add ioctl numbers for PECI subsystem Jae Hyun Yoo
2019-12-11 19:46 ` [PATCH v11 04/14] dt-bindings: Add bindings document of Aspeed PECI adapter Jae Hyun Yoo
2019-12-18 2:57 ` Rob Herring
2019-12-18 23:21 ` Jae Hyun Yoo
2019-12-11 19:46 ` [PATCH v11 05/14] ARM: dts: aspeed: Add PECI node Jae Hyun Yoo
2019-12-11 19:46 ` [PATCH v11 06/14] peci: Add Aspeed PECI adapter driver Jae Hyun Yoo
2019-12-11 20:28 ` Andy Shevchenko
2019-12-12 0:50 ` Jae Hyun Yoo
2019-12-12 8:47 ` Andy Shevchenko
2019-12-12 18:51 ` Jae Hyun Yoo
2019-12-11 19:46 ` [PATCH v11 07/14] dt-bindings: peci: add NPCM PECI documentation Jae Hyun Yoo
2019-12-18 14:42 ` Rob Herring
2019-12-18 23:30 ` Jae Hyun Yoo
2019-12-11 19:46 ` [PATCH v11 08/14] ARM: dts: npcm7xx: Add PECI node Jae Hyun Yoo
2019-12-11 19:46 ` [PATCH v11 09/14] peci: npcm: add NPCM PECI driver Jae Hyun Yoo
2019-12-11 19:46 ` [PATCH v11 10/14] dt-bindings: mfd: Add Intel PECI client bindings document Jae Hyun Yoo
2019-12-11 19:46 ` [PATCH v11 11/14] mfd: intel-peci-client: Add Intel PECI client driver Jae Hyun Yoo
2019-12-16 16:01 ` Lee Jones
2019-12-16 21:57 ` Jae Hyun Yoo
2019-12-11 19:46 ` [PATCH v11 12/14] Documentation: hwmon: Add documents for PECI hwmon drivers Jae Hyun Yoo
2019-12-11 19:46 ` [PATCH v11 13/14] hwmon: Add PECI cputemp driver Jae Hyun Yoo
2019-12-13 6:24 ` Guenter Roeck
2019-12-16 20:43 ` Jae Hyun Yoo
2019-12-11 19:46 ` [PATCH v11 14/14] hwmon: Add PECI dimmtemp driver Jae Hyun Yoo
2019-12-13 6:32 ` Guenter Roeck
2019-12-16 21:04 ` Jae Hyun Yoo
2019-12-16 21:21 ` Guenter Roeck
2019-12-16 22:17 ` Jae Hyun Yoo
2019-12-16 23:27 ` Guenter Roeck [this message]
2019-12-16 23:31 ` Jae Hyun Yoo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191216232720.GA17398@roeck-us.net \
--to=linux@roeck-us.net \
--cc=akpm@linux-foundation.org \
--cc=alan@linux.intel.com \
--cc=andrew@aj.id.au \
--cc=andrew@lunn.ch \
--cc=andriy.shevchenko@linux.intel.com \
--cc=arnd@arndb.de \
--cc=bryantly@linux.vnet.ibm.com \
--cc=corbet@lwn.net \
--cc=cyrille.pitchen@wedev4u.fr \
--cc=darrick.wong@oracle.com \
--cc=davem@davemloft.net \
--cc=david.kershner@unisys.com \
--cc=devicetree@vger.kernel.org \
--cc=fbarrat@linux.vnet.ibm.com \
--cc=gregkh@linuxfoundation.org \
--cc=gustavo.pimentel@synopsys.com \
--cc=hao.wu@intel.com \
--cc=jae.hyun.yoo@linux.intel.com \
--cc=jason.m.bills@linux.intel.com \
--cc=jdelvare@suse.com \
--cc=jgross@suse.com \
--cc=joel@jms.id.au \
--cc=johan@kernel.org \
--cc=kishon@ti.com \
--cc=kusumi.tomohiro@gmail.com \
--cc=lee.jones@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=mark.rutland@arm.com \
--cc=mchehab+samsung@kernel.org \
--cc=miguel.ojeda.sandonis@gmail.com \
--cc=openbmc@lists.ozlabs.org \
--cc=pombredanne@nexb.com \
--cc=rdunlap@infradead.org \
--cc=robh+dt@kernel.org \
--cc=sandeen@redhat.com \
--cc=sboyd@codeaurora.org \
--cc=sdharia@codeaurora.org \
--cc=stef.van.os@prodrive-technologies.com \
--cc=tglx@linutronix.de \
--cc=tmaimon77@gmail.com \
--cc=u.kleine-koenig@pengutronix.de \
--cc=vkoul@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).