by Mario Limonciello

[permalink] [raw]

Subject: RE: [PATCH v2 8/9] i2c: designware: Add doorbell support for Skyrim

[Public]

> -----Original Message-----
> From: Grzegorz Bernacki <[email protected]>
> Sent: Friday, March 3, 2023 06:00
> To: Limonciello, Mario <[email protected]>
> Cc: Jan Dąbroś <[email protected]>; Thomas, Rijo-john <Rijo-
> [email protected]>; Lendacky, Thomas
> <[email protected]>; [email protected]; Jarkko
> Nikula <[email protected]>; Andy Shevchenko
> <[email protected]>; Mika Westerberg
> <[email protected]>; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH v2 8/9] i2c: designware: Add doorbell support for Skyrim
>
> Hi Mario,
>
> I am not sure if adding a new ACPI ID is a good idea. Actually we are
> talking about the same devices. The only difference is in the
> communication protocol between PSP and CPU. This could be easily
> detected at runtime by checking cpu id. There is no need to introduce
> a new id and create dependency on the FW version.

An ACPI ID seems more scalable to me to represent the difference in protocol.
Otherwise what happens when the follow on to Skyrim comes? Do you add
a new ID/case to the code? If it was an ACPI ID then it's a one line change
in the firmware to represent this.

What I'll do for v3 is do with a CPU ID in this patch, and then introduce an ACPI
ID in a new patch. You can test it without the ACPI ID, and when it's working
patch the BIOS with the new ID and see if that continues to work.

>
> thanks,
> greg
>
> czw., 2 mar 2023 o 20:43 Mario Limonciello <[email protected]>
> napisał(a):
> >
> > Skyrim doesn't use the platform feature mailbox for communication
> > for I2C arbitration, it relies upon ringing a doorbell.
> >
> > Add a new ACPI ID to use for Skyrim and when that ID is detected
> > use the doorbell instead.
> >
> > Link: https://lore.kernel.org/linux-i2c/20220916131854.687371-3-
> [email protected]/
> > Signed-off-by: Mario Limonciello <[email protected]>
> > ---
> > v1->v2:
> > * New patch
> > Note: This ACPI ID hasn't currently been reserved, but as Skyrim runs
> coreboot
> > it should be easy enough to modify the BIOS and test it out.
> > If this approach works and has general agreement it can be reserved.
> > ---
> > drivers/i2c/busses/i2c-designware-amdpsp.c | 29 ++++++++++++++-------
> > drivers/i2c/busses/i2c-designware-platdrv.c | 1 +
> > 2 files changed, 20 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-designware-amdpsp.c
> b/drivers/i2c/busses/i2c-designware-amdpsp.c
> > index 105584abcf8f..e1a4d3002c80 100644
> > --- a/drivers/i2c/busses/i2c-designware-amdpsp.c
> > +++ b/drivers/i2c/busses/i2c-designware-amdpsp.c
> > @@ -73,34 +73,44 @@ static int psp_send_check_i2c_req(struct
> psp_i2c_req *req)
> > return check_i2c_req_sts(req);
> > }
> >
> > -static int psp_send_i2c_req(enum psp_i2c_req_type i2c_req_type)
> > +static int psp_send_i2c_req_amdi0019(enum psp_i2c_req_type
> i2c_req_type)
> > {
> > struct psp_i2c_req *req;
> > - unsigned long start;
> > int status, ret;
> >
> > /* Allocate command-response buffer */
> > req = kzalloc(sizeof(*req), GFP_KERNEL);
> > if (!req)
> > return -ENOMEM;
> > -
> > req->hdr.payload_size = sizeof(*req);
> > req->type = i2c_req_type;
> > -
> > - start = jiffies;
> > ret = read_poll_timeout(psp_send_check_i2c_req, status,
> > (status != -EBUSY),
> > PSP_I2C_REQ_RETRY_DELAY_US,
> > PSP_I2C_REQ_RETRY_CNT *
> PSP_I2C_REQ_RETRY_DELAY_US,
> > 0, req);
> > - if (ret) {
> > + kfree(req);
> > +
> > + if (ret)
> > dev_err(psp_i2c_dev, "Timed out waiting for PSP to %s I2C bus\n",
> > (i2c_req_type == PSP_I2C_REQ_ACQUIRE) ?
> > "release" : "acquire");
> > - goto cleanup;
> > - }
> >
> > - ret = status;
> > + return ret ? ret : status;
> > +}
> > +
> > +static int psp_send_i2c_req(enum psp_i2c_req_type i2c_req_type)
> > +{
> > + const char *hid = acpi_device_hid(ACPI_COMPANION(psp_i2c_dev));
> > + unsigned long start = jiffies;
> > + int ret;
> > +
> > + /* Use doorbell for Skyrim and mailbox for Cezanne */
> > + if (!strcmp(hid, "AMDI0020"))
> > + ret = psp_ring_platform_doorbell(i2c_req_type);
> > + else
> > + ret = psp_send_i2c_req_amdi0019(i2c_req_type);
> > +
> > if (ret) {
> > dev_err(psp_i2c_dev, "PSP communication error\n");
> > goto cleanup;
> > @@ -115,7 +125,6 @@ static int psp_send_i2c_req(enum
> psp_i2c_req_type i2c_req_type)
> > psp_i2c_mbox_fail = true;
> > }
> >
> > - kfree(req);
> > return ret;
> > }
> >
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> > index 89ad88c54754..5ca71bda9ac2 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -51,6 +51,7 @@ static const struct acpi_device_id
> dw_i2c_acpi_match[] = {
> > { "AMD0010", ACCESS_INTR_MASK },
> > { "AMDI0010", ACCESS_INTR_MASK },
> > { "AMDI0019", ACCESS_INTR_MASK | ARBITRATION_SEMAPHORE },
> > + { "AMDI0020", ACCESS_INTR_MASK | ARBITRATION_SEMAPHORE },
> > { "AMDI0510", 0 },
> > { "APMC0D0F", 0 },
> > { "HISI02A1", 0 },
> > --
> > 2.34.1
> >

2023-03-06 11:37:47

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH v2 8/9] i2c: designware: Add doorbell support for Skyrim

On Fri, Mar 03, 2023 at 03:00:55PM +0000, Limonciello, Mario wrote:
> > -----Original Message-----
> > From: Grzegorz Bernacki <[email protected]>
> > Sent: Friday, March 3, 2023 06:00

> > I am not sure if adding a new ACPI ID is a good idea. Actually we are
> > talking about the same devices. The only difference is in the
> > communication protocol between PSP and CPU. This could be easily
> > detected at runtime by checking cpu id. There is no need to introduce
> > a new id and create dependency on the FW version.
>
> An ACPI ID seems more scalable to me to represent the difference in protocol.
> Otherwise what happens when the follow on to Skyrim comes? Do you add
> a new ID/case to the code? If it was an ACPI ID then it's a one line change
> in the firmware to represent this.
>
> What I'll do for v3 is do with a CPU ID in this patch, and then introduce an ACPI
> ID in a new patch. You can test it without the ACPI ID, and when it's working
> patch the BIOS with the new ID and see if that continues to work.

I don't think we need this complexity.

Please, use just an ACPI ID and that's it. For testing one may apply additional
(HACK) patch(es).

As telling from my experience, to distinguish such things via CPU ID is a bad
idea. Yes, we (used?) to have a PITA (find yourself what this stands for :)
times with the (mixed) approach. So, no, please don't add CPU ID into the I?C
platform driver (keyword: platform).

--
With Best Regards,
Andy Shevchenko