Re: [PATCH 2/4] x86/amd_nb: add support for newer PCI topologies
On Mon, Nov 05, 2018 at 10:42:33PM +0100, Borislav Petkov wrote: > Yes please. Because this is the usual kernel coding style of calling a > function (or a loop which has some result in this case) and testing that > result immediately after the function call. Done. > You say "correct" as there is a special one. But the text before it says > they're "functionally the same" wrt DF/SMN access so it sounds to me > like we wanna map the first one we find and ignore the others. > > I.e., we wanna say > > "... so the DF/SMN interfaces get mapped to the *first* PCI root and the > others N-1 ignored." > > Or am I misreading this? > > Thx. > > -- > Regards/Gruss, > Boris. > > Good mailing practices for 400: avoid top-posting and trim the reply. Your understanding is correct. It's more so that the following DF/SMN interface gets mapped correctly. /* * If there are more PCI root devices than data fabric/ * system management network interfaces, then the (N) * PCI roots per DF/SMN interface are functionally the * same (for DF/SMN access) and N-1 are redundant. N-1 * PCI roots should be skipped per DF/SMN interface so * the following DF/SMN interfaces get mapped to * correct PCI roots. */ Does that read clearer? -- Brian Woods
Re: [PATCH 2/4] x86/amd_nb: add support for newer PCI topologies
[+cc Takashi, Andy, Colin, Myron for potential distro impact] [Beginning of thread: https://lore.kernel.org/linux-pci/20181102181055.130531-1-brian.wo...@amd.com/] On Sat, Nov 03, 2018 at 12:29:48AM +0100, Borislav Petkov wrote: > On Fri, Nov 02, 2018 at 02:59:25PM -0500, Bjorn Helgaas wrote: > > This isn't my code, and I'm not really objecting to these changes, but > > from where I sit, the fact that you need this sort of vendor-specific > > topology discovery is a little bit ugly and seems like something of a > > maintenance issue. I think this is the most important part, and I should have elaborated on it instead of getting into the driver structure details below. It is a major goal of ACPI and PCI that an old kernel should work unchanged on a new platform unless it needs to use new functionality introduced in the new platform. amd_nb.c prevents us from achieving that goal. These patches don't add new functionality; they merely describe minor topographical differences in new hardware. We usually try to do that in a more generic way, e.g., via an ACPI method, so the new platform can update the ACPI method and use an old, already-qualified, already-shipped kernel. I'm not strenuously objecting to these because this isn't a *huge* deal, but I suspect it is a source of friction for distros that don't want to update and requalify their software for every new platform. > > You could argue that this is sort of an "AMD CPU > > driver", which is entitled to be device-specific, and that does make > > some sense. > > It is a bunch of glue code which enumerates the PCI devices a CPU > has and other in-kernel users can use that instead of doing the > discovery/enumeration themselves. > > > But device-specific code is typically packaged as a driver that uses > > driver registration interfaces like acpi_bus_register_driver(), > > pci_register_driver(), etc. That gives you a consistent structure > > and, more importantly, a framework for dealing with hotplug. It > > doesn't look like amd_nb.c would deal well with hot-add of CPUs. > > If you mean physical hotadd, then that's a non-issue as, AFAIK, AMD > doesn't support that. > > Now, TBH I've never tried soft-offlining the cores of a node and then > check whether using the PCI devices of that node would work. > > Now, I don't mind this getting converted to a proper PCI driver as long > as it is not a module as it has to be present at all times. Other than > that, I'm a happy camper. amd_nb.c uses pci_get_device(), which is incompatible with hotplug and subverts the usual driver/device ownership model. We could pursue this part of the conversation, but I think it's more fruitful to approach this from the "new machine, old kernel" angle above.
Re: [PATCH 2/4] x86/amd_nb: add support for newer PCI topologies
On Mon, Nov 05, 2018 at 08:33:34PM +, Woods, Brian wrote: > I think having them togeter is cleaner. If you aren't finding any > misc IDs, I highly doubt you'll find any root IDs. There shouldn't > be much of a difference in how fast the function exits, either way. > If you want it the other way though, I don't mind changing it. Yes please. Because this is the usual kernel coding style of calling a function (or a loop which has some result in this case) and testing that result immediately after the function call. > Would > > /* >* If there are more PCI root devices than data fabric/ >* system management network interfaces, then the (N) >* PCI roots per DF/SMN interface are functionally the >* same (for DF/SMN access) and N-1 are redundant. The >* N-1 PCI roots should be skipped per DF/SMN interface >* so the DF/SMN interfaces get mapped to the correct >* PCI root. You say "correct" as there is a special one. But the text before it says they're "functionally the same" wrt DF/SMN access so it sounds to me like we wanna map the first one we find and ignore the others. I.e., we wanna say "... so the DF/SMN interfaces get mapped to the *first* PCI root and the others N-1 ignored." Or am I misreading this? Thx. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
[PATCH v5 1/4] hwmon: (ina3221) Check channel status for alarms attribute read
There is nothing critically wrong to read these two attributes
without having a is_enabled() check at this point. But reading
the MASK_ENABLE register would clear the CVRF bit according to
the datasheet. So it'd be safer to fence for disabled channels
in order to add pm runtime feature.
Signed-off-by: Nicolin Chen
---
Changelog
v2->v5:
* N/A
v1->v2:
* Returned 0 for alert flags instead of -ENODATA
drivers/hwmon/ina3221.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index d61688f04594..26cdf3342d80 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -200,6 +200,12 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
return 0;
case hwmon_curr_crit_alarm:
case hwmon_curr_max_alarm:
+ /* No actual register read if channel is disabled */
+ if (!ina3221_is_enabled(ina, channel)) {
+ /* Return 0 for alert flags */
+ *val = 0;
+ return 0;
+ }
ret = regmap_field_read(ina->fields[reg], );
if (ret)
return ret;
--
2.17.1
[PATCH v5 3/4] hwmon: (ina3221) Make sure data is ready before reading
The data might need some time to get ready after channel enabling,
although the data register is always readable. The CVRF bit is to
indicate that data conversion is finished, so polling the CVRF bit
before data reading could ensure the result being valid.
An alternative way could be to wait for expected time between the
channel enabling and the data reading. And this could avoid extra
I2C communications. However, INA3221 seemly takes longer time than
what's stated in the datasheet. Test results show that sometimes
it couldn't finish data conversion in time.
So this patch plays safe by adding a CVRF polling to make sure the
data register is updated with the new data.
Signed-off-by: Nicolin Chen
---
Changelog
v3->v5:
* N/A
v2->v3:
* Dropped timeout dev_err messages as it's indicated in the errno
v1->v2:
* Moved CVRF polling to data read routine
* Added calculation of wait time based on conversion time setting
drivers/hwmon/ina3221.c | 42 +
1 file changed, 42 insertions(+)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 10e8347a3c80..07dd6ef58d3e 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -44,6 +44,13 @@
#define INA3221_CONFIG_MODE_SHUNT BIT(0)
#define INA3221_CONFIG_MODE_BUSBIT(1)
#define INA3221_CONFIG_MODE_CONTINUOUS BIT(2)
+#define INA3221_CONFIG_VSH_CT_SHIFT3
+#define INA3221_CONFIG_VSH_CT_MASK GENMASK(5, 3)
+#define INA3221_CONFIG_VSH_CT(x) (((x) & GENMASK(5, 3)) >> 3)
+#define INA3221_CONFIG_VBUS_CT_SHIFT 6
+#define INA3221_CONFIG_VBUS_CT_MASKGENMASK(8, 6)
+#define INA3221_CONFIG_VBUS_CT(x) (((x) & GENMASK(8, 6)) >> 6)
+#define INA3221_CONFIG_CHs_EN_MASK GENMASK(14, 12)
#define INA3221_CONFIG_CHx_EN(x) BIT(14 - (x))
#define INA3221_RSHUNT_DEFAULT 1
@@ -52,6 +59,9 @@ enum ina3221_fields {
/* Configuration */
F_RST,
+ /* Status Flags */
+ F_CVRF,
+
/* Alert Flags */
F_WF3, F_WF2, F_WF1,
F_CF3, F_CF2, F_CF1,
@@ -63,6 +73,7 @@ enum ina3221_fields {
static const struct reg_field ina3221_reg_fields[] = {
[F_RST] = REG_FIELD(INA3221_CONFIG, 15, 15),
+ [F_CVRF] = REG_FIELD(INA3221_MASK_ENABLE, 0, 0),
[F_WF3] = REG_FIELD(INA3221_MASK_ENABLE, 3, 3),
[F_WF2] = REG_FIELD(INA3221_MASK_ENABLE, 4, 4),
[F_WF1] = REG_FIELD(INA3221_MASK_ENABLE, 5, 5),
@@ -111,6 +122,28 @@ static inline bool ina3221_is_enabled(struct ina3221_data
*ina, int channel)
return ina->reg_config & INA3221_CONFIG_CHx_EN(channel);
}
+/* Lookup table for Bus and Shunt conversion times in usec */
+static const u16 ina3221_conv_time[] = {
+ 140, 204, 332, 588, 1100, 2116, 4156, 8244,
+};
+
+static inline int ina3221_wait_for_data(struct ina3221_data *ina)
+{
+ u32 channels = hweight16(ina->reg_config & INA3221_CONFIG_CHs_EN_MASK);
+ u32 vbus_ct_idx = INA3221_CONFIG_VBUS_CT(ina->reg_config);
+ u32 vsh_ct_idx = INA3221_CONFIG_VSH_CT(ina->reg_config);
+ u32 vbus_ct = ina3221_conv_time[vbus_ct_idx];
+ u32 vsh_ct = ina3221_conv_time[vsh_ct_idx];
+ u32 wait, cvrf;
+
+ /* Calculate total conversion time */
+ wait = channels * (vbus_ct + vsh_ct);
+
+ /* Polling the CVRF bit to make sure read data is ready */
+ return regmap_field_read_poll_timeout(ina->fields[F_CVRF],
+ cvrf, cvrf, wait, 10);
+}
+
static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
int *val)
{
@@ -150,6 +183,10 @@ static int ina3221_read_in(struct device *dev, u32 attr,
int channel, long *val)
if (!ina3221_is_enabled(ina, channel))
return -ENODATA;
+ ret = ina3221_wait_for_data(ina);
+ if (ret)
+ return ret;
+
ret = ina3221_read_value(ina, reg, );
if (ret)
return ret;
@@ -189,6 +226,11 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
case hwmon_curr_input:
if (!ina3221_is_enabled(ina, channel))
return -ENODATA;
+
+ ret = ina3221_wait_for_data(ina);
+ if (ret)
+ return ret;
+
/* fall through */
case hwmon_curr_crit:
case hwmon_curr_max:
--
2.17.1
[PATCH v5 2/4] hwmon: (ina3221) Serialize sysfs ABI accesses
This change adds a mutex to serialize accesses of sysfs attributes.
This is required when polling CVRF bit of the MASK/ENABLE register
because this bit is cleared on a read of this MASK/ENABLE register
or a write to CONFIG register, which means that this bit might be
accidentally cleared by reading other fields like alert flags.
So this patch adds a mutex lock to protect the write() and read()
callbacks. The read_string() callback won't need the lock since it
just returns the label without touching any hardware register.
Signed-off-by: Nicolin Chen
---
Changlog
v1->v5:
* N/A
drivers/hwmon/ina3221.c | 51 -
1 file changed, 45 insertions(+), 6 deletions(-)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 26cdf3342d80..10e8347a3c80 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
@@ -94,12 +95,14 @@ struct ina3221_input {
* @regmap: Register map of the device
* @fields: Register fields of the device
* @inputs: Array of channel input source specific structures
+ * @lock: mutex lock to serialize sysfs attribute accesses
* @reg_config: Register value of INA3221_CONFIG
*/
struct ina3221_data {
struct regmap *regmap;
struct regmap_field *fields[F_MAX_FIELDS];
struct ina3221_input inputs[INA3221_NUM_CHANNELS];
+ struct mutex lock;
u32 reg_config;
};
@@ -265,29 +268,53 @@ static int ina3221_write_enable(struct device *dev, int
channel, bool enable)
static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,
u32 attr, int channel, long *val)
{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ int ret;
+
+ mutex_lock(>lock);
+
switch (type) {
case hwmon_in:
/* 0-align channel ID */
- return ina3221_read_in(dev, attr, channel - 1, val);
+ ret = ina3221_read_in(dev, attr, channel - 1, val);
+ break;
case hwmon_curr:
- return ina3221_read_curr(dev, attr, channel, val);
+ ret = ina3221_read_curr(dev, attr, channel, val);
+ break;
default:
- return -EOPNOTSUPP;
+ ret = -EOPNOTSUPP;
+ break;
}
+
+ mutex_unlock(>lock);
+
+ return ret;
}
static int ina3221_write(struct device *dev, enum hwmon_sensor_types type,
u32 attr, int channel, long val)
{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ int ret;
+
+ mutex_lock(>lock);
+
switch (type) {
case hwmon_in:
/* 0-align channel ID */
- return ina3221_write_enable(dev, channel - 1, val);
+ ret = ina3221_write_enable(dev, channel - 1, val);
+ break;
case hwmon_curr:
- return ina3221_write_curr(dev, attr, channel, val);
+ ret = ina3221_write_curr(dev, attr, channel, val);
+ break;
default:
- return -EOPNOTSUPP;
+ ret = -EOPNOTSUPP;
+ break;
}
+
+ mutex_unlock(>lock);
+
+ return ret;
}
static int ina3221_read_string(struct device *dev, enum hwmon_sensor_types
type,
@@ -582,6 +609,7 @@ static int ina3221_probe(struct i2c_client *client,
if (ret)
return ret;
+ mutex_init(>lock);
dev_set_drvdata(dev, ina);
hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, ina,
@@ -589,12 +617,22 @@ static int ina3221_probe(struct i2c_client *client,
ina3221_groups);
if (IS_ERR(hwmon_dev)) {
dev_err(dev, "Unable to register hwmon device\n");
+ mutex_destroy(>lock);
return PTR_ERR(hwmon_dev);
}
return 0;
}
+static int ina3221_remove(struct i2c_client *client)
+{
+ struct ina3221_data *ina = dev_get_drvdata(>dev);
+
+ mutex_destroy(>lock);
+
+ return 0;
+}
+
static int __maybe_unused ina3221_suspend(struct device *dev)
{
struct ina3221_data *ina = dev_get_drvdata(dev);
@@ -663,6 +701,7 @@ MODULE_DEVICE_TABLE(i2c, ina3221_ids);
static struct i2c_driver ina3221_i2c_driver = {
.probe = ina3221_probe,
+ .remove = ina3221_remove,
.driver = {
.name = INA3221_DRIVER_NAME,
.of_match_table = ina3221_of_match_table,
--
2.17.1
[PATCH v5 4/4] hwmon: (ina3221) Add PM runtime support
If all three channels are disabled via in[123]_enable ABI,
the driver could suspend the chip for power saving purpose.
So this patch adds the PM runtime support in order to gain
more power control than system suspend and resume use case.
For PM runtime, there are a few related changes happening:
1) Added a new pm_dev device pointer for all the PM runtime
callbacks. This is because hwmon core registers a child
device for each hwmon driver and passes it back to each
driver. So there might be a mismatch between two device
pointers in the driver if mixing using them.
2) Added a check in ina3221_is_enabled() to make sure that
the chip is resumed.
3) Bypassed the unchanged status in ina3221_write_enable()
in order to keep the PM runtime refcount being matched.
4) Removed the reset routine in the probe() by calling the
resume() via pm_runtime_get_sync() instead, as they're
similar. It's also necessary to do so to match initial
PM refcount with the number of enabled channels.
Signed-off-by: Nicolin Chen
---
Changelog
v4->v5:
* Dropped the code of passing pm pointer via _info API
** Moved all changes back to the status of v3
* Used i2c_client dev pointer for pm runtime callbacks
v3->v4:
* Passed pm pointer via _with_info API instead the i2c driver
v2->v3:
* Improved a dev_err message
* Added comments at pm_runtime_put_noidle() callbacks
* Added pm_runtime header file in an alphabetical order
v1->v2:
* Bypassed i2c_client->dev in suspend/resume()
* Added a missing '\n' in one dev_err()
drivers/hwmon/ina3221.c | 93 -
1 file changed, 74 insertions(+), 19 deletions(-)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 07dd6ef58d3e..17a57dbc0424 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -20,6 +20,7 @@
#include
#include
#include
+#include
#include
#define INA3221_DRIVER_NAME"ina3221"
@@ -53,6 +54,7 @@
#define INA3221_CONFIG_CHs_EN_MASK GENMASK(14, 12)
#define INA3221_CONFIG_CHx_EN(x) BIT(14 - (x))
+#define INA3221_CONFIG_DEFAULT 0x7127
#define INA3221_RSHUNT_DEFAULT 1
enum ina3221_fields {
@@ -103,6 +105,7 @@ struct ina3221_input {
/**
* struct ina3221_data - device specific information
+ * @pm_dev: Device pointer for pm runtime
* @regmap: Register map of the device
* @fields: Register fields of the device
* @inputs: Array of channel input source specific structures
@@ -110,6 +113,7 @@ struct ina3221_input {
* @reg_config: Register value of INA3221_CONFIG
*/
struct ina3221_data {
+ struct device *pm_dev;
struct regmap *regmap;
struct regmap_field *fields[F_MAX_FIELDS];
struct ina3221_input inputs[INA3221_NUM_CHANNELS];
@@ -119,7 +123,8 @@ struct ina3221_data {
static inline bool ina3221_is_enabled(struct ina3221_data *ina, int channel)
{
- return ina->reg_config & INA3221_CONFIG_CHx_EN(channel);
+ return pm_runtime_active(ina->pm_dev) &&
+ (ina->reg_config & INA3221_CONFIG_CHx_EN(channel));
}
/* Lookup table for Bus and Shunt conversion times in usec */
@@ -290,21 +295,48 @@ static int ina3221_write_enable(struct device *dev, int
channel, bool enable)
{
struct ina3221_data *ina = dev_get_drvdata(dev);
u16 config, mask = INA3221_CONFIG_CHx_EN(channel);
+ u16 config_old = ina->reg_config & mask;
int ret;
config = enable ? mask : 0;
+ /* Bypass if enable status is not being changed */
+ if (config_old == config)
+ return 0;
+
+ /* For enabling routine, increase refcount and resume() at first */
+ if (enable) {
+ ret = pm_runtime_get_sync(ina->pm_dev);
+ if (ret < 0) {
+ dev_err(dev, "Failed to get PM runtime\n");
+ return ret;
+ }
+ }
+
/* Enable or disable the channel */
ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, config);
if (ret)
- return ret;
+ goto fail;
/* Cache the latest config register value */
ret = regmap_read(ina->regmap, INA3221_CONFIG, >reg_config);
if (ret)
- return ret;
+ goto fail;
+
+ /* For disabling routine, decrease refcount or suspend() at last */
+ if (!enable)
+ pm_runtime_put_sync(ina->pm_dev);
return 0;
+
+fail:
+ if (enable) {
+ dev_err(dev, "Failed to enable channel %d: error %d\n",
+ channel, ret);
+ pm_runtime_put_sync(ina->pm_dev);
+ }
+
+ return ret;
}
static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,
@@ -631,44 +663,65 @@ static int ina3221_probe(struct i2c_client *client,
return ret;
}
- ret = regmap_field_write(ina->fields[F_RST], true);
- if (ret) {
-
[PATCH v5 0/4] hwmon: (ina3221) Implement PM runtime to save power
This series patches implement PM runtime feature in the ina3221 hwmon driver (PATCH-4). However, PATCH-[1:3] are required to make sure that the PM runtime feature would be functional and safe. Changelog v4->v5: * Dropped the change for hwmon core; Shifted all the patch indexes * Used i2c_client dev pointer for pm runtime callbacks (PATCH-4) v3->v4: * Added generic pm runtime functions to hwmon class (PATCH-1) * Updated to pass pm pointer via _with_info API (PATCH-5) v2->v3: * Dropped timeout dev_err messages as it's indicated in errno (PATCH-4) * Improved a dev_err message and comments (PATCH-5) v1->v2: * Added device pointer check (PATCH-1) * Returned 0 for alert flags (PATCH-2) * Moved CVRF polling to data read routine (PATCH-4) * Bypassed i2c_client->dev in suspend/resume() (PATCH-5) Nicolin Chen (4): hwmon: (ina3221) Check channel status for alarms attribute read hwmon: (ina3221) Serialize sysfs ABI accesses hwmon: (ina3221) Make sure data is ready before reading hwmon: (ina3221) Add PM runtime support drivers/hwmon/ina3221.c | 190 +++- 1 file changed, 166 insertions(+), 24 deletions(-) -- 2.17.1
Re: [PATCH 4/4] hwmon: k10temp: add support for AMD F17h M30h CPUs
On Fri, Nov 02, 2018 at 06:11:12PM +, Woods, Brian wrote: > Add support for AMD family 17h model 30h processors for k10temp. Model > 30h is functionally the same as model 01h processors (as far as k10temp > is concerned), just the PCI device IDs need to be updated. > > Signed-off-by: Brian Woods > --- > arch/x86/kernel/amd_nb.c | 1 - > drivers/hwmon/k10temp.c | 1 + > include/linux/pci_ids.h | 1 + > 3 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c > index fd69067f6eb1..e491aa4a1970 100644 > --- a/arch/x86/kernel/amd_nb.c > +++ b/arch/x86/kernel/amd_nb.c > @@ -17,7 +17,6 @@ > #define PCI_DEVICE_ID_AMD_17H_ROOT 0x1450 > #define PCI_DEVICE_ID_AMD_17H_M10H_ROOT 0x15d0 > #define PCI_DEVICE_ID_AMD_17H_M30H_ROOT 0x1480 > -#define PCI_DEVICE_ID_AMD_17H_M30H_DF_F3 0x1493 > #define PCI_DEVICE_ID_AMD_17H_DF_F4 0x1464 > #define PCI_DEVICE_ID_AMD_17H_M10H_DF_F4 0x15ec > #define PCI_DEVICE_ID_AMD_17H_M30H_DF_F4 0x1494 Patch 3 added it here and now patch 4 moves it to pci_ids.h Just put it straight there to avoid the unnecessary churn. Thx. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH 2/4] x86/amd_nb: add support for newer PCI topologies
On Fri, Nov 02, 2018 at 06:11:07PM +, Woods, Brian wrote:
> Add support for new processors which have multiple PCI root complexes
> per data fabric/SMN interface.
Please write out abbreviations. I believe it is only you and I who know
what SMN means. :)
> The interfaces per root complex are redundant and should be skipped.
And I believe it is only you who understands that sentence. :)
Please elaborate why interfaces need to be skipped, *which* interfaces
need to be skipped and which is the correct interface to access DF/SMN
through?
> This makes sure the DF/SMN interfaces get accessed via the correct
> root complex.
>
> Ex:
> DF/SMN 0 -> 60
> 40
> 20
> 00
> DF/SMN 1 -> e0
> c0
> a0
> 80
>
> Signed-off-by: Brian Woods
> ---
> arch/x86/kernel/amd_nb.c | 41 +++--
> 1 file changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
> index 19d489ee2b1e..c0bf26aeb7c3 100644
> --- a/arch/x86/kernel/amd_nb.c
> +++ b/arch/x86/kernel/amd_nb.c
> @@ -213,7 +213,10 @@ int amd_cache_northbridges(void)
> const struct pci_device_id *root_ids = amd_root_ids;
> struct pci_dev *root, *misc, *link;
> struct amd_northbridge *nb;
> - u16 i = 0;
> + u16 roots_per_misc = 0;
> + u16 misc_count = 0;
> + u16 root_count = 0;
> + u16 i, j;
>
> if (amd_northbridges.num)
> return 0;
> @@ -226,26 +229,52 @@ int amd_cache_northbridges(void)
>
> misc = NULL;
> while ((misc = next_northbridge(misc, misc_ids)) != NULL)
> - i++;
> + misc_count++;
>
> - if (!i)
> + root = NULL;
> + while ((root = next_northbridge(root, root_ids)) != NULL)
> + root_count++;
> +
> + if (!misc_count)
> return -ENODEV;
So you're doing the root_count above but returning in the !misc_count
case. So that root_count iteration was unnecessary work. IOW, you should
keep the misc_count check after its loop.
>
> - nb = kcalloc(i, sizeof(struct amd_northbridge), GFP_KERNEL);
> + if (root_count) {
> + roots_per_misc = root_count / misc_count;
> +
> + /*
> + * There should be _exactly_ N roots for each DF/SMN
> + * interface.
> + */
> + if (!roots_per_misc || (root_count % roots_per_misc)) {
> + pr_info("Unsupported AMD DF/PCI configuration found\n");
> + return -ENODEV;
> + }
> + }
> +
> + nb = kcalloc(misc_count, sizeof(struct amd_northbridge), GFP_KERNEL);
> if (!nb)
> return -ENOMEM;
>
> amd_northbridges.nb = nb;
> - amd_northbridges.num = i;
> + amd_northbridges.num = misc_count;
>
> link = misc = root = NULL;
> - for (i = 0; i != amd_northbridges.num; i++) {
> + for (i = 0; i < amd_northbridges.num; i++) {
> node_to_amd_nb(i)->root = root =
> next_northbridge(root, root_ids);
> node_to_amd_nb(i)->misc = misc =
> next_northbridge(misc, misc_ids);
> node_to_amd_nb(i)->link = link =
> next_northbridge(link, link_ids);
> +
> + /*
> + * If there are more root devices than data fabric/SMN,
> + * interfaces, then the root devices per DF/SMN
> + * interface are redundant and N-1 should be skipped so
> + * they aren't mapped incorrectly.
> + */
This text is trying to explain it a bit better but you still still need
to specify which are the redundant ones. All N-1 or is there a special
root device through which the DF/SMN gets accessed or?
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH v2 2/2] hwmon: add generic GPIO brownout support
On 18-11-02 23:05, Trent Piepho wrote:
> On Fri, 2018-11-02 at 07:38 +0100, Marco Felsch wrote:
> > On 18-11-01 18:21, Trent Piepho wrote:
> > > On Thu, 2018-11-01 at 08:14 -0700, Guenter Roeck wrote:
> > > > On Thu, Nov 01, 2018 at 03:53:12PM +0100, Marco Felsch wrote:
> > > > >
> > > > > >
> > > > > > Isn't that configurable with devicetree flags ? I don't think a
> > > > > > driver
> > > > > > should get involved in deciding the active edge.
> > > > >
> > > > > No, AFAIK we can only specify the active level types for gpios. This
> > > > > made sense to me, because I saw no gpio-controller which support
> > > > > 'edge-level' reporting (however it will be called) currently.
> > >
> > > Interrupts types are specific to each interrupt controller, but there
> > > is a standard set of flags that, AFAIK, every Linux controller uses.
> > > These include IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_EDGE_RISING,
> > > IRQ_TYPE_LEVEL_HIGH, and so on.
> > >
> > > So you can support hardware that is inherently edge or level triggered.
> >
> > I've been spoken about gpio-controllers and AFAIK there are no edge
> > types. Interrupt-Controller are a different story, as you pointed out
> > above.
>
> You can use edge triggering with gpios. Just try writing "rising" or
> "falling" into /sys/class/gpio/gpioX/edge
Can we access the gpios trough the sysfs if they are requested by a
driver?
> It's level you can't do sysfs. The irq masking necessary isn't
> supported to get it to work in a useful way, i.e. without a live-lock
> IRQ loop.
>
> But you can in the kernel.
>
> Normal process is to call gpiod_to_irq() and then use standard IRQF
> flags to select level, edge, etc.
Currently I using the gpiod_to_irq() function to convert the sense gpio
into a irq, but I do some magic to determine the edge. I tought there
might be reasons why there are no edge defines in
include/dt-bindings/gpio/gpio.h.
> > Too fast state changes sounds like a glitch. Anyway, IMHO we should
>
> Linux has no hard real-time guarantee about interrupt latency, so
> there's no way you can be sure each interrupt is processed before the
> next.
>
> Trying to track level by interrupting on both edges doesn't work well.
> You get out of sync and stuck waiting for something that's already
> happened.
Yes, I'm with you.
> > support support both interrupts and gpios. If the users use gpios they
> > have to poll the gpio, as Guenter pointed out, else we register a
> > irq-handler.
>
> If gpio(d?)_to_irq returns a valid value, why poll? It usually works
> to call this. Plenty of call sites in the kernel that use it and don't
> fallback to polling if it doesn't work.
>
> I think it's fine to call gpiod_to_irq() and fail if that fails, and
> let a polling fallback be written if and when the need arises.
Okay, so no polling for the current solution. Let me summarize our
solution:
- no polling (currently)
- dt-node specifies a gpio instead of a interrupt
-> gpio <-> irq mapping is done by gpiod_to_irq() and fails if gpio
doesn't support irq's
- more alarms per sensor
Only one last thing I tought about:
Using a flat design like you mentioned would lead into a "virtual"
address conflict, since both sensors are on the same level. I tought
about i2c/spi/muxes/graph-devices which don't support such "addressing"
scheme.
hwmon_dev {
compatible = "gpio-alarm";
bat@0 {
reg = <0>;
label = "Battery Pack1 Voltage";
type = "voltage";
alarm-type = ;
alarm-gpios = < 15 GPIO_ACTIVE_LOW
16 GPIO_ACTIVE_LOW>;
};
bat@1 {
reg = <1>;
label = "Battery Pack2 Voltage";
alarm-type = ;
alarm-gpios = < 9 GPIO_ACTIVE_LOW
1 GPIO_ACTIVE_LOW>;
};
cputemp@0 {
reg = <0>;
label = "CPU Temperature Critical";
type = "temperature";
alarm-type = ;
alarm-gpios = < 17 GPIO_ACTIVE_LOW>;
};
};
Where a more structured layout don't have this "issue".
hwmon_dev {
compatible = "gpio-alarm";
voltage {
bat@0 {
reg = <0>;
label = "Battery Pack1 Voltage";
alarm-type = ;
alarm-gpios = < 15 GPIO_ACTIVE_LOW
16 GPIO_ACTIVE_LOW>;
};
bat@1 {
reg = <1>;
label = "Battery Pack2 Voltage";
alarm-type = ;
alarm-gpios = < 9 GPIO_ACTIVE_LOW
1 GPIO_ACTIVE_LOW>;
};
};
temperature {
cputemp {
label = "CPU Temperature Critical";
alarm-type
