Skip to content

Alertl for sbtsi #141

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 10, 2025
Merged

Alertl for sbtsi #141

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
34e8ebf
misc: amd-apml: Probe sbtsi device as per instance ID
akky16 Jun 2, 2025
5ce60db
misc: amd-apml: Add device IDs for Fam:0x1A, Fam:0x50
akky16 Jun 2, 2025
9472f32
misc: amd-apml: Update label for hwmon entry to include socket_iod ID
akky16 Jun 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 38 additions & 11 deletions drivers/misc/amd-apml/apml_sbtsi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,23 @@ static const struct file_operations sbtsi_fops = {
.compat_ioctl = sbtsi_ioctl,
};

/* Convert the static address to socket_die index */
static const char *sbtsi_addr_to_label(u8 addr)
{
switch (addr) {
case 0x44:
return "0.1";
case 0x45:
return "1.1";
case 0x48:
return "1.0";
case 0x4c:
return "0.0";
default:
return NULL;
}
}

static int create_misc_tsi_device(struct apml_sbtsi_device *tsi_dev,
struct device *dev)
{
Expand Down Expand Up @@ -299,12 +316,11 @@ static int sbtsi_i3c_probe(struct i3c_device *i3cdev)
.val_bits = 8,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit message - srtart with driver:misc:

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, will update

};
struct regmap *regmap;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit message: please start with drivers:misc

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, will update

const char *hwmon_dev_name;

dev_err(dev, "SBTSI: PID: %llx\n", i3cdev->desc->info.pid);
if (!((i3cdev->desc->info.pid == 0x0) || (i3cdev->desc->info.pid == 0x22400000001) ||
(i3cdev->desc->info.pid == 0x118) || (i3cdev->desc->info.pid == 0x010118) ||
(i3cdev->desc->info.pid == 0x01000118) || (i3cdev->desc->info.pid == 0x01010118)))
{
if (!(I3C_PID_INSTANCE_ID(i3cdev->desc->info.pid) == 0 ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to be good for both sp5 and sp7 based platforms . If not Need to look option to Valid PID bases check instead of hard coding.
static const u64 valid_sbtsi_pids[] = {
0x22400000001ULL,
// Add more if needed
};

Copy link
Collaborator

@akky16 akky16 Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ojayanth
PID: 0x22400000001 is constant till Turin irrespective of socket number or die number.
From Venice the PID is changed to include, socket and die number info.
As there is only one PID till Turin, my understanding is, currently we can have the changes as is.

i3cdev->desc->info.pid == 0x22400000001)) {
dev_err(dev, "SBTSI: Error PID: %llx\n", i3cdev->desc->info.pid);
return -ENXIO;
}
Expand All @@ -326,8 +342,14 @@ static int sbtsi_i3c_probe(struct i3c_device *i3cdev)
tsi_dev->regmap = regmap;
mutex_init(&tsi_dev->lock);

/* Need to verify for the static address for i3cdev */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Helper Function + Lookup Table approch to avoid duplication here and line 389

static const char *sbtsi_addr_to_label(u8 addr)
{
switch (addr) {
case 0x44: return "0.1";
case 0x45: return "1.1";
case 0x48: return "1.0";
case 0x4c: return "0.0";
default: return NULL;
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, will update.

tsi_dev->dev_static_addr = i3cdev->desc->info.static_addr;

hwmon_dev_name = devm_kasprintf(dev, GFP_KERNEL, "sbtsi_%s",
sbtsi_addr_to_label(tsi_dev->dev_static_addr));

dev_set_drvdata(dev, (void *)tsi_dev);
hwmon_dev = devm_hwmon_device_register_with_info(dev, "sbtsi_i3c", tsi_dev,
hwmon_dev = devm_hwmon_device_register_with_info(dev, hwmon_dev_name, tsi_dev,
&sbtsi_chip_info, NULL);

if (!hwmon_dev)
Expand All @@ -336,9 +358,6 @@ static int sbtsi_i3c_probe(struct i3c_device *i3cdev)
return PTR_ERR_OR_ZERO(hwmon_dev);
}

/* Need to verify for the static address for i3cdev */
tsi_dev->dev_static_addr = i3cdev->desc->info.static_addr;

return create_misc_tsi_device(tsi_dev, dev);
}

Expand All @@ -356,6 +375,7 @@ static int sbtsi_i2c_probe(struct i2c_client *client)
.reg_bits = 8,
.val_bits = 8,
};
const char *hwmon_dev_name;

tsi_dev = devm_kzalloc(dev, sizeof(struct apml_sbtsi_device), GFP_KERNEL);
if (!tsi_dev)
Expand All @@ -368,16 +388,19 @@ static int sbtsi_i2c_probe(struct i2c_client *client)

dev_set_drvdata(dev, (void *)tsi_dev);

hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
tsi_dev->dev_static_addr = client->addr;

hwmon_dev_name = devm_kasprintf(dev, GFP_KERNEL, "sbtsi_%s",
sbtsi_addr_to_label(tsi_dev->dev_static_addr));

hwmon_dev = devm_hwmon_device_register_with_info(dev, hwmon_dev_name,
tsi_dev,
&sbtsi_chip_info,
NULL);

if (!hwmon_dev)
return PTR_ERR_OR_ZERO(hwmon_dev);

tsi_dev->dev_static_addr = client->addr;

return create_misc_tsi_device(tsi_dev, dev);
}

Expand Down Expand Up @@ -423,6 +446,10 @@ static const struct i3c_device_id sbtsi_i3c_id[] = {
I3C_DEVICE_EXTRA_INFO(0, 0x0101, 0x118, NULL), /* P1 - IOD1 - SBTSI */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow commit message align to other commits, in the drivers.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, will update

I3C_DEVICE_EXTRA_INFO(0x112, 0, 0x1, NULL),
I3C_DEVICE_EXTRA_INFO(0, 0x0, 0x0, NULL),
I3C_DEVICE_EXTRA_INFO(0x112, 0x0, 0x118, NULL), /* Socket:0, IOD:0 */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the diffrence between PX vs Socket?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what is P here? is it processor?

we are following terminology as used in PPR

From PPR:
Table 186: SBI Address Encoding
Socket ID / Socket number SB-RMI Address Comment
000b 3Ch IOD0 Socket 0 SB-RMI address
001b 38h IOD0 Socket 1 SB-RMI address
100b 34h IOD1 Socket 0 SB-RMI address
101b 35h IOD1 Socket 1 SB-RMI address

I3C_DEVICE_EXTRA_INFO(0x112, 0x1, 0x118, NULL), /* Socket:0, IOD:1 */
I3C_DEVICE_EXTRA_INFO(0x112, 0x100, 0x118, NULL), /* Socket:1 IOD:0 */
I3C_DEVICE_EXTRA_INFO(0x112, 0x101, 0x118, NULL), /* Socket:1 IOD:1 */
{}
};
MODULE_DEVICE_TABLE(i3c, sbtsi_i3c_id);
Expand Down