Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220916131854.687371-1-jsd@semihalf.com> <20220916131854.687371-2-jsd@semihalf.com>
 <eafc7bb5-a406-132b-4b7d-167917cdab05@amd.com>
In-Reply-To: <eafc7bb5-a406-132b-4b7d-167917cdab05@amd.com>
From:   =?UTF-8?B?SmFuIETEhWJyb8Wb?= <jsd@semihalf.com>
Date:   Tue, 20 Sep 2022 18:24:39 +0200
Message-ID: <CAOtMz3Pgh+cERiXVetDZJrQa9C0kUUbZ9dRRhdghgm5Or8kwhg@mail.gmail.com>
Subject: Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access
 to SMN access
To:     "Limonciello, Mario" <mario.limonciello@amd.com>
Cc:     linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org,
        jarkko.nikula@linux.intel.com, andriy.shevchenko@linux.intel.com,
        wsa@kernel.org, rrangel@chromium.org, upstream@semihalf.com,
        Muralidhara M K <muralimk@amd.com>,
        Naveen Krishna Chatradhi <nchatrad@amd.com>,
        Borislav Petkov <bp@suse.de>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Mario,

(...)

> > @@ -303,6 +303,7 @@ static int amd_cache_northbridges(void)
> >
> >       return 0;
> >   }
> > +EXPORT_SYMBOL_GPL(amd_cache_northbridges);
> >
>
> I feel changes to amd_nb.c/amd_nb.h should stand on their own patch in
> the series rather than being in this patch.

I was thinking about this initially, however I wanted to avoid a
situation where we are exporting a symbol without actually using it
(since this will be done in consecutive patch). It is also way easier
for me to explain why I'm doing this in the context of
i2c-designware-amdpsp.c changes.
That being said, if you think this should be a separate patch, that's
fine with me - I don't have that strong feelings about it.

(...)

> > @@ -31,6 +30,10 @@
> >   #define PSP_MBOX_FIELDS_RECOVERY    BIT(30)
> >   #define PSP_MBOX_FIELDS_READY               BIT(31)
> >
> > +#define PSP_MBOX_CMD_OFFSET          0x3810570
> > +#define PSP_MBOX_BUFFER_L_OFFSET     0x3810574
> > +#define PSP_MBOX_BUFFER_H_OFFSET     0x3810578
> > +
>
> Just in case these offsets change for a future program, I think you
> should leave a comment here on the two APU programs they're valid for in
> case you need to special case it later.
>
> Another approach is that you can have a lookup function and match some
> PCI device like the root device or the CPUID and then when another
> program comes along explicitly add it to this list.
>
> This is what is done in drivers/platform/x86/amd/pmc.c for example.

Yes, I see your point. I assumed that this will _not_ change and the
new APU will just work out of the box - without a need to introduce a
new ID to the table.
Bad thing with my approach is if this _will_ change, then it may not
be that obvious for the developer what the issue actually is.

Considering your next comment - new PCI ID will anyway needs to be
added to arch/x86/kernel/amd_nb.c - let's be consistent with this
approach and I will introduce a list of supported CPU IDs.

>
> >   struct psp_req_buffer_hdr {
> >       u32 total_size;
> >       u32 status;
> > @@ -47,14 +50,8 @@ struct psp_i2c_req {
> >       enum psp_i2c_req_type type;
> >   };
> >
> > -struct psp_mbox {
> > -     u32 cmd_fields;
> > -     u64 i2c_req_addr;
> > -} __packed;
> > -
> >   static DEFINE_MUTEX(psp_i2c_access_mutex);
> >   static unsigned long psp_i2c_sem_acquired;
> > -static void __iomem *mbox_iomem;
> >   static u32 psp_i2c_access_count;
> >   static bool psp_i2c_mbox_fail;
> >   static struct device *psp_i2c_dev;
> > @@ -64,47 +61,43 @@ static struct device *psp_i2c_dev;
> >    * family of SoCs.
> >    */
> >
> > -static int psp_get_mbox_addr(unsigned long *mbox_addr)
> > +static int psp_mbox_probe(void)
> >   {
> > -     unsigned long long psp_mmio;
> > -
> > -     if (rdmsrl_safe(MSR_AMD_PSP_ADDR, &psp_mmio))
> > -             return -EIO;
> > -
> > -     *mbox_addr = (unsigned long)(psp_mmio + PSP_MBOX_OFFSET);
> > -
> > -     return 0;
> > +     /*
> > +      * Explicitly initialize system management network interface here, since
> > +      * usual init happens only after PCI subsystem is ready. This is too late
> > +      * for I2C controller driver which may be executed earlier.
> > +      */
> > +     return amd_cache_northbridges();
>
> When a future program is added even if SMN addresses are the same and no
> special casing needed there is an implicit dependency on the PCI IDs
> being in arch/x86/kernel/amd_nb.c now.  In order to make your life
> easier to debug in the future, suggest that you capture the return
> variable and emit a specific error message if amd_cache_northbridges()
> fails.

Very good point, thanks!

Best Regards,
Jan