| Message ID | 20240724071245.3833404-3-Shyam-sundar.S-k@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce initial AMD I3C HCI driver support | expand |
Hi On 7/24/24 10:12 AM, Shyam Sundar S K wrote: > The AMD HCI controller currently only supports PIO mode but exposes DMA > rings to the OS, which leads to the controller being configured in DMA > mode. To address this, add a quirk to avoid configuring the controller in > DMA mode and default to PIO mode. > > Additionally, introduce a generic quirk infrastructure to the mipi-i3c-hci > driver to facilitate seamless future quirk additions. > > Co-developed-by: Krishnamoorthi M <krishnamoorthi.m@amd.com> > Signed-off-by: Krishnamoorthi M <krishnamoorthi.m@amd.com> > Co-developed-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com> > Signed-off-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com> > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> > --- > drivers/i3c/master/mipi-i3c-hci/Makefile | 3 ++- > drivers/i3c/master/mipi-i3c-hci/core.c | 15 ++++++++++++++- > drivers/i3c/master/mipi-i3c-hci/hci.h | 3 +++ > 3 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/drivers/i3c/master/mipi-i3c-hci/Makefile b/drivers/i3c/master/mipi-i3c-hci/Makefile > index a658e7b8262c..1f8cd5c48fde 100644 > --- a/drivers/i3c/master/mipi-i3c-hci/Makefile > +++ b/drivers/i3c/master/mipi-i3c-hci/Makefile > @@ -3,4 +3,5 @@ > obj-$(CONFIG_MIPI_I3C_HCI) += mipi-i3c-hci.o > mipi-i3c-hci-y := core.o ext_caps.o pio.o dma.o \ > cmd_v1.o cmd_v2.o \ > - dat_v1.o dct_v1.o > + dat_v1.o dct_v1.o \ > + hci_quirks.o This doesn't build since hci_quirks.c is added by the patch 4/5. One idea below. > diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c > index dbc8c38bd962..8bb422ab1d01 100644 > --- a/drivers/i3c/master/mipi-i3c-hci/core.c > +++ b/drivers/i3c/master/mipi-i3c-hci/core.c > @@ -33,6 +33,7 @@ > #define reg_clear(r, v) reg_write(r, reg_read(r) & ~(v)) > > #define HCI_VERSION 0x00 /* HCI Version (in BCD) */ > +#define HCI_VERSION_V1 0x100 /* MIPI HCI Version number V1.0 */ > > #define HC_CONTROL 0x04 > #define HC_CONTROL_BUS_ENABLE BIT(31) > @@ -745,6 +746,14 @@ static int i3c_hci_init(struct i3c_hci *hci) > return -EINVAL; > } > > + /* Initialize quirks for AMD platforms */ > + amd_i3c_hci_quirks_init(hci); > + > + regval = reg_read(HCI_VERSION); > + > + if (hci->quirks & HCI_QUIRK_AMD_PIO_MODE) > + hci->RHS_regs = NULL; > + > /* Try activating DMA operations first */ > if (hci->RHS_regs) { > reg_clear(HC_CONTROL, HC_CONTROL_PIO_MODE); > @@ -760,7 +769,11 @@ static int i3c_hci_init(struct i3c_hci *hci) > /* If no DMA, try PIO */ > if (!hci->io && hci->PIO_regs) { > reg_set(HC_CONTROL, HC_CONTROL_PIO_MODE); > - if (!(reg_read(HC_CONTROL) & HC_CONTROL_PIO_MODE)) { > + /* > + * HC_CONTROL_PIO_MODE bit not present in HC_CONTROL register w.r.t V1.0 > + * specification. So skip checking PIO_MODE bit status > + */ > + if (regval != HCI_VERSION_V1 && !(reg_read(HC_CONTROL) & HC_CONTROL_PIO_MODE)) { > dev_err(&hci->master.dev, "DMA mode is stuck\n"); > ret = -EIO; > } else { This is true, I see this now from pre-v1.0, v1.0. v1.1 and v1.2 specs too, HC_CONTROL_PIO_MODE bit is present only after v1.0. And therefore version != HCI_VERSION_V1 check is not fully correct since bit is not present in pre-v1.0 HW versions either. I'd split this patch and do version check alone here (perhaps as a first patch) and do quirk stuff later where hci_quirks.c is added.
Hi, On 8/2/2024 19:28, Jarkko Nikula wrote: > Hi > > On 7/24/24 10:12 AM, Shyam Sundar S K wrote: >> The AMD HCI controller currently only supports PIO mode but exposes DMA >> rings to the OS, which leads to the controller being configured in DMA >> mode. To address this, add a quirk to avoid configuring the >> controller in >> DMA mode and default to PIO mode. >> >> Additionally, introduce a generic quirk infrastructure to the >> mipi-i3c-hci >> driver to facilitate seamless future quirk additions. >> >> Co-developed-by: Krishnamoorthi M <krishnamoorthi.m@amd.com> >> Signed-off-by: Krishnamoorthi M <krishnamoorthi.m@amd.com> >> Co-developed-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com> >> Signed-off-by: Guruvendra Punugupati <Guruvendra.Punugupati@amd.com> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >> --- >> drivers/i3c/master/mipi-i3c-hci/Makefile | 3 ++- >> drivers/i3c/master/mipi-i3c-hci/core.c | 15 ++++++++++++++- >> drivers/i3c/master/mipi-i3c-hci/hci.h | 3 +++ >> 3 files changed, 19 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/i3c/master/mipi-i3c-hci/Makefile >> b/drivers/i3c/master/mipi-i3c-hci/Makefile >> index a658e7b8262c..1f8cd5c48fde 100644 >> --- a/drivers/i3c/master/mipi-i3c-hci/Makefile >> +++ b/drivers/i3c/master/mipi-i3c-hci/Makefile >> @@ -3,4 +3,5 @@ >> obj-$(CONFIG_MIPI_I3C_HCI) += mipi-i3c-hci.o >> mipi-i3c-hci-y := core.o ext_caps.o pio.o dma.o \ >> cmd_v1.o cmd_v2.o \ >> - dat_v1.o dct_v1.o >> + dat_v1.o dct_v1.o \ >> + hci_quirks.o > > This doesn't build since hci_quirks.c is added by the patch 4/5. One > idea below. > Ah! will fix this in next revision. >> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c >> b/drivers/i3c/master/mipi-i3c-hci/core.c >> index dbc8c38bd962..8bb422ab1d01 100644 >> --- a/drivers/i3c/master/mipi-i3c-hci/core.c >> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c >> @@ -33,6 +33,7 @@ >> #define reg_clear(r, v) reg_write(r, reg_read(r) & ~(v)) >> #define HCI_VERSION 0x00 /* HCI Version (in BCD) */ >> +#define HCI_VERSION_V1 0x100 /* MIPI HCI Version >> number V1.0 */ >> #define HC_CONTROL 0x04 >> #define HC_CONTROL_BUS_ENABLE BIT(31) >> @@ -745,6 +746,14 @@ static int i3c_hci_init(struct i3c_hci *hci) >> return -EINVAL; >> } >> + /* Initialize quirks for AMD platforms */ >> + amd_i3c_hci_quirks_init(hci); >> + >> + regval = reg_read(HCI_VERSION); >> + >> + if (hci->quirks & HCI_QUIRK_AMD_PIO_MODE) >> + hci->RHS_regs = NULL; >> + >> /* Try activating DMA operations first */ >> if (hci->RHS_regs) { >> reg_clear(HC_CONTROL, HC_CONTROL_PIO_MODE); >> @@ -760,7 +769,11 @@ static int i3c_hci_init(struct i3c_hci *hci) >> /* If no DMA, try PIO */ >> if (!hci->io && hci->PIO_regs) { >> reg_set(HC_CONTROL, HC_CONTROL_PIO_MODE); >> - if (!(reg_read(HC_CONTROL) & HC_CONTROL_PIO_MODE)) { >> + /* >> + * HC_CONTROL_PIO_MODE bit not present in HC_CONTROL >> register w.r.t V1.0 >> + * specification. So skip checking PIO_MODE bit status >> + */ >> + if (regval != HCI_VERSION_V1 && !(reg_read(HC_CONTROL) & >> HC_CONTROL_PIO_MODE)) { >> dev_err(&hci->master.dev, "DMA mode is stuck\n"); >> ret = -EIO; >> } else { > > This is true, I see this now from pre-v1.0, v1.0. v1.1 and v1.2 specs > too, HC_CONTROL_PIO_MODE bit is present only after v1.0. And therefore > version != HCI_VERSION_V1 check is not fully correct since bit is not > present in pre-v1.0 HW versions either. > Agreed. HC_CONTROL_PIO_MODE is only present in v1.1 and v1.2. anything below v1.0, the version check handling is already present; switch (regval & ~0xf) { case 0x100: /* version 1.0 */ case 0x110: /* version 1.1 */ case 0x200: /* version 2.0 */ break; default: dev_err(&hci->master.dev, "unsupported HCI version\n"); return -EPROTONOSUPPORT; } Let me know your thoughts. Thanks, Shyam > I'd split this patch and do version check alone here (perhaps as a > first patch) and do quirk stuff later where hci_quirks.c is added.
On 8/5/24 12:26 PM, Shyam Sundar S K wrote: > Hi, > > On 8/2/2024 19:28, Jarkko Nikula wrote: >> >> This is true, I see this now from pre-v1.0, v1.0. v1.1 and v1.2 specs >> too, HC_CONTROL_PIO_MODE bit is present only after v1.0. And therefore >> version != HCI_VERSION_V1 check is not fully correct since bit is not >> present in pre-v1.0 HW versions either. >> > > Agreed. HC_CONTROL_PIO_MODE is only present in v1.1 and v1.2. > > anything below v1.0, the version check handling is already present; > > switch (regval & ~0xf) { > case 0x100: /* version 1.0 */ > case 0x110: /* version 1.1 */ > case 0x200: /* version 2.0 */ > break; > default: > dev_err(&hci->master.dev, "unsupported HCI version\n"); > return -EPROTONOSUPPORT; > } > > Let me know your thoughts. > Yeah, true. I'd still use version > v1.0 check because that reflects the current situation. Mostly to avoid confusion when reading the code but also if someone adds support for pre-v1.0 HW (perhaps unlikely) and then doesn't need to change version check here.
diff --git a/drivers/i3c/master/mipi-i3c-hci/Makefile b/drivers/i3c/master/mipi-i3c-hci/Makefile index a658e7b8262c..1f8cd5c48fde 100644 --- a/drivers/i3c/master/mipi-i3c-hci/Makefile +++ b/drivers/i3c/master/mipi-i3c-hci/Makefile @@ -3,4 +3,5 @@ obj-$(CONFIG_MIPI_I3C_HCI) += mipi-i3c-hci.o mipi-i3c-hci-y := core.o ext_caps.o pio.o dma.o \ cmd_v1.o cmd_v2.o \ - dat_v1.o dct_v1.o + dat_v1.o dct_v1.o \ + hci_quirks.o diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c index dbc8c38bd962..8bb422ab1d01 100644 --- a/drivers/i3c/master/mipi-i3c-hci/core.c +++ b/drivers/i3c/master/mipi-i3c-hci/core.c @@ -33,6 +33,7 @@ #define reg_clear(r, v) reg_write(r, reg_read(r) & ~(v)) #define HCI_VERSION 0x00 /* HCI Version (in BCD) */ +#define HCI_VERSION_V1 0x100 /* MIPI HCI Version number V1.0 */ #define HC_CONTROL 0x04 #define HC_CONTROL_BUS_ENABLE BIT(31) @@ -745,6 +746,14 @@ static int i3c_hci_init(struct i3c_hci *hci) return -EINVAL; } + /* Initialize quirks for AMD platforms */ + amd_i3c_hci_quirks_init(hci); + + regval = reg_read(HCI_VERSION); + + if (hci->quirks & HCI_QUIRK_AMD_PIO_MODE) + hci->RHS_regs = NULL; + /* Try activating DMA operations first */ if (hci->RHS_regs) { reg_clear(HC_CONTROL, HC_CONTROL_PIO_MODE); @@ -760,7 +769,11 @@ static int i3c_hci_init(struct i3c_hci *hci) /* If no DMA, try PIO */ if (!hci->io && hci->PIO_regs) { reg_set(HC_CONTROL, HC_CONTROL_PIO_MODE); - if (!(reg_read(HC_CONTROL) & HC_CONTROL_PIO_MODE)) { + /* + * HC_CONTROL_PIO_MODE bit not present in HC_CONTROL register w.r.t V1.0 + * specification. So skip checking PIO_MODE bit status + */ + if (regval != HCI_VERSION_V1 && !(reg_read(HC_CONTROL) & HC_CONTROL_PIO_MODE)) { dev_err(&hci->master.dev, "DMA mode is stuck\n"); ret = -EIO; } else { diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h index f94d95e024be..046b65d43e63 100644 --- a/drivers/i3c/master/mipi-i3c-hci/hci.h +++ b/drivers/i3c/master/mipi-i3c-hci/hci.h @@ -135,6 +135,7 @@ struct i3c_hci_dev_data { /* list of quirks */ #define HCI_QUIRK_RAW_CCC BIT(1) /* CCC framing must be explicit */ +#define HCI_QUIRK_AMD_PIO_MODE BIT(2) /* Set PIO mode for AMD platforms */ /* global functions */ @@ -142,4 +143,6 @@ void mipi_i3c_hci_resume(struct i3c_hci *hci); void mipi_i3c_hci_pio_reset(struct i3c_hci *hci); void mipi_i3c_hci_dct_index_reset(struct i3c_hci *hci); +void amd_i3c_hci_quirks_init(struct i3c_hci *hci); + #endif