[lm-sensors] [PATCH/RFC] sensors-detect: Add detection of PMBus

[lm-sensors] [PATCH/RFC] sensors-detect: Add detection of PMBus

[Guenter Roeck]

This patch adds basic support for detection of PMBus devices.
It can easily be extended to support additional PMBus devices.
Key question right now is if this approach is acceptable.

Notable changes:
- Chip detection code now uses byte read to detect devices in address range [0x03..0x0a, 0x0d..0x3f]
  TBD: Maybe we should use SMBUS_QUICK first followed by SMBUS_BYTE if it fails
  or if it is not supported by the adapter.
- Added support for SMBus block read command
- Fixed bug seen if I2C master numbering is non-sequential (which I should probably
  commit separately if the fix is ok).

Thanks,
Guenter

--
Index: prog/detect/sensors-detect
=================================--- prog/detect/sensors-detect	(revision 5865)
+++ prog/detect/sensors-detect	(working copy)
@@ -1249,6 +1249,31 @@
 		driver => "sbs", # ACPI driver, not sure if it always works
 		i2c_addrs => [0x0b],
 		i2c_detect => sub { smartbatt_detect(@_); },
+	}, {
+		name => "Linear Technology LTC2978",
+		driver => "to-be-written",
+		i2c_addrs => [0x5c..0x64],
+		i2c_detect => sub { pmbus_detect(@_, 0); },
+	}, {
+		name => "Ericsson BMR450",
+		driver => "to-be-written",
+		i2c_addrs => [0x03..0x0a, 0x0d..0x3f],
+		i2c_detect => sub { pmbus_detect(@_, 1); },
+	}, {
+		name => "Ericsson BMR451",
+		driver => "to-be-written",
+		i2c_addrs => [0x03..0x0a, 0x0d..0x3f],
+		i2c_detect => sub { pmbus_detect(@_, 2); },
+	}, {
+		name => "Ericsson BMR453",
+		driver => "to-be-written",
+		i2c_addrs => [0x03..0x0a, 0x0d..0x3f],
+		i2c_detect => sub { pmbus_detect(@_, 3); },
+	}, {
+		name => "Ericsson BMR454",
+		driver => "to-be-written",
+		i2c_addrs => [0x03..0x0a, 0x0d..0x3f],
+		i2c_detect => sub { pmbus_detect(@_, 4); },
 	}
 );
 
@@ -2812,10 +2837,12 @@
 use constant SMBUS_BYTE		=> 1;
 use constant SMBUS_BYTE_DATA	=> 2;
 use constant SMBUS_WORD_DATA	=> 3;
+use constant SMBUS_BLOCK_DATA	=> 5;
 
 use constant I2C_FUNC_SMBUS_QUICK	=> 0x00010000;
 use constant I2C_FUNC_SMBUS_READ_BYTE	=> 0x00020000;
 use constant I2C_FUNC_SMBUS_READ_BYTE_DATA	=> 0x00080000;
+use constant I2C_FUNC_SMBUS_READ_BLOCK_DATA	=> 0x01000000;
 
 # Get the i2c adapter's functionalities
 # $_[0]: Reference to an opened filehandle
@@ -2919,6 +2946,26 @@
 }
 
 # $_[0]: Reference to an opened filehandle
+# $_[1]: Command byte (usually register number)
+# Returns: -1 on failure, the read block on success.
+# Use this function with care, some devices don't like block reads,
+# so you should do as much of the detection as possible using byte reads,
+# and only start using block reads when there is a good chance that
+# the detection will succeed.
+sub i2c_smbus_read_block_data
+{
+	my ($file, $command) = @_;
+	my @data;
+	my $len;
+	i2c_smbus_access($file, SMBUS_READ, $command, SMBUS_BLOCK_DATA, \@data)
+		or return -1;
+	# First returned byte is length, followed by up to 32 bytes of data.
+	# Convert to string.
+	$len = $data[0];
+	return substr(pack("C33", @data), 1, $len);
+}
+
+# $_[0]: Reference to an opened filehandle
 # $_[1]: Address
 # $_[2]: Functionalities of this i2c adapter
 # Returns: 1 on successful probing, 0 else.
