Received-SPF: Pass (protection.outlook.com: domain of amd.com designates
 165.204.84.17 as permitted sender) receiver=protection.outlook.com;
 client-ip=165.204.84.17; helo=SATLEXMB04.amd.com;
Date:   Mon, 10 Jan 2022 10:22:53 +0100
From:   Robert Richter <rrichter@amd.com>
To:     Guenter Roeck <linux@roeck-us.net>
CC:     Terry Bowman <Terry.Bowman@amd.com>,
        <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>
Subject: Re: [PATCH v2 3/4] Watchdog: sp5100_tco: Add EFCH SMBus controller
 initialization using MMIO
Message-ID: <Ydv67VKcrCk81Y9R@rric.localdomain>
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>
 <ac8e1173-3002-54f1-0264-6f0cdf26c475@roeck-us.net>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ac8e1173-3002-54f1-0264-6f0cdf26c475@roeck-us.net>
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jan 2022 09:22:58.1520
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 91c126a9-5302-4deb-e3a8-08d9d41aca8f
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com]
X-MS-Exchange-CrossTenant-AuthSource: DM6NAM11FT023.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR12MB3075
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 07.01.22 09:12:32, Guenter Roeck wrote:
> 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.

> 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.

Yes, that is feasible, in fact it is option 2) I suggested.

-Robert

> > 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
> > 
> > > > >   };
>