[PATCH v2] drm/amdgpu: Move EEPROM I2C adapter to amdgpu_device
Andrey Grodzovsky
Andrey.Grodzovsky at amd.com
Fri Mar 13 20:27:50 UTC 2020
On 3/13/20 2:29 PM, Alex Deucher wrote:
> On Fri, Mar 13, 2020 at 1:20 PM Andrey Grodzovsky
> <andrey.grodzovsky at amd.com> wrote:
>> Puts the i2c adapter in common place for sharing by RAS
>> and upcoming data read from FRU EEPROM feature.
>>
>> v2:
>> Move i2c adapter to amdgpu_pm and rename it.
>>
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky at amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 35 +++++++++++++++++++
>> drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h | 2 ++
>> drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c | 48 +++++---------------------
>> drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.h | 2 --
>> drivers/gpu/drm/amd/amdgpu/smu_v11_0_i2c.c | 14 ++++----
>> drivers/gpu/drm/amd/powerplay/arcturus_ppt.c | 2 +-
>> 6 files changed, 54 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index a35c899..c04107b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -68,6 +68,8 @@
>> #include <linux/suspend.h>
>> #include <drm/task_barrier.h>
>>
>> +#include "smu_v11_0_i2c.h"
>> +
>> MODULE_FIRMWARE("amdgpu/vega10_gpu_info.bin");
>> MODULE_FIRMWARE("amdgpu/vega12_gpu_info.bin");
>> MODULE_FIRMWARE("amdgpu/raven_gpu_info.bin");
>> @@ -1848,6 +1850,33 @@ static int amdgpu_device_fw_loading(struct amdgpu_device *adev)
>> return r;
>> }
>>
>> +static int amdgpu_eeprom_init(struct amdgpu_device *adev)
>> +{
>> + switch (adev->asic_type) {
>> + case CHIP_VEGA20:
>> + return smu_v11_0_i2c_eeprom_control_init(&adev->pm.smu_i2c);
>> + case CHIP_ARCTURUS:
>> + return smu_i2c_eeprom_init(&adev->smu, &adev->pm.smu_i2c);
>> + default:
>> + return 0;
>> + }
>> +}
>> +
>> +void amdgpu_eeprom_fini(struct amdgpu_device *adev)
>> +{
>> +
>> + switch (adev->asic_type) {
>> + case CHIP_VEGA20:
>> + smu_v11_0_i2c_eeprom_control_fini(&adev->pm.smu_i2c);
>> + return;
>> + case CHIP_ARCTURUS:
>> + smu_i2c_eeprom_fini(&adev->smu, &adev->pm.smu_i2c);
>> + return;
>> + default:
>> + return;
>> + }
>> +}
>> +
> I think maybe you missed my comments on this part. I think it would
> make sense move these function calls into the relevant SMU sw init
> code. E.g., call smu_v11_0_i2c_eeprom_control_fini() into
> vega20_smu_init() in vega20_smumgr.c. then add the whole switch
> statement to smu_sw_init() in
> amdgpu_smu.c for VEGA20 (alternative powerplay code) and ARCTURUS.
> And the clean up in vega20_smu_fini() and smu_sw_fini()
>
> Alex
Sorry, missed that indeed. For the ARCTURUS use case at least i am
worried it might brake the code because from the comment here
https://elixir.bootlin.com/linux/v5.6-rc5/source/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c#L1907
i remember that smu_i2c_eeprom_init (ARCTURUS init call) must be called
after HW init fully done and SMU is active while SW init happens way
earlier.
Andrey
>
>
>> /**
>> * amdgpu_device_ip_init - run init for hardware IPs
>> *
>> @@ -1936,6 +1965,10 @@ static int amdgpu_device_ip_init(struct amdgpu_device *adev)
>> if (r)
>> goto init_failed;
>>
>> + r = amdgpu_eeprom_init(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
>> @@ -2196,6 +2229,8 @@ static int amdgpu_device_ip_fini(struct amdgpu_device *adev)
>>
>> amdgpu_ras_pre_fini(adev);
>>
>> + amdgpu_eeprom_fini(adev);
>> +
>> if (adev->gmc.xgmi.num_physical_nodes > 1)
>> amdgpu_xgmi_remove_device(adev);
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
>> index 1685794..936d85a 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
>> @@ -448,6 +448,8 @@ struct amdgpu_pm {
>> /* powerplay feature */
>> uint32_t pp_feature;
>>
>> + /* Used for I2C access to various EEPROMs on relevant ASICs */
>> + struct i2c_adapter smu_i2c;
>> };
>>
>> #define R600_SSTU_DFLT 0
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
>> index ed15b1f..c009609 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
>> @@ -25,7 +25,6 @@
>> #include "amdgpu.h"
>> #include "amdgpu_ras.h"
>> #include <linux/bits.h>
>> -#include "smu_v11_0_i2c.h"
>> #include "atom.h"
>>
>> #define EEPROM_I2C_TARGET_ADDR_VEGA20 0xA0
>> @@ -124,6 +123,7 @@ static int __update_table_header(struct amdgpu_ras_eeprom_control *control,
>> unsigned char *buff)
>> {
>> int ret = 0;
>> + struct amdgpu_device *adev = to_amdgpu_device(control);
>> struct i2c_msg msg = {
>> .addr = 0,
>> .flags = 0,
>> @@ -137,7 +137,7 @@ static int __update_table_header(struct amdgpu_ras_eeprom_control *control,
>>
>> msg.addr = control->i2c_address;
>>
>> - ret = i2c_transfer(&control->eeprom_accessor, &msg, 1);
>> + ret = i2c_transfer(&adev->pm.smu_i2c, &msg, 1);
>> if (ret < 1)
>> DRM_ERROR("Failed to write EEPROM table header, ret:%d", ret);
>>
>> @@ -251,33 +251,18 @@ int amdgpu_ras_eeprom_init(struct amdgpu_ras_eeprom_control *control)
>> .buf = buff,
>> };
>>
>> + /* Verify i2c adapter is initialized */
>> + if (!adev->pm.smu_i2c.algo)
>> + return -ENOENT;
>> +
>> if (!__get_eeprom_i2c_addr(adev, &control->i2c_address))
>> return -EINVAL;
>>
>> mutex_init(&control->tbl_mutex);
>>
>> - switch (adev->asic_type) {
>> - case CHIP_VEGA20:
>> - 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;
>> - }
>> -
>> - if (ret) {
>> - DRM_ERROR("Failed to init I2C controller, ret:%d", ret);
>> - return ret;
>> - }
>> -
>> msg.addr = control->i2c_address;
>> -
>> /* Read/Create table header from EEPROM address 0 */
>> - ret = i2c_transfer(&control->eeprom_accessor, &msg, 1);
>> + ret = i2c_transfer(&adev->pm.smu_i2c, &msg, 1);
>> if (ret < 1) {
>> DRM_ERROR("Failed to read EEPROM table header, ret:%d", ret);
>> return ret;
>> @@ -303,23 +288,6 @@ int amdgpu_ras_eeprom_init(struct amdgpu_ras_eeprom_control *control)
>> return ret == 1 ? 0 : -EIO;
>> }
>>
>> -void amdgpu_ras_eeprom_fini(struct amdgpu_ras_eeprom_control *control)
>> -{
>> - struct amdgpu_device *adev = to_amdgpu_device(control);
>> -
>> - switch (adev->asic_type) {
>> - 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;
>> - }
>> -}
>> -
>> static void __encode_table_record_to_buff(struct amdgpu_ras_eeprom_control *control,
>> struct eeprom_table_record *record,
>> unsigned char *buff)
>> @@ -476,7 +444,7 @@ int amdgpu_ras_eeprom_process_recods(struct amdgpu_ras_eeprom_control *control,
>> control->next_addr += EEPROM_TABLE_RECORD_SIZE;
>> }
>>
>> - ret = i2c_transfer(&control->eeprom_accessor, msgs, num);
>> + ret = i2c_transfer(&adev->pm.smu_i2c, msgs, num);
>> if (ret < 1) {
>> DRM_ERROR("Failed to process EEPROM table records, ret:%d", ret);
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.h
>> index ca78f81..7e8647a 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.h
>> @@ -44,7 +44,6 @@ struct amdgpu_ras_eeprom_table_header {
>>
>> struct amdgpu_ras_eeprom_control {
>> struct amdgpu_ras_eeprom_table_header tbl_hdr;
>> - struct i2c_adapter eeprom_accessor;
>> uint32_t next_addr;
>> unsigned int num_recs;
>> struct mutex tbl_mutex;
>> @@ -79,7 +78,6 @@ struct eeprom_table_record {
>> }__attribute__((__packed__));
>>
>> int amdgpu_ras_eeprom_init(struct amdgpu_ras_eeprom_control *control);
>> -void amdgpu_ras_eeprom_fini(struct amdgpu_ras_eeprom_control *control);
>> int amdgpu_ras_eeprom_reset_table(struct amdgpu_ras_eeprom_control *control);
>>
>> int amdgpu_ras_eeprom_process_recods(struct amdgpu_ras_eeprom_control *control,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/smu_v11_0_i2c.c b/drivers/gpu/drm/amd/amdgpu/smu_v11_0_i2c.c
>> index c902f26..9bffbab 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/smu_v11_0_i2c.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/smu_v11_0_i2c.c
>> @@ -46,8 +46,7 @@
>> #define I2C_NO_STOP 1
>> #define I2C_RESTART 2
>>
>> -#define to_amdgpu_device(x) (container_of(x, struct amdgpu_ras, eeprom_control.eeprom_accessor))->adev
>> -#define to_eeprom_control(x) container_of(x, struct amdgpu_ras_eeprom_control, eeprom_accessor)
>> +#define to_amdgpu_device(x) (container_of(x, struct amdgpu_device, pm.smu_i2c))
>>
>> static void smu_v11_0_i2c_set_clock_gating(struct i2c_adapter *control, bool en)
>> {
>> @@ -592,7 +591,8 @@ static uint32_t smu_v11_0_i2c_eeprom_write_data(struct i2c_adapter *control,
>>
>> static void lock_bus(struct i2c_adapter *i2c, unsigned int flags)
>> {
>> - struct amdgpu_ras_eeprom_control *control = to_eeprom_control(i2c);
>> + struct amdgpu_device *adev = to_amdgpu_device(i2c);
>> + struct amdgpu_ras_eeprom_control *control = &adev->psp.ras.ras->eeprom_control;
>>
>> if (!smu_v11_0_i2c_bus_lock(i2c)) {
>> DRM_ERROR("Failed to lock the bus from SMU");
>> @@ -610,7 +610,8 @@ static int trylock_bus(struct i2c_adapter *i2c, unsigned int flags)
>>
>> static void unlock_bus(struct i2c_adapter *i2c, unsigned int flags)
>> {
>> - struct amdgpu_ras_eeprom_control *control = to_eeprom_control(i2c);
>> + struct amdgpu_device *adev = to_amdgpu_device(i2c);
>> + struct amdgpu_ras_eeprom_control *control = &adev->psp.ras.ras->eeprom_control;
>>
>> if (!smu_v11_0_i2c_bus_unlock(i2c)) {
>> DRM_ERROR("Failed to unlock the bus from SMU");
>> @@ -630,7 +631,8 @@ static int smu_v11_0_i2c_eeprom_i2c_xfer(struct i2c_adapter *i2c_adap,
>> struct i2c_msg *msgs, int num)
>> {
>> int i, ret;
>> - struct amdgpu_ras_eeprom_control *control = to_eeprom_control(i2c_adap);
>> + struct amdgpu_device *adev = to_amdgpu_device(i2c_adap);
>> + struct amdgpu_ras_eeprom_control *control = &adev->psp.ras.ras->eeprom_control;
>>
>> if (!control->bus_locked) {
>> DRM_ERROR("I2C bus unlocked, stopping transaction!");
>> @@ -679,7 +681,7 @@ int smu_v11_0_i2c_eeprom_control_init(struct i2c_adapter *control)
>> control->class = I2C_CLASS_SPD;
>> control->dev.parent = &adev->pdev->dev;
>> control->algo = &smu_v11_0_i2c_eeprom_i2c_algo;
>> - snprintf(control->name, sizeof(control->name), "RAS EEPROM");
>> + snprintf(control->name, sizeof(control->name), "AMDGPU EEPROM");
>> control->lock_ops = &smu_v11_0_i2c_i2c_lock_ops;
>>
>> res = i2c_add_adapter(control);
>> diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
>> index 61596e8..3c55a2d 100644
>> --- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
>> +++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
>> @@ -41,7 +41,7 @@
>> #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 to_amdgpu_device(x) (container_of(x, struct amdgpu_device, pm.smu_i2c))
>>
>> #define CTF_OFFSET_EDGE 5
>> #define CTF_OFFSET_HOTSPOT 5
>> --
>> 2.7.4
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Candrey.grodzovsky%40amd.com%7Cd2dc864bf8084e145daf08d7c77c899a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637197210006888820&sdata=vw%2BE2j%2Fx8mkaTEvRyHMB4xRwRslv7sHMIBXZ92i7YqY%3D&reserved=0
More information about the amd-gfx
mailing list