[PATCH 0/4] Add RAS EEPROM table support for Arcturus.

[PATCH 0/4] Add RAS EEPROM table support for Arcturus.

[Andrey Grodzovsky]

This patch set adds support for Arcturus EEPROM to store RAS 
errors which rise during run time so on next driver load those 
errors can be retrieved and action taken on them 
(e.g. Reserve bad memory pages to disallow their usage by the GPU).

The I2C communication is done through SMU table  which is what in patch 2
while patch 4 relocates RAS recovery init to much later place then before
since SMU must be fully operational for this to work on Arcturus.


Andrey Grodzovsky (4):
  drm/amd/powerplay: Add interface for I2C transactions to SMU.
  drm/amd/powerplay: Add EEPROM I2C read/write support to Arcturus.
  drm/amdgpu: Use ARCTURUS in RAS EEPROM.
  drm/amdgpu: Move amdgpu_ras_recovery_init to after SMU ready.

 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c     |  13 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c |   9 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c        |  11 --
 drivers/gpu/drm/amd/powerplay/arcturus_ppt.c   | 229 +++++++++++++++++++++++++
 drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h |   9 +
 5 files changed, 259 insertions(+), 12 deletions(-)

-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/4] drm/amd/powerplay: Add interface for I2C transactions to SMU.

[Andrey Grodzovsky]

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
index bf13bf3..24244eb 100644
--- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
@@ -394,6 +394,8 @@ struct smu_context
 
 };
 
+struct i2c_adapter;
+
 struct pptable_funcs {
 	int (*alloc_dpm_context)(struct smu_context *smu);
 	int (*store_powerplay_table)(struct smu_context *smu);
@@ -470,6 +472,8 @@ struct pptable_funcs {
 				   uint32_t dpm_level, uint32_t *freq);
 	int (*set_df_cstate)(struct smu_context *smu, enum pp_df_cstate state);
 	int (*update_pcie_parameters)(struct smu_context *smu, uint32_t pcie_gen_cap, uint32_t pcie_width_cap);
+	int (*i2c_eeprom_init)(struct i2c_adapter *control);
+	void (*i2c_eeprom_fini)(struct i2c_adapter *control);
 	int (*get_dpm_clock_table)(struct smu_context *smu, struct dpm_clocks *clock_table);
 };
 
@@ -782,6 +786,11 @@ struct smu_funcs
 #define smu_override_pcie_parameters(smu) \
 		((smu)->funcs->override_pcie_parameters ? (smu)->funcs->override_pcie_parameters((smu)) : 0)
 
+#define smu_i2c_eeprom_init(smu, control) \
+		((smu)->ppt_funcs->i2c_eeprom_init ? (smu)->ppt_funcs->i2c_eeprom_init((control)) : -EINVAL)
+#define smu_i2c_eeprom_fini(smu, control) \
+		((smu)->ppt_funcs->i2c_eeprom_fini ? (smu)->ppt_funcs->i2c_eeprom_fini((control)) : -EINVAL)
+
 #define smu_update_pcie_parameters(smu, pcie_gen_cap, pcie_width_cap) \
 		((smu)->ppt_funcs->update_pcie_parameters ? (smu)->ppt_funcs->update_pcie_parameters((smu), (pcie_gen_cap), (pcie_width_cap)) : 0)
 
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/4] drm/amd/powerplay: Add EEPROM I2C read/write support to Arcturus.

[Andrey Grodzovsky]

The communication is done through SMU table and hence the code
is in powerplay.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/amd/powerplay/arcturus_ppt.c | 229 +++++++++++++++++++++++++++
 1 file changed, 229 insertions(+)

diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
index 90d871a..53d08de5 100644
--- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
+++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
@@ -36,6 +36,11 @@
 #include "smu_v11_0_pptable.h"
 #include "arcturus_ppsmc.h"
 #include "nbio/nbio_7_4_sh_mask.h"
+#include <linux/i2c.h>
+#include <linux/pci.h>
+#include "amdgpu_ras.h"
+
+#define to_amdgpu_device(x) (container_of(x, struct amdgpu_ras, eeprom_control.eeprom_accessor))->adev
 
 #define CTF_OFFSET_EDGE			5
 #define CTF_OFFSET_HOTSPOT		5
@@ -171,6 +176,7 @@ static struct smu_11_0_cmn2aisc_mapping arcturus_table_map[SMU_TABLE_COUNT] = {
 	TAB_MAP(SMU_METRICS),
 	TAB_MAP(DRIVER_SMU_CONFIG),
 	TAB_MAP(OVERDRIVE),
+	TAB_MAP(I2C_COMMANDS),
 };
 
 static struct smu_11_0_cmn2aisc_mapping arcturus_pwr_src_map[SMU_POWER_SOURCE_COUNT] = {
@@ -293,6 +299,9 @@ static int arcturus_tables_init(struct smu_context *smu, struct smu_table *table
 	SMU_TABLE_INIT(tables, SMU_TABLE_SMU_METRICS, sizeof(SmuMetrics_t),
 		       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
 
+	SMU_TABLE_INIT(tables, SMU_TABLE_I2C_COMMANDS, sizeof(SwI2cRequest_t),
+			       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
+
 	smu_table->metrics_table = kzalloc(sizeof(SmuMetrics_t), GFP_KERNEL);
 	if (!smu_table->metrics_table)
 		return -ENOMEM;
@@ -1927,6 +1936,224 @@ static int arcturus_dpm_set_uvd_enable(struct smu_context *smu, bool enable)
 	return ret;
 }
 
+
+static void arcturus_fill_eeprom_i2c_req(SwI2cRequest_t  *req, bool write,
+				  uint8_t address, uint32_t numbytes,
+				  uint8_t *data)
+{
+	int i;
+
+	BUG_ON(numbytes > MAX_SW_I2C_COMMANDS);
+
+	req->I2CcontrollerPort = 0;
+	req->I2CSpeed = 2;
+	req->SlaveAddress = address;
+	req->NumCmds = numbytes;
+
+	for (i = 0; i < numbytes; i++) {
+		SwI2cCmd_t *cmd =  &req->SwI2cCmds[i];
+
+		/* First 2 bytes are always write for lower 2b EEPROM address */
+		if (i < 2)
+			cmd->Cmd = 1;
+		else
+			cmd->Cmd = write;
+
+
+		/* Add RESTART for read  after address filled */
+		cmd->CmdConfig |= (i == 2 && !write) ? CMDCONFIG_RESTART_MASK : 0;
+
+		/* Add STOP in the end */
+		cmd->CmdConfig |= (i == (numbytes - 1)) ? CMDCONFIG_STOP_MASK : 0;
+
+		/* Fill with data regardless if read or write to simplify code */
+		cmd->RegisterAddr = data[i];
+	}
+}
+
+static int arcturus_i2c_eeprom_read_data(struct i2c_adapter *control,
+					       uint8_t address,
+					       uint8_t *data,
+					       uint32_t numbytes)
+{
+	uint32_t  i, ret = 0;
+	SwI2cRequest_t req;
+	struct amdgpu_device *adev = to_amdgpu_device(control);
+	struct smu_table_context *smu_table = &adev->smu.smu_table;
+	struct smu_table *table = &smu_table->tables[SMU_TABLE_I2C_COMMANDS];
+
+	memset(&req, 0, sizeof(req));
+	arcturus_fill_eeprom_i2c_req(&req, false, address, numbytes, data);
+
+	mutex_lock(&adev->smu.mutex);
+	/* Now read data starting with that address */
+	ret = smu_update_table(&adev->smu, SMU_TABLE_I2C_COMMANDS, 0, &req,
+					true);
+	mutex_unlock(&adev->smu.mutex);
+
+	if (!ret) {
+		SwI2cRequest_t *res = (SwI2cRequest_t *)table->cpu_addr;
+
+		/* Assume SMU  fills res.SwI2cCmds[i].Data with read bytes */
+		for (i = 0; i < numbytes; i++)
+			data[i] = res->SwI2cCmds[i].Data;
+
+		pr_debug("arcturus_i2c_eeprom_read_data, address = %x, bytes = %d, data :",
+				  (uint16_t)address, numbytes);
+
+		print_hex_dump(KERN_DEBUG, "data: ", DUMP_PREFIX_NONE,
+			       8, 1, data, numbytes, false);
+	} else
+		pr_err("arcturus_i2c_eeprom_read_data - error occurred :%x", ret);
+
+	return ret;
+}
+
+static int arcturus_i2c_eeprom_write_data(struct i2c_adapter *control,
+						uint8_t address,
+						uint8_t *data,
+						uint32_t numbytes)
+{
+	uint32_t ret;
+	SwI2cRequest_t req;
+	struct amdgpu_device *adev = to_amdgpu_device(control);
+
+	memset(&req, 0, sizeof(req));
+	arcturus_fill_eeprom_i2c_req(&req, true, address, numbytes, data);
+
+	mutex_lock(&adev->smu.mutex);
+	ret = smu_update_table(&adev->smu, SMU_TABLE_I2C_COMMANDS, 0, &req, true);
+	mutex_unlock(&adev->smu.mutex);
+
+	if (!ret) {
+		pr_debug("arcturus_i2c_write(), address = %x, bytes = %d , data: ",
+					 (uint16_t)address, numbytes);
+
+		print_hex_dump(KERN_DEBUG, "data: ", DUMP_PREFIX_NONE,
+			       8, 1, data, numbytes, false);
+		/*
+		 * According to EEPROM spec there is a MAX of 10 ms required for
+		 * EEPROM to flush internal RX buffer after STOP was issued at the
+		 * end of write transaction. During this time the EEPROM will not be
+		 * responsive to any more commands - so wait a bit more.
+		 */
+		msleep(10);
+
+	} else
+		pr_err("arcturus_i2c_write- error occurred :%x", ret);
+
+	return ret;
+}
+
+static int arcturus_i2c_eeprom_i2c_xfer(struct i2c_adapter *i2c_adap,
+			      struct i2c_msg *msgs, int num)
+{
+	uint32_t  i, j, ret, data_size, data_chunk_size, next_eeprom_addr = 0;
+	uint8_t *data_ptr, data_chunk[MAX_SW_I2C_COMMANDS] = { 0 };
+
+	for (i = 0; i < num; i++) {
+		/*
+		 * SMU interface allows at most MAX_SW_I2C_COMMANDS bytes of data at
+		 * once and hence the data needs to be spliced into chunks and sent each
+		 * chunk separately
+		 */
+		data_size = msgs[i].len - 2;
+		data_chunk_size = MAX_SW_I2C_COMMANDS - 2;
+		next_eeprom_addr = (msgs[i].buf[0] << 8 & 0xff00) | (msgs[i].buf[1] & 0xff);
+		data_ptr = msgs[i].buf + 2;
+
+		for (j = 0; j < data_size / data_chunk_size; j++) {
+			/* Insert the EEPROM dest addess, bits 0-15 */
+			data_chunk[0] = ((next_eeprom_addr >> 8) & 0xff);
+			data_chunk[1] = (next_eeprom_addr & 0xff);
+
+			if (msgs[i].flags & I2C_M_RD) {
+				ret = arcturus_i2c_eeprom_read_data(i2c_adap,
+								(uint8_t)msgs[i].addr,
+								data_chunk, MAX_SW_I2C_COMMANDS);
+
+				memcpy(data_ptr, data_chunk + 2, data_chunk_size);
+			} else {
+
+				memcpy(data_chunk + 2, data_ptr, data_chunk_size);
+
+				ret = arcturus_i2c_eeprom_write_data(i2c_adap,
+								 (uint8_t)msgs[i].addr,
+								 data_chunk, MAX_SW_I2C_COMMANDS);
+			}
+
+			if (ret) {
+				num = -EIO;
+				goto fail;
+			}
+
+			next_eeprom_addr += data_chunk_size;
+			data_ptr += data_chunk_size;
+		}
+
+		if (data_size % data_chunk_size) {
+			data_chunk[0] = ((next_eeprom_addr >> 8) & 0xff);
+			data_chunk[1] = (next_eeprom_addr & 0xff);
+
+			if (msgs[i].flags & I2C_M_RD) {
+				ret = arcturus_i2c_eeprom_read_data(i2c_adap,
+								(uint8_t)msgs[i].addr,
+								data_chunk, (data_size % data_chunk_size) + 2);
+
+				memcpy(data_ptr, data_chunk + 2, data_size % data_chunk_size);
+			} else {
+				memcpy(data_chunk + 2, data_ptr, data_size % data_chunk_size);
+
+				ret = arcturus_i2c_eeprom_write_data(i2c_adap,
+								 (uint8_t)msgs[i].addr,
+								 data_chunk, (data_size % data_chunk_size) + 2);
+			}
+
+			if (ret) {
+				num = -EIO;
+				goto fail;
+			}
+		}
+	}
+
+fail:
+	return num;
+}
+
+static u32 arcturus_i2c_eeprom_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+
+static const struct i2c_algorithm arcturus_i2c_eeprom_i2c_algo = {
+	.master_xfer = arcturus_i2c_eeprom_i2c_xfer,
+	.functionality = arcturus_i2c_eeprom_i2c_func,
+};
+
+int arcturus_i2c_eeprom_control_init(struct i2c_adapter *control)
+{
+	struct amdgpu_device *adev = to_amdgpu_device(control);
+	int res;
+
+	control->owner = THIS_MODULE;
+	control->class = I2C_CLASS_SPD;
+	control->dev.parent = &adev->pdev->dev;
+	control->algo = &arcturus_i2c_eeprom_i2c_algo;
+	snprintf(control->name, sizeof(control->name), "RAS EEPROM");
+
+	res = i2c_add_adapter(control);
+	if (res)
+		DRM_ERROR("Failed to register hw i2c, err: %d\n", res);
+
+	return res;
+}
+
+void arcturus_i2c_eeprom_control_fini(struct i2c_adapter *control)
+{
+	i2c_del_adapter(control);
+}
+
 static const struct pptable_funcs arcturus_ppt_funcs = {
 	/* translate smu index into arcturus specific index */
 	.get_smu_msg_index = arcturus_get_smu_msg_index,
@@ -1966,6 +2193,8 @@ static const struct pptable_funcs arcturus_ppt_funcs = {
 	.get_power_limit = arcturus_get_power_limit,
 	.is_dpm_running = arcturus_is_dpm_running,
 	.dpm_set_uvd_enable = arcturus_dpm_set_uvd_enable,
+	.i2c_eeprom_init = arcturus_i2c_eeprom_control_init,
+	.i2c_eeprom_fini = arcturus_i2c_eeprom_control_fini,
 };
 
 void arcturus_set_ppt_funcs(struct smu_context *smu)
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/4] drm/amdgpu: Use ARCTURUS in RAS EEPROM.

[Andrey Grodzovsky]

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
index 20af0a1..7de16c0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
@@ -216,6 +216,10 @@ int amdgpu_ras_eeprom_init(struct amdgpu_ras_eeprom_control *control)
 		ret = smu_v11_0_i2c_eeprom_control_init(&control->eeprom_accessor);
 		break;
 
+	case CHIP_ARCTURUS:
+		ret = smu_i2c_eeprom_init(&adev->smu, &control->eeprom_accessor);
+		break;
+
 	default:
 		return 0;
 	}
@@ -260,6 +264,9 @@ void amdgpu_ras_eeprom_fini(struct amdgpu_ras_eeprom_control *control)
 	case CHIP_VEGA20:
 		smu_v11_0_i2c_eeprom_control_fini(&control->eeprom_accessor);
 		break;
+	case CHIP_ARCTURUS:
+		smu_i2c_eeprom_fini(&adev->smu, &control->eeprom_accessor);
+		break;
 
 	default:
 		return;
@@ -364,7 +371,7 @@ int amdgpu_ras_eeprom_process_recods(struct amdgpu_ras_eeprom_control *control,
 	struct eeprom_table_record *record;
 	struct amdgpu_device *adev = to_amdgpu_device(control);
 
-	if (adev->asic_type != CHIP_VEGA20)
+	if (adev->asic_type != CHIP_VEGA20 && adev->asic_type != CHIP_ARCTURUS)
 		return 0;
 
 	buffs = kcalloc(num, EEPROM_ADDRESS_SIZE + EEPROM_TABLE_RECORD_SIZE,
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 4/4] drm/amdgpu: Move amdgpu_ras_recovery_init to after SMU ready.

[Andrey Grodzovsky]

For Arcturus the I2C traffic is done through SMU tables and so
we must postpone RAS recovery init to after they are ready
which is in amdgpu_device_ip_hw_init_phase2.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 13 +++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c    | 11 -----------
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 17cfdaf..c40e9a5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1850,6 +1850,19 @@ static int amdgpu_device_ip_init(struct amdgpu_device *adev)
 	if (r)
 		goto init_failed;
 
+	/*
+	 * retired pages will be loaded from eeprom and reserved here,
+	 * it should be called after amdgpu_device_ip_hw_init_phase2  since
+	 * for some ASICs the RAS EEPROM code relies on SMU fully functioning
+	 * for I2C communication which only true at this point.
+	 * recovery_init may fail, but it can free all resources allocated by
+	 * itself and its failure should not stop amdgpu init process.
+	 *
+	 * Note: theoretically, this should be called before all vram allocations
+	 * to protect retired page from abusing
+	 */
+	amdgpu_ras_recovery_init(adev);
+
 	if (adev->gmc.xgmi.num_physical_nodes > 1)
 		amdgpu_xgmi_add_device(adev);
 	amdgpu_amdkfd_device_init(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 2e85a51..1045c3f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1721,17 +1721,6 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
 #endif
 
 	/*
-	 * retired pages will be loaded from eeprom and reserved here,
-	 * it should be called after ttm init since new bo may be created,
-	 * recovery_init may fail, but it can free all resources allocated by
-	 * itself and its failure should not stop amdgpu init process.
-	 *
-	 * Note: theoretically, this should be called before all vram allocations
-	 * to protect retired page from abusing
-	 */
-	amdgpu_ras_recovery_init(adev);
-
-	/*
 	 *The reserved vram for firmware must be pinned to the specified
 	 *place on the VRAM, so reserve it early.
 	 */
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 0/4] Add RAS EEPROM table support for Arcturus.

[Quan, Evan]

Patch 1 - 2 are reviewed-by: Evan Quan <evan.quan@amd.com>
Patch 3 - 4 are acked-by: Evan Quan <evan.quan@amd.com>

-----Original Message-----
From: Andrey Grodzovsky <andrey.grodzovsky@amd.com> 
Sent: Saturday, October 19, 2019 4:48 AM
To: amd-gfx@lists.freedesktop.org
Cc: Chen, Guchun <Guchun.Chen@amd.com>; Zhou1, Tao <Tao.Zhou1@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; noreply-confluence@amd.com; Quan, Evan <Evan.Quan@amd.com>; Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
Subject: [PATCH 0/4] Add RAS EEPROM table support for Arcturus.

This patch set adds support for Arcturus EEPROM to store RAS 
errors which rise during run time so on next driver load those 
errors can be retrieved and action taken on them 
(e.g. Reserve bad memory pages to disallow their usage by the GPU).

The I2C communication is done through SMU table  which is what in patch 2
while patch 4 relocates RAS recovery init to much later place then before
since SMU must be fully operational for this to work on Arcturus.


Andrey Grodzovsky (4):
  drm/amd/powerplay: Add interface for I2C transactions to SMU.
  drm/amd/powerplay: Add EEPROM I2C read/write support to Arcturus.
  drm/amdgpu: Use ARCTURUS in RAS EEPROM.
  drm/amdgpu: Move amdgpu_ras_recovery_init to after SMU ready.

 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c     |  13 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c |   9 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c        |  11 --
 drivers/gpu/drm/amd/powerplay/arcturus_ppt.c   | 229 +++++++++++++++++++++++++
 drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h |   9 +
 5 files changed, 259 insertions(+), 12 deletions(-)

-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 2/4] drm/amd/powerplay: Add EEPROM I2C read/write support to Arcturus.

[Chen, Guchun]

Comment inline.


Regards,
Guchun

-----Original Message-----
From: Andrey Grodzovsky <andrey.grodzovsky@amd.com> 
Sent: Saturday, October 19, 2019 4:48 AM
To: amd-gfx@lists.freedesktop.org
Cc: Chen, Guchun <Guchun.Chen@amd.com>; Zhou1, Tao <Tao.Zhou1@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; noreply-confluence@amd.com; Quan, Evan <Evan.Quan@amd.com>; Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
Subject: [PATCH 2/4] drm/amd/powerplay: Add EEPROM I2C read/write support to Arcturus.

The communication is done through SMU table and hence the code is in powerplay.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/amd/powerplay/arcturus_ppt.c | 229 +++++++++++++++++++++++++++
 1 file changed, 229 insertions(+)

diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
index 90d871a..53d08de5 100644
--- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
+++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
@@ -36,6 +36,11 @@
 #include "smu_v11_0_pptable.h"
 #include "arcturus_ppsmc.h"
 #include "nbio/nbio_7_4_sh_mask.h"
+#include <linux/i2c.h>
+#include <linux/pci.h>
+#include "amdgpu_ras.h"
+
+#define to_amdgpu_device(x) (container_of(x, struct amdgpu_ras, 
+eeprom_control.eeprom_accessor))->adev
 
 #define CTF_OFFSET_EDGE			5
 #define CTF_OFFSET_HOTSPOT		5
@@ -171,6 +176,7 @@ static struct smu_11_0_cmn2aisc_mapping arcturus_table_map[SMU_TABLE_COUNT] = {
 	TAB_MAP(SMU_METRICS),
 	TAB_MAP(DRIVER_SMU_CONFIG),
 	TAB_MAP(OVERDRIVE),
+	TAB_MAP(I2C_COMMANDS),
 };
 
 static struct smu_11_0_cmn2aisc_mapping arcturus_pwr_src_map[SMU_POWER_SOURCE_COUNT] = { @@ -293,6 +299,9 @@ static int arcturus_tables_init(struct smu_context *smu, struct smu_table *table
 	SMU_TABLE_INIT(tables, SMU_TABLE_SMU_METRICS, sizeof(SmuMetrics_t),
 		       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
 
+	SMU_TABLE_INIT(tables, SMU_TABLE_I2C_COMMANDS, sizeof(SwI2cRequest_t),
+			       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
+
 	smu_table->metrics_table = kzalloc(sizeof(SmuMetrics_t), GFP_KERNEL);
 	if (!smu_table->metrics_table)
 		return -ENOMEM;
@@ -1927,6 +1936,224 @@ static int arcturus_dpm_set_uvd_enable(struct smu_context *smu, bool enable)
 	return ret;
 }
 
+
+static void arcturus_fill_eeprom_i2c_req(SwI2cRequest_t  *req, bool write,
+				  uint8_t address, uint32_t numbytes,
+				  uint8_t *data)
+{
+	int i;
+
+	BUG_ON(numbytes > MAX_SW_I2C_COMMANDS);
+
+	req->I2CcontrollerPort = 0;
+	req->I2CSpeed = 2;
+	req->SlaveAddress = address;
+	req->NumCmds = numbytes;
+
+	for (i = 0; i < numbytes; i++) {
+		SwI2cCmd_t *cmd =  &req->SwI2cCmds[i];
+
+		/* First 2 bytes are always write for lower 2b EEPROM address */
+		if (i < 2)
+			cmd->Cmd = 1;
+		else
+			cmd->Cmd = write;
+
+
+		/* Add RESTART for read  after address filled */
+		cmd->CmdConfig |= (i == 2 && !write) ? CMDCONFIG_RESTART_MASK : 0;
+
+		/* Add STOP in the end */
+		cmd->CmdConfig |= (i == (numbytes - 1)) ? CMDCONFIG_STOP_MASK : 0;
+
+		/* Fill with data regardless if read or write to simplify code */
+		cmd->RegisterAddr = data[i];
+	}
+}
+
+static int arcturus_i2c_eeprom_read_data(struct i2c_adapter *control,
+					       uint8_t address,
+					       uint8_t *data,
+					       uint32_t numbytes)
+{
+	uint32_t  i, ret = 0;
+	SwI2cRequest_t req;
+	struct amdgpu_device *adev = to_amdgpu_device(control);
+	struct smu_table_context *smu_table = &adev->smu.smu_table;
+	struct smu_table *table = &smu_table->tables[SMU_TABLE_I2C_COMMANDS];
+
+	memset(&req, 0, sizeof(req));
+	arcturus_fill_eeprom_i2c_req(&req, false, address, numbytes, data);
+
+	mutex_lock(&adev->smu.mutex);
+	/* Now read data starting with that address */
+	ret = smu_update_table(&adev->smu, SMU_TABLE_I2C_COMMANDS, 0, &req,
+					true);
+	mutex_unlock(&adev->smu.mutex);
+
+	if (!ret) {
+		SwI2cRequest_t *res = (SwI2cRequest_t *)table->cpu_addr;
+
+		/* Assume SMU  fills res.SwI2cCmds[i].Data with read bytes */
+		for (i = 0; i < numbytes; i++)
+			data[i] = res->SwI2cCmds[i].Data;
+
+		pr_debug("arcturus_i2c_eeprom_read_data, address = %x, bytes = %d, data :",
+				  (uint16_t)address, numbytes);
+
+		print_hex_dump(KERN_DEBUG, "data: ", DUMP_PREFIX_NONE,
+			       8, 1, data, numbytes, false);
+	} else
+		pr_err("arcturus_i2c_eeprom_read_data - error occurred :%x", ret);
+
+	return ret;
+}
+
+static int arcturus_i2c_eeprom_write_data(struct i2c_adapter *control,
+						uint8_t address,
+						uint8_t *data,
+						uint32_t numbytes)
+{
+	uint32_t ret;
+	SwI2cRequest_t req;
+	struct amdgpu_device *adev = to_amdgpu_device(control);
+
+	memset(&req, 0, sizeof(req));
+	arcturus_fill_eeprom_i2c_req(&req, true, address, numbytes, data);
+
+	mutex_lock(&adev->smu.mutex);
+	ret = smu_update_table(&adev->smu, SMU_TABLE_I2C_COMMANDS, 0, &req, true);
+	mutex_unlock(&adev->smu.mutex);
+
+	if (!ret) {
+		pr_debug("arcturus_i2c_write(), address = %x, bytes = %d , data: ",
+					 (uint16_t)address, numbytes);
+
+		print_hex_dump(KERN_DEBUG, "data: ", DUMP_PREFIX_NONE,
+			       8, 1, data, numbytes, false);
+		/*
+		 * According to EEPROM spec there is a MAX of 10 ms required for
+		 * EEPROM to flush internal RX buffer after STOP was issued at the
+		 * end of write transaction. During this time the EEPROM will not be
+		 * responsive to any more commands - so wait a bit more.
+		 */
+		msleep(10);
+
+	} else
+		pr_err("arcturus_i2c_write- error occurred :%x", ret);
+
+	return ret;
+}
+
+static int arcturus_i2c_eeprom_i2c_xfer(struct i2c_adapter *i2c_adap,
+			      struct i2c_msg *msgs, int num) {
+	uint32_t  i, j, ret, data_size, data_chunk_size, next_eeprom_addr = 0;
+	uint8_t *data_ptr, data_chunk[MAX_SW_I2C_COMMANDS] = { 0 };
+
+	for (i = 0; i < num; i++) {
+		/*
+		 * SMU interface allows at most MAX_SW_I2C_COMMANDS bytes of data at
+		 * once and hence the data needs to be spliced into chunks and sent each
+		 * chunk separately
+		 */
+		data_size = msgs[i].len - 2;
+		data_chunk_size = MAX_SW_I2C_COMMANDS - 2;
+		next_eeprom_addr = (msgs[i].buf[0] << 8 & 0xff00) | (msgs[i].buf[1] & 0xff);
+		data_ptr = msgs[i].buf + 2;
+
+		for (j = 0; j < data_size / data_chunk_size; j++) {
+			/* Insert the EEPROM dest addess, bits 0-15 */
+			data_chunk[0] = ((next_eeprom_addr >> 8) & 0xff);
+			data_chunk[1] = (next_eeprom_addr & 0xff);
+
+			if (msgs[i].flags & I2C_M_RD) {
+				ret = arcturus_i2c_eeprom_read_data(i2c_adap,
+								(uint8_t)msgs[i].addr,
+								data_chunk, MAX_SW_I2C_COMMANDS);
+
+				memcpy(data_ptr, data_chunk + 2, data_chunk_size);
+			} else {
+
+				memcpy(data_chunk + 2, data_ptr, data_chunk_size);
+
+				ret = arcturus_i2c_eeprom_write_data(i2c_adap,
+								 (uint8_t)msgs[i].addr,
+								 data_chunk, MAX_SW_I2C_COMMANDS);
+			}
+
+			if (ret) {
+				num = -EIO;
+				goto fail;
+			}
+
+			next_eeprom_addr += data_chunk_size;
+			data_ptr += data_chunk_size;
+		}
+
+		if (data_size % data_chunk_size) {
+			data_chunk[0] = ((next_eeprom_addr >> 8) & 0xff);
+			data_chunk[1] = (next_eeprom_addr & 0xff);
+
+			if (msgs[i].flags & I2C_M_RD) {
+				ret = arcturus_i2c_eeprom_read_data(i2c_adap,
+								(uint8_t)msgs[i].addr,
+								data_chunk, (data_size % data_chunk_size) + 2);
+
+				memcpy(data_ptr, data_chunk + 2, data_size % data_chunk_size);
+			} else {
+				memcpy(data_chunk + 2, data_ptr, data_size % data_chunk_size);
+
+				ret = arcturus_i2c_eeprom_write_data(i2c_adap,
+								 (uint8_t)msgs[i].addr,
+								 data_chunk, (data_size % data_chunk_size) + 2);
+			}
+
+			if (ret) {
+				num = -EIO;
+				goto fail;
+			}
+		}
+	}
+
+fail:
+	return num;
+}
+
+static u32 arcturus_i2c_eeprom_i2c_func(struct i2c_adapter *adap) {
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; }
+
+
+static const struct i2c_algorithm arcturus_i2c_eeprom_i2c_algo = {
+	.master_xfer = arcturus_i2c_eeprom_i2c_xfer,
+	.functionality = arcturus_i2c_eeprom_i2c_func,
+};
+
+int arcturus_i2c_eeprom_control_init(struct i2c_adapter *control)
+{
+	struct amdgpu_device *adev = to_amdgpu_device(control);
+	int res;
+
+	control->owner = THIS_MODULE;
+	control->class = I2C_CLASS_SPD;
+	control->dev.parent = &adev->pdev->dev;
+	control->algo = &arcturus_i2c_eeprom_i2c_algo;
+	snprintf(control->name, sizeof(control->name), "RAS EEPROM");
+
+	res = i2c_add_adapter(control);
+	if (res)
+		DRM_ERROR("Failed to register hw i2c, err: %d\n", res);
+ 
+	return res;
+}
[Guchun]Can we move this register code to one common file? As I saw on vega20, we have the same code smu_v11_0_i2c_eeprom_control_init to register i2c device.
If we can move such code to a common file, then in each asic's i2c code, what we should do is to pass necessary ptrs/name/ops to that common file, and from there, we will register i2c device.
This can help clean some code, whatever, maybe convenient to other asics if we want to do the same i2c adding job.

+void arcturus_i2c_eeprom_control_fini(struct i2c_adapter *control)
+{
+	i2c_del_adapter(control);
+}
+
[Guchun]The same as above.

 static const struct pptable_funcs arcturus_ppt_funcs = {
 	/* translate smu index into arcturus specific index */
 	.get_smu_msg_index = arcturus_get_smu_msg_index,
@@ -1966,6 +2193,8 @@ static const struct pptable_funcs arcturus_ppt_funcs = {
 	.get_power_limit = arcturus_get_power_limit,
 	.is_dpm_running = arcturus_is_dpm_running,
 	.dpm_set_uvd_enable = arcturus_dpm_set_uvd_enable,
+	.i2c_eeprom_init = arcturus_i2c_eeprom_control_init,
+	.i2c_eeprom_fini = arcturus_i2c_eeprom_control_fini,
 };
 
 void arcturus_set_ppt_funcs(struct smu_context *smu)
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 4/4] drm/amdgpu: Move amdgpu_ras_recovery_init to after SMU ready.

[Chen, Guchun]

I don't think postpone RAS recovery init is not one reasonable proposal. What we do in recovery init is to read the retired page if there is, and retire corresponding memory, this will make sure these pages are reserved well before setting up memory manager and reserving other memory blocks, it will be safe.

Regards,
Guchun

-----Original Message-----
From: Andrey Grodzovsky <andrey.grodzovsky@amd.com> 
Sent: Saturday, October 19, 2019 4:49 AM
To: amd-gfx@lists.freedesktop.org
Cc: Chen, Guchun <Guchun.Chen@amd.com>; Zhou1, Tao <Tao.Zhou1@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; noreply-confluence@amd.com; Quan, Evan <Evan.Quan@amd.com>; Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
Subject: [PATCH 4/4] drm/amdgpu: Move amdgpu_ras_recovery_init to after SMU ready.

For Arcturus the I2C traffic is done through SMU tables and so we must postpone RAS recovery init to after they are ready which is in amdgpu_device_ip_hw_init_phase2.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 13 +++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c    | 11 -----------
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 17cfdaf..c40e9a5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1850,6 +1850,19 @@ static int amdgpu_device_ip_init(struct amdgpu_device *adev)
 	if (r)
 		goto init_failed;
 
+	/*
+	 * retired pages will be loaded from eeprom and reserved here,
+	 * it should be called after amdgpu_device_ip_hw_init_phase2  since
+	 * for some ASICs the RAS EEPROM code relies on SMU fully functioning
+	 * for I2C communication which only true at this point.
+	 * recovery_init may fail, but it can free all resources allocated by
+	 * itself and its failure should not stop amdgpu init process.
+	 *
+	 * Note: theoretically, this should be called before all vram allocations
+	 * to protect retired page from abusing
+	 */
+	amdgpu_ras_recovery_init(adev);
+
 	if (adev->gmc.xgmi.num_physical_nodes > 1)
 		amdgpu_xgmi_add_device(adev);
 	amdgpu_amdkfd_device_init(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 2e85a51..1045c3f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1721,17 +1721,6 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)  #endif
 
 	/*
-	 * retired pages will be loaded from eeprom and reserved here,
-	 * it should be called after ttm init since new bo may be created,
-	 * recovery_init may fail, but it can free all resources allocated by
-	 * itself and its failure should not stop amdgpu init process.
-	 *
-	 * Note: theoretically, this should be called before all vram allocations
-	 * to protect retired page from abusing
-	 */
-	amdgpu_ras_recovery_init(adev);
-
-	/*
 	 *The reserved vram for firmware must be pinned to the specified
 	 *place on the VRAM, so reserve it early.
 	 */
--
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 2/4] drm/amd/powerplay: Add EEPROM I2C read/write support to Arcturus.

[Li, Dennis]

To make the function behavior more clear, It's better to change the function declaration from 

+ static void arcturus_fill_eeprom_i2c_req(SwI2cRequest_t  *req, bool write,
+				  uint8_t address, uint32_t numbytes,
+				  uint8_t *data)

to

+static void arcturus_fill_eeprom_i2c_req(SwI2cRequest_t  *req, bool write_or_read,
+				  uint8_t address, uint32_t numbytes,
+				  uint8_t *data)

Best Regards
Dennis Li
-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Andrey Grodzovsky
Sent: Saturday, October 19, 2019 4:48 AM
To: amd-gfx@lists.freedesktop.org
Cc: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>; Zhou1, Tao <Tao.Zhou1@amd.com>; noreply-confluence@amd.com; Deucher, Alexander <Alexander.Deucher@amd.com>; Quan, Evan <Evan.Quan@amd.com>
Subject: [PATCH 2/4] drm/amd/powerplay: Add EEPROM I2C read/write support to Arcturus.

The communication is done through SMU table and hence the code is in powerplay.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/amd/powerplay/arcturus_ppt.c | 229 +++++++++++++++++++++++++++
 1 file changed, 229 insertions(+)

diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
index 90d871a..53d08de5 100644
--- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
+++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
@@ -36,6 +36,11 @@
 #include "smu_v11_0_pptable.h"
 #include "arcturus_ppsmc.h"
 #include "nbio/nbio_7_4_sh_mask.h"
+#include <linux/i2c.h>
+#include <linux/pci.h>
+#include "amdgpu_ras.h"
+
+#define to_amdgpu_device(x) (container_of(x, struct amdgpu_ras, 
+eeprom_control.eeprom_accessor))->adev
 
 #define CTF_OFFSET_EDGE			5
 #define CTF_OFFSET_HOTSPOT		5
@@ -171,6 +176,7 @@ static struct smu_11_0_cmn2aisc_mapping arcturus_table_map[SMU_TABLE_COUNT] = {
 	TAB_MAP(SMU_METRICS),
 	TAB_MAP(DRIVER_SMU_CONFIG),
 	TAB_MAP(OVERDRIVE),
+	TAB_MAP(I2C_COMMANDS),
 };
 
 static struct smu_11_0_cmn2aisc_mapping arcturus_pwr_src_map[SMU_POWER_SOURCE_COUNT] = { @@ -293,6 +299,9 @@ static int arcturus_tables_init(struct smu_context *smu, struct smu_table *table
 	SMU_TABLE_INIT(tables, SMU_TABLE_SMU_METRICS, sizeof(SmuMetrics_t),
 		       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
 
+	SMU_TABLE_INIT(tables, SMU_TABLE_I2C_COMMANDS, sizeof(SwI2cRequest_t),
+			       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
+
 	smu_table->metrics_table = kzalloc(sizeof(SmuMetrics_t), GFP_KERNEL);
 	if (!smu_table->metrics_table)
 		return -ENOMEM;
@@ -1927,6 +1936,224 @@ static int arcturus_dpm_set_uvd_enable(struct smu_context *smu, bool enable)
 	return ret;
 }
 
+
+static void arcturus_fill_eeprom_i2c_req(SwI2cRequest_t  *req, bool write,
+				  uint8_t address, uint32_t numbytes,
+				  uint8_t *data)
+{
+	int i;
+
+	BUG_ON(numbytes > MAX_SW_I2C_COMMANDS);
+
+	req->I2CcontrollerPort = 0;
+	req->I2CSpeed = 2;
+	req->SlaveAddress = address;
+	req->NumCmds = numbytes;
+
+	for (i = 0; i < numbytes; i++) {
+		SwI2cCmd_t *cmd =  &req->SwI2cCmds[i];
+
+		/* First 2 bytes are always write for lower 2b EEPROM address */
+		if (i < 2)
+			cmd->Cmd = 1;
+		else
+			cmd->Cmd = write;
+
+
+		/* Add RESTART for read  after address filled */
+		cmd->CmdConfig |= (i == 2 && !write) ? CMDCONFIG_RESTART_MASK : 0;
+
+		/* Add STOP in the end */
+		cmd->CmdConfig |= (i == (numbytes - 1)) ? CMDCONFIG_STOP_MASK : 0;
+
+		/* Fill with data regardless if read or write to simplify code */
+		cmd->RegisterAddr = data[i];
+	}
+}
+
+static int arcturus_i2c_eeprom_read_data(struct i2c_adapter *control,
+					       uint8_t address,
+					       uint8_t *data,
+					       uint32_t numbytes)
+{
+	uint32_t  i, ret = 0;
+	SwI2cRequest_t req;
+	struct amdgpu_device *adev = to_amdgpu_device(control);
+	struct smu_table_context *smu_table = &adev->smu.smu_table;
+	struct smu_table *table = &smu_table->tables[SMU_TABLE_I2C_COMMANDS];
+
+	memset(&req, 0, sizeof(req));
+	arcturus_fill_eeprom_i2c_req(&req, false, address, numbytes, data);
+
+	mutex_lock(&adev->smu.mutex);
+	/* Now read data starting with that address */
+	ret = smu_update_table(&adev->smu, SMU_TABLE_I2C_COMMANDS, 0, &req,
+					true);
+	mutex_unlock(&adev->smu.mutex);
+
+	if (!ret) {
+		SwI2cRequest_t *res = (SwI2cRequest_t *)table->cpu_addr;
+
+		/* Assume SMU  fills res.SwI2cCmds[i].Data with read bytes */
+		for (i = 0; i < numbytes; i++)
+			data[i] = res->SwI2cCmds[i].Data;
+
+		pr_debug("arcturus_i2c_eeprom_read_data, address = %x, bytes = %d, data :",
+				  (uint16_t)address, numbytes);
+
+		print_hex_dump(KERN_DEBUG, "data: ", DUMP_PREFIX_NONE,
+			       8, 1, data, numbytes, false);
+	} else
+		pr_err("arcturus_i2c_eeprom_read_data - error occurred :%x", ret);
+
+	return ret;
+}
+
+static int arcturus_i2c_eeprom_write_data(struct i2c_adapter *control,
+						uint8_t address,
+						uint8_t *data,
+						uint32_t numbytes)
+{
+	uint32_t ret;
+	SwI2cRequest_t req;
+	struct amdgpu_device *adev = to_amdgpu_device(control);
+
+	memset(&req, 0, sizeof(req));
+	arcturus_fill_eeprom_i2c_req(&req, true, address, numbytes, data);
+
+	mutex_lock(&adev->smu.mutex);
+	ret = smu_update_table(&adev->smu, SMU_TABLE_I2C_COMMANDS, 0, &req, true);
+	mutex_unlock(&adev->smu.mutex);
+
+	if (!ret) {
+		pr_debug("arcturus_i2c_write(), address = %x, bytes = %d , data: ",
+					 (uint16_t)address, numbytes);
+
+		print_hex_dump(KERN_DEBUG, "data: ", DUMP_PREFIX_NONE,
+			       8, 1, data, numbytes, false);
+		/*
+		 * According to EEPROM spec there is a MAX of 10 ms required for
+		 * EEPROM to flush internal RX buffer after STOP was issued at the
+		 * end of write transaction. During this time the EEPROM will not be
+		 * responsive to any more commands - so wait a bit more.
+		 */
+		msleep(10);
+
+	} else
+		pr_err("arcturus_i2c_write- error occurred :%x", ret);
+
+	return ret;
+}
+
+static int arcturus_i2c_eeprom_i2c_xfer(struct i2c_adapter *i2c_adap,
+			      struct i2c_msg *msgs, int num) {
+	uint32_t  i, j, ret, data_size, data_chunk_size, next_eeprom_addr = 0;
+	uint8_t *data_ptr, data_chunk[MAX_SW_I2C_COMMANDS] = { 0 };
+
+	for (i = 0; i < num; i++) {
+		/*
+		 * SMU interface allows at most MAX_SW_I2C_COMMANDS bytes of data at
+		 * once and hence the data needs to be spliced into chunks and sent each
+		 * chunk separately
+		 */
+		data_size = msgs[i].len - 2;
+		data_chunk_size = MAX_SW_I2C_COMMANDS - 2;
+		next_eeprom_addr = (msgs[i].buf[0] << 8 & 0xff00) | (msgs[i].buf[1] & 0xff);
+		data_ptr = msgs[i].buf + 2;
+
+		for (j = 0; j < data_size / data_chunk_size; j++) {
+			/* Insert the EEPROM dest addess, bits 0-15 */
+			data_chunk[0] = ((next_eeprom_addr >> 8) & 0xff);
+			data_chunk[1] = (next_eeprom_addr & 0xff);
+
+			if (msgs[i].flags & I2C_M_RD) {
+				ret = arcturus_i2c_eeprom_read_data(i2c_adap,
+								(uint8_t)msgs[i].addr,
+								data_chunk, MAX_SW_I2C_COMMANDS);
+
+				memcpy(data_ptr, data_chunk + 2, data_chunk_size);
+			} else {
+
+				memcpy(data_chunk + 2, data_ptr, data_chunk_size);
+
+				ret = arcturus_i2c_eeprom_write_data(i2c_adap,
+								 (uint8_t)msgs[i].addr,
+								 data_chunk, MAX_SW_I2C_COMMANDS);
+			}
+
+			if (ret) {
+				num = -EIO;
+				goto fail;
+			}
+
+			next_eeprom_addr += data_chunk_size;
+			data_ptr += data_chunk_size;
+		}
+
+		if (data_size % data_chunk_size) {
+			data_chunk[0] = ((next_eeprom_addr >> 8) & 0xff);
+			data_chunk[1] = (next_eeprom_addr & 0xff);
+
+			if (msgs[i].flags & I2C_M_RD) {
+				ret = arcturus_i2c_eeprom_read_data(i2c_adap,
+								(uint8_t)msgs[i].addr,
+								data_chunk, (data_size % data_chunk_size) + 2);
+
+				memcpy(data_ptr, data_chunk + 2, data_size % data_chunk_size);
+			} else {
+				memcpy(data_chunk + 2, data_ptr, data_size % data_chunk_size);
+
+				ret = arcturus_i2c_eeprom_write_data(i2c_adap,
+								 (uint8_t)msgs[i].addr,
+								 data_chunk, (data_size % data_chunk_size) + 2);
+			}
+
+			if (ret) {
+				num = -EIO;
+				goto fail;
+			}
+		}
+	}
+
+fail:
+	return num;
+}
+
+static u32 arcturus_i2c_eeprom_i2c_func(struct i2c_adapter *adap) {
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; }
+
+
+static const struct i2c_algorithm arcturus_i2c_eeprom_i2c_algo = {
+	.master_xfer = arcturus_i2c_eeprom_i2c_xfer,
+	.functionality = arcturus_i2c_eeprom_i2c_func,
+};
+
+int arcturus_i2c_eeprom_control_init(struct i2c_adapter *control)
+{
+	struct amdgpu_device *adev = to_amdgpu_device(control);
+	int res;
+
+	control->owner = THIS_MODULE;
+	control->class = I2C_CLASS_SPD;
+	control->dev.parent = &adev->pdev->dev;
+	control->algo = &arcturus_i2c_eeprom_i2c_algo;
+	snprintf(control->name, sizeof(control->name), "RAS EEPROM");
+
+	res = i2c_add_adapter(control);
+	if (res)
+		DRM_ERROR("Failed to register hw i2c, err: %d\n", res);
+
+	return res;
+}
+
+void arcturus_i2c_eeprom_control_fini(struct i2c_adapter *control)
+{
+	i2c_del_adapter(control);
+}
+
 static const struct pptable_funcs arcturus_ppt_funcs = {
 	/* translate smu index into arcturus specific index */
 	.get_smu_msg_index = arcturus_get_smu_msg_index,
@@ -1966,6 +2193,8 @@ static const struct pptable_funcs arcturus_ppt_funcs = {
 	.get_power_limit = arcturus_get_power_limit,
 	.is_dpm_running = arcturus_is_dpm_running,
 	.dpm_set_uvd_enable = arcturus_dpm_set_uvd_enable,
+	.i2c_eeprom_init = arcturus_i2c_eeprom_control_init,
+	.i2c_eeprom_fini = arcturus_i2c_eeprom_control_fini,
 };
 
 void arcturus_set_ppt_funcs(struct smu_context *smu)
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 1/4] drm/amd/powerplay: Add interface for I2C transactions to SMU.

[Deucher, Alexander]

> -----Original Message-----
> From: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> Sent: Friday, October 18, 2019 4:48 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Chen, Guchun <Guchun.Chen@amd.com>; Zhou1, Tao
> <Tao.Zhou1@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; noreply-confluence@amd.com; Quan,
> Evan <Evan.Quan@amd.com>; Grodzovsky, Andrey
> <Andrey.Grodzovsky@amd.com>
> Subject: [PATCH 1/4] drm/amd/powerplay: Add interface for I2C transactions
> to SMU.
> 
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>

Please add a patch description.  With that fixed:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>


> ---
>  drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> index bf13bf3..24244eb 100644
> --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> @@ -394,6 +394,8 @@ struct smu_context
> 
>  };
> 
> +struct i2c_adapter;
> +
>  struct pptable_funcs {
>  	int (*alloc_dpm_context)(struct smu_context *smu);
>  	int (*store_powerplay_table)(struct smu_context *smu); @@ -470,6
> +472,8 @@ struct pptable_funcs {
>  				   uint32_t dpm_level, uint32_t *freq);
>  	int (*set_df_cstate)(struct smu_context *smu, enum pp_df_cstate
> state);
>  	int (*update_pcie_parameters)(struct smu_context *smu, uint32_t
> pcie_gen_cap, uint32_t pcie_width_cap);
> +	int (*i2c_eeprom_init)(struct i2c_adapter *control);
> +	void (*i2c_eeprom_fini)(struct i2c_adapter *control);
>  	int (*get_dpm_clock_table)(struct smu_context *smu, struct
> dpm_clocks *clock_table);  };
> 
> @@ -782,6 +786,11 @@ struct smu_funcs
>  #define smu_override_pcie_parameters(smu) \
>  		((smu)->funcs->override_pcie_parameters ? (smu)->funcs-
> >override_pcie_parameters((smu)) : 0)
> 
> +#define smu_i2c_eeprom_init(smu, control) \
> +		((smu)->ppt_funcs->i2c_eeprom_init ?
> +(smu)->ppt_funcs->i2c_eeprom_init((control)) : -EINVAL) #define
> smu_i2c_eeprom_fini(smu, control) \
> +		((smu)->ppt_funcs->i2c_eeprom_fini ?
> +(smu)->ppt_funcs->i2c_eeprom_fini((control)) : -EINVAL)
> +
>  #define smu_update_pcie_parameters(smu, pcie_gen_cap,
> pcie_width_cap) \
>  		((smu)->ppt_funcs->update_pcie_parameters ? (smu)-
> >ppt_funcs->update_pcie_parameters((smu), (pcie_gen_cap),
> (pcie_width_cap)) : 0)
> 
> --
> 2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 2/4] drm/amd/powerplay: Add EEPROM I2C read/write support to Arcturus.

[Deucher, Alexander]

> -----Original Message-----
> From: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> Sent: Friday, October 18, 2019 4:48 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Chen, Guchun <Guchun.Chen@amd.com>; Zhou1, Tao
> <Tao.Zhou1@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; noreply-confluence@amd.com; Quan,
> Evan <Evan.Quan@amd.com>; Grodzovsky, Andrey
> <Andrey.Grodzovsky@amd.com>
> Subject: [PATCH 2/4] drm/amd/powerplay: Add EEPROM I2C read/write
> support to Arcturus.
> 
> The communication is done through SMU table and hence the code is in
> powerplay.
> 
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/amd/powerplay/arcturus_ppt.c | 229
> +++++++++++++++++++++++++++
>  1 file changed, 229 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> index 90d871a..53d08de5 100644
> --- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> +++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> @@ -36,6 +36,11 @@
>  #include "smu_v11_0_pptable.h"
>  #include "arcturus_ppsmc.h"
>  #include "nbio/nbio_7_4_sh_mask.h"
> +#include <linux/i2c.h>
> +#include <linux/pci.h>
> +#include "amdgpu_ras.h"
> +
> +#define to_amdgpu_device(x) (container_of(x, struct amdgpu_ras,
> +eeprom_control.eeprom_accessor))->adev
> 
>  #define CTF_OFFSET_EDGE			5
>  #define CTF_OFFSET_HOTSPOT		5
> @@ -171,6 +176,7 @@ static struct smu_11_0_cmn2aisc_mapping
> arcturus_table_map[SMU_TABLE_COUNT] = {
>  	TAB_MAP(SMU_METRICS),
>  	TAB_MAP(DRIVER_SMU_CONFIG),
>  	TAB_MAP(OVERDRIVE),
> +	TAB_MAP(I2C_COMMANDS),
>  };
> 
>  static struct smu_11_0_cmn2aisc_mapping
> arcturus_pwr_src_map[SMU_POWER_SOURCE_COUNT] = { @@ -293,6
> +299,9 @@ static int arcturus_tables_init(struct smu_context *smu, struct
> smu_table *table
>  	SMU_TABLE_INIT(tables, SMU_TABLE_SMU_METRICS,
> sizeof(SmuMetrics_t),
>  		       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
> 
> +	SMU_TABLE_INIT(tables, SMU_TABLE_I2C_COMMANDS,
> sizeof(SwI2cRequest_t),
> +			       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
> +
>  	smu_table->metrics_table = kzalloc(sizeof(SmuMetrics_t),
> GFP_KERNEL);
>  	if (!smu_table->metrics_table)
>  		return -ENOMEM;
> @@ -1927,6 +1936,224 @@ static int arcturus_dpm_set_uvd_enable(struct
> smu_context *smu, bool enable)
>  	return ret;
>  }
> 
> +
> +static void arcturus_fill_eeprom_i2c_req(SwI2cRequest_t  *req, bool write,
> +				  uint8_t address, uint32_t numbytes,
> +				  uint8_t *data)
> +{
> +	int i;
> +
> +	BUG_ON(numbytes > MAX_SW_I2C_COMMANDS);
> +
> +	req->I2CcontrollerPort = 0;
> +	req->I2CSpeed = 2;
> +	req->SlaveAddress = address;
> +	req->NumCmds = numbytes;
> +
> +	for (i = 0; i < numbytes; i++) {
> +		SwI2cCmd_t *cmd =  &req->SwI2cCmds[i];
> +
> +		/* First 2 bytes are always write for lower 2b EEPROM
> address */
> +		if (i < 2)
> +			cmd->Cmd = 1;
> +		else
> +			cmd->Cmd = write;
> +
> +
> +		/* Add RESTART for read  after address filled */
> +		cmd->CmdConfig |= (i == 2 && !write) ?
> CMDCONFIG_RESTART_MASK : 0;
> +
> +		/* Add STOP in the end */
> +		cmd->CmdConfig |= (i == (numbytes - 1)) ?
> CMDCONFIG_STOP_MASK : 0;
> +
> +		/* Fill with data regardless if read or write to simplify code */
> +		cmd->RegisterAddr = data[i];
> +	}
> +}
> +
> +static int arcturus_i2c_eeprom_read_data(struct i2c_adapter *control,
> +					       uint8_t address,
> +					       uint8_t *data,
> +					       uint32_t numbytes)
> +{
> +	uint32_t  i, ret = 0;
> +	SwI2cRequest_t req;
> +	struct amdgpu_device *adev = to_amdgpu_device(control);
> +	struct smu_table_context *smu_table = &adev->smu.smu_table;
> +	struct smu_table *table = &smu_table-
> >tables[SMU_TABLE_I2C_COMMANDS];
> +
> +	memset(&req, 0, sizeof(req));
> +	arcturus_fill_eeprom_i2c_req(&req, false, address, numbytes, data);
> +
> +	mutex_lock(&adev->smu.mutex);
> +	/* Now read data starting with that address */
> +	ret = smu_update_table(&adev->smu,
> SMU_TABLE_I2C_COMMANDS, 0, &req,
> +					true);
> +	mutex_unlock(&adev->smu.mutex);
> +
> +	if (!ret) {
> +		SwI2cRequest_t *res = (SwI2cRequest_t *)table->cpu_addr;
> +
> +		/* Assume SMU  fills res.SwI2cCmds[i].Data with read bytes
> */
> +		for (i = 0; i < numbytes; i++)
> +			data[i] = res->SwI2cCmds[i].Data;
> +
> +		pr_debug("arcturus_i2c_eeprom_read_data, address = %x,
> bytes = %d, data :",
> +				  (uint16_t)address, numbytes);
> +
> +		print_hex_dump(KERN_DEBUG, "data: ",
> DUMP_PREFIX_NONE,
> +			       8, 1, data, numbytes, false);
> +	} else
> +		pr_err("arcturus_i2c_eeprom_read_data - error occurred
> :%x", ret);
> +
> +	return ret;
> +}
> +
> +static int arcturus_i2c_eeprom_write_data(struct i2c_adapter *control,
> +						uint8_t address,
> +						uint8_t *data,
> +						uint32_t numbytes)
> +{
> +	uint32_t ret;
> +	SwI2cRequest_t req;
> +	struct amdgpu_device *adev = to_amdgpu_device(control);
> +
> +	memset(&req, 0, sizeof(req));
> +	arcturus_fill_eeprom_i2c_req(&req, true, address, numbytes, data);
> +
> +	mutex_lock(&adev->smu.mutex);
> +	ret = smu_update_table(&adev->smu,
> SMU_TABLE_I2C_COMMANDS, 0, &req, true);
> +	mutex_unlock(&adev->smu.mutex);
> +
> +	if (!ret) {
> +		pr_debug("arcturus_i2c_write(), address = %x, bytes = %d ,
> data: ",
> +					 (uint16_t)address, numbytes);
> +
> +		print_hex_dump(KERN_DEBUG, "data: ",
> DUMP_PREFIX_NONE,
> +			       8, 1, data, numbytes, false);
> +		/*
> +		 * According to EEPROM spec there is a MAX of 10 ms
> required for
> +		 * EEPROM to flush internal RX buffer after STOP was issued
> at the
> +		 * end of write transaction. During this time the EEPROM will
> not be
> +		 * responsive to any more commands - so wait a bit more.
> +		 */
> +		msleep(10);
> +
> +	} else
> +		pr_err("arcturus_i2c_write- error occurred :%x", ret);
> +
> +	return ret;
> +}
> +
> +static int arcturus_i2c_eeprom_i2c_xfer(struct i2c_adapter *i2c_adap,
> +			      struct i2c_msg *msgs, int num) {
> +	uint32_t  i, j, ret, data_size, data_chunk_size, next_eeprom_addr =
> 0;
> +	uint8_t *data_ptr, data_chunk[MAX_SW_I2C_COMMANDS] = { 0 };
> +
> +	for (i = 0; i < num; i++) {
> +		/*
> +		 * SMU interface allows at most MAX_SW_I2C_COMMANDS
> bytes of data at
> +		 * once and hence the data needs to be spliced into chunks
> and sent each
> +		 * chunk separately
> +		 */
> +		data_size = msgs[i].len - 2;
> +		data_chunk_size = MAX_SW_I2C_COMMANDS - 2;
> +		next_eeprom_addr = (msgs[i].buf[0] << 8 & 0xff00) |
> (msgs[i].buf[1] & 0xff);
> +		data_ptr = msgs[i].buf + 2;
> +
> +		for (j = 0; j < data_size / data_chunk_size; j++) {
> +			/* Insert the EEPROM dest addess, bits 0-15 */
> +			data_chunk[0] = ((next_eeprom_addr >> 8) & 0xff);
> +			data_chunk[1] = (next_eeprom_addr & 0xff);
> +
> +			if (msgs[i].flags & I2C_M_RD) {
> +				ret =
> arcturus_i2c_eeprom_read_data(i2c_adap,
> +
> 	(uint8_t)msgs[i].addr,
> +								data_chunk,
> MAX_SW_I2C_COMMANDS);
> +
> +				memcpy(data_ptr, data_chunk + 2,
> data_chunk_size);
> +			} else {
> +
> +				memcpy(data_chunk + 2, data_ptr,
> data_chunk_size);
> +
> +				ret =
> arcturus_i2c_eeprom_write_data(i2c_adap,
> +
> (uint8_t)msgs[i].addr,
> +								 data_chunk,
> MAX_SW_I2C_COMMANDS);
> +			}
> +
> +			if (ret) {
> +				num = -EIO;
> +				goto fail;
> +			}
> +
> +			next_eeprom_addr += data_chunk_size;
> +			data_ptr += data_chunk_size;
> +		}
> +
> +		if (data_size % data_chunk_size) {
> +			data_chunk[0] = ((next_eeprom_addr >> 8) & 0xff);
> +			data_chunk[1] = (next_eeprom_addr & 0xff);
> +
> +			if (msgs[i].flags & I2C_M_RD) {
> +				ret =
> arcturus_i2c_eeprom_read_data(i2c_adap,
> +
> 	(uint8_t)msgs[i].addr,
> +								data_chunk,
> (data_size % data_chunk_size) + 2);
> +
> +				memcpy(data_ptr, data_chunk + 2, data_size
> % data_chunk_size);
> +			} else {
> +				memcpy(data_chunk + 2, data_ptr, data_size
> % data_chunk_size);
> +
> +				ret =
> arcturus_i2c_eeprom_write_data(i2c_adap,
> +
> (uint8_t)msgs[i].addr,
> +								 data_chunk,
> (data_size % data_chunk_size) + 2);
> +			}
> +
> +			if (ret) {
> +				num = -EIO;
> +				goto fail;
> +			}
> +		}
> +	}
> +
> +fail:
> +	return num;
> +}
> +
> +static u32 arcturus_i2c_eeprom_i2c_func(struct i2c_adapter *adap) {
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; }
> +
> +
> +static const struct i2c_algorithm arcturus_i2c_eeprom_i2c_algo = {
> +	.master_xfer = arcturus_i2c_eeprom_i2c_xfer,
> +	.functionality = arcturus_i2c_eeprom_i2c_func,
> +};
> +
> +int arcturus_i2c_eeprom_control_init(struct i2c_adapter *control)
> +{
> +	struct amdgpu_device *adev = to_amdgpu_device(control);
> +	int res;
> +
> +	control->owner = THIS_MODULE;
> +	control->class = I2C_CLASS_SPD;
> +	control->dev.parent = &adev->pdev->dev;
> +	control->algo = &arcturus_i2c_eeprom_i2c_algo;
> +	snprintf(control->name, sizeof(control->name), "RAS EEPROM");
> +
> +	res = i2c_add_adapter(control);
> +	if (res)
> +		DRM_ERROR("Failed to register hw i2c, err: %d\n", res);
> +
> +	return res;
> +}
> +
> +void arcturus_i2c_eeprom_control_fini(struct i2c_adapter *control)
> +{
> +	i2c_del_adapter(control);
> +}
> +
>  static const struct pptable_funcs arcturus_ppt_funcs = {
>  	/* translate smu index into arcturus specific index */
>  	.get_smu_msg_index = arcturus_get_smu_msg_index,
> @@ -1966,6 +2193,8 @@ static const struct pptable_funcs
> arcturus_ppt_funcs = {
>  	.get_power_limit = arcturus_get_power_limit,
>  	.is_dpm_running = arcturus_is_dpm_running,
>  	.dpm_set_uvd_enable = arcturus_dpm_set_uvd_enable,
> +	.i2c_eeprom_init = arcturus_i2c_eeprom_control_init,
> +	.i2c_eeprom_fini = arcturus_i2c_eeprom_control_fini,
>  };
> 
>  void arcturus_set_ppt_funcs(struct smu_context *smu)
> --
> 2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 3/4] drm/amdgpu: Use ARCTURUS in RAS EEPROM.

[Deucher, Alexander]

> -----Original Message-----
> From: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> Sent: Friday, October 18, 2019 4:49 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Chen, Guchun <Guchun.Chen@amd.com>; Zhou1, Tao
> <Tao.Zhou1@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; noreply-confluence@amd.com; Quan,
> Evan <Evan.Quan@amd.com>; Grodzovsky, Andrey
> <Andrey.Grodzovsky@amd.com>
> Subject: [PATCH 3/4] drm/amdgpu: Use ARCTURUS in RAS EEPROM.
> 

Please add a patch description.  With that fixed:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
> index 20af0a1..7de16c0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
> @@ -216,6 +216,10 @@ int amdgpu_ras_eeprom_init(struct
> amdgpu_ras_eeprom_control *control)
>  		ret = smu_v11_0_i2c_eeprom_control_init(&control-
> >eeprom_accessor);
>  		break;
> 
> +	case CHIP_ARCTURUS:
> +		ret = smu_i2c_eeprom_init(&adev->smu, &control-
> >eeprom_accessor);
> +		break;
> +
>  	default:
>  		return 0;
>  	}
> @@ -260,6 +264,9 @@ void amdgpu_ras_eeprom_fini(struct
> amdgpu_ras_eeprom_control *control)
>  	case CHIP_VEGA20:
>  		smu_v11_0_i2c_eeprom_control_fini(&control-
> >eeprom_accessor);
>  		break;
> +	case CHIP_ARCTURUS:
> +		smu_i2c_eeprom_fini(&adev->smu, &control-
> >eeprom_accessor);
> +		break;
> 
>  	default:
>  		return;
> @@ -364,7 +371,7 @@ int amdgpu_ras_eeprom_process_recods(struct
> amdgpu_ras_eeprom_control *control,
>  	struct eeprom_table_record *record;
>  	struct amdgpu_device *adev = to_amdgpu_device(control);
> 
> -	if (adev->asic_type != CHIP_VEGA20)
> +	if (adev->asic_type != CHIP_VEGA20 && adev->asic_type !=
> CHIP_ARCTURUS)
>  		return 0;
> 
>  	buffs = kcalloc(num, EEPROM_ADDRESS_SIZE +
> EEPROM_TABLE_RECORD_SIZE,
> --
> 2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 4/4] drm/amdgpu: Move amdgpu_ras_recovery_init to after SMU ready.

[Deucher, Alexander]

> -----Original Message-----
> From: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> Sent: Friday, October 18, 2019 4:49 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Chen, Guchun <Guchun.Chen@amd.com>; Zhou1, Tao
> <Tao.Zhou1@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; noreply-confluence@amd.com; Quan,
> Evan <Evan.Quan@amd.com>; Grodzovsky, Andrey
> <Andrey.Grodzovsky@amd.com>
> Subject: [PATCH 4/4] drm/amdgpu: Move amdgpu_ras_recovery_init to
> after SMU ready.
> 
> For Arcturus the I2C traffic is done through SMU tables and so we must
> postpone RAS recovery init to after they are ready which is in
> amdgpu_device_ip_hw_init_phase2.
> 
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 13 +++++++++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c    | 11 -----------
>  2 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 17cfdaf..c40e9a5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1850,6 +1850,19 @@ static int amdgpu_device_ip_init(struct
> amdgpu_device *adev)
>  	if (r)
>  		goto init_failed;
> 
> +	/*
> +	 * retired pages will be loaded from eeprom and reserved here,
> +	 * it should be called after amdgpu_device_ip_hw_init_phase2  since
> +	 * for some ASICs the RAS EEPROM code relies on SMU fully
> functioning
> +	 * for I2C communication which only true at this point.
> +	 * recovery_init may fail, but it can free all resources allocated by
> +	 * itself and its failure should not stop amdgpu init process.
> +	 *
> +	 * Note: theoretically, this should be called before all vram allocations
> +	 * to protect retired page from abusing
> +	 */
> +	amdgpu_ras_recovery_init(adev);
> +
>  	if (adev->gmc.xgmi.num_physical_nodes > 1)
>  		amdgpu_xgmi_add_device(adev);
>  	amdgpu_amdkfd_device_init(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 2e85a51..1045c3f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1721,17 +1721,6 @@ int amdgpu_ttm_init(struct amdgpu_device
> *adev)  #endif
> 
>  	/*
> -	 * retired pages will be loaded from eeprom and reserved here,
> -	 * it should be called after ttm init since new bo may be created,
> -	 * recovery_init may fail, but it can free all resources allocated by
> -	 * itself and its failure should not stop amdgpu init process.
> -	 *
> -	 * Note: theoretically, this should be called before all vram allocations
> -	 * to protect retired page from abusing
> -	 */
> -	amdgpu_ras_recovery_init(adev);
> -
> -	/*
>  	 *The reserved vram for firmware must be pinned to the specified
>  	 *place on the VRAM, so reserve it early.
>  	 */
> --
> 2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 2/4] drm/amd/powerplay: Add EEPROM I2C read/write support to Arcturus.

[Deucher, Alexander]

> -----Original Message-----
> From: Li, Dennis <Dennis.Li@amd.com>
> Sent: Sunday, October 20, 2019 11:02 PM
> To: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>; amd-
> gfx@lists.freedesktop.org
> Cc: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>; Chen, Guchun
> <Guchun.Chen@amd.com>; Zhou1, Tao <Tao.Zhou1@amd.com>; noreply-
> confluence@amd.com; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Quan, Evan <Evan.Quan@amd.com>
> Subject: RE: [PATCH 2/4] drm/amd/powerplay: Add EEPROM I2C read/write
> support to Arcturus.
> 
> To make the function behavior more clear, It's better to change the function
> declaration from
> 
> + static void arcturus_fill_eeprom_i2c_req(SwI2cRequest_t  *req, bool write,
> +				  uint8_t address, uint32_t numbytes,
> +				  uint8_t *data)
> 
> to
> 
> +static void arcturus_fill_eeprom_i2c_req(SwI2cRequest_t  *req, bool
> write_or_read,
> +				  uint8_t address, uint32_t numbytes,
> +				  uint8_t *data)

I'm not sure I agree with that.  If the variable is write_or_read then how does the driver code know how to set it?  E.g., do you set it for write or for read or both?

Alex

> 
> Best Regards
> Dennis Li
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Andrey Grodzovsky
> Sent: Saturday, October 19, 2019 4:48 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>; Chen, Guchun
> <Guchun.Chen@amd.com>; Zhou1, Tao <Tao.Zhou1@amd.com>; noreply-
> confluence@amd.com; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Quan, Evan <Evan.Quan@amd.com>
> Subject: [PATCH 2/4] drm/amd/powerplay: Add EEPROM I2C read/write
> support to Arcturus.
> 
> The communication is done through SMU table and hence the code is in
> powerplay.
> 
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>  drivers/gpu/drm/amd/powerplay/arcturus_ppt.c | 229
> +++++++++++++++++++++++++++
>  1 file changed, 229 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> index 90d871a..53d08de5 100644
> --- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> +++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> @@ -36,6 +36,11 @@
>  #include "smu_v11_0_pptable.h"
>  #include "arcturus_ppsmc.h"
>  #include "nbio/nbio_7_4_sh_mask.h"
> +#include <linux/i2c.h>
> +#include <linux/pci.h>
> +#include "amdgpu_ras.h"
> +
> +#define to_amdgpu_device(x) (container_of(x, struct amdgpu_ras,
> +eeprom_control.eeprom_accessor))->adev
> 
>  #define CTF_OFFSET_EDGE			5
>  #define CTF_OFFSET_HOTSPOT		5
> @@ -171,6 +176,7 @@ static struct smu_11_0_cmn2aisc_mapping
> arcturus_table_map[SMU_TABLE_COUNT] = {
>  	TAB_MAP(SMU_METRICS),
>  	TAB_MAP(DRIVER_SMU_CONFIG),
>  	TAB_MAP(OVERDRIVE),
> +	TAB_MAP(I2C_COMMANDS),
>  };
> 
>  static struct smu_11_0_cmn2aisc_mapping
> arcturus_pwr_src_map[SMU_POWER_SOURCE_COUNT] = { @@ -293,6
> +299,9 @@ static int arcturus_tables_init(struct smu_context *smu, struct
> smu_table *table
>  	SMU_TABLE_INIT(tables, SMU_TABLE_SMU_METRICS,
> sizeof(SmuMetrics_t),
>  		       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
> 
> +	SMU_TABLE_INIT(tables, SMU_TABLE_I2C_COMMANDS,
> sizeof(SwI2cRequest_t),
> +			       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
> +
>  	smu_table->metrics_table = kzalloc(sizeof(SmuMetrics_t),
> GFP_KERNEL);
>  	if (!smu_table->metrics_table)
>  		return -ENOMEM;
> @@ -1927,6 +1936,224 @@ static int arcturus_dpm_set_uvd_enable(struct
> smu_context *smu, bool enable)
>  	return ret;
>  }
> 
> +
> +static void arcturus_fill_eeprom_i2c_req(SwI2cRequest_t  *req, bool write,
> +				  uint8_t address, uint32_t numbytes,
> +				  uint8_t *data)
> +{
> +	int i;
> +
> +	BUG_ON(numbytes > MAX_SW_I2C_COMMANDS);
> +
> +	req->I2CcontrollerPort = 0;
> +	req->I2CSpeed = 2;
> +	req->SlaveAddress = address;
> +	req->NumCmds = numbytes;
> +
> +	for (i = 0; i < numbytes; i++) {
> +		SwI2cCmd_t *cmd =  &req->SwI2cCmds[i];
> +
> +		/* First 2 bytes are always write for lower 2b EEPROM
> address */
> +		if (i < 2)
> +			cmd->Cmd = 1;
> +		else
> +			cmd->Cmd = write;
> +
> +
> +		/* Add RESTART for read  after address filled */
> +		cmd->CmdConfig |= (i == 2 && !write) ?
> CMDCONFIG_RESTART_MASK : 0;
> +
> +		/* Add STOP in the end */
> +		cmd->CmdConfig |= (i == (numbytes - 1)) ?
> CMDCONFIG_STOP_MASK : 0;
> +
> +		/* Fill with data regardless if read or write to simplify code */
> +		cmd->RegisterAddr = data[i];
> +	}
> +}
> +
> +static int arcturus_i2c_eeprom_read_data(struct i2c_adapter *control,
> +					       uint8_t address,
> +					       uint8_t *data,
> +					       uint32_t numbytes)
> +{
> +	uint32_t  i, ret = 0;
> +	SwI2cRequest_t req;
> +	struct amdgpu_device *adev = to_amdgpu_device(control);
> +	struct smu_table_context *smu_table = &adev->smu.smu_table;
> +	struct smu_table *table = &smu_table-
> >tables[SMU_TABLE_I2C_COMMANDS];
> +
> +	memset(&req, 0, sizeof(req));
> +	arcturus_fill_eeprom_i2c_req(&req, false, address, numbytes, data);
> +
> +	mutex_lock(&adev->smu.mutex);
> +	/* Now read data starting with that address */
> +	ret = smu_update_table(&adev->smu,
> SMU_TABLE_I2C_COMMANDS, 0, &req,
> +					true);
> +	mutex_unlock(&adev->smu.mutex);
> +
> +	if (!ret) {
> +		SwI2cRequest_t *res = (SwI2cRequest_t *)table->cpu_addr;
> +
> +		/* Assume SMU  fills res.SwI2cCmds[i].Data with read bytes
> */
> +		for (i = 0; i < numbytes; i++)
> +			data[i] = res->SwI2cCmds[i].Data;
> +
> +		pr_debug("arcturus_i2c_eeprom_read_data, address = %x,
> bytes = %d, data :",
> +				  (uint16_t)address, numbytes);
> +
> +		print_hex_dump(KERN_DEBUG, "data: ",
> DUMP_PREFIX_NONE,
> +			       8, 1, data, numbytes, false);
> +	} else
> +		pr_err("arcturus_i2c_eeprom_read_data - error occurred
> :%x", ret);
> +
> +	return ret;
> +}
> +
> +static int arcturus_i2c_eeprom_write_data(struct i2c_adapter *control,
> +						uint8_t address,
> +						uint8_t *data,
> +						uint32_t numbytes)
> +{
> +	uint32_t ret;
> +	SwI2cRequest_t req;
> +	struct amdgpu_device *adev = to_amdgpu_device(control);
> +
> +	memset(&req, 0, sizeof(req));
> +	arcturus_fill_eeprom_i2c_req(&req, true, address, numbytes, data);
> +
> +	mutex_lock(&adev->smu.mutex);
> +	ret = smu_update_table(&adev->smu,
> SMU_TABLE_I2C_COMMANDS, 0, &req, true);
> +	mutex_unlock(&adev->smu.mutex);
> +
> +	if (!ret) {
> +		pr_debug("arcturus_i2c_write(), address = %x, bytes = %d ,
> data: ",
> +					 (uint16_t)address, numbytes);
> +
> +		print_hex_dump(KERN_DEBUG, "data: ",
> DUMP_PREFIX_NONE,
> +			       8, 1, data, numbytes, false);
> +		/*
> +		 * According to EEPROM spec there is a MAX of 10 ms
> required for
> +		 * EEPROM to flush internal RX buffer after STOP was issued
> at the
> +		 * end of write transaction. During this time the EEPROM will
> not be
> +		 * responsive to any more commands - so wait a bit more.
> +		 */
> +		msleep(10);
> +
> +	} else
> +		pr_err("arcturus_i2c_write- error occurred :%x", ret);
> +
> +	return ret;
> +}
> +
> +static int arcturus_i2c_eeprom_i2c_xfer(struct i2c_adapter *i2c_adap,
> +			      struct i2c_msg *msgs, int num) {
> +	uint32_t  i, j, ret, data_size, data_chunk_size, next_eeprom_addr =
> 0;
> +	uint8_t *data_ptr, data_chunk[MAX_SW_I2C_COMMANDS] = { 0 };
> +
> +	for (i = 0; i < num; i++) {
> +		/*
> +		 * SMU interface allows at most MAX_SW_I2C_COMMANDS
> bytes of data at
> +		 * once and hence the data needs to be spliced into chunks
> and sent each
> +		 * chunk separately
> +		 */
> +		data_size = msgs[i].len - 2;
> +		data_chunk_size = MAX_SW_I2C_COMMANDS - 2;
> +		next_eeprom_addr = (msgs[i].buf[0] << 8 & 0xff00) |
> (msgs[i].buf[1] & 0xff);
> +		data_ptr = msgs[i].buf + 2;
> +
> +		for (j = 0; j < data_size / data_chunk_size; j++) {
> +			/* Insert the EEPROM dest addess, bits 0-15 */
> +			data_chunk[0] = ((next_eeprom_addr >> 8) & 0xff);
> +			data_chunk[1] = (next_eeprom_addr & 0xff);
> +
> +			if (msgs[i].flags & I2C_M_RD) {
> +				ret =
> arcturus_i2c_eeprom_read_data(i2c_adap,
> +
> 	(uint8_t)msgs[i].addr,
> +								data_chunk,
> MAX_SW_I2C_COMMANDS);
> +
> +				memcpy(data_ptr, data_chunk + 2,
> data_chunk_size);
> +			} else {
> +
> +				memcpy(data_chunk + 2, data_ptr,
> data_chunk_size);
> +
> +				ret =
> arcturus_i2c_eeprom_write_data(i2c_adap,
> +
> (uint8_t)msgs[i].addr,
> +								 data_chunk,
> MAX_SW_I2C_COMMANDS);
> +			}
> +
> +			if (ret) {
> +				num = -EIO;
> +				goto fail;
> +			}
> +
> +			next_eeprom_addr += data_chunk_size;
> +			data_ptr += data_chunk_size;
> +		}
> +
> +		if (data_size % data_chunk_size) {
> +			data_chunk[0] = ((next_eeprom_addr >> 8) & 0xff);
> +			data_chunk[1] = (next_eeprom_addr & 0xff);
> +
> +			if (msgs[i].flags & I2C_M_RD) {
> +				ret =
> arcturus_i2c_eeprom_read_data(i2c_adap,
> +
> 	(uint8_t)msgs[i].addr,
> +								data_chunk,
> (data_size % data_chunk_size) + 2);
> +
> +				memcpy(data_ptr, data_chunk + 2, data_size
> % data_chunk_size);
> +			} else {
> +				memcpy(data_chunk + 2, data_ptr, data_size
> % data_chunk_size);
> +
> +				ret =
> arcturus_i2c_eeprom_write_data(i2c_adap,
> +
> (uint8_t)msgs[i].addr,
> +								 data_chunk,
> (data_size % data_chunk_size) + 2);
> +			}
> +
> +			if (ret) {
> +				num = -EIO;
> +				goto fail;
> +			}
> +		}
> +	}
> +
> +fail:
> +	return num;
> +}
> +
> +static u32 arcturus_i2c_eeprom_i2c_func(struct i2c_adapter *adap) {
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; }
> +
> +
> +static const struct i2c_algorithm arcturus_i2c_eeprom_i2c_algo = {
> +	.master_xfer = arcturus_i2c_eeprom_i2c_xfer,
> +	.functionality = arcturus_i2c_eeprom_i2c_func, };
> +
> +int arcturus_i2c_eeprom_control_init(struct i2c_adapter *control) {
> +	struct amdgpu_device *adev = to_amdgpu_device(control);
> +	int res;
> +
> +	control->owner = THIS_MODULE;
> +	control->class = I2C_CLASS_SPD;
> +	control->dev.parent = &adev->pdev->dev;
> +	control->algo = &arcturus_i2c_eeprom_i2c_algo;
> +	snprintf(control->name, sizeof(control->name), "RAS EEPROM");
> +
> +	res = i2c_add_adapter(control);
> +	if (res)
> +		DRM_ERROR("Failed to register hw i2c, err: %d\n", res);
> +
> +	return res;
> +}
> +
> +void arcturus_i2c_eeprom_control_fini(struct i2c_adapter *control) {
> +	i2c_del_adapter(control);
> +}
> +
>  static const struct pptable_funcs arcturus_ppt_funcs = {
>  	/* translate smu index into arcturus specific index */
>  	.get_smu_msg_index = arcturus_get_smu_msg_index, @@ -1966,6
> +2193,8 @@ static const struct pptable_funcs arcturus_ppt_funcs = {
>  	.get_power_limit = arcturus_get_power_limit,
>  	.is_dpm_running = arcturus_is_dpm_running,
>  	.dpm_set_uvd_enable = arcturus_dpm_set_uvd_enable,
> +	.i2c_eeprom_init = arcturus_i2c_eeprom_control_init,
> +	.i2c_eeprom_fini = arcturus_i2c_eeprom_control_fini,
>  };
> 
>  void arcturus_set_ppt_funcs(struct smu_context *smu)
> --
> 2.7.4
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/4] drm/amdgpu: Move amdgpu_ras_recovery_init to after SMU ready.

[Grodzovsky, Andrey]

I am well aware that we want to do it ASAP, but what is your suggestion 
for the Arcturus use case where we have to do it AFTER SMU is up and 
running ? Do you want to call amdgpu_ras_recovery_init in two different 
places depending if this is Vega 20 or Arcturus ? This will over 
complicate an already complicated init sequence of RAS.

Andrey

On 10/20/19 9:51 PM, Chen, Guchun wrote:
> I don't think postpone RAS recovery init is not one reasonable proposal. What we do in recovery init is to read the retired page if there is, and retire corresponding memory, this will make sure these pages are reserved well before setting up memory manager and reserving other memory blocks, it will be safe.
>
> Regards,
> Guchun
>
> -----Original Message-----
> From: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> Sent: Saturday, October 19, 2019 4:49 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Chen, Guchun <Guchun.Chen@amd.com>; Zhou1, Tao <Tao.Zhou1@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; noreply-confluence@amd.com; Quan, Evan <Evan.Quan@amd.com>; Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
> Subject: [PATCH 4/4] drm/amdgpu: Move amdgpu_ras_recovery_init to after SMU ready.
>
> For Arcturus the I2C traffic is done through SMU tables and so we must postpone RAS recovery init to after they are ready which is in amdgpu_device_ip_hw_init_phase2.
>
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 13 +++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c    | 11 -----------
>   2 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 17cfdaf..c40e9a5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1850,6 +1850,19 @@ static int amdgpu_device_ip_init(struct amdgpu_device *adev)
>   	if (r)
>   		goto init_failed;
>   
> +	/*
> +	 * retired pages will be loaded from eeprom and reserved here,
> +	 * it should be called after amdgpu_device_ip_hw_init_phase2  since
> +	 * for some ASICs the RAS EEPROM code relies on SMU fully functioning
> +	 * for I2C communication which only true at this point.
> +	 * recovery_init may fail, but it can free all resources allocated by
> +	 * itself and its failure should not stop amdgpu init process.
> +	 *
> +	 * Note: theoretically, this should be called before all vram allocations
> +	 * to protect retired page from abusing
> +	 */
> +	amdgpu_ras_recovery_init(adev);
> +
>   	if (adev->gmc.xgmi.num_physical_nodes > 1)
>   		amdgpu_xgmi_add_device(adev);
>   	amdgpu_amdkfd_device_init(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 2e85a51..1045c3f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1721,17 +1721,6 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)  #endif
>   
>   	/*
> -	 * retired pages will be loaded from eeprom and reserved here,
> -	 * it should be called after ttm init since new bo may be created,
> -	 * recovery_init may fail, but it can free all resources allocated by
> -	 * itself and its failure should not stop amdgpu init process.
> -	 *
> -	 * Note: theoretically, this should be called before all vram allocations
> -	 * to protect retired page from abusing
> -	 */
> -	amdgpu_ras_recovery_init(adev);
> -
> -	/*
>   	 *The reserved vram for firmware must be pinned to the specified
>   	 *place on the VRAM, so reserve it early.
>   	 */
> --
> 2.7.4
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/4] drm/amd/powerplay: Add EEPROM I2C read/write support to Arcturus.

[Grodzovsky, Andrey]

On 10/20/19 9:44 PM, Chen, Guchun wrote:
> Comment inline.
>
>
> Regards,
> Guchun
>
> -----Original Message-----
> From: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> Sent: Saturday, October 19, 2019 4:48 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Chen, Guchun <Guchun.Chen@amd.com>; Zhou1, Tao <Tao.Zhou1@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; noreply-confluence@amd.com; Quan, Evan <Evan.Quan@amd.com>; Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
> Subject: [PATCH 2/4] drm/amd/powerplay: Add EEPROM I2C read/write support to Arcturus.
>
> The communication is done through SMU table and hence the code is in powerplay.
>
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>   drivers/gpu/drm/amd/powerplay/arcturus_ppt.c | 229 +++++++++++++++++++++++++++
>   1 file changed, 229 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> index 90d871a..53d08de5 100644
> --- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> +++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> @@ -36,6 +36,11 @@
>   #include "smu_v11_0_pptable.h"
>   #include "arcturus_ppsmc.h"
>   #include "nbio/nbio_7_4_sh_mask.h"
> +#include <linux/i2c.h>
> +#include <linux/pci.h>
> +#include "amdgpu_ras.h"
> +
> +#define to_amdgpu_device(x) (container_of(x, struct amdgpu_ras,
> +eeprom_control.eeprom_accessor))->adev
>   
>   #define CTF_OFFSET_EDGE			5
>   #define CTF_OFFSET_HOTSPOT		5
> @@ -171,6 +176,7 @@ static struct smu_11_0_cmn2aisc_mapping arcturus_table_map[SMU_TABLE_COUNT] = {
>   	TAB_MAP(SMU_METRICS),
>   	TAB_MAP(DRIVER_SMU_CONFIG),
>   	TAB_MAP(OVERDRIVE),
> +	TAB_MAP(I2C_COMMANDS),
>   };
>   
>   static struct smu_11_0_cmn2aisc_mapping arcturus_pwr_src_map[SMU_POWER_SOURCE_COUNT] = { @@ -293,6 +299,9 @@ static int arcturus_tables_init(struct smu_context *smu, struct smu_table *table
>   	SMU_TABLE_INIT(tables, SMU_TABLE_SMU_METRICS, sizeof(SmuMetrics_t),
>   		       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
>   
> +	SMU_TABLE_INIT(tables, SMU_TABLE_I2C_COMMANDS, sizeof(SwI2cRequest_t),
> +			       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
> +
>   	smu_table->metrics_table = kzalloc(sizeof(SmuMetrics_t), GFP_KERNEL);
>   	if (!smu_table->metrics_table)
>   		return -ENOMEM;
> @@ -1927,6 +1936,224 @@ static int arcturus_dpm_set_uvd_enable(struct smu_context *smu, bool enable)
>   	return ret;
>   }
>   
> +
> +static void arcturus_fill_eeprom_i2c_req(SwI2cRequest_t  *req, bool write,
> +				  uint8_t address, uint32_t numbytes,
> +				  uint8_t *data)
> +{
> +	int i;
> +
> +	BUG_ON(numbytes > MAX_SW_I2C_COMMANDS);
> +
> +	req->I2CcontrollerPort = 0;
> +	req->I2CSpeed = 2;
> +	req->SlaveAddress = address;
> +	req->NumCmds = numbytes;
> +
> +	for (i = 0; i < numbytes; i++) {
> +		SwI2cCmd_t *cmd =  &req->SwI2cCmds[i];
> +
> +		/* First 2 bytes are always write for lower 2b EEPROM address */
> +		if (i < 2)
> +			cmd->Cmd = 1;
> +		else
> +			cmd->Cmd = write;
> +
> +
> +		/* Add RESTART for read  after address filled */
> +		cmd->CmdConfig |= (i == 2 && !write) ? CMDCONFIG_RESTART_MASK : 0;
> +
> +		/* Add STOP in the end */
> +		cmd->CmdConfig |= (i == (numbytes - 1)) ? CMDCONFIG_STOP_MASK : 0;
> +
> +		/* Fill with data regardless if read or write to simplify code */
> +		cmd->RegisterAddr = data[i];
> +	}
> +}
> +
> +static int arcturus_i2c_eeprom_read_data(struct i2c_adapter *control,
> +					       uint8_t address,
> +					       uint8_t *data,
> +					       uint32_t numbytes)
> +{
> +	uint32_t  i, ret = 0;
> +	SwI2cRequest_t req;
> +	struct amdgpu_device *adev = to_amdgpu_device(control);
> +	struct smu_table_context *smu_table = &adev->smu.smu_table;
> +	struct smu_table *table = &smu_table->tables[SMU_TABLE_I2C_COMMANDS];
> +
> +	memset(&req, 0, sizeof(req));
> +	arcturus_fill_eeprom_i2c_req(&req, false, address, numbytes, data);
> +
> +	mutex_lock(&adev->smu.mutex);
> +	/* Now read data starting with that address */
> +	ret = smu_update_table(&adev->smu, SMU_TABLE_I2C_COMMANDS, 0, &req,
> +					true);
> +	mutex_unlock(&adev->smu.mutex);
> +
> +	if (!ret) {
> +		SwI2cRequest_t *res = (SwI2cRequest_t *)table->cpu_addr;
> +
> +		/* Assume SMU  fills res.SwI2cCmds[i].Data with read bytes */
> +		for (i = 0; i < numbytes; i++)
> +			data[i] = res->SwI2cCmds[i].Data;
> +
> +		pr_debug("arcturus_i2c_eeprom_read_data, address = %x, bytes = %d, data :",
> +				  (uint16_t)address, numbytes);
> +
> +		print_hex_dump(KERN_DEBUG, "data: ", DUMP_PREFIX_NONE,
> +			       8, 1, data, numbytes, false);
> +	} else
> +		pr_err("arcturus_i2c_eeprom_read_data - error occurred :%x", ret);
> +
> +	return ret;
> +}
> +
> +static int arcturus_i2c_eeprom_write_data(struct i2c_adapter *control,
> +						uint8_t address,
> +						uint8_t *data,
> +						uint32_t numbytes)
> +{
> +	uint32_t ret;
> +	SwI2cRequest_t req;
> +	struct amdgpu_device *adev = to_amdgpu_device(control);
> +
> +	memset(&req, 0, sizeof(req));
> +	arcturus_fill_eeprom_i2c_req(&req, true, address, numbytes, data);
> +
> +	mutex_lock(&adev->smu.mutex);
> +	ret = smu_update_table(&adev->smu, SMU_TABLE_I2C_COMMANDS, 0, &req, true);
> +	mutex_unlock(&adev->smu.mutex);
> +
> +	if (!ret) {
> +		pr_debug("arcturus_i2c_write(), address = %x, bytes = %d , data: ",
> +					 (uint16_t)address, numbytes);
> +
> +		print_hex_dump(KERN_DEBUG, "data: ", DUMP_PREFIX_NONE,
> +			       8, 1, data, numbytes, false);
> +		/*
> +		 * According to EEPROM spec there is a MAX of 10 ms required for
> +		 * EEPROM to flush internal RX buffer after STOP was issued at the
> +		 * end of write transaction. During this time the EEPROM will not be
> +		 * responsive to any more commands - so wait a bit more.
> +		 */
> +		msleep(10);
> +
> +	} else
> +		pr_err("arcturus_i2c_write- error occurred :%x", ret);
> +
> +	return ret;
> +}
> +
> +static int arcturus_i2c_eeprom_i2c_xfer(struct i2c_adapter *i2c_adap,
> +			      struct i2c_msg *msgs, int num) {
> +	uint32_t  i, j, ret, data_size, data_chunk_size, next_eeprom_addr = 0;
> +	uint8_t *data_ptr, data_chunk[MAX_SW_I2C_COMMANDS] = { 0 };
> +
> +	for (i = 0; i < num; i++) {
> +		/*
> +		 * SMU interface allows at most MAX_SW_I2C_COMMANDS bytes of data at
> +		 * once and hence the data needs to be spliced into chunks and sent each
> +		 * chunk separately
> +		 */
> +		data_size = msgs[i].len - 2;
> +		data_chunk_size = MAX_SW_I2C_COMMANDS - 2;
> +		next_eeprom_addr = (msgs[i].buf[0] << 8 & 0xff00) | (msgs[i].buf[1] & 0xff);
> +		data_ptr = msgs[i].buf + 2;
> +
> +		for (j = 0; j < data_size / data_chunk_size; j++) {
> +			/* Insert the EEPROM dest addess, bits 0-15 */
> +			data_chunk[0] = ((next_eeprom_addr >> 8) & 0xff);
> +			data_chunk[1] = (next_eeprom_addr & 0xff);
> +
> +			if (msgs[i].flags & I2C_M_RD) {
> +				ret = arcturus_i2c_eeprom_read_data(i2c_adap,
> +								(uint8_t)msgs[i].addr,
> +								data_chunk, MAX_SW_I2C_COMMANDS);
> +
> +				memcpy(data_ptr, data_chunk + 2, data_chunk_size);
> +			} else {
> +
> +				memcpy(data_chunk + 2, data_ptr, data_chunk_size);
> +
> +				ret = arcturus_i2c_eeprom_write_data(i2c_adap,
> +								 (uint8_t)msgs[i].addr,
> +								 data_chunk, MAX_SW_I2C_COMMANDS);
> +			}
> +
> +			if (ret) {
> +				num = -EIO;
> +				goto fail;
> +			}
> +
> +			next_eeprom_addr += data_chunk_size;
> +			data_ptr += data_chunk_size;
> +		}
> +
> +		if (data_size % data_chunk_size) {
> +			data_chunk[0] = ((next_eeprom_addr >> 8) & 0xff);
> +			data_chunk[1] = (next_eeprom_addr & 0xff);
> +
> +			if (msgs[i].flags & I2C_M_RD) {
> +				ret = arcturus_i2c_eeprom_read_data(i2c_adap,
> +								(uint8_t)msgs[i].addr,
> +								data_chunk, (data_size % data_chunk_size) + 2);
> +
> +				memcpy(data_ptr, data_chunk + 2, data_size % data_chunk_size);
> +			} else {
> +				memcpy(data_chunk + 2, data_ptr, data_size % data_chunk_size);
> +
> +				ret = arcturus_i2c_eeprom_write_data(i2c_adap,
> +								 (uint8_t)msgs[i].addr,
> +								 data_chunk, (data_size % data_chunk_size) + 2);
> +			}
> +
> +			if (ret) {
> +				num = -EIO;
> +				goto fail;
> +			}
> +		}
> +	}
> +
> +fail:
> +	return num;
> +}
> +
> +static u32 arcturus_i2c_eeprom_i2c_func(struct i2c_adapter *adap) {
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; }
> +
> +
> +static const struct i2c_algorithm arcturus_i2c_eeprom_i2c_algo = {
> +	.master_xfer = arcturus_i2c_eeprom_i2c_xfer,
> +	.functionality = arcturus_i2c_eeprom_i2c_func,
> +};
> +
> +int arcturus_i2c_eeprom_control_init(struct i2c_adapter *control)
> +{
> +	struct amdgpu_device *adev = to_amdgpu_device(control);
> +	int res;
> +
> +	control->owner = THIS_MODULE;
> +	control->class = I2C_CLASS_SPD;
> +	control->dev.parent = &adev->pdev->dev;
> +	control->algo = &arcturus_i2c_eeprom_i2c_algo;
> +	snprintf(control->name, sizeof(control->name), "RAS EEPROM");
> +
> +	res = i2c_add_adapter(control);
> +	if (res)
> +		DRM_ERROR("Failed to register hw i2c, err: %d\n", res);
> +
> +	return res;
> +}
> [Guchun]Can we move this register code to one common file? As I saw on vega20, we have the same code smu_v11_0_i2c_eeprom_control_init to register i2c device.
> If we can move such code to a common file, then in each asic's i2c code, what we should do is to pass necessary ptrs/name/ops to that common file, and from there, we will register i2c device.
> This can help clean some code, whatever, maybe convenient to other asics if we want to do the same i2c adding job.
>
> +void arcturus_i2c_eeprom_control_fini(struct i2c_adapter *control)
> +{
> +	i2c_del_adapter(control);
> +}
> +
> [Guchun]The same as above.

This will be very small duplication reduction but I will look into 
putting this inside amdgpu_ras_eeprom.c

Andrey

>
>   static const struct pptable_funcs arcturus_ppt_funcs = {
>   	/* translate smu index into arcturus specific index */
>   	.get_smu_msg_index = arcturus_get_smu_msg_index,
> @@ -1966,6 +2193,8 @@ static const struct pptable_funcs arcturus_ppt_funcs = {
>   	.get_power_limit = arcturus_get_power_limit,
>   	.is_dpm_running = arcturus_is_dpm_running,
>   	.dpm_set_uvd_enable = arcturus_dpm_set_uvd_enable,
> +	.i2c_eeprom_init = arcturus_i2c_eeprom_control_init,
> +	.i2c_eeprom_fini = arcturus_i2c_eeprom_control_fini,
>   };
>   
>   void arcturus_set_ppt_funcs(struct smu_context *smu)
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/4] drm/amd/powerplay: Add EEPROM I2C read/write support to Arcturus.

[Grodzovsky, Andrey]

On 10/22/19 10:58 AM, Andrey Grodzovsky wrote:
>
> On 10/20/19 9:44 PM, Chen, Guchun wrote:
>> Comment inline.
>>
>>
>> Regards,
>> Guchun
>>
>> -----Original Message-----
>> From: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>> Sent: Saturday, October 19, 2019 4:48 AM
>> To: amd-gfx@lists.freedesktop.org
>> Cc: Chen, Guchun <Guchun.Chen@amd.com>; Zhou1, Tao 
>> <Tao.Zhou1@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; 
>> noreply-confluence@amd.com; Quan, Evan <Evan.Quan@amd.com>; 
>> Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
>> Subject: [PATCH 2/4] drm/amd/powerplay: Add EEPROM I2C read/write 
>> support to Arcturus.
>>
>> The communication is done through SMU table and hence the code is in 
>> powerplay.
>>
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>> ---
>>   drivers/gpu/drm/amd/powerplay/arcturus_ppt.c | 229 
>> +++++++++++++++++++++++++++
>>   1 file changed, 229 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c 
>> b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
>> index 90d871a..53d08de5 100644
>> --- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
>> +++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
>> @@ -36,6 +36,11 @@
>>   #include "smu_v11_0_pptable.h"
>>   #include "arcturus_ppsmc.h"
>>   #include "nbio/nbio_7_4_sh_mask.h"
>> +#include <linux/i2c.h>
>> +#include <linux/pci.h>
>> +#include "amdgpu_ras.h"
>> +
>> +#define to_amdgpu_device(x) (container_of(x, struct amdgpu_ras,
>> +eeprom_control.eeprom_accessor))->adev
>>     #define CTF_OFFSET_EDGE            5
>>   #define CTF_OFFSET_HOTSPOT        5
>> @@ -171,6 +176,7 @@ static struct smu_11_0_cmn2aisc_mapping 
>> arcturus_table_map[SMU_TABLE_COUNT] = {
>>       TAB_MAP(SMU_METRICS),
>>       TAB_MAP(DRIVER_SMU_CONFIG),
>>       TAB_MAP(OVERDRIVE),
>> +    TAB_MAP(I2C_COMMANDS),
>>   };
>>     static struct smu_11_0_cmn2aisc_mapping 
>> arcturus_pwr_src_map[SMU_POWER_SOURCE_COUNT] = { @@ -293,6 +299,9 @@ 
>> static int arcturus_tables_init(struct smu_context *smu, struct 
>> smu_table *table
>>       SMU_TABLE_INIT(tables, SMU_TABLE_SMU_METRICS, 
>> sizeof(SmuMetrics_t),
>>                  PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
>>   +    SMU_TABLE_INIT(tables, SMU_TABLE_I2C_COMMANDS, 
>> sizeof(SwI2cRequest_t),
>> +                   PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
>> +
>>       smu_table->metrics_table = kzalloc(sizeof(SmuMetrics_t), 
>> GFP_KERNEL);
>>       if (!smu_table->metrics_table)
>>           return -ENOMEM;
>> @@ -1927,6 +1936,224 @@ static int arcturus_dpm_set_uvd_enable(struct 
>> smu_context *smu, bool enable)
>>       return ret;
>>   }
>>   +
>> +static void arcturus_fill_eeprom_i2c_req(SwI2cRequest_t  *req, bool 
>> write,
>> +                  uint8_t address, uint32_t numbytes,
>> +                  uint8_t *data)
>> +{
>> +    int i;
>> +
>> +    BUG_ON(numbytes > MAX_SW_I2C_COMMANDS);
>> +
>> +    req->I2CcontrollerPort = 0;
>> +    req->I2CSpeed = 2;
>> +    req->SlaveAddress = address;
>> +    req->NumCmds = numbytes;
>> +
>> +    for (i = 0; i < numbytes; i++) {
>> +        SwI2cCmd_t *cmd =  &req->SwI2cCmds[i];
>> +
>> +        /* First 2 bytes are always write for lower 2b EEPROM 
>> address */
>> +        if (i < 2)
>> +            cmd->Cmd = 1;
>> +        else
>> +            cmd->Cmd = write;
>> +
>> +
>> +        /* Add RESTART for read  after address filled */
>> +        cmd->CmdConfig |= (i == 2 && !write) ? 
>> CMDCONFIG_RESTART_MASK : 0;
>> +
>> +        /* Add STOP in the end */
>> +        cmd->CmdConfig |= (i == (numbytes - 1)) ? 
>> CMDCONFIG_STOP_MASK : 0;
>> +
>> +        /* Fill with data regardless if read or write to simplify 
>> code */
>> +        cmd->RegisterAddr = data[i];
>> +    }
>> +}
>> +
>> +static int arcturus_i2c_eeprom_read_data(struct i2c_adapter *control,
>> +                           uint8_t address,
>> +                           uint8_t *data,
>> +                           uint32_t numbytes)
>> +{
>> +    uint32_t  i, ret = 0;
>> +    SwI2cRequest_t req;
>> +    struct amdgpu_device *adev = to_amdgpu_device(control);
>> +    struct smu_table_context *smu_table = &adev->smu.smu_table;
>> +    struct smu_table *table = 
>> &smu_table->tables[SMU_TABLE_I2C_COMMANDS];
>> +
>> +    memset(&req, 0, sizeof(req));
>> +    arcturus_fill_eeprom_i2c_req(&req, false, address, numbytes, data);
>> +
>> +    mutex_lock(&adev->smu.mutex);
>> +    /* Now read data starting with that address */
>> +    ret = smu_update_table(&adev->smu, SMU_TABLE_I2C_COMMANDS, 0, &req,
>> +                    true);
>> +    mutex_unlock(&adev->smu.mutex);
>> +
>> +    if (!ret) {
>> +        SwI2cRequest_t *res = (SwI2cRequest_t *)table->cpu_addr;
>> +
>> +        /* Assume SMU  fills res.SwI2cCmds[i].Data with read bytes */
>> +        for (i = 0; i < numbytes; i++)
>> +            data[i] = res->SwI2cCmds[i].Data;
>> +
>> +        pr_debug("arcturus_i2c_eeprom_read_data, address = %x, bytes 
>> = %d, data :",
>> +                  (uint16_t)address, numbytes);
>> +
>> +        print_hex_dump(KERN_DEBUG, "data: ", DUMP_PREFIX_NONE,
>> +                   8, 1, data, numbytes, false);
>> +    } else
>> +        pr_err("arcturus_i2c_eeprom_read_data - error occurred :%x", 
>> ret);
>> +
>> +    return ret;
>> +}
>> +
>> +static int arcturus_i2c_eeprom_write_data(struct i2c_adapter *control,
>> +                        uint8_t address,
>> +                        uint8_t *data,
>> +                        uint32_t numbytes)
>> +{
>> +    uint32_t ret;
>> +    SwI2cRequest_t req;
>> +    struct amdgpu_device *adev = to_amdgpu_device(control);
>> +
>> +    memset(&req, 0, sizeof(req));
>> +    arcturus_fill_eeprom_i2c_req(&req, true, address, numbytes, data);
>> +
>> +    mutex_lock(&adev->smu.mutex);
>> +    ret = smu_update_table(&adev->smu, SMU_TABLE_I2C_COMMANDS, 0, 
>> &req, true);
>> +    mutex_unlock(&adev->smu.mutex);
>> +
>> +    if (!ret) {
>> +        pr_debug("arcturus_i2c_write(), address = %x, bytes = %d , 
>> data: ",
>> +                     (uint16_t)address, numbytes);
>> +
>> +        print_hex_dump(KERN_DEBUG, "data: ", DUMP_PREFIX_NONE,
>> +                   8, 1, data, numbytes, false);
>> +        /*
>> +         * According to EEPROM spec there is a MAX of 10 ms required 
>> for
>> +         * EEPROM to flush internal RX buffer after STOP was issued 
>> at the
>> +         * end of write transaction. During this time the EEPROM 
>> will not be
>> +         * responsive to any more commands - so wait a bit more.
>> +         */
>> +        msleep(10);
>> +
>> +    } else
>> +        pr_err("arcturus_i2c_write- error occurred :%x", ret);
>> +
>> +    return ret;
>> +}
>> +
>> +static int arcturus_i2c_eeprom_i2c_xfer(struct i2c_adapter *i2c_adap,
>> +                  struct i2c_msg *msgs, int num) {
>> +    uint32_t  i, j, ret, data_size, data_chunk_size, 
>> next_eeprom_addr = 0;
>> +    uint8_t *data_ptr, data_chunk[MAX_SW_I2C_COMMANDS] = { 0 };
>> +
>> +    for (i = 0; i < num; i++) {
>> +        /*
>> +         * SMU interface allows at most MAX_SW_I2C_COMMANDS bytes of 
>> data at
>> +         * once and hence the data needs to be spliced into chunks 
>> and sent each
>> +         * chunk separately
>> +         */
>> +        data_size = msgs[i].len - 2;
>> +        data_chunk_size = MAX_SW_I2C_COMMANDS - 2;
>> +        next_eeprom_addr = (msgs[i].buf[0] << 8 & 0xff00) | 
>> (msgs[i].buf[1] & 0xff);
>> +        data_ptr = msgs[i].buf + 2;
>> +
>> +        for (j = 0; j < data_size / data_chunk_size; j++) {
>> +            /* Insert the EEPROM dest addess, bits 0-15 */
>> +            data_chunk[0] = ((next_eeprom_addr >> 8) & 0xff);
>> +            data_chunk[1] = (next_eeprom_addr & 0xff);
>> +
>> +            if (msgs[i].flags & I2C_M_RD) {
>> +                ret = arcturus_i2c_eeprom_read_data(i2c_adap,
>> +                                (uint8_t)msgs[i].addr,
>> +                                data_chunk, MAX_SW_I2C_COMMANDS);
>> +
>> +                memcpy(data_ptr, data_chunk + 2, data_chunk_size);
>> +            } else {
>> +
>> +                memcpy(data_chunk + 2, data_ptr, data_chunk_size);
>> +
>> +                ret = arcturus_i2c_eeprom_write_data(i2c_adap,
>> +                                 (uint8_t)msgs[i].addr,
>> +                                 data_chunk, MAX_SW_I2C_COMMANDS);
>> +            }
>> +
>> +            if (ret) {
>> +                num = -EIO;
>> +                goto fail;
>> +            }
>> +
>> +            next_eeprom_addr += data_chunk_size;
>> +            data_ptr += data_chunk_size;
>> +        }
>> +
>> +        if (data_size % data_chunk_size) {
>> +            data_chunk[0] = ((next_eeprom_addr >> 8) & 0xff);
>> +            data_chunk[1] = (next_eeprom_addr & 0xff);
>> +
>> +            if (msgs[i].flags & I2C_M_RD) {
>> +                ret = arcturus_i2c_eeprom_read_data(i2c_adap,
>> +                                (uint8_t)msgs[i].addr,
>> +                                data_chunk, (data_size % 
>> data_chunk_size) + 2);
>> +
>> +                memcpy(data_ptr, data_chunk + 2, data_size % 
>> data_chunk_size);
>> +            } else {
>> +                memcpy(data_chunk + 2, data_ptr, data_size % 
>> data_chunk_size);
>> +
>> +                ret = arcturus_i2c_eeprom_write_data(i2c_adap,
>> +                                 (uint8_t)msgs[i].addr,
>> +                                 data_chunk, (data_size % 
>> data_chunk_size) + 2);
>> +            }
>> +
>> +            if (ret) {
>> +                num = -EIO;
>> +                goto fail;
>> +            }
>> +        }
>> +    }
>> +
>> +fail:
>> +    return num;
>> +}
>> +
>> +static u32 arcturus_i2c_eeprom_i2c_func(struct i2c_adapter *adap) {
>> +    return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; }
>> +
>> +
>> +static const struct i2c_algorithm arcturus_i2c_eeprom_i2c_algo = {
>> +    .master_xfer = arcturus_i2c_eeprom_i2c_xfer,
>> +    .functionality = arcturus_i2c_eeprom_i2c_func,
>> +};
>> +
>> +int arcturus_i2c_eeprom_control_init(struct i2c_adapter *control)
>> +{
>> +    struct amdgpu_device *adev = to_amdgpu_device(control);
>> +    int res;
>> +
>> +    control->owner = THIS_MODULE;
>> +    control->class = I2C_CLASS_SPD;
>> +    control->dev.parent = &adev->pdev->dev;
>> +    control->algo = &arcturus_i2c_eeprom_i2c_algo;
>> +    snprintf(control->name, sizeof(control->name), "RAS EEPROM");
>> +
>> +    res = i2c_add_adapter(control);
>> +    if (res)
>> +        DRM_ERROR("Failed to register hw i2c, err: %d\n", res);
>> +
>> +    return res;
>> +}
>> [Guchun]Can we move this register code to one common file? As I saw 
>> on vega20, we have the same code smu_v11_0_i2c_eeprom_control_init to 
>> register i2c device.
>> If we can move such code to a common file, then in each asic's i2c 
>> code, what we should do is to pass necessary ptrs/name/ops to that 
>> common file, and from there, we will register i2c device.
>> This can help clean some code, whatever, maybe convenient to other 
>> asics if we want to do the same i2c adding job.
>>
>> +void arcturus_i2c_eeprom_control_fini(struct i2c_adapter *control)
>> +{
>> +    i2c_del_adapter(control);
>> +}
>> +
>> [Guchun]The same as above.
>
> This will be very small duplication reduction but I will look into 
> putting this inside amdgpu_ras_eeprom.c
>
> Andrey


I took a look and i honestly don't see a point now in this as i save 
only a few lines of code because arcturus_i2c_eeprom_i2c_algo or 
smu_v11_0_i2c_eeprom_i2c_xfer are ASIC specific and so it boils down to 
a few assignments and calls to i2c_add_adapter and i2c_del_adapter. On 
the other hand i need to change the SMU interface and 
smu_v11_0_i2c_eeprom header to return me something like a pointer to 
i2c_algorithm.functionality which doesn't seem intuitive to me.

Andrey


>
>>
>>   static const struct pptable_funcs arcturus_ppt_funcs = {
>>       /* translate smu index into arcturus specific index */
>>       .get_smu_msg_index = arcturus_get_smu_msg_index,
>> @@ -1966,6 +2193,8 @@ static const struct pptable_funcs 
>> arcturus_ppt_funcs = {
>>       .get_power_limit = arcturus_get_power_limit,
>>       .is_dpm_running = arcturus_is_dpm_running,
>>       .dpm_set_uvd_enable = arcturus_dpm_set_uvd_enable,
>> +    .i2c_eeprom_init = arcturus_i2c_eeprom_control_init,
>> +    .i2c_eeprom_fini = arcturus_i2c_eeprom_control_fini,
>>   };
>>     void arcturus_set_ppt_funcs(struct smu_context *smu)
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 4/4] drm/amdgpu: Move amdgpu_ras_recovery_init to after SMU ready.

[Chen, Guchun]

Hi Andrey,

No, I don't want to see amdgpu_ras_recovery_init being called at different places.
If calling amdgpu_ras_recovery_init as much early as we can is not doable, due to arcturus i2c code ready timeline, I am fine with this patch.

Regards,
Guchun

-----Original Message-----
From: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com> 
Sent: Tuesday, October 22, 2019 10:33 PM
To: Chen, Guchun <Guchun.Chen@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Zhou1, Tao <Tao.Zhou1@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; noreply-confluence@amd.com; Quan, Evan <Evan.Quan@amd.com>
Subject: Re: [PATCH 4/4] drm/amdgpu: Move amdgpu_ras_recovery_init to after SMU ready.

I am well aware that we want to do it ASAP, but what is your suggestion for the Arcturus use case where we have to do it AFTER SMU is up and running ? Do you want to call amdgpu_ras_recovery_init in two different places depending if this is Vega 20 or Arcturus ? This will over complicate an already complicated init sequence of RAS.

Andrey

On 10/20/19 9:51 PM, Chen, Guchun wrote:
> I don't think postpone RAS recovery init is not one reasonable proposal. What we do in recovery init is to read the retired page if there is, and retire corresponding memory, this will make sure these pages are reserved well before setting up memory manager and reserving other memory blocks, it will be safe.
>
> Regards,
> Guchun
>
> -----Original Message-----
> From: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> Sent: Saturday, October 19, 2019 4:49 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Chen, Guchun <Guchun.Chen@amd.com>; Zhou1, Tao 
> <Tao.Zhou1@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; 
> noreply-confluence@amd.com; Quan, Evan <Evan.Quan@amd.com>; 
> Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
> Subject: [PATCH 4/4] drm/amdgpu: Move amdgpu_ras_recovery_init to after SMU ready.
>
> For Arcturus the I2C traffic is done through SMU tables and so we must postpone RAS recovery init to after they are ready which is in amdgpu_device_ip_hw_init_phase2.
>
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 13 +++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c    | 11 -----------
>   2 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 17cfdaf..c40e9a5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1850,6 +1850,19 @@ static int amdgpu_device_ip_init(struct amdgpu_device *adev)
>   	if (r)
>   		goto init_failed;
>   
> +	/*
> +	 * retired pages will be loaded from eeprom and reserved here,
> +	 * it should be called after amdgpu_device_ip_hw_init_phase2  since
> +	 * for some ASICs the RAS EEPROM code relies on SMU fully functioning
> +	 * for I2C communication which only true at this point.
> +	 * recovery_init may fail, but it can free all resources allocated by
> +	 * itself and its failure should not stop amdgpu init process.
> +	 *
> +	 * Note: theoretically, this should be called before all vram allocations
> +	 * to protect retired page from abusing
> +	 */
> +	amdgpu_ras_recovery_init(adev);
> +
>   	if (adev->gmc.xgmi.num_physical_nodes > 1)
>   		amdgpu_xgmi_add_device(adev);
>   	amdgpu_amdkfd_device_init(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 2e85a51..1045c3f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1721,17 +1721,6 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)  
> #endif
>   
>   	/*
> -	 * retired pages will be loaded from eeprom and reserved here,
> -	 * it should be called after ttm init since new bo may be created,
> -	 * recovery_init may fail, but it can free all resources allocated by
> -	 * itself and its failure should not stop amdgpu init process.
> -	 *
> -	 * Note: theoretically, this should be called before all vram allocations
> -	 * to protect retired page from abusing
> -	 */
> -	amdgpu_ras_recovery_init(adev);
> -
> -	/*
>   	 *The reserved vram for firmware must be pinned to the specified
>   	 *place on the VRAM, so reserve it early.
>   	 */
> --
> 2.7.4
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 2/4] drm/amd/powerplay: Add EEPROM I2C read/write support to Arcturus.

[Chen, Guchun]

> -----Original Message-----
> From: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
> Sent: Wednesday, October 23, 2019 3:10 AM
> To: Chen, Guchun <Guchun.Chen@amd.com>; amd-
> gfx@lists.freedesktop.org
> Cc: Zhou1, Tao <Tao.Zhou1@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; noreply-confluence@amd.com; Quan,
> Evan <Evan.Quan@amd.com>
> Subject: Re: [PATCH 2/4] drm/amd/powerplay: Add EEPROM I2C read/write
> support to Arcturus.
> 
> 
> On 10/22/19 10:58 AM, Andrey Grodzovsky wrote:
> >
> > On 10/20/19 9:44 PM, Chen, Guchun wrote:
> >> Comment inline.
> >>
> >>
> >> Regards,
> >> Guchun
> >>
> >> -----Original Message-----
> >> From: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> >> Sent: Saturday, October 19, 2019 4:48 AM
> >> To: amd-gfx@lists.freedesktop.org
> >> Cc: Chen, Guchun <Guchun.Chen@amd.com>; Zhou1, Tao
> >> <Tao.Zhou1@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>;
> >> noreply-confluence@amd.com; Quan, Evan <Evan.Quan@amd.com>;
> >> Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>
> >> Subject: [PATCH 2/4] drm/amd/powerplay: Add EEPROM I2C read/write
> >> support to Arcturus.
> >>
> >> The communication is done through SMU table and hence the code is in
> >> powerplay.
> >>
> >> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> >> ---
> >>   drivers/gpu/drm/amd/powerplay/arcturus_ppt.c | 229
> >> +++++++++++++++++++++++++++
> >>   1 file changed, 229 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> >> b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> >> index 90d871a..53d08de5 100644
> >> --- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> >> +++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> >> @@ -36,6 +36,11 @@
> >>   #include "smu_v11_0_pptable.h"
> >>   #include "arcturus_ppsmc.h"
> >>   #include "nbio/nbio_7_4_sh_mask.h"
> >> +#include <linux/i2c.h>
> >> +#include <linux/pci.h>
> >> +#include "amdgpu_ras.h"
> >> +
> >> +#define to_amdgpu_device(x) (container_of(x, struct amdgpu_ras,
> >> +eeprom_control.eeprom_accessor))->adev
> >>     #define CTF_OFFSET_EDGE            5
> >>   #define CTF_OFFSET_HOTSPOT        5 @@ -171,6 +176,7 @@ static
> >> struct smu_11_0_cmn2aisc_mapping
> arcturus_table_map[SMU_TABLE_COUNT]
> >> = {
> >>       TAB_MAP(SMU_METRICS),
> >>       TAB_MAP(DRIVER_SMU_CONFIG),
> >>       TAB_MAP(OVERDRIVE),
> >> +    TAB_MAP(I2C_COMMANDS),
> >>   };
> >>     static struct smu_11_0_cmn2aisc_mapping
> >> arcturus_pwr_src_map[SMU_POWER_SOURCE_COUNT] = { @@ -293,6
> +299,9 @@
> >> static int arcturus_tables_init(struct smu_context *smu, struct
> >> smu_table *table
> >>       SMU_TABLE_INIT(tables, SMU_TABLE_SMU_METRICS,
> >> sizeof(SmuMetrics_t),
> >>                  PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
> >>   +    SMU_TABLE_INIT(tables, SMU_TABLE_I2C_COMMANDS,
> >> sizeof(SwI2cRequest_t),
> >> +                   PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
> >> +
> >>       smu_table->metrics_table = kzalloc(sizeof(SmuMetrics_t),
> >> GFP_KERNEL);
> >>       if (!smu_table->metrics_table)
> >>           return -ENOMEM;
> >> @@ -1927,6 +1936,224 @@ static int
> arcturus_dpm_set_uvd_enable(struct
> >> smu_context *smu, bool enable)
> >>       return ret;
> >>   }
> >>   +
> >> +static void arcturus_fill_eeprom_i2c_req(SwI2cRequest_t  *req, bool
> >> write,
> >> +                  uint8_t address, uint32_t numbytes,
> >> +                  uint8_t *data)
> >> +{
> >> +    int i;
> >> +
> >> +    BUG_ON(numbytes > MAX_SW_I2C_COMMANDS);
> >> +
> >> +    req->I2CcontrollerPort = 0;
> >> +    req->I2CSpeed = 2;
> >> +    req->SlaveAddress = address;
> >> +    req->NumCmds = numbytes;
> >> +
> >> +    for (i = 0; i < numbytes; i++) {
> >> +        SwI2cCmd_t *cmd =  &req->SwI2cCmds[i];
> >> +
> >> +        /* First 2 bytes are always write for lower 2b EEPROM
> >> address */
> >> +        if (i < 2)
> >> +            cmd->Cmd = 1;
> >> +        else
> >> +            cmd->Cmd = write;
> >> +
> >> +
> >> +        /* Add RESTART for read  after address filled */
> >> +        cmd->CmdConfig |= (i == 2 && !write) ?
> >> CMDCONFIG_RESTART_MASK : 0;
> >> +
> >> +        /* Add STOP in the end */
> >> +        cmd->CmdConfig |= (i == (numbytes - 1)) ?
> >> CMDCONFIG_STOP_MASK : 0;
> >> +
> >> +        /* Fill with data regardless if read or write to simplify
> >> code */
> >> +        cmd->RegisterAddr = data[i];
> >> +    }
> >> +}
> >> +
> >> +static int arcturus_i2c_eeprom_read_data(struct i2c_adapter
> >> +*control,
> >> +                           uint8_t address,
> >> +                           uint8_t *data,
> >> +                           uint32_t numbytes) {
> >> +    uint32_t  i, ret = 0;
> >> +    SwI2cRequest_t req;
> >> +    struct amdgpu_device *adev = to_amdgpu_device(control);
> >> +    struct smu_table_context *smu_table = &adev->smu.smu_table;
> >> +    struct smu_table *table =
> >> &smu_table->tables[SMU_TABLE_I2C_COMMANDS];
> >> +
> >> +    memset(&req, 0, sizeof(req));
> >> +    arcturus_fill_eeprom_i2c_req(&req, false, address, numbytes,
> >> +data);
> >> +
> >> +    mutex_lock(&adev->smu.mutex);
> >> +    /* Now read data starting with that address */
> >> +    ret = smu_update_table(&adev->smu, SMU_TABLE_I2C_COMMANDS,
> 0,
> >> +&req,
> >> +                    true);
> >> +    mutex_unlock(&adev->smu.mutex);
> >> +
> >> +    if (!ret) {
> >> +        SwI2cRequest_t *res = (SwI2cRequest_t *)table->cpu_addr;
> >> +
> >> +        /* Assume SMU  fills res.SwI2cCmds[i].Data with read bytes
> >> +*/
> >> +        for (i = 0; i < numbytes; i++)
> >> +            data[i] = res->SwI2cCmds[i].Data;
> >> +
> >> +        pr_debug("arcturus_i2c_eeprom_read_data, address = %x, bytes
> >> = %d, data :",
> >> +                  (uint16_t)address, numbytes);
> >> +
> >> +        print_hex_dump(KERN_DEBUG, "data: ", DUMP_PREFIX_NONE,
> >> +                   8, 1, data, numbytes, false);
> >> +    } else
> >> +        pr_err("arcturus_i2c_eeprom_read_data - error occurred :%x",
> >> ret);
> >> +
> >> +    return ret;
> >> +}
> >> +
> >> +static int arcturus_i2c_eeprom_write_data(struct i2c_adapter
> >> +*control,
> >> +                        uint8_t address,
> >> +                        uint8_t *data,
> >> +                        uint32_t numbytes) {
> >> +    uint32_t ret;
> >> +    SwI2cRequest_t req;
> >> +    struct amdgpu_device *adev = to_amdgpu_device(control);
> >> +
> >> +    memset(&req, 0, sizeof(req));
> >> +    arcturus_fill_eeprom_i2c_req(&req, true, address, numbytes,
> >> +data);
> >> +
> >> +    mutex_lock(&adev->smu.mutex);
> >> +    ret = smu_update_table(&adev->smu, SMU_TABLE_I2C_COMMANDS,
> 0,
> >> &req, true);
> >> +    mutex_unlock(&adev->smu.mutex);
> >> +
> >> +    if (!ret) {
> >> +        pr_debug("arcturus_i2c_write(), address = %x, bytes = %d ,
> >> data: ",
> >> +                     (uint16_t)address, numbytes);
> >> +
> >> +        print_hex_dump(KERN_DEBUG, "data: ", DUMP_PREFIX_NONE,
> >> +                   8, 1, data, numbytes, false);
> >> +        /*
> >> +         * According to EEPROM spec there is a MAX of 10 ms required
> >> for
> >> +         * EEPROM to flush internal RX buffer after STOP was issued
> >> at the
> >> +         * end of write transaction. During this time the EEPROM
> >> will not be
> >> +         * responsive to any more commands - so wait a bit more.
> >> +         */
> >> +        msleep(10);
> >> +
> >> +    } else
> >> +        pr_err("arcturus_i2c_write- error occurred :%x", ret);
> >> +
> >> +    return ret;
> >> +}
> >> +
> >> +static int arcturus_i2c_eeprom_i2c_xfer(struct i2c_adapter
> >> +*i2c_adap,
> >> +                  struct i2c_msg *msgs, int num) {
> >> +    uint32_t  i, j, ret, data_size, data_chunk_size,
> >> next_eeprom_addr = 0;
> >> +    uint8_t *data_ptr, data_chunk[MAX_SW_I2C_COMMANDS] = { 0 };
> >> +
> >> +    for (i = 0; i < num; i++) {
> >> +        /*
> >> +         * SMU interface allows at most MAX_SW_I2C_COMMANDS bytes of
> >> data at
> >> +         * once and hence the data needs to be spliced into chunks
> >> and sent each
> >> +         * chunk separately
> >> +         */
> >> +        data_size = msgs[i].len - 2;
> >> +        data_chunk_size = MAX_SW_I2C_COMMANDS - 2;
> >> +        next_eeprom_addr = (msgs[i].buf[0] << 8 & 0xff00) |
> >> (msgs[i].buf[1] & 0xff);
> >> +        data_ptr = msgs[i].buf + 2;
> >> +
> >> +        for (j = 0; j < data_size / data_chunk_size; j++) {
> >> +            /* Insert the EEPROM dest addess, bits 0-15 */
> >> +            data_chunk[0] = ((next_eeprom_addr >> 8) & 0xff);
> >> +            data_chunk[1] = (next_eeprom_addr & 0xff);
> >> +
> >> +            if (msgs[i].flags & I2C_M_RD) {
> >> +                ret = arcturus_i2c_eeprom_read_data(i2c_adap,
> >> +                                (uint8_t)msgs[i].addr,
> >> +                                data_chunk, MAX_SW_I2C_COMMANDS);
> >> +
> >> +                memcpy(data_ptr, data_chunk + 2, data_chunk_size);
> >> +            } else {
> >> +
> >> +                memcpy(data_chunk + 2, data_ptr, data_chunk_size);
> >> +
> >> +                ret = arcturus_i2c_eeprom_write_data(i2c_adap,
> >> +                                 (uint8_t)msgs[i].addr,
> >> +                                 data_chunk, MAX_SW_I2C_COMMANDS);
> >> +            }
> >> +
> >> +            if (ret) {
> >> +                num = -EIO;
> >> +                goto fail;
> >> +            }
> >> +
> >> +            next_eeprom_addr += data_chunk_size;
> >> +            data_ptr += data_chunk_size;
> >> +        }
> >> +
> >> +        if (data_size % data_chunk_size) {
> >> +            data_chunk[0] = ((next_eeprom_addr >> 8) & 0xff);
> >> +            data_chunk[1] = (next_eeprom_addr & 0xff);
> >> +
> >> +            if (msgs[i].flags & I2C_M_RD) {
> >> +                ret = arcturus_i2c_eeprom_read_data(i2c_adap,
> >> +                                (uint8_t)msgs[i].addr,
> >> +                                data_chunk, (data_size %
> >> data_chunk_size) + 2);
> >> +
> >> +                memcpy(data_ptr, data_chunk + 2, data_size %
> >> data_chunk_size);
> >> +            } else {
> >> +                memcpy(data_chunk + 2, data_ptr, data_size %
> >> data_chunk_size);
> >> +
> >> +                ret = arcturus_i2c_eeprom_write_data(i2c_adap,
> >> +                                 (uint8_t)msgs[i].addr,
> >> +                                 data_chunk, (data_size %
> >> data_chunk_size) + 2);
> >> +            }
> >> +
> >> +            if (ret) {
> >> +                num = -EIO;
> >> +                goto fail;
> >> +            }
> >> +        }
> >> +    }
> >> +
> >> +fail:
> >> +    return num;
> >> +}
> >> +
> >> +static u32 arcturus_i2c_eeprom_i2c_func(struct i2c_adapter *adap) {
> >> +    return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; }
> >> +
> >> +
> >> +static const struct i2c_algorithm arcturus_i2c_eeprom_i2c_algo = {
> >> +    .master_xfer = arcturus_i2c_eeprom_i2c_xfer,
> >> +    .functionality = arcturus_i2c_eeprom_i2c_func, };
> >> +
> >> +int arcturus_i2c_eeprom_control_init(struct i2c_adapter *control) {
> >> +    struct amdgpu_device *adev = to_amdgpu_device(control);
> >> +    int res;
> >> +
> >> +    control->owner = THIS_MODULE;
> >> +    control->class = I2C_CLASS_SPD;
> >> +    control->dev.parent = &adev->pdev->dev;
> >> +    control->algo = &arcturus_i2c_eeprom_i2c_algo;
> >> +    snprintf(control->name, sizeof(control->name), "RAS EEPROM");
> >> +
> >> +    res = i2c_add_adapter(control);
> >> +    if (res)
> >> +        DRM_ERROR("Failed to register hw i2c, err: %d\n", res);
> >> +
> >> +    return res;
> >> +}
> >> [Guchun]Can we move this register code to one common file? As I saw
> >> on vega20, we have the same code smu_v11_0_i2c_eeprom_control_init
> to
> >> register i2c device.
> >> If we can move such code to a common file, then in each asic's i2c
> >> code, what we should do is to pass necessary ptrs/name/ops to that
> >> common file, and from there, we will register i2c device.
> >> This can help clean some code, whatever, maybe convenient to other
> >> asics if we want to do the same i2c adding job.
> >>
> >> +void arcturus_i2c_eeprom_control_fini(struct i2c_adapter *control) {
> >> +    i2c_del_adapter(control);
> >> +}
> >> +
> >> [Guchun]The same as above.
> >
> > This will be very small duplication reduction but I will look into
> > putting this inside amdgpu_ras_eeprom.c
> >
> > Andrey
> 
> 
> I took a look and i honestly don't see a point now in this as i save only a few
> lines of code because arcturus_i2c_eeprom_i2c_algo or
> smu_v11_0_i2c_eeprom_i2c_xfer are ASIC specific and so it boils down to a
> few assignments and calls to i2c_add_adapter and i2c_del_adapter. On the
> other hand i need to change the SMU interface and smu_v11_0_i2c_eeprom
> header to return me something like a pointer to i2c_algorithm.functionality
> which doesn't seem intuitive to me.
> 
> Andrey

That's fine. Series is: Reviewed-by: Guchun Chen <guchun.chen@amd.com>

Regards,
Guchun
> 
> 
> >
> >>
> >>   static const struct pptable_funcs arcturus_ppt_funcs = {
> >>       /* translate smu index into arcturus specific index */
> >>       .get_smu_msg_index = arcturus_get_smu_msg_index, @@ -1966,6
> >> +2193,8 @@ static const struct pptable_funcs arcturus_ppt_funcs = {
> >>       .get_power_limit = arcturus_get_power_limit,
> >>       .is_dpm_running = arcturus_is_dpm_running,
> >>       .dpm_set_uvd_enable = arcturus_dpm_set_uvd_enable,
> >> +    .i2c_eeprom_init = arcturus_i2c_eeprom_control_init,
> >> +    .i2c_eeprom_fini = arcturus_i2c_eeprom_control_fini,
> >>   };
> >>     void arcturus_set_ppt_funcs(struct smu_context *smu)
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx