Re: [PATCH] drm/amdgpu/smu: rework i2c adpater registration

2020-08-04 Thread Wang, Kevin(Yang) 
Reviewed-By: Kevin Wang 

Best Regards
Kevin

> 在 2020年8月5日，上午12:57，Alex Deucher  写道：
> 
> Ping?
> 
> 
>> On Thu, Jul 30, 2020 at 3:24 PM Alex Deucher  wrote:
>> 
>> The i2c init/fini functions just register the i2c adapter.
>> There is no need to call them during hw init/fini.  They only
>> need to be called once per driver init/fini.  The previous
>> behavior broke runtime pm because we unregistered the i2c
>> adapter during suspend.
>> 
>> Signed-off-by: Alex Deucher 
>> ---
>> drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 12 ++--
>> drivers/gpu/drm/amd/powerplay/arcturus_ppt.c   | 14 --
>> drivers/gpu/drm/amd/powerplay/navi10_ppt.c | 14 --
>> drivers/gpu/drm/amd/powerplay/sienna_cichlid_ppt.c | 14 --
>> 4 files changed, 6 insertions(+), 48 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c 
>> b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
>> index 55463e7a11e2..d03b4852ed5f 100644
>> --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
>> +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
>> @@ -579,6 +579,10 @@ static int smu_smc_table_sw_init(struct smu_context 
>> *smu)
>>if (ret)
>>return ret;
>> 
>> +   ret = smu_i2c_init(smu, >adev->pm.smu_i2c);
>> +   if (ret)
>> +   return ret;
>> +
>>return 0;
>> }
>> 
>> @@ -586,6 +590,8 @@ static int smu_smc_table_sw_fini(struct smu_context *smu)
>> {
>>int ret;
>> 
>> +   smu_i2c_fini(smu, >adev->pm.smu_i2c);
>> +
>>ret = smu_free_memory_pool(smu);
>>if (ret)
>>return ret;
>> @@ -845,10 +851,6 @@ static int smu_smc_hw_setup(struct smu_context *smu)
>>return ret;
>>}
>> 
>> -   ret = smu_i2c_init(smu, >pm.smu_i2c);
>> -   if (ret)
>> -   return ret;
>> -
>>ret = smu_disable_umc_cdr_12gbps_workaround(smu);
>>if (ret) {
>>dev_err(adev->dev, "Workaround failed to disable UMC CDR 
>> feature on 12Gbps SKU!\n");
>> @@ -1047,8 +1049,6 @@ static int smu_smc_hw_cleanup(struct smu_context *smu)
>>struct amdgpu_device *adev = smu->adev;
>>int ret = 0;
>> 
>> -   smu_i2c_fini(smu, >pm.smu_i2c);
>> -
>>cancel_work_sync(>throttling_logging_work);
>> 
>>ret = smu_disable_thermal_alert(smu);
>> diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c 
>> b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
>> index f13979687b9e..0147a5b9b06d 100644
>> --- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
>> +++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
>> @@ -2080,22 +2080,11 @@ static const struct i2c_algorithm arcturus_i2c_algo 
>> = {
>>.functionality = arcturus_i2c_func,
>> };
>> 
>> -static bool arcturus_i2c_adapter_is_added(struct i2c_adapter *control)
>> -{
>> -   struct amdgpu_device *adev = to_amdgpu_device(control);
>> -
>> -   return control->dev.parent == >pdev->dev;
>> -}
>> -
>> static int arcturus_i2c_control_init(struct smu_context *smu, struct 
>> i2c_adapter *control)
>> {
>>struct amdgpu_device *adev = to_amdgpu_device(control);
>>int res;
>> 
>> -   /* smu_i2c_eeprom_init may be called twice in sriov */
>> -   if (arcturus_i2c_adapter_is_added(control))
>> -   return 0;
>> -
>>control->owner = THIS_MODULE;
>>control->class = I2C_CLASS_SPD;
>>control->dev.parent = >pdev->dev;
>> @@ -2111,9 +2100,6 @@ static int arcturus_i2c_control_init(struct 
>> smu_context *smu, struct i2c_adapter
>> 
>> static void arcturus_i2c_control_fini(struct smu_context *smu, struct 
>> i2c_adapter *control)
>> {
>> -   if (!arcturus_i2c_adapter_is_added(control))
>> -   return;
>> -
>>i2c_del_adapter(control);
>> }
>> 
>> diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c 
>> b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
>> index 6aaf483858a0..c33bdc6747f2 100644
>> --- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
>> +++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
>> @@ -2457,22 +2457,11 @@ static const struct i2c_algorithm navi10_i2c_algo = {
>>.functionality = navi10_i2c_func,
>> };
>> 
>> -static bool navi10_i2c_adapter_is_added(struct i2c_adapter *control)
>> -{
>> -   struct amdgpu_device *adev = to_amdgpu_device(control);
>> -
>> -   return control->dev.parent == >pdev->dev;
>> -}
>> -
>> static int navi10_i2c_control_init(struct smu_context *smu, struct 
>> i2c_adapter *control)
>> {
>>struct amdgpu_device *adev = to_amdgpu_device(control);
>>int res;
>> 
>> -   /* smu_i2c_eeprom_init may be called twice in sriov */
>> -   if (navi10_i2c_adapter_is_added(control))
>> -   return 0;
>> -
>>control->owner = THIS_MODULE;
>>control->class = I2C_CLASS_SPD;
>>control->dev.parent = >pdev->dev;
>> @@ -2488,9 +2477,6 @@ static int navi10_i2c_control_init(struct smu_context 
>> *smu, struct i2c_adapter *
>> 
>> static

Re: [PATCH] drm/amdgpu/smu: rework i2c adpater registration

2020-08-04 Thread Alex Deucher 
Ping?


On Thu, Jul 30, 2020 at 3:24 PM Alex Deucher  wrote:
>
> The i2c init/fini functions just register the i2c adapter.
> There is no need to call them during hw init/fini.  They only
> need to be called once per driver init/fini.  The previous
> behavior broke runtime pm because we unregistered the i2c
> adapter during suspend.
>
> Signed-off-by: Alex Deucher 
> ---
>  drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 12 ++--
>  drivers/gpu/drm/amd/powerplay/arcturus_ppt.c   | 14 --
>  drivers/gpu/drm/amd/powerplay/navi10_ppt.c | 14 --
>  drivers/gpu/drm/amd/powerplay/sienna_cichlid_ppt.c | 14 --
>  4 files changed, 6 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c 
> b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> index 55463e7a11e2..d03b4852ed5f 100644
> --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> @@ -579,6 +579,10 @@ static int smu_smc_table_sw_init(struct smu_context *smu)
> if (ret)
> return ret;
>
> +   ret = smu_i2c_init(smu, >adev->pm.smu_i2c);
> +   if (ret)
> +   return ret;
> +
> return 0;
>  }
>
> @@ -586,6 +590,8 @@ static int smu_smc_table_sw_fini(struct smu_context *smu)
>  {
> int ret;
>
> +   smu_i2c_fini(smu, >adev->pm.smu_i2c);
> +
> ret = smu_free_memory_pool(smu);
> if (ret)
> return ret;
> @@ -845,10 +851,6 @@ static int smu_smc_hw_setup(struct smu_context *smu)
> return ret;
> }
>
> -   ret = smu_i2c_init(smu, >pm.smu_i2c);
> -   if (ret)
> -   return ret;
> -
> ret = smu_disable_umc_cdr_12gbps_workaround(smu);
> if (ret) {
> dev_err(adev->dev, "Workaround failed to disable UMC CDR 
> feature on 12Gbps SKU!\n");
> @@ -1047,8 +1049,6 @@ static int smu_smc_hw_cleanup(struct smu_context *smu)
> struct amdgpu_device *adev = smu->adev;
> int ret = 0;
>
> -   smu_i2c_fini(smu, >pm.smu_i2c);
> -
> cancel_work_sync(>throttling_logging_work);
>
> ret = smu_disable_thermal_alert(smu);
> diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c 
> b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> index f13979687b9e..0147a5b9b06d 100644
> --- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> +++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> @@ -2080,22 +2080,11 @@ static const struct i2c_algorithm arcturus_i2c_algo = 
> {
> .functionality = arcturus_i2c_func,
>  };
>
> -static bool arcturus_i2c_adapter_is_added(struct i2c_adapter *control)
> -{
> -   struct amdgpu_device *adev = to_amdgpu_device(control);
> -
> -   return control->dev.parent == >pdev->dev;
> -}
> -
>  static int arcturus_i2c_control_init(struct smu_context *smu, struct 
> i2c_adapter *control)
>  {
> struct amdgpu_device *adev = to_amdgpu_device(control);
> int res;
>
> -   /* smu_i2c_eeprom_init may be called twice in sriov */
> -   if (arcturus_i2c_adapter_is_added(control))
> -   return 0;
> -
> control->owner = THIS_MODULE;
> control->class = I2C_CLASS_SPD;
> control->dev.parent = >pdev->dev;
> @@ -2111,9 +2100,6 @@ static int arcturus_i2c_control_init(struct smu_context 
> *smu, struct i2c_adapter
>
>  static void arcturus_i2c_control_fini(struct smu_context *smu, struct 
> i2c_adapter *control)
>  {
> -   if (!arcturus_i2c_adapter_is_added(control))
> -   return;
> -
> i2c_del_adapter(control);
>  }
>
> diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c 
> b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> index 6aaf483858a0..c33bdc6747f2 100644
> --- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> +++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> @@ -2457,22 +2457,11 @@ static const struct i2c_algorithm navi10_i2c_algo = {
> .functionality = navi10_i2c_func,
>  };
>
> -static bool navi10_i2c_adapter_is_added(struct i2c_adapter *control)
> -{
> -   struct amdgpu_device *adev = to_amdgpu_device(control);
> -
> -   return control->dev.parent == >pdev->dev;
> -}
> -
>  static int navi10_i2c_control_init(struct smu_context *smu, struct 
> i2c_adapter *control)
>  {
> struct amdgpu_device *adev = to_amdgpu_device(control);
> int res;
>
> -   /* smu_i2c_eeprom_init may be called twice in sriov */
> -   if (navi10_i2c_adapter_is_added(control))
> -   return 0;
> -
> control->owner = THIS_MODULE;
> control->class = I2C_CLASS_SPD;
> control->dev.parent = >pdev->dev;
> @@ -2488,9 +2477,6 @@ static int navi10_i2c_control_init(struct smu_context 
> *smu, struct i2c_adapter *
>
>  static void navi10_i2c_control_fini(struct smu_context *smu, struct 
> i2c_adapter *control)
>  {
> -   if (!navi10_i2c_adapter_is_added(control))
> -   return;
> -
> i2c_del_adapter(control);
>

[PATCH] drm/amdgpu/smu: rework i2c adpater registration

2020-07-30 Thread Alex Deucher 
The i2c init/fini functions just register the i2c adapter.
There is no need to call them during hw init/fini.  They only
need to be called once per driver init/fini.  The previous
behavior broke runtime pm because we unregistered the i2c
adapter during suspend.

Signed-off-by: Alex Deucher 
---
 drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 12 ++--
 drivers/gpu/drm/amd/powerplay/arcturus_ppt.c   | 14 --
 drivers/gpu/drm/amd/powerplay/navi10_ppt.c | 14 --
 drivers/gpu/drm/amd/powerplay/sienna_cichlid_ppt.c | 14 --
 4 files changed, 6 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c 
b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
index 55463e7a11e2..d03b4852ed5f 100644
--- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
@@ -579,6 +579,10 @@ static int smu_smc_table_sw_init(struct smu_context *smu)
if (ret)
return ret;
 
+   ret = smu_i2c_init(smu, >adev->pm.smu_i2c);
+   if (ret)
+   return ret;
+
return 0;
 }
 
@@ -586,6 +590,8 @@ static int smu_smc_table_sw_fini(struct smu_context *smu)
 {
int ret;
 
+   smu_i2c_fini(smu, >adev->pm.smu_i2c);
+
ret = smu_free_memory_pool(smu);
if (ret)
return ret;
@@ -845,10 +851,6 @@ static int smu_smc_hw_setup(struct smu_context *smu)
return ret;
}
 
-   ret = smu_i2c_init(smu, >pm.smu_i2c);
-   if (ret)
-   return ret;
-
ret = smu_disable_umc_cdr_12gbps_workaround(smu);
if (ret) {
dev_err(adev->dev, "Workaround failed to disable UMC CDR 
feature on 12Gbps SKU!\n");
@@ -1047,8 +1049,6 @@ static int smu_smc_hw_cleanup(struct smu_context *smu)
struct amdgpu_device *adev = smu->adev;
int ret = 0;
 
-   smu_i2c_fini(smu, >pm.smu_i2c);
-
cancel_work_sync(>throttling_logging_work);
 
ret = smu_disable_thermal_alert(smu);
diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c 
b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
index f13979687b9e..0147a5b9b06d 100644
--- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
+++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
@@ -2080,22 +2080,11 @@ static const struct i2c_algorithm arcturus_i2c_algo = {
.functionality = arcturus_i2c_func,
 };
 
-static bool arcturus_i2c_adapter_is_added(struct i2c_adapter *control)
-{
-   struct amdgpu_device *adev = to_amdgpu_device(control);
-
-   return control->dev.parent == >pdev->dev;
-}
-
 static int arcturus_i2c_control_init(struct smu_context *smu, struct 
i2c_adapter *control)
 {
struct amdgpu_device *adev = to_amdgpu_device(control);
int res;
 
-   /* smu_i2c_eeprom_init may be called twice in sriov */
-   if (arcturus_i2c_adapter_is_added(control))
-   return 0;
-
control->owner = THIS_MODULE;
control->class = I2C_CLASS_SPD;
control->dev.parent = >pdev->dev;
@@ -2111,9 +2100,6 @@ static int arcturus_i2c_control_init(struct smu_context 
*smu, struct i2c_adapter
 
 static void arcturus_i2c_control_fini(struct smu_context *smu, struct 
i2c_adapter *control)
 {
-   if (!arcturus_i2c_adapter_is_added(control))
-   return;
-
i2c_del_adapter(control);
 }
 
diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c 
b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
index 6aaf483858a0..c33bdc6747f2 100644
--- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
+++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
@@ -2457,22 +2457,11 @@ static const struct i2c_algorithm navi10_i2c_algo = {
.functionality = navi10_i2c_func,
 };
 
-static bool navi10_i2c_adapter_is_added(struct i2c_adapter *control)
-{
-   struct amdgpu_device *adev = to_amdgpu_device(control);
-
-   return control->dev.parent == >pdev->dev;
-}
-
 static int navi10_i2c_control_init(struct smu_context *smu, struct i2c_adapter 
*control)
 {
struct amdgpu_device *adev = to_amdgpu_device(control);
int res;
 
-   /* smu_i2c_eeprom_init may be called twice in sriov */
-   if (navi10_i2c_adapter_is_added(control))
-   return 0;
-
control->owner = THIS_MODULE;
control->class = I2C_CLASS_SPD;
control->dev.parent = >pdev->dev;
@@ -2488,9 +2477,6 @@ static int navi10_i2c_control_init(struct smu_context 
*smu, struct i2c_adapter *
 
 static void navi10_i2c_control_fini(struct smu_context *smu, struct 
i2c_adapter *control)
 {
-   if (!navi10_i2c_adapter_is_added(control))
-   return;
-
i2c_del_adapter(control);
 }
 
diff --git a/drivers/gpu/drm/amd/powerplay/sienna_cichlid_ppt.c 
b/drivers/gpu/drm/amd/powerplay/sienna_cichlid_ppt.c
index f64a1be94cb8..f373e2d0d31c 100644
--- a/drivers/gpu/drm/amd/powerplay/sienna_cichlid_ppt.c
+++ b/drivers/gpu/drm/amd/powerplay/sienna_cichlid_ppt.c
@@ -2630,22 +2630,11 @@ static const