Re: [PATCH v2] drm/amdgpu: Move EEPROM I2C adapter to amdgpu_device

[Andrey Grodzovsky <Andrey.Grodzovsky@xxxxxxx>]

On 3/13/20 4:35 PM, Alex Deucher wrote:
> On Fri, Mar 13, 2020 at 4:27 PM Andrey Grodzovsky
> <Andrey.Grodzovsky@xxxxxxx> wrote:
> > On 3/13/20 2:29 PM, Alex Deucher wrote:
> > > On Fri, Mar 13, 2020 at 1:20 PM Andrey Grodzovsky
> > > <andrey.grodzovsky@xxxxxxx> 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@xxxxxxx>
> > > > ---
> > > >    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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.6-rc5%2Fsource%2Fdrivers%2Fgpu%2Fdrm%2Famd%2Famdgpu%2Famdgpu_device.c%23L1907&amp;data=02%7C01%7CAndrey.Grodzovsky%40amd.com%7Ca9b64f335ebe4240a99208d7c78e0abe%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637197285188033276&amp;sdata=b6QUINZjP%2BuadZW76IIf3xi9u729OyFBTokBKzo0pX4%3D&amp;reserved=0
> > 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.
> ras_recovery_init can stay there, it does a lot of other stuff.
> smu_i2c_eeprom_init() and smu_v11_0_i2c_eeprom_control_init() don't
> actually touch the hw, they just create the i2c device.  It doesn't
> get used until we actually try and access the EEPROM.
>
> Alex


Right - now i remember the original issue was with trying to access the 
bus - this part indeed looks harmless.

Andrey


> > 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@xxxxxxxxxxxxxxxxxxxxx
> > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7CAndrey.Grodzovsky%40amd.com%7Ca9b64f335ebe4240a99208d7c78e0abe%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637197285188043270&amp;sdata=MRYASixIMQnp0oQF4RqXACiqGpLLGqAPGvvfUjH%2FZzA%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx