Date:   Mon, 31 Jan 2022 13:10:01 +0100
From:   Jan Dąbroś <jsd@...ihalf.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-i2c <linux-i2c@...r.kernel.org>,
        Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Wolfram Sang <wsa@...nel.org>,
        Raul E Rangel <rrangel@...omium.org>,
        Marcin Wojtas <mw@...ihalf.com>,
        Grzegorz Jaszczyk <jaz@...ihalf.com>, upstream@...ihalf.com,
        Tom Lendacky <thomas.lendacky@....com>,
        "Deucher, Alexander" <alexander.deucher@....com>,
        "Easow, Nimesh" <Nimesh.Easow@....com>,
        "Limonciello, Mario" <mario.limonciello@....com>,
        kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v2 2/2] i2c: designware: Add AMD PSP I2C bus support

Aargh.. so if this won't be enough to use wrong email address in v2 -
I not used plain text above. Mailing list (understandably) aren't
happy with this, thus resending my answers to Andy.. Again sorry for
noise.


pt., 28 sty 2022 o 16:50 Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> napisał(a):
>
> On Fri, Jan 28, 2022 at 03:59:40PM +0100, Jan Dąbroś wrote:
> > Hi,
> >
> > Adding proper Andy's email address (and removing wrong one) in the
> > whole patchset. Sorry for noise!
>
> Thanks!
>
> > pt., 28 sty 2022 o 15:48 Jan Dabros <jsd@...ihalf.com> napisał(a):
> > >
> > > Implement an I2C controller sharing mechanism between the host (kernel)
> > > and PSP co-processor on some platforms equipped with AMD Cezanne SoC.
> > >
> > > On these platforms we need to implement "software" i2c arbitration.
> > > Default arbitration owner is PSP and kernel asks for acquire as well
> > > as inform about release of the i2c bus via mailbox mechanism.
> > >
> > >             +---------+
> > >  <- ACQUIRE |         |
> > >   +---------|   CPU   |\
> > >   |         |         | \      +----------+  SDA
> > >   |         +---------+  \     |          |-------
> > > MAILBOX                   +--> |  I2C-DW  |  SCL
> > >   |         +---------+        |          |-------
> > >   |         |         |        +----------+
> > >   +---------|   PSP   |
> > >    <- ACK   |         |
> > >             +---------+
> > >
> > >             +---------+
> > >  <- RELEASE |         |
> > >   +---------|   CPU   |
> > >   |         |         |        +----------+  SDA
> > >   |         +---------+        |          |-------
> > > MAILBOX                   +--> |  I2C-DW  |  SCL
> > >   |         +---------+  /     |          |-------
> > >   |         |         | /      +----------+
> > >   +---------|   PSP   |/
> > >    <- ACK   |         |
> > >             +---------+
> > >
> > > The solution is similar to i2c-designware-baytrail.c implementation, where
> > > we are using a generic i2c-designware-* driver with a small "wrapper".
> > >
> > > In contrary to baytrail semaphore implementation, beside internal
> > > acquire_lock() and release_lock() methods we are also applying quirks to
> > > lock_bus() and unlock_bus() global adapter methods. With this in place
> > > all i2c clients drivers may lock i2c bus for a desired number of i2c
> > > transactions (e.g. write-wait-read) without being aware of that such bus
> > > is shared with another entity.
> > >
> > > Modify i2c_dw_probe_lock_support() to select correct semaphore
> > > implementation at runtime, since now we have more than one available.
> > >
> > > Configure new matching ACPI ID "AMDI0019" and register
> > > ARBITRATION_SEMAPHORE flag in order to distinguish setup with PSP
> > > arbitration.
> > >
> > > Add myself as a reviewer for I2C DesignWare in order to help with reviewing
> > > and testing possible changes touching new i2c-designware-amdpsp.c module.
> > >
> > > Signed-off-by: Jan Dabros <jsd@...ihalf.com>
>
> > > Reported-by: kernel test robot <lkp@...el.com>
> > > Reported-by: kernel test robot <lkp@...el.com>
>
> New feature can't be reported.
> If you want to give a credit to CI, do it in changelog.

OK, will remove this.

> ...
>
> > > +       depends on X86_64
>
> Not sure if it's better than using non-atomic IO helpers.

There are 2 issues reported by kernel robot for my patchset:
1. Lack of <asm/msr.h>;
2. Missing declaration for 'writeq'.
Actually above was my idea to fix first issue, but please see below.

> At least you can't run 32-bit kernels on that platforms
> in order to get this functionality working. Doest it mean
> those platforms do not have 32-bit compatibility mode
> anymore?

Correct, I was focusing too much on my use case, where I'm building
only 64-bit. This isn't right. Furthermore I should rather use
dependency on CONFIG_X86_MSR which is better suited for ensuring above
msr.h header is present.

>
> ...
>
> > > +#include <linux/io-64-nonatomic-lo-hi.h>
>
> Ah, this is not needed if you keep code running exclusively on 64-bit
> platforms.

Will keep this, since switching to "depends on X86_MSR".

> ...
>
> > > +struct psp_mbox {
> > > +       u32 cmd_fields;
>
> > > +       phys_addr_t i2c_req_addr;
>
> But phys_addr_t is platform-dependent type. Perhaps you meant to use u64 here
> always?

Once I remove the "depends on X86_64" I believe this should be left
platform-dependent.

> > > +} __packed;
>
> ...
>
> > > +       struct psp_mbox __iomem *mbox = (struct psp_mbox __iomem *)mbox_iomem;
>
> For void * pointers the cast is implied, i.o.w. it's not needed here.

ACK.

> ...
>
> > > +static int psp_send_check_i2c_req(struct psp_i2c_req *req)
> > > +{
> > > +       if (psp_send_cmd(req))
>
> > > +               return -EIO;
>
> Why is error code shadowed?
>
> > > +       return check_i2c_req_sts(req);
> > > +}

Just as a side note - it wasn't modified in v2 when moving above to
psp_send_check_i2c_req(), but let me explain why I have introduced
this initially.

We have two means of timeouts in the context of this driver:
1. Timeout of internal mailbox, which means we cannot communicate with
a PSP for a programmed timeout. This timeout is encountered inside
psp_send_cmd().
2. Timeout of i2c arbitration - which means that we can communicate
with PSP, but PSP refuses to release i2c bus for too long. This
timeout is returned by psp_send_i2c_req() in case of error.
(side note: both error conditions are very unlikely to happen at runtime)

I wanted to clearly distinguish between these two and thus put all
errors around mailbox into "-EIO category", which is actually true.

> ...
>
> > > +cleanup:
> > > +       mutex_unlock(&psp_i2c_access_mutex);
> > > +       return 0;
>
> Not sure I understand why we ignore all above errors here.

Actually we are not ignoring them, since each error sets
"psp_i2c_mbox_fail = true;". This means that if there is any error on
x86-PSP interface, we are ignoring i2c-arbitration and just fall back
to normal (that is no-quirk) operation.

>From the i2c-client perspective (who is eventually gathering error
code from above) I think we can claim that everything is fine, since
bus is granted to it. For developers there is an error message in case
some debug will be necessary.

> ...
>
> > > +       if (!dev || !dev->dev)
> > > +               return -ENODEV;
>
> At which circumstances may we get
>         dev != NULL
>         dev->dev == NULL
> ?
>
> ...
>
> > >         if (!dev || !dev->dev)
> > > -               return 0;
> > > +               return -ENODEV;
>
> I see the same here, perhaps Hans knows the answer :-)

Right, so I must admit that I simply used *-baytrail.c as a reference
and thinking that additional check shouldn't hurt us (always better
than not enough safety..). Looking more at this now -
`dw_i2c_plat_probe()` will boil-out earlier if `dev->dev == NULL`.
Should I remove this extra check in *-baytrail.c in the same commit?

> ...
>
> > > +static int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
> > > +{
> > > +       const struct i2c_dw_semaphore_callbacks *ptr;
> > > +       int i = 0;
> > > +       int ret;
> > > +
> > > +       ptr = i2c_dw_semaphore_cb_table;
> > > +
> > > +       dev->semaphore_idx = -1;
> > > +
> > > +       while (ptr->probe) {
> > > +               ret = ptr->probe(dev);
> > > +               if (ret) {
>
> > > +                       /*
> > > +                        * If there is no semaphore device attached to this
> > > +                        * controller, we shouldn't abort general i2c_controller
> > > +                        * probe.
> > > +                        */
> > > +                       if (ret == -ENODEV) {
> > > +                               i++;
> > > +                               ptr++;
> > > +                               continue;
> > > +                       } else {
>
> Redundant 'else', but see below.
>
> > > +                               return ret;
> > > +                       }
>
> May it be
>
>             if (ret != -ENODEV)
>                 return ret;
>
>             i++;
>             ptr++;
>             continue;
>
> ?

Yes, looks good. Thanks!

Best Regards,
Jan

>
> > > +               }
> > > +
> > > +               dev->semaphore_idx = i;
> > > +               break;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Powered by blists - more mailing lists