Sender: Guenter Roeck <groeck7@gmail.com>
To:     Robert Richter <rrichter@amd.com>,
        Terry Bowman <Terry.Bowman@amd.com>
Cc:     linux-watchdog@vger.kernel.org, linux-kernel@vger.kernel.org,
        wim@linux-watchdog.org, ssg.sos.patches@amd.com,
        sudheesh.mavila@amd.com,
        "Lendacky, Thomas" <thomas.lendacky@amd.com>
References: <20211103161521.43447-1-terry.bowman@amd.com>
 <20211103161521.43447-4-terry.bowman@amd.com>
 <20220106181809.GA240027@roeck-us.net>
 <9afabe55-6429-2284-cafd-d59ce481f067@amd.com>
 <YdgeU2p+i5hN1XCU@rric.localdomain>
From:   Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH v2 3/4] Watchdog: sp5100_tco: Add EFCH SMBus controller
 initialization using MMIO
Message-ID: <ac8e1173-3002-54f1-0264-6f0cdf26c475@roeck-us.net>
Date:   Fri, 7 Jan 2022 09:12:32 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <YdgeU2p+i5hN1XCU@rric.localdomain>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 1/7/22 3:05 AM, Robert Richter wrote:
> On 06.01.22 13:07:11, Terry Bowman wrote:
>> On 1/6/22 12:18 PM, Guenter Roeck wrote:
>>> On Wed, Nov 03, 2021 at 11:15:20AM -0500, Terry Bowman wrote:
> 
>>>> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
>>>> index 80ae42ae7aaa..4777e672a8ad 100644
>>>> --- a/drivers/watchdog/sp5100_tco.c
>>>> +++ b/drivers/watchdog/sp5100_tco.c
>>>> @@ -48,12 +48,14 @@
>>>>   /* internal variables */
>>>>   
>>>>   enum tco_reg_layout {
>>>> -	sp5100, sb800, efch
>>>> +	sp5100, sb800, efch, efch_mmio
>>>>   };
>>>>   
>>>>   struct sp5100_tco {
>>>>   	struct watchdog_device wdd;
>>>>   	void __iomem *tcobase;
>>>> +	void __iomem *addr;
>>>> +	struct resource *res;
>>>
>>> I must admit that I really don't like this code. Both res and
>>> addr are only used during initialization, yet their presence suggests
>>> runtime usage. Any chance to reqork this to not require those variables ?
> 
> We did that in an earlier version, see struct efch_cfg of:
> 
>   https://patchwork.kernel.org/project/linux-watchdog/patch/20210813213216.54780-1-Terry.Bowman@amd.com/
> 
> The motivation of it was the same as you suggested to only use it
> during init.
> 
> Having it in struct sp5100_tco made things simpler esp. in the
> definition of the function interfaces where those new members are
> used.
> 

'res' is only used in the context of sp5100_request_region_mmio()
and sp5100_release_region_mmio(), and both are called from
sp5100_tco_setupdevice_mmio(). I do not see a need for carrying it around
anywhere else, including efch_read_pm_reg8() and efch_update_pm_reg8().

efch_read_pm_reg8() is only called from sp5100_tco_setupdevice_mmio(),
and it only uses struct sp5100_tco *tco to get the address. I don't see
why the address can not be passed to it directly.

efch_update_pm_reg8() also uses tco only to get the address. Again, passing
it instead of a pointer to sp5100_tco *tco would easily be possible.

efch_update_pm_reg8() is only called fromm sp5100_tco_setupdevice_mmio(),
where the change would be easy. It is also called from tco_timer_enable(),
which in turn is called from sp5100_tco_timer_init(). This, again, is called
from sp5100_tco_setupdevice_mmio(). Since the first operation in
sp5100_tco_timer_init() is to call tco_timer_enable() and that function
does nothing but calling efch_update_pm_reg8(), it would easily be possible
to pull out that code from tco_timer_enable() and move it into
sp5100_tco_setupdevice_mmio().

So, no, I neither see the need for having the information in struct
sp5100_tco nor for keeping it in its own structure. If you'd merge
sp5100_request_region_mmio() and sp5100_release_region_mmio() into
sp5100_tco_setupdevice_mmio() you would not even need any pointers
to pass the values from sp5100_request_region_mmio(). Otherwise you
could have sp5100_request_region_mmio() return a pointer to res or
an ERR_PTR, and pass the address as pointer parameter.

Guenter

> If that init vars are no longer in struct sp5100_tco then callers of
> efch_read_pm_reg8() and efch_update_pm_reg8() will need to carry a
> pointer to them. To avoid this I see those options:
> 
> 1) Implement them as global (or a single global struct) and possibly
> protect it by a mutex. There is only a single device anyway and we
> wouldn't need a protection.
> 
> 2) Have an own mmio implementation of tco_timer_enable() and/or
> sp5100_tco_timer_init().
> 
>> Yes, v3 will include refactoring to remove 'res' and 'addr'. I will also
>> correct the trailing newline you mentioned in an earlier email.
>>
>> Regards,
>> Terry
>>
>>>>   	enum tco_reg_layout tco_reg_layout;
> 
> While at it, tco_reg_layout is also only used during initialization
> and can be moved there too. This would raise option 3:
> 
> 3) Add a pointer of struct sp5100_tco to struct efch_cfg and use that
> struct instead in init funtions only. But that causes the most rework
> (which would be ok to me).
> 
> Going with 3) looks the cleanest way, I would try that. But all
> options have its advantages.
> 
> -Robert
> 
>>>>   };