@@ -2946,6 +2993,12 @@
 		# this probe method is safe.
 		return 0 unless ($funcs & I2C_FUNC_SMBUS_READ_BYTE_DATA);
 		return i2c_smbus_access($file, SMBUS_READ, 0, SMBUS_BYTE_DATA, []);
+	} elsif (($addr >= 0x03 && $addr <= 0x0a) 
+	 || ($addr >= 0x0d && $addr <= 0x3f)) {
+		# This covers all PMBus devices we know of which don't accept
+		# SMBUS_QUICK commands.
+		return 0 unless ($funcs & I2C_FUNC_SMBUS_READ_BYTE);
+		return i2c_smbus_access($file, SMBUS_READ, 0, SMBUS_BYTE, []);
 	} else {
 		return 0 unless ($funcs & I2C_FUNC_SMBUS_QUICK);
 		return i2c_smbus_access($file, SMBUS_WRITE, 0, SMBUS_QUICK, []);
@@ -5517,6 +5570,75 @@
 	return 5;
 }
 
+# Chips to detect: 0 = LTC2978, 1=BMR450, 2=BMR451, 3=BMR453, 4=BMR454
+#
+# Registers used:
+#   0x19: CAPABILITY
+#   0x20: VOUT_MODE
+#   0x78: STATUS_BYTE
+#   0x98: PMBUS_REVISION
+#
+# For some chips:
+#   LTC2978	0xe7	MFG_SPECIAL_ID
+#		0x99	MFR_ID
+#   BMR45x	0x99	MFR_ID (as block data)
+#   		0x9a	MFR_MODEL (as block data)
+#
+sub pmbus_detect
+{
+	my ($file, $addr, $chip) = @_;
+	my ($reg, $vout_mode, $status, $revision, $capability, $data, $funcs);
+
+	$vout_mode = i2c_smbus_read_byte_data($file, 0x20);
+	$status = i2c_smbus_read_byte_data($file, 0x78);
+	$revision = i2c_smbus_read_byte_data($file, 0x98);
+
+	return if $vout_mode < 0;
+	return if $status < 0;
+
+	$capability = i2c_smbus_read_byte_data($file, 0x19);
+
+	if ($chip = 0) {
+		return if $revision != 0x11;	# PMBus Version 1.1
+		return unless $vout_mode = 0x13;
+		return unless $capability = 0xe0;
+		$reg = i2c_smbus_read_word_data($file, 0xe7);
+		# Per datasheet, chip id should be 0x0121. 0x0122 is from observation.
+		return unless $reg = 0x0121 || $reg = 0x0122; # LTC 2978
+	} elsif ($chip > 0 && $chip < 5) {
+		return if $revision != 0x21;	# PMBus version 1.1, per spec
+		return unless $vout_mode = 0x15;
+		return unless $capability = 0xb0;
+		# MFR_ID returns "Ericsson AB" with a length of 12 bytes.
+		# Reading it as word returns 0x450c.
+		$reg = i2c_smbus_read_word_data($file, 0x99);
+		return unless $reg = 0x450c;
+		# MFR_MODEL returns "BMR45x...." with a length of 20 bytes.
+		# Reading it as word returns 0x4214.
+		$reg = i2c_smbus_read_word_data($file, 0x9a);
+		return unless $reg = 0x4214;
+
+		# Now try again with full strings if block data reads are supported.
+		# Otherwise report BMR450 with lower probability.
+		$funcs = i2c_get_funcs($file);
+		if (!($funcs & I2C_FUNC_SMBUS_READ_BLOCK_DATA)) {
+			return 5 if $chip = 1;
+			return 0;
+		}
+		$data = i2c_smbus_read_block_data($file, 0x99);
+		# Should return "Ericsson AB"
+		return if substr($data, 0, 11) ne "Ericsson AB";
+		$data = i2c_smbus_read_block_data($file, 0x9a);
+		# Should return "BMR45x"
+		return if $chip = 1 && substr($data, 0, 6) ne "BMR450";
+		return if $chip = 2 && substr($data, 0, 6) ne "BMR451";
+		return if $chip = 3 && substr($data, 0, 6) ne "BMR453";
+		return if $chip = 2 && substr($data, 0, 6) ne "BMR454";
+	}
+
+	return 9;
+}
+
 # This isn't very good detection.
 # Verify the i2c address, and the stepping ID (which is 0xb0 on
 # my chip but could be different for others...
