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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>
> > > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > > Reported-by: kernel test robot <lkp@xxxxxxxxx>
>
> 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
>
>