@@ -5929,7 +6051,9 @@
 	# If we added any module option to handle aliases, we need to load all
 	# the adapter drivers so that the numbers will be the same. If not, then
 	# we only load the adapter drivers which are useful.
-	foreach $adap (@i2c_adapters) {
+	for (my $dev_nr = 0; $dev_nr < @i2c_adapters; $dev_nr++) {
+		next unless exists $i2c_adapters[$dev_nr];
+		$adap = $i2c_adapters[$dev_nr];
 		next if $adap->{autoload};
 		next if $adap->{driver} eq 'UNKNOWN';
 		next if not defined $configfile and not $adap->{used};

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH/RFC] sensors-detect: Add detection of PMBus

[Jean Delvare]

Hi Guenter,

Sorry for the late reply.

On Thu, 14 Oct 2010 08:40:42 -0700, Guenter Roeck wrote:
> This patch adds basic support for detection of PMBus devices.
> It can easily be extended to support additional PMBus devices.
> Key question right now is if this approach is acceptable.
> 
> Notable changes:
> - Chip detection code now uses byte read to detect devices in address range [0x03..0x0a, 0x0d..0x3f]

This is a huge address range. And some of these addresses may be
problematic: 0x03 to 0x07 were recently excluded from the list of
scanned addresses in i2c-core. And 0x08 is generally the slave address
on SMBus 2.0 controllers.

Also, this is a range in which a lot of already supported devices live
(in particular at 0x18-0x1a and 0x28-0x2f) so we have to be very
careful. But in fact I am more worried about the addresses we were not
probing so far, as it is very difficult to predict the result on all
the PC motherboards out there.

>   TBD: Maybe we should use SMBUS_QUICK first followed by SMBUS_BYTE if it fails
>   or if it is not supported by the adapter.

This would slow down the probing, as each non-responsive address would
be tested twice instead of once. Now of course I can imagine that some
chips may not reply to (or even hang on) the receive byte command.
After all there is a reason why we defaulted to a quick write long ago,
rather than a quick read: many devices don't have any legitimate use of
transfers starting with a read message. So it would indeed be more
conservative to always try a quick write first.

> - Added support for SMBus block read command
> - Fixed bug seen if I2C master numbering is non-sequential (which I should probably
>   commit separately if the fix is ok).
> 
> Thanks,
> Guenter
> 
> --
> Index: prog/detect/sensors-detect
> =================================> --- prog/detect/sensors-detect	(revision 5865)
> +++ prog/detect/sensors-detect	(working copy)
> @@ -1249,6 +1249,31 @@
>  		driver => "sbs", # ACPI driver, not sure if it always works
>  		i2c_addrs => [0x0b],
>  		i2c_detect => sub { smartbatt_detect(@_); },
> +	}, {
> +		name => "Linear Technology LTC2978",
> +		driver => "to-be-written",
> +		i2c_addrs => [0x5c..0x64],
> +		i2c_detect => sub { pmbus_detect(@_, 0); },
> +	}, {
> +		name => "Ericsson BMR450",
> +		driver => "to-be-written",
> +		i2c_addrs => [0x03..0x0a, 0x0d..0x3f],
> +		i2c_detect => sub { pmbus_detect(@_, 1); },
> +	}, {
> +		name => "Ericsson BMR451",
> +		driver => "to-be-written",
> +		i2c_addrs => [0x03..0x0a, 0x0d..0x3f],
> +		i2c_detect => sub { pmbus_detect(@_, 2); },
> +	}, {
> +		name => "Ericsson BMR453",
> +		driver => "to-be-written",
> +		i2c_addrs => [0x03..0x0a, 0x0d..0x3f],
> +		i2c_detect => sub { pmbus_detect(@_, 3); },
> +	}, {
> +		name => "Ericsson BMR454",
> +		driver => "to-be-written",
> +		i2c_addrs => [0x03..0x0a, 0x0d..0x3f],
> +		i2c_detect => sub { pmbus_detect(@_, 4); },
>  	}
>  );

One key question is: are these devices something which you expect to
show up in consumer PC machines? The main target for sensors-detect is
this. We don't necessarily want to add support for all
hardware-monitoring-style devices in the world to this script.

A second question is: out of the possible addresses, do you expect all
of these to be used in practice? The LM78 and W83781D devices, which we
support for a long long time, can use virtually any I2C address, but in
practice the vendors always stick to a few possibilities, and
sensors-detect only probes for these addresses.

A third question is: do these devices support SMBus ARP? This would be
an alternative to systematic device detection as we do today. Someone
is working on this already but progress is very slow (I think the guy
started over a year ago but doesn't have much time to invest in it.) If
your devices support ARP, that would be the right time to work on it
seriously.

As a summary, this patch seems dangerous and I would like you to take a
more conservative and pragmatic path if you have to go for old-style
probing. Even better if you can do without probing (using ARP).

As a side note, I am pretty surprised to see devices with such a large
range of possible I2C addresses. I presume these aren't selected by
pins but using a secondary bus access? One reason why I am asking is
that having such a large range of I2C addresses to probe would be a
problem in the kernel too, so we certainly need an alternative way to
find out the address of these devices.

>  
> @@ -2812,10 +2837,12 @@
>  use constant SMBUS_BYTE		=> 1;
>  use constant SMBUS_BYTE_DATA	=> 2;
>  use constant SMBUS_WORD_DATA	=> 3;
> +use constant SMBUS_BLOCK_DATA	=> 5;
>  
>  use constant I2C_FUNC_SMBUS_QUICK	=> 0x00010000;
>  use constant I2C_FUNC_SMBUS_READ_BYTE	=> 0x00020000;
>  use constant I2C_FUNC_SMBUS_READ_BYTE_DATA	=> 0x00080000;
> +use constant I2C_FUNC_SMBUS_READ_BLOCK_DATA	=> 0x01000000;
>  
>  # Get the i2c adapter's functionalities
>  # $_[0]: Reference to an opened filehandle
> @@ -2919,6 +2946,26 @@
>  }
>  
>  # $_[0]: Reference to an opened filehandle
> +# $_[1]: Command byte (usually register number)
> +# Returns: -1 on failure, the read block on success.
> +# Use this function with care, some devices don't like block reads,
> +# so you should do as much of the detection as possible using byte reads,
> +# and only start using block reads when there is a good chance that
> +# the detection will succeed.
> +sub i2c_smbus_read_block_data
> +{
> +	my ($file, $command) = @_;
> +	my @data;
> +	my $len;
> +	i2c_smbus_access($file, SMBUS_READ, $command, SMBUS_BLOCK_DATA, \@data)
> +		or return -1;
> +	# First returned byte is length, followed by up to 32 bytes of data.
> +	# Convert to string.
> +	$len = $data[0];
> +	return substr(pack("C33", @data), 1, $len);
> +}
> +
> +# $_[0]: Reference to an opened filehandle
>  # $_[1]: Address
>  # $_[2]: Functionalities of this i2c adapter
>  # Returns: 1 on successful probing, 0 else.
> @@ -2946,6 +2993,12 @@
>  		# this probe method is safe.
>  		return 0 unless ($funcs & I2C_FUNC_SMBUS_READ_BYTE_DATA);
>  		return i2c_smbus_access($file, SMBUS_READ, 0, SMBUS_BYTE_DATA, []);
> +	} elsif (($addr >= 0x03 && $addr <= 0x0a) 
> +	 || ($addr >= 0x0d && $addr <= 0x3f)) {
> +		# This covers all PMBus devices we know of which don't accept
> +		# SMBUS_QUICK commands.
> +		return 0 unless ($funcs & I2C_FUNC_SMBUS_READ_BYTE);
> +		return i2c_smbus_access($file, SMBUS_READ, 0, SMBUS_BYTE, []);
>  	} else {
>  		return 0 unless ($funcs & I2C_FUNC_SMBUS_QUICK);
>  		return i2c_smbus_access($file, SMBUS_WRITE, 0, SMBUS_QUICK, []);
> @@ -5517,6 +5570,75 @@
>  	return 5;
>  }
>  
> +# Chips to detect: 0 = LTC2978, 1=BMR450, 2=BMR451, 3=BMR453, 4=BMR454
> +#
> +# Registers used:
> +#   0x19: CAPABILITY
> +#   0x20: VOUT_MODE
> +#   0x78: STATUS_BYTE
> +#   0x98: PMBUS_REVISION
> +#
> +# For some chips:
> +#   LTC2978	0xe7	MFG_SPECIAL_ID
> +#		0x99	MFR_ID
> +#   BMR45x	0x99	MFR_ID (as block data)
> +#   		0x9a	MFR_MODEL (as block data)
> +#
> +sub pmbus_detect
> +{
> +	my ($file, $addr, $chip) = @_;
> +	my ($reg, $vout_mode, $status, $revision, $capability, $data, $funcs);
> +
> +	$vout_mode = i2c_smbus_read_byte_data($file, 0x20);
> +	$status = i2c_smbus_read_byte_data($file, 0x78);
> +	$revision = i2c_smbus_read_byte_data($file, 0x98);
> +
> +	return if $vout_mode < 0;
> +	return if $status < 0;
> +
> +	$capability = i2c_smbus_read_byte_data($file, 0x19);
> +
> +	if ($chip = 0) {
> +		return if $revision != 0x11;	# PMBus Version 1.1
> +		return unless $vout_mode = 0x13;
> +		return unless $capability = 0xe0;
> +		$reg = i2c_smbus_read_word_data($file, 0xe7);
> +		# Per datasheet, chip id should be 0x0121. 0x0122 is from observation.
> +		return unless $reg = 0x0121 || $reg = 0x0122; # LTC 2978
> +	} elsif ($chip > 0 && $chip < 5) {

I think this is better written $chip >= 1 && $chip <= 4.

> +		return if $revision != 0x21;	# PMBus version 1.1, per spec

Did you mean PMBus 2.1?

> +		return unless $vout_mode = 0x15;
> +		return unless $capability = 0xb0;
> +		# MFR_ID returns "Ericsson AB" with a length of 12 bytes.
> +		# Reading it as word returns 0x450c.
> +		$reg = i2c_smbus_read_word_data($file, 0x99);
> +		return unless $reg = 0x450c;
> +		# MFR_MODEL returns "BMR45x...." with a length of 20 bytes.
> +		# Reading it as word returns 0x4214.
> +		$reg = i2c_smbus_read_word_data($file, 0x9a);
> +		return unless $reg = 0x4214;
> +
> +		# Now try again with full strings if block data reads are supported.
> +		# Otherwise report BMR450 with lower probability.

Why does the BMR450 need to be handled differently here?

> +		$funcs = i2c_get_funcs($file);
> +		if (!($funcs & I2C_FUNC_SMBUS_READ_BLOCK_DATA)) {
> +			return 5 if $chip = 1;
> +			return 0;
> +		}
> +		$data = i2c_smbus_read_block_data($file, 0x99);
> +		# Should return "Ericsson AB"
> +		return if substr($data, 0, 11) ne "Ericsson AB";
> +		$data = i2c_smbus_read_block_data($file, 0x9a);
> +		# Should return "BMR45x"
> +		return if $chip = 1 && substr($data, 0, 6) ne "BMR450";
> +		return if $chip = 2 && substr($data, 0, 6) ne "BMR451";
> +		return if $chip = 3 && substr($data, 0, 6) ne "BMR453";
> +		return if $chip = 2 && substr($data, 0, 6) ne "BMR454";
> +	}
> +
> +	return 9;

9 is higher that I would expect for the LTC2978. 8 would be more
reasonable.

> +}
> +

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH/RFC] sensors-detect: Add detection of PMBus

[Guenter Roeck]

Hi Jean,

On Fri, Oct 22, 2010 at 04:54:53AM -0400, Jean Delvare wrote:
> Hi Guenter,
> 
> Sorry for the late reply.
> 
No problem.

> On Thu, 14 Oct 2010 08:40:42 -0700, Guenter Roeck wrote:
> > This patch adds basic support for detection of PMBus devices.
> > It can easily be extended to support additional PMBus devices.
> > Key question right now is if this approach is acceptable.
> > 
> > Notable changes:
> > - Chip detection code now uses byte read to detect devices in address range [0x03..0x0a, 0x0d..0x3f]
> 
> This is a huge address range. And some of these addresses may be
I know :(.

> problematic: 0x03 to 0x07 were recently excluded from the list of
> scanned addresses in i2c-core. And 0x08 is generally the slave address
> on SMBus 2.0 controllers.
> 
I'll drop 0x03..0x08.

> Also, this is a range in which a lot of already supported devices live
> (in particular at 0x18-0x1a and 0x28-0x2f) so we have to be very
> careful. But in fact I am more worried about the addresses we were not
> probing so far, as it is very difficult to predict the result on all
> the PC motherboards out there.
> 
> >   TBD: Maybe we should use SMBUS_QUICK first followed by SMBUS_BYTE if it fails
> >   or if it is not supported by the adapter.
> 
> This would slow down the probing, as each non-responsive address would
> be tested twice instead of once. Now of course I can imagine that some
> chips may not reply to (or even hang on) the receive byte command.
> After all there is a reason why we defaulted to a quick write long ago,
> rather than a quick read: many devices don't have any legitimate use of
> transfers starting with a read message. So it would indeed be more
> conservative to always try a quick write first.
> 
That was the idea.

> > - Added support for SMBus block read command
> > - Fixed bug seen if I2C master numbering is non-sequential (which I should probably
> >   commit separately if the fix is ok).
> > 
> > Thanks,
> > Guenter
> > 
> > --
> > Index: prog/detect/sensors-detect
> > =================================> > --- prog/detect/sensors-detect	(revision 5865)
> > +++ prog/detect/sensors-detect	(working copy)
> > @@ -1249,6 +1249,31 @@
> >  		driver => "sbs", # ACPI driver, not sure if it always works
> >  		i2c_addrs => [0x0b],
> >  		i2c_detect => sub { smartbatt_detect(@_); },
> > +	}, {
> > +		name => "Linear Technology LTC2978",
> > +		driver => "to-be-written",
> > +		i2c_addrs => [0x5c..0x64],
> > +		i2c_detect => sub { pmbus_detect(@_, 0); },
> > +	}, {
> > +		name => "Ericsson BMR450",
> > +		driver => "to-be-written",
> > +		i2c_addrs => [0x03..0x0a, 0x0d..0x3f],
> > +		i2c_detect => sub { pmbus_detect(@_, 1); },
> > +	}, {
> > +		name => "Ericsson BMR451",
> > +		driver => "to-be-written",
> > +		i2c_addrs => [0x03..0x0a, 0x0d..0x3f],
> > +		i2c_detect => sub { pmbus_detect(@_, 2); },
> > +	}, {
> > +		name => "Ericsson BMR453",
> > +		driver => "to-be-written",
> > +		i2c_addrs => [0x03..0x0a, 0x0d..0x3f],
> > +		i2c_detect => sub { pmbus_detect(@_, 3); },
> > +	}, {
> > +		name => "Ericsson BMR454",
> > +		driver => "to-be-written",
> > +		i2c_addrs => [0x03..0x0a, 0x0d..0x3f],
> > +		i2c_detect => sub { pmbus_detect(@_, 4); },
> >  	}
> >  );
> 
> One key question is: are these devices something which you expect to
> show up in consumer PC machines? The main target for sensors-detect is
> this. We don't necessarily want to add support for all
> hardware-monitoring-style devices in the world to this script.
> 
Those are not only HW monitoring chips, but also power supply controllers.
So it might well be that such chips will show up on PCs. Difficult to say,
though. Might be too expensive for PCs.

> A second question is: out of the possible addresses, do you expect all
> of these to be used in practice? The LM78 and W83781D devices, which we
> support for a long long time, can use virtually any I2C address, but in
> practice the vendors always stick to a few possibilities, and
> sensors-detect only probes for these addresses.
> 
I don't have enough data to determine if there is a "preferred" address range.

Some of the devices only support a limited address range, but several
use a set of two resistors. I hear talk about a board with 20+ of the
BMR45x chips, all to be accessible from PMBus.

> A third question is: do these devices support SMBus ARP? This would be
> an alternative to systematic device detection as we do today. Someone
> is working on this already but progress is very slow (I think the guy
> started over a year ago but doesn't have much time to invest in it.) If
> your devices support ARP, that would be the right time to work on it
> seriously.
> 
I don't think so; I don't see it mentioned anywhere in the PMBus specification.

> As a summary, this patch seems dangerous and I would like you to take a
> more conservative and pragmatic path if you have to go for old-style
> probing. Even better if you can do without probing (using ARP).
> 
Maybe not bother probing for PMBus devices at all would be the best approach,
at least until any show up on PCs. Or only probe for devices supporting
the QUICK command.

> As a side note, I am pretty surprised to see devices with such a large
> range of possible I2C addresses. I presume these aren't selected by
> pins but using a secondary bus access? One reason why I am asking is
> that having such a large range of I2C addresses to probe would be a
> problem in the kernel too, so we certainly need an alternative way to
> find out the address of these devices.
> 
As mentioned above, the chips use two resistors to set the I2C address.
Each resistor controls three address bits, which creates a 6-bit dynamic range
for the address.

> > +     } elsif ($chip > 0 && $chip < 5) {
> 
> I think this is better written $chip >= 1 && $chip <= 4.

Agreed; I had changed that already.

> > +		return if $revision != 0x21;	# PMBus version 1.1, per spec
> 
> Did you mean PMBus 2.1?
> 
No.

There is an inconsistency in the PMBus specification. The idea
was to have two sets of 4 bit in the revision register to report
which portion of the device is compliant to which PMBus version.
Unfortunately, there is a typo in the spec, indicating the upper four bit
of the version would be in bit 5..7. So some manufacturers use bit 4..7
to report the respective part of the version, others use bit 5..7.
As a result, some chips report 0x11 to indicate they are PMBus 1.1 compliant,
others report 0x21.

There was an attempt to clean that up in PMBus 1.2, but that got dropped, 
and it is still in there.

> > +		return unless $vout_mode = 0x15;
> > +		return unless $capability = 0xb0;
> > +		# MFR_ID returns "Ericsson AB" with a length of 12 bytes.
> > +		# Reading it as word returns 0x450c.
> > +		$reg = i2c_smbus_read_word_data($file, 0x99);
> > +		return unless $reg = 0x450c;
> > +		# MFR_MODEL returns "BMR45x...." with a length of 20 bytes.
> > +		# Reading it as word returns 0x4214.
> > +		$reg = i2c_smbus_read_word_data($file, 0x9a);
> > +		return unless $reg = 0x4214;
> > +
> > +		# Now try again with full strings if block data reads are supported.
> > +		# Otherwise report BMR450 with lower probability.
> 
> Why does the BMR450 need to be handled differently here?
> 
The idea was that we know that it is a BMR45x, but we don't know which one.
Returning BMR450 with lower probability seemed to make some sense.
Not really sure what the best approach would be. Another option might be 
to just check for (and report) BMR45x in the first place.

> > +		$funcs = i2c_get_funcs($file);
> > +		if (!($funcs & I2C_FUNC_SMBUS_READ_BLOCK_DATA)) {
> > +			return 5 if $chip = 1;
> > +			return 0;
> > +		}
> > +		$data = i2c_smbus_read_block_data($file, 0x99);
> > +		# Should return "Ericsson AB"
> > +		return if substr($data, 0, 11) ne "Ericsson AB";
> > +		$data = i2c_smbus_read_block_data($file, 0x9a);
> > +		# Should return "BMR45x"
> > +		return if $chip = 1 && substr($data, 0, 6) ne "BMR450";
> > +		return if $chip = 2 && substr($data, 0, 6) ne "BMR451";
> > +		return if $chip = 3 && substr($data, 0, 6) ne "BMR453";
> > +		return if $chip = 2 && substr($data, 0, 6) ne "BMR454";
> > +	}
> > +
> > +	return 9;
> 
> 9 is higher that I would expect for the LTC2978. 8 would be more
> reasonable.
> 
Ok.

Overall it seems to be better to either drop this for now, or not try
to detect devices which don't support the QUICK command. What do you think ?

Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH/RFC] sensors-detect: Add detection of PMBus

[Jean Delvare]

On Fri, 22 Oct 2010 04:14:14 -0700, Guenter Roeck wrote:
> On Fri, Oct 22, 2010 at 04:54:53AM -0400, Jean Delvare wrote:
> > A third question is: do these devices support SMBus ARP? This would be
> > an alternative to systematic device detection as we do today. Someone
> > is working on this already but progress is very slow (I think the guy
> > started over a year ago but doesn't have much time to invest in it.) If
> > your devices support ARP, that would be the right time to work on it
> > seriously.
> > 
> I don't think so; I don't see it mentioned anywhere in the PMBus specification.

You can just check (with i2cdetect) if anything responds to address
0x61. This is the SMBus ARP address.

> > (...)
> > As a side note, I am pretty surprised to see devices with such a large
> > range of possible I2C addresses. I presume these aren't selected by
> > pins but using a secondary bus access? One reason why I am asking is
> > that having such a large range of I2C addresses to probe would be a
> > problem in the kernel too, so we certainly need an alternative way to
> > find out the address of these devices.
>
> As mentioned above, the chips use two resistors to set the I2C address.
> Each resistor controls three address bits, which creates a 6-bit dynamic range
> for the address.

I see. This really doesn't seem to be compatible with device probing at
all.

> > > (...)
> > > +		return if $revision != 0x21;	# PMBus version 1.1, per spec
> > 
> > Did you mean PMBus 2.1?
> > 
> No.
> 
> There is an inconsistency in the PMBus specification. The idea
> was to have two sets of 4 bit in the revision register to report
> which portion of the device is compliant to which PMBus version.
> Unfortunately, there is a typo in the spec, indicating the upper four bit
> of the version would be in bit 5..7. So some manufacturers use bit 4..7
> to report the respective part of the version, others use bit 5..7.
> As a result, some chips report 0x11 to indicate they are PMBus 1.1 compliant,
> others report 0x21.
> 
> There was an attempt to clean that up in PMBus 1.2, but that got dropped, 
> and it is still in there.

Thanks for the clarification.

> > > +		return unless $vout_mode = 0x15;
> > > +		return unless $capability = 0xb0;
> > > +		# MFR_ID returns "Ericsson AB" with a length of 12 bytes.
> > > +		# Reading it as word returns 0x450c.
> > > +		$reg = i2c_smbus_read_word_data($file, 0x99);
> > > +		return unless $reg = 0x450c;
> > > +		# MFR_MODEL returns "BMR45x...." with a length of 20 bytes.
> > > +		# Reading it as word returns 0x4214.
> > > +		$reg = i2c_smbus_read_word_data($file, 0x9a);
> > > +		return unless $reg = 0x4214;
> > > +
> > > +		# Now try again with full strings if block data reads are supported.
> > > +		# Otherwise report BMR450 with lower probability.
> > 
> > Why does the BMR450 need to be handled differently here?
> > 
> The idea was that we know that it is a BMR45x, but we don't know which one.
> Returning BMR450 with lower probability seemed to make some sense.

Ah, OK, I get it now. Sorry for being slow.

> Not really sure what the best approach would be. Another option might be 
> to just check for (and report) BMR45x in the first place.

If they are all supported by the same driver, yes, that would make
sense.

I admit that the implementation we have in sensors-detect isn't very
flexible for that kind of case. It would be better if functions could
return more than a confidence value, but were also able to refine the
device name. It didn't bother me enough so far that I would actually
try to improve it.

> (...)
> Overall it seems to be better to either drop this for now, or not try
> to detect devices which don't support the QUICK command. What do you think ?

Indeed these devices don't seem to fit in our detection scheme, and
overall seems detection-unfriendly by design. So I wouldn't bother
adding detection support for them for the time being. Better add kernel
support first, and see the actual use cases for these devices. Then we
can discuss the user-spac detection again if needed.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH/RFC] sensors-detect: Add detection of PMBus

[Guenter Roeck]

Hi Jean,

On Fri, Oct 22, 2010 at 09:32:31AM -0400, Jean Delvare wrote:

[ ... ]

> 
> Indeed these devices don't seem to fit in our detection scheme, and
> overall seems detection-unfriendly by design. So I wouldn't bother
> adding detection support for them for the time being. Better add kernel
> support first, and see the actual use cases for these devices. Then we
> can discuss the user-spac detection again if needed.
> 
Ok, makes sense. And, besides, someone complaining about non-support
by sensors-detect is a good indicator of chip use ... 

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors