* [PATCH -next 0/2] Add i2c arbitration support for new SoCs
@ 2022-09-16 13:18 Jan Dabros
2022-09-16 13:18 ` [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access Jan Dabros
` (2 more replies)
0 siblings, 3 replies; 30+ messages in thread
From: Jan Dabros @ 2022-09-16 13:18 UTC (permalink / raw)
To: linux-kernel, linux-i2c, jarkko.nikula, andriy.shevchenko
Cc: wsa, rrangel, upstream, mario.limonciello, jsd
This patchset comprises changes into i2c-designware-amdpsp.c module
which aims to add support for new SoCs, while keep backward
compatibility with Cezanne platforms.
Beside new algorithm introduced for the PSP-x86 communication, it also
switches from MSR/MMIO access to SMN (System Management Network) since
only the latter is working on both old new revisions of SoCs.
Jan Dabros (2):
i2c: designware: Switch from using MMIO access to SMN access
i2c: designware: Add support for new SoCs in AMDPSP driver
arch/x86/include/asm/amd_nb.h | 1 +
arch/x86/kernel/amd_nb.c | 3 +-
drivers/i2c/busses/i2c-designware-amdpsp.c | 199 +++++++++++++-------
drivers/i2c/busses/i2c-designware-core.h | 1 -
drivers/i2c/busses/i2c-designware-platdrv.c | 1 -
5 files changed, 134 insertions(+), 71 deletions(-)
--
2.37.3.968.ga6b4b080e4-goog
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2022-09-16 13:18 [PATCH -next 0/2] Add i2c arbitration support for new SoCs Jan Dabros @ 2022-09-16 13:18 ` Jan Dabros 2022-09-16 17:38 ` Limonciello, Mario 2022-09-19 13:59 ` Andy Shevchenko 2022-09-16 13:18 ` [PATCH -next 2/2] i2c: designware: Add support for new SoCs in AMDPSP driver Jan Dabros 2022-09-19 13:59 ` [PATCH -next 0/2] Add i2c arbitration support for new SoCs Andy Shevchenko 2 siblings, 2 replies; 30+ messages in thread From: Jan Dabros @ 2022-09-16 13:18 UTC (permalink / raw) To: linux-kernel, linux-i2c, jarkko.nikula, andriy.shevchenko Cc: wsa, rrangel, upstream, mario.limonciello, jsd Due to a change in silicon compared to Cezanne, in future revisions MSR access can't be used to get the base address of the PSP MMIO region that contains the PSP mailbox interface. Modify driver to use SMN access also for Cezanne platforms (it is working there) in order to simplify codebase when adding support for new SoC versions. Export amd_cache_northbridges() which was unexported by e1907d3: "x86/amd_nb: Unexport amd_cache_northbridges()" since function which registers i2c-designware-platdrv is a subsys_initcall that is executed before fs_initcall (when enumeration of NB descriptors occurs). Thus in order to use SMN accesses it's necessary to explicitly call amd_cache_northrbidges() from within this driver. Signed-off-by: Jan Dabros <jsd@semihalf.com> --- arch/x86/include/asm/amd_nb.h | 1 + arch/x86/kernel/amd_nb.c | 3 +- drivers/i2c/busses/i2c-designware-amdpsp.c | 112 +++++++++++--------- drivers/i2c/busses/i2c-designware-core.h | 1 - drivers/i2c/busses/i2c-designware-platdrv.c | 1 - 5 files changed, 66 insertions(+), 52 deletions(-) diff --git a/arch/x86/include/asm/amd_nb.h b/arch/x86/include/asm/amd_nb.h index ed0eaf65c437..00d1a400b7a1 100644 --- a/arch/x86/include/asm/amd_nb.h +++ b/arch/x86/include/asm/amd_nb.h @@ -16,6 +16,7 @@ extern const struct amd_nb_bus_dev_range amd_nb_bus_dev_ranges[]; extern bool early_is_amd_nb(u32 value); extern struct resource *amd_get_mmconfig_range(struct resource *res); +extern int amd_cache_northbridges(void); extern void amd_flush_garts(void); extern int amd_numa_init(void); extern int amd_get_subcaches(int); diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c index 4266b64631a4..2077b6cfa8ad 100644 --- a/arch/x86/kernel/amd_nb.c +++ b/arch/x86/kernel/amd_nb.c @@ -201,7 +201,7 @@ int amd_smn_write(u16 node, u32 address, u32 value) EXPORT_SYMBOL_GPL(amd_smn_write); -static int amd_cache_northbridges(void) +int amd_cache_northbridges(void) { const struct pci_device_id *misc_ids = amd_nb_misc_ids; const struct pci_device_id *link_ids = amd_nb_link_ids; @@ -303,6 +303,7 @@ static int amd_cache_northbridges(void) return 0; } +EXPORT_SYMBOL_GPL(amd_cache_northbridges); /* * Ignores subdevice/subvendor but as far as I can figure out diff --git a/drivers/i2c/busses/i2c-designware-amdpsp.c b/drivers/i2c/busses/i2c-designware-amdpsp.c index 8f36167bce62..1d467fc83f59 100644 --- a/drivers/i2c/busses/i2c-designware-amdpsp.c +++ b/drivers/i2c/busses/i2c-designware-amdpsp.c @@ -8,12 +8,11 @@ #include <linux/types.h> #include <linux/workqueue.h> +#include <asm/amd_nb.h> #include <asm/msr.h> #include "i2c-designware-core.h" -#define MSR_AMD_PSP_ADDR 0xc00110a2 -#define PSP_MBOX_OFFSET 0x10570 #define PSP_CMD_TIMEOUT_US (500 * USEC_PER_MSEC) #define PSP_I2C_RESERVATION_TIME_MS 100 @@ -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 + 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(); } -static int psp_mbox_probe(void) +static int psp_smn_write(u32 smn_addr, u32 value) { - unsigned long mbox_addr; - int ret; - - ret = psp_get_mbox_addr(&mbox_addr); - if (ret) - return ret; - - mbox_iomem = ioremap(mbox_addr, sizeof(struct psp_mbox)); - if (!mbox_iomem) - return -ENOMEM; + return amd_smn_write(0, smn_addr, value); +} - return 0; +static int psp_smn_read(u32 smn_addr, u32 *value) +{ + return amd_smn_read(0, smn_addr, value); } /* Recovery field should be equal 0 to start sending commands */ -static int psp_check_mbox_recovery(struct psp_mbox __iomem *mbox) +static int psp_check_mbox_recovery(void) { u32 tmp; + int status; - tmp = readl(&mbox->cmd_fields); + status = psp_smn_read(PSP_MBOX_CMD_OFFSET, &tmp); + if (status) + return status; return FIELD_GET(PSP_MBOX_FIELDS_RECOVERY, tmp); } -static int psp_wait_cmd(struct psp_mbox __iomem *mbox) +static int psp_wait_cmd(void) { u32 tmp, expected; + int ret, status; /* Expect mbox_cmd to be cleared and ready bit to be set by PSP */ expected = FIELD_PREP(PSP_MBOX_FIELDS_READY, 1); @@ -113,30 +106,55 @@ static int psp_wait_cmd(struct psp_mbox __iomem *mbox) * Check for readiness of PSP mailbox in a tight loop in order to * process further as soon as command was consumed. */ - return readl_poll_timeout(&mbox->cmd_fields, tmp, (tmp == expected), - 0, PSP_CMD_TIMEOUT_US); + ret = read_poll_timeout(psp_smn_read, status, + (status < 0) || (tmp == expected), 0, + PSP_CMD_TIMEOUT_US, 0, PSP_MBOX_CMD_OFFSET, + &tmp); + if (status) + ret = status; + + return ret; } /* Status equal to 0 means that PSP succeed processing command */ -static u32 psp_check_mbox_sts(struct psp_mbox __iomem *mbox) +static int psp_check_mbox_sts(void) { u32 cmd_reg; + int status; - cmd_reg = readl(&mbox->cmd_fields); + status = psp_smn_read(PSP_MBOX_CMD_OFFSET, &cmd_reg); + if (status) + return status; return FIELD_GET(PSP_MBOX_FIELDS_STS, cmd_reg); } +static int psp_wr_mbox_buffer(phys_addr_t buf) +{ + u32 buf_addr_h = upper_32_bits(buf); + u32 buf_addr_l = lower_32_bits(buf); + int status; + + status = psp_smn_write(PSP_MBOX_BUFFER_H_OFFSET, buf_addr_h); + if (status) + return status; + + status = psp_smn_write(PSP_MBOX_BUFFER_L_OFFSET, buf_addr_l); + if (status) + return status; + + return 0; +} + static int psp_send_cmd(struct psp_i2c_req *req) { - struct psp_mbox __iomem *mbox = mbox_iomem; phys_addr_t req_addr; u32 cmd_reg; - if (psp_check_mbox_recovery(mbox)) + if (psp_check_mbox_recovery()) return -EIO; - if (psp_wait_cmd(mbox)) + if (psp_wait_cmd()) return -EBUSY; /* @@ -145,16 +163,18 @@ static int psp_send_cmd(struct psp_i2c_req *req) * PSP. Use physical address of buffer, since PSP will map this region. */ req_addr = __psp_pa((void *)req); - writeq(req_addr, &mbox->i2c_req_addr); + if (psp_wr_mbox_buffer(req_addr)) + return -EIO; /* Write command register to trigger processing */ cmd_reg = FIELD_PREP(PSP_MBOX_FIELDS_CMD, PSP_I2C_REQ_BUS_CMD); - writel(cmd_reg, &mbox->cmd_fields); + if (psp_smn_write(PSP_MBOX_CMD_OFFSET, cmd_reg)) + return -EIO; - if (psp_wait_cmd(mbox)) + if (psp_wait_cmd()) return -ETIMEDOUT; - if (psp_check_mbox_sts(mbox)) + if (psp_check_mbox_sts()) return -EIO; return 0; @@ -417,9 +437,3 @@ int i2c_dw_amdpsp_probe_lock_support(struct dw_i2c_dev *dev) return 0; } - -/* Unmap area used as a mailbox with PSP */ -void i2c_dw_amdpsp_remove_lock_support(struct dw_i2c_dev *dev) -{ - iounmap(mbox_iomem); -} diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index 70b80e710990..80dad6c8f321 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -382,7 +382,6 @@ int i2c_dw_baytrail_probe_lock_support(struct dw_i2c_dev *dev); #if IS_ENABLED(CONFIG_I2C_DESIGNWARE_AMDPSP) int i2c_dw_amdpsp_probe_lock_support(struct dw_i2c_dev *dev); -void i2c_dw_amdpsp_remove_lock_support(struct dw_i2c_dev *dev); #endif int i2c_dw_validate_speed(struct dw_i2c_dev *dev); diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index ba043b547393..99f54fe583e1 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -214,7 +214,6 @@ static const struct i2c_dw_semaphore_callbacks i2c_dw_semaphore_cb_table[] = { #ifdef CONFIG_I2C_DESIGNWARE_AMDPSP { .probe = i2c_dw_amdpsp_probe_lock_support, - .remove = i2c_dw_amdpsp_remove_lock_support, }, #endif {} -- 2.37.3.968.ga6b4b080e4-goog ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2022-09-16 13:18 ` [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access Jan Dabros @ 2022-09-16 17:38 ` Limonciello, Mario 2022-09-20 16:24 ` Jan Dąbroś 2022-09-19 13:59 ` Andy Shevchenko 1 sibling, 1 reply; 30+ messages in thread From: Limonciello, Mario @ 2022-09-16 17:38 UTC (permalink / raw) To: Jan Dabros, linux-kernel, linux-i2c, jarkko.nikula, andriy.shevchenko Cc: wsa, rrangel, upstream, Muralidhara M K, Naveen Krishna Chatradhi, Borislav Petkov + the people who signed off on that original commit that is essentially unreverted. Comments below. On 9/16/2022 08:18, Jan Dabros wrote: > Due to a change in silicon compared to Cezanne, in future revisions MSR > access can't be used to get the base address of the PSP MMIO region that > contains the PSP mailbox interface. > > Modify driver to use SMN access also for Cezanne platforms (it is > working there) in order to simplify codebase when adding support for new > SoC versions. > > Export amd_cache_northbridges() which was unexported by > > e1907d3: "x86/amd_nb: Unexport amd_cache_northbridges()" > > since function which registers i2c-designware-platdrv is a > subsys_initcall that is executed before fs_initcall (when enumeration of > NB descriptors occurs). Thus in order to use SMN accesses it's necessary > to explicitly call amd_cache_northrbidges() from within this driver. > > Signed-off-by: Jan Dabros <jsd@semihalf.com> > --- > arch/x86/include/asm/amd_nb.h | 1 + > arch/x86/kernel/amd_nb.c | 3 +- > drivers/i2c/busses/i2c-designware-amdpsp.c | 112 +++++++++++--------- > drivers/i2c/busses/i2c-designware-core.h | 1 - > drivers/i2c/busses/i2c-designware-platdrv.c | 1 - > 5 files changed, 66 insertions(+), 52 deletions(-) > > diff --git a/arch/x86/include/asm/amd_nb.h b/arch/x86/include/asm/amd_nb.h > index ed0eaf65c437..00d1a400b7a1 100644 > --- a/arch/x86/include/asm/amd_nb.h > +++ b/arch/x86/include/asm/amd_nb.h > @@ -16,6 +16,7 @@ extern const struct amd_nb_bus_dev_range amd_nb_bus_dev_ranges[]; > > extern bool early_is_amd_nb(u32 value); > extern struct resource *amd_get_mmconfig_range(struct resource *res); > +extern int amd_cache_northbridges(void); > extern void amd_flush_garts(void); > extern int amd_numa_init(void); > extern int amd_get_subcaches(int); > diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c > index 4266b64631a4..2077b6cfa8ad 100644 > --- a/arch/x86/kernel/amd_nb.c > +++ b/arch/x86/kernel/amd_nb.c > @@ -201,7 +201,7 @@ int amd_smn_write(u16 node, u32 address, u32 value) > EXPORT_SYMBOL_GPL(amd_smn_write); > > > -static int amd_cache_northbridges(void) > +int amd_cache_northbridges(void) > { > const struct pci_device_id *misc_ids = amd_nb_misc_ids; > const struct pci_device_id *link_ids = amd_nb_link_ids; > @@ -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. > /* > * Ignores subdevice/subvendor but as far as I can figure out > diff --git a/drivers/i2c/busses/i2c-designware-amdpsp.c b/drivers/i2c/busses/i2c-designware-amdpsp.c > index 8f36167bce62..1d467fc83f59 100644 > --- a/drivers/i2c/busses/i2c-designware-amdpsp.c > +++ b/drivers/i2c/busses/i2c-designware-amdpsp.c > @@ -8,12 +8,11 @@ > #include <linux/types.h> > #include <linux/workqueue.h> > > +#include <asm/amd_nb.h> > #include <asm/msr.h> > > #include "i2c-designware-core.h" > > -#define MSR_AMD_PSP_ADDR 0xc00110a2 > -#define PSP_MBOX_OFFSET 0x10570 > #define PSP_CMD_TIMEOUT_US (500 * USEC_PER_MSEC) > > #define PSP_I2C_RESERVATION_TIME_MS 100 > @@ -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. > 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. > } > > -static int psp_mbox_probe(void) > +static int psp_smn_write(u32 smn_addr, u32 value) > { > - unsigned long mbox_addr; > - int ret; > - > - ret = psp_get_mbox_addr(&mbox_addr); > - if (ret) > - return ret; > - > - mbox_iomem = ioremap(mbox_addr, sizeof(struct psp_mbox)); > - if (!mbox_iomem) > - return -ENOMEM; > + return amd_smn_write(0, smn_addr, value); > +} > > - return 0; > +static int psp_smn_read(u32 smn_addr, u32 *value) > +{ > + return amd_smn_read(0, smn_addr, value); > } > > /* Recovery field should be equal 0 to start sending commands */ > -static int psp_check_mbox_recovery(struct psp_mbox __iomem *mbox) > +static int psp_check_mbox_recovery(void) > { > u32 tmp; > + int status; > > - tmp = readl(&mbox->cmd_fields); > + status = psp_smn_read(PSP_MBOX_CMD_OFFSET, &tmp); > + if (status) > + return status; > > return FIELD_GET(PSP_MBOX_FIELDS_RECOVERY, tmp); > } > > -static int psp_wait_cmd(struct psp_mbox __iomem *mbox) > +static int psp_wait_cmd(void) > { > u32 tmp, expected; > + int ret, status; > > /* Expect mbox_cmd to be cleared and ready bit to be set by PSP */ > expected = FIELD_PREP(PSP_MBOX_FIELDS_READY, 1); > @@ -113,30 +106,55 @@ static int psp_wait_cmd(struct psp_mbox __iomem *mbox) > * Check for readiness of PSP mailbox in a tight loop in order to > * process further as soon as command was consumed. > */ > - return readl_poll_timeout(&mbox->cmd_fields, tmp, (tmp == expected), > - 0, PSP_CMD_TIMEOUT_US); > + ret = read_poll_timeout(psp_smn_read, status, > + (status < 0) || (tmp == expected), 0, > + PSP_CMD_TIMEOUT_US, 0, PSP_MBOX_CMD_OFFSET, > + &tmp); > + if (status) > + ret = status; > + > + return ret; > } > > /* Status equal to 0 means that PSP succeed processing command */ > -static u32 psp_check_mbox_sts(struct psp_mbox __iomem *mbox) > +static int psp_check_mbox_sts(void) > { > u32 cmd_reg; > + int status; > > - cmd_reg = readl(&mbox->cmd_fields); > + status = psp_smn_read(PSP_MBOX_CMD_OFFSET, &cmd_reg); > + if (status) > + return status; > > return FIELD_GET(PSP_MBOX_FIELDS_STS, cmd_reg); > } > > +static int psp_wr_mbox_buffer(phys_addr_t buf) > +{ > + u32 buf_addr_h = upper_32_bits(buf); > + u32 buf_addr_l = lower_32_bits(buf); > + int status; > + > + status = psp_smn_write(PSP_MBOX_BUFFER_H_OFFSET, buf_addr_h); > + if (status) > + return status; > + > + status = psp_smn_write(PSP_MBOX_BUFFER_L_OFFSET, buf_addr_l); > + if (status) > + return status; > + > + return 0; > +} > + > static int psp_send_cmd(struct psp_i2c_req *req) > { > - struct psp_mbox __iomem *mbox = mbox_iomem; > phys_addr_t req_addr; > u32 cmd_reg; > > - if (psp_check_mbox_recovery(mbox)) > + if (psp_check_mbox_recovery()) > return -EIO; > > - if (psp_wait_cmd(mbox)) > + if (psp_wait_cmd()) > return -EBUSY; > > /* > @@ -145,16 +163,18 @@ static int psp_send_cmd(struct psp_i2c_req *req) > * PSP. Use physical address of buffer, since PSP will map this region. > */ > req_addr = __psp_pa((void *)req); > - writeq(req_addr, &mbox->i2c_req_addr); > + if (psp_wr_mbox_buffer(req_addr)) > + return -EIO; > > /* Write command register to trigger processing */ > cmd_reg = FIELD_PREP(PSP_MBOX_FIELDS_CMD, PSP_I2C_REQ_BUS_CMD); > - writel(cmd_reg, &mbox->cmd_fields); > + if (psp_smn_write(PSP_MBOX_CMD_OFFSET, cmd_reg)) > + return -EIO; > > - if (psp_wait_cmd(mbox)) > + if (psp_wait_cmd()) > return -ETIMEDOUT; > > - if (psp_check_mbox_sts(mbox)) > + if (psp_check_mbox_sts()) > return -EIO; > > return 0; > @@ -417,9 +437,3 @@ int i2c_dw_amdpsp_probe_lock_support(struct dw_i2c_dev *dev) > > return 0; > } > - > -/* Unmap area used as a mailbox with PSP */ > -void i2c_dw_amdpsp_remove_lock_support(struct dw_i2c_dev *dev) > -{ > - iounmap(mbox_iomem); > -} > diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h > index 70b80e710990..80dad6c8f321 100644 > --- a/drivers/i2c/busses/i2c-designware-core.h > +++ b/drivers/i2c/busses/i2c-designware-core.h > @@ -382,7 +382,6 @@ int i2c_dw_baytrail_probe_lock_support(struct dw_i2c_dev *dev); > > #if IS_ENABLED(CONFIG_I2C_DESIGNWARE_AMDPSP) > int i2c_dw_amdpsp_probe_lock_support(struct dw_i2c_dev *dev); > -void i2c_dw_amdpsp_remove_lock_support(struct dw_i2c_dev *dev); > #endif > > int i2c_dw_validate_speed(struct dw_i2c_dev *dev); > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c > index ba043b547393..99f54fe583e1 100644 > --- a/drivers/i2c/busses/i2c-designware-platdrv.c > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c > @@ -214,7 +214,6 @@ static const struct i2c_dw_semaphore_callbacks i2c_dw_semaphore_cb_table[] = { > #ifdef CONFIG_I2C_DESIGNWARE_AMDPSP > { > .probe = i2c_dw_amdpsp_probe_lock_support, > - .remove = i2c_dw_amdpsp_remove_lock_support, > }, > #endif > {} ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2022-09-16 17:38 ` Limonciello, Mario @ 2022-09-20 16:24 ` Jan Dąbroś 2022-09-21 20:12 ` Borislav Petkov 0 siblings, 1 reply; 30+ messages in thread From: Jan Dąbroś @ 2022-09-20 16:24 UTC (permalink / raw) To: Limonciello, Mario Cc: linux-kernel, linux-i2c, jarkko.nikula, andriy.shevchenko, wsa, rrangel, upstream, Muralidhara M K, Naveen Krishna Chatradhi, Borislav Petkov 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2022-09-20 16:24 ` Jan Dąbroś @ 2022-09-21 20:12 ` Borislav Petkov 2022-09-21 20:19 ` Limonciello, Mario 0 siblings, 1 reply; 30+ messages in thread From: Borislav Petkov @ 2022-09-21 20:12 UTC (permalink / raw) To: Jan Dąbroś Cc: Limonciello, Mario, linux-kernel, linux-i2c, jarkko.nikula, andriy.shevchenko, wsa, rrangel, upstream, Muralidhara M K, Naveen Krishna Chatradhi On Tue, Sep 20, 2022 at 06:24:39PM +0200, Jan Dąbroś wrote: > > > +EXPORT_SYMBOL_GPL(amd_cache_northbridges); Why is this being exported again? It is called unconditionally as a fs_initcall()... -- Regards/Gruss, Boris. SUSE Software Solutions Germany GmbH GF: Ivo Totev, Andrew Myers, Andrew McDonald, Martje Boudien Moerman (HRB 36809, AG Nürnberg) ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2022-09-21 20:12 ` Borislav Petkov @ 2022-09-21 20:19 ` Limonciello, Mario 2022-09-21 20:50 ` Borislav Petkov 0 siblings, 1 reply; 30+ messages in thread From: Limonciello, Mario @ 2022-09-21 20:19 UTC (permalink / raw) To: Borislav Petkov, Jan Dąbroś Cc: linux-kernel, linux-i2c, jarkko.nikula, andriy.shevchenko, wsa, rrangel, upstream, Muralidhara M K, Naveen Krishna Chatradhi On 9/21/2022 15:12, Borislav Petkov wrote: > On Tue, Sep 20, 2022 at 06:24:39PM +0200, Jan Dąbroś wrote: >>>> +EXPORT_SYMBOL_GPL(amd_cache_northbridges); > > Why is this being exported again? > > It is called unconditionally as a fs_initcall()... > Jan mentioned this in the commit message: > The function which registers i2c-designware-platdrv is a > subsys_initcall that is executed before fs_initcall (when enumeration > of NB descriptors occurs). So if it's not exported again, then it means that we somehow need to get i2c-designware-platdrv to register earlier too. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2022-09-21 20:19 ` Limonciello, Mario @ 2022-09-21 20:50 ` Borislav Petkov 2022-09-22 9:49 ` Andy Shevchenko 0 siblings, 1 reply; 30+ messages in thread From: Borislav Petkov @ 2022-09-21 20:50 UTC (permalink / raw) To: Limonciello, Mario Cc: Jan Dąbroś, linux-kernel, linux-i2c, jarkko.nikula, andriy.shevchenko, wsa, rrangel, upstream, Muralidhara M K, Naveen Krishna Chatradhi On Wed, Sep 21, 2022 at 03:19:26PM -0500, Limonciello, Mario wrote: > Jan mentioned this in the commit message: > > > The function which registers i2c-designware-platdrv is a > > subsys_initcall that is executed before fs_initcall (when enumeration > of > NB descriptors occurs). > > So if it's not exported again, then it means that we somehow > need to get i2c-designware-platdrv to register earlier too. So I have this there: /* This has to go after the PCI subsystem */ fs_initcall(init_amd_nbs); as I need PCI. It itself does arch_initcall(pci_arch_init); so I guess init_amd_nbs() could be a subsys_initcall... Or why is subsys_initcall(dw_i2c_init_driver); a subsys initcall in the first place? Looking at 104522806a7d ("i2c: designware: dw_i2c_init_driver as subsys initcall") I don't see a particular reason why it should be a subsys_initcall... In any case, this should be fixed without an export which was crap in the first place. Hm. -- Regards/Gruss, Boris. SUSE Software Solutions Germany GmbH GF: Ivo Totev, Andrew Myers, Andrew McDonald, Martje Boudien Moerman (HRB 36809, AG Nürnberg) ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2022-09-21 20:50 ` Borislav Petkov @ 2022-09-22 9:49 ` Andy Shevchenko 2022-09-22 9:51 ` Andy Shevchenko 0 siblings, 1 reply; 30+ messages in thread From: Andy Shevchenko @ 2022-09-22 9:49 UTC (permalink / raw) To: Borislav Petkov Cc: Limonciello, Mario, Jan Dąbroś, linux-kernel, linux-i2c, jarkko.nikula, wsa, rrangel, upstream, Muralidhara M K, Naveen Krishna Chatradhi On Wed, Sep 21, 2022 at 10:50:53PM +0200, Borislav Petkov wrote: > On Wed, Sep 21, 2022 at 03:19:26PM -0500, Limonciello, Mario wrote: > > Jan mentioned this in the commit message: > > > > > The function which registers i2c-designware-platdrv is a > > > subsys_initcall that is executed before fs_initcall (when enumeration > of > > NB descriptors occurs). > > > > So if it's not exported again, then it means that we somehow > > need to get i2c-designware-platdrv to register earlier too. > > So I have this there: > > /* This has to go after the PCI subsystem */ > fs_initcall(init_amd_nbs); > > as I need PCI. It itself does > > arch_initcall(pci_arch_init); > > so I guess init_amd_nbs() could be a subsys_initcall... > > Or why is > > subsys_initcall(dw_i2c_init_driver); > > a subsys initcall in the first place? > > Looking at > > 104522806a7d ("i2c: designware: dw_i2c_init_driver as subsys initcall") > > I don't see a particular reason why it should be a subsys_initcall... > > In any case, this should be fixed without an export which was crap in > the first place. > > Hm. I'm speculating here, but IIRC the I2C controllers may serve PMICs on some platform that are required to be present earlier due to some ACPI code accessing them. This Hans de Goede can confirm or correct me. Another case comes to my mind is that I2C framework wants to initialize I2C peripherals which were supplied via struct i2c_board_info on earlier stages. And again comes to the specifics of the certain peripherals that needs for power / reset / etc control, i.o.w. critical hardware for the platforms. But it's still what I remember and I can be mistaken. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2022-09-22 9:49 ` Andy Shevchenko @ 2022-09-22 9:51 ` Andy Shevchenko 2022-09-22 13:48 ` Hans de Goede 0 siblings, 1 reply; 30+ messages in thread From: Andy Shevchenko @ 2022-09-22 9:51 UTC (permalink / raw) To: Borislav Petkov, Hans de Goede Cc: Limonciello, Mario, Jan Dąbroś, linux-kernel, linux-i2c, jarkko.nikula, wsa, rrangel, upstream, Muralidhara M K, Naveen Krishna Chatradhi +Cc: Hans (mentioned your name and was under impression that you are in Cc list already) On Thu, Sep 22, 2022 at 12:49:15PM +0300, Andy Shevchenko wrote: > On Wed, Sep 21, 2022 at 10:50:53PM +0200, Borislav Petkov wrote: > > On Wed, Sep 21, 2022 at 03:19:26PM -0500, Limonciello, Mario wrote: > > > Jan mentioned this in the commit message: > > > > > > > The function which registers i2c-designware-platdrv is a > > > > subsys_initcall that is executed before fs_initcall (when enumeration > of > > > NB descriptors occurs). > > > > > > So if it's not exported again, then it means that we somehow > > > need to get i2c-designware-platdrv to register earlier too. > > > > So I have this there: > > > > /* This has to go after the PCI subsystem */ > > fs_initcall(init_amd_nbs); > > > > as I need PCI. It itself does > > > > arch_initcall(pci_arch_init); > > > > so I guess init_amd_nbs() could be a subsys_initcall... > > > > Or why is > > > > subsys_initcall(dw_i2c_init_driver); > > > > a subsys initcall in the first place? > > > > Looking at > > > > 104522806a7d ("i2c: designware: dw_i2c_init_driver as subsys initcall") > > > > I don't see a particular reason why it should be a subsys_initcall... > > > > In any case, this should be fixed without an export which was crap in > > the first place. > > > > Hm. > > I'm speculating here, but IIRC the I2C controllers may serve PMICs on some > platform that are required to be present earlier due to some ACPI code > accessing them. This Hans de Goede can confirm or correct me. > > Another case comes to my mind is that I2C framework wants to initialize I2C > peripherals which were supplied via struct i2c_board_info on earlier stages. > And again comes to the specifics of the certain peripherals that needs for > power / reset / etc control, i.o.w. critical hardware for the platforms. > > But it's still what I remember and I can be mistaken. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2022-09-22 9:51 ` Andy Shevchenko @ 2022-09-22 13:48 ` Hans de Goede 2022-09-22 14:04 ` Borislav Petkov 0 siblings, 1 reply; 30+ messages in thread From: Hans de Goede @ 2022-09-22 13:48 UTC (permalink / raw) To: Andy Shevchenko, Borislav Petkov Cc: Limonciello, Mario, Jan Dąbroś, linux-kernel, linux-i2c, jarkko.nikula, wsa, rrangel, upstream, Muralidhara M K, Naveen Krishna Chatradhi Hi, On 9/22/22 11:51, Andy Shevchenko wrote: > +Cc: Hans (mentioned your name and was under impression that you are in Cc list already) > > On Thu, Sep 22, 2022 at 12:49:15PM +0300, Andy Shevchenko wrote: >> On Wed, Sep 21, 2022 at 10:50:53PM +0200, Borislav Petkov wrote: >>> On Wed, Sep 21, 2022 at 03:19:26PM -0500, Limonciello, Mario wrote: >>>> Jan mentioned this in the commit message: >>>> >>>>> The function which registers i2c-designware-platdrv is a >>>>> subsys_initcall that is executed before fs_initcall (when enumeration > of >>>> NB descriptors occurs). >>>> >>>> So if it's not exported again, then it means that we somehow >>>> need to get i2c-designware-platdrv to register earlier too. >>> >>> So I have this there: >>> >>> /* This has to go after the PCI subsystem */ >>> fs_initcall(init_amd_nbs); >>> >>> as I need PCI. It itself does >>> >>> arch_initcall(pci_arch_init); >>> >>> so I guess init_amd_nbs() could be a subsys_initcall... >>> >>> Or why is >>> >>> subsys_initcall(dw_i2c_init_driver); >>> >>> a subsys initcall in the first place? >>> >>> Looking at >>> >>> 104522806a7d ("i2c: designware: dw_i2c_init_driver as subsys initcall") >>> >>> I don't see a particular reason why it should be a subsys_initcall... >>> >>> In any case, this should be fixed without an export which was crap in >>> the first place. >>> >>> Hm. >> >> I'm speculating here, but IIRC the I2C controllers may serve PMICs on some >> platform that are required to be present earlier due to some ACPI code >> accessing them. This Hans de Goede can confirm or correct me. Right, thank you for Cc-ing me. At least on X86 there are several platforms (and 100-s of device models) which use a PMIC connected to the i2c-designware controller and this PMIC gets poked directly from ACPI _S0 and _S3 (power on/off) methods. So the I2C bus driver needs to *bind* to the controller as soon as we find its description in ACPI, otherwise we get a whole bunch of failed ACPI OpRegion access errors as well as various actual really issues. So please keep this as a subsys initcall. Regards, Hans ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2022-09-22 13:48 ` Hans de Goede @ 2022-09-22 14:04 ` Borislav Petkov 2022-09-22 14:29 ` Hans de Goede 0 siblings, 1 reply; 30+ messages in thread From: Borislav Petkov @ 2022-09-22 14:04 UTC (permalink / raw) To: Hans de Goede Cc: Andy Shevchenko, Limonciello, Mario, Jan Dąbroś, linux-kernel, linux-i2c, jarkko.nikula, wsa, rrangel, upstream, Muralidhara M K, Naveen Krishna Chatradhi On Thu, Sep 22, 2022 at 03:48:07PM +0200, Hans de Goede wrote: > Right, thank you for Cc-ing me. At least on X86 there are several platforms > (and 100-s of device models) which use a PMIC connected to the i2c-designware > controller and this PMIC gets poked directly from ACPI _S0 and _S3 > (power on/off) methods. So the I2C bus driver needs to *bind* to the controller > as soon as we find its description in ACPI, otherwise we get a whole bunch > of failed ACPI OpRegion access errors as well as various actual really issues. Thanks for explaining - I couldn't find the reason why it has to be a subsys_initcall. > So please keep this as a subsys initcall. Which means, init_amd_nbs() would have to be sorted to run before dw_i2c_init_driver()... -- Regards/Gruss, Boris. SUSE Software Solutions Germany GmbH GF: Ivo Totev, Andrew Myers, Andrew McDonald, Martje Boudien Moerman (HRB 36809, AG Nürnberg) ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2022-09-22 14:04 ` Borislav Petkov @ 2022-09-22 14:29 ` Hans de Goede 2022-09-26 12:49 ` Jan Dąbroś 0 siblings, 1 reply; 30+ messages in thread From: Hans de Goede @ 2022-09-22 14:29 UTC (permalink / raw) To: Borislav Petkov Cc: Andy Shevchenko, Limonciello, Mario, Jan Dąbroś, linux-kernel, linux-i2c, jarkko.nikula, wsa, rrangel, upstream, Muralidhara M K, Naveen Krishna Chatradhi Hi, On 9/22/22 16:04, Borislav Petkov wrote: > On Thu, Sep 22, 2022 at 03:48:07PM +0200, Hans de Goede wrote: >> Right, thank you for Cc-ing me. At least on X86 there are several platforms >> (and 100-s of device models) which use a PMIC connected to the i2c-designware >> controller and this PMIC gets poked directly from ACPI _S0 and _S3 >> (power on/off) methods. So the I2C bus driver needs to *bind* to the controller >> as soon as we find its description in ACPI, otherwise we get a whole bunch >> of failed ACPI OpRegion access errors as well as various actual really issues. > > Thanks for explaining - I couldn't find the reason why it has to be a > subsys_initcall. > >> So please keep this as a subsys initcall. > > Which means, init_amd_nbs() would have to be sorted to run before > dw_i2c_init_driver()... Yes if possible. One solution might be to make it a arch_initcall_sync() which AFAIK runs after regular arch_initcall()-s but before subsys_initcall()-s. Regards, Hans ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2022-09-22 14:29 ` Hans de Goede @ 2022-09-26 12:49 ` Jan Dąbroś 2022-09-26 14:44 ` Borislav Petkov 0 siblings, 1 reply; 30+ messages in thread From: Jan Dąbroś @ 2022-09-26 12:49 UTC (permalink / raw) To: Hans de Goede Cc: Borislav Petkov, Andy Shevchenko, Limonciello, Mario, linux-kernel, linux-i2c, jarkko.nikula, wsa, rrangel, upstream, Muralidhara M K, Naveen Krishna Chatradhi Hi Borislav, czw., 22 wrz 2022 o 16:29 Hans de Goede <hdegoede@redhat.com> napisał(a): > > Hi, > > On 9/22/22 16:04, Borislav Petkov wrote: > > On Thu, Sep 22, 2022 at 03:48:07PM +0200, Hans de Goede wrote: > >> Right, thank you for Cc-ing me. At least on X86 there are several platforms > >> (and 100-s of device models) which use a PMIC connected to the i2c-designware > >> controller and this PMIC gets poked directly from ACPI _S0 and _S3 > >> (power on/off) methods. So the I2C bus driver needs to *bind* to the controller > >> as soon as we find its description in ACPI, otherwise we get a whole bunch > >> of failed ACPI OpRegion access errors as well as various actual really issues. > > > > Thanks for explaining - I couldn't find the reason why it has to be a > > subsys_initcall. > > > >> So please keep this as a subsys initcall. > > > > Which means, init_amd_nbs() would have to be sorted to run before > > dw_i2c_init_driver()... > > Yes if possible. One solution might be to make it a arch_initcall_sync() > which AFAIK runs after regular arch_initcall()-s but before subsys_initcall()-s. What do you think about this? Best Regards, Jan ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2022-09-26 12:49 ` Jan Dąbroś @ 2022-09-26 14:44 ` Borislav Petkov 2022-10-28 8:32 ` Jan Dąbroś 0 siblings, 1 reply; 30+ messages in thread From: Borislav Petkov @ 2022-09-26 14:44 UTC (permalink / raw) To: Jan Dąbroś Cc: Hans de Goede, Andy Shevchenko, Limonciello, Mario, linux-kernel, linux-i2c, jarkko.nikula, wsa, rrangel, upstream, Muralidhara M K, Naveen Krishna Chatradhi On Mon, Sep 26, 2022 at 02:49:12PM +0200, Jan Dąbroś wrote: > What do you think about this? I don't mind as long as it is tested properly so that nothing breaks - I can help out on my end as I have a lot of AMD hw - and then properly documented in a comment above it *why* it needs to be arch_initcall_sync(). Thx. -- Regards/Gruss, Boris. SUSE Software Solutions Germany GmbH GF: Ivo Totev, Andrew Myers, Andrew McDonald, Martje Boudien Moerman (HRB 36809, AG Nürnberg) ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2022-09-26 14:44 ` Borislav Petkov @ 2022-10-28 8:32 ` Jan Dąbroś 2023-01-09 11:12 ` Borislav Petkov 0 siblings, 1 reply; 30+ messages in thread From: Jan Dąbroś @ 2022-10-28 8:32 UTC (permalink / raw) To: Borislav Petkov Cc: Hans de Goede, Andy Shevchenko, Limonciello, Mario, linux-kernel, linux-i2c, jarkko.nikula, wsa, rrangel, upstream, Muralidhara M K, Naveen Krishna Chatradhi Hi, Sorry for a late reply. I did some research and experiments and right now got stuck with this problem. It's not enough for running init_amd_nbs() to have only pci_arch_init() done. We need the pci bus to be created and registered with all devices found on the bus. We are traversing through them and trying to find northbridge VID/DID. Due to the above, we need to run init_amd_nbs() only after acpi_scan_init() that is invoked from acpi_init() which is registered as subsys_initcall. That's why the trick with switching init_amd_nbs() to arch_initcall_sync will not work. So to summarize everything, I would like below order: acpi_init() -> init_amd_nbs() -> dw_i2c_init_driver() ^--subsys_initcall ^--fs_initicall ^--subsys_initcall but I don't have a clear idea how to achieve this in a clean way. The only option seems to be to register init_amd_nbs() as subsys_initcall and force it to execute after acpi_init() and before dw_i2c_init_drvier(). However the only option (if I'm not mistaken) for forcing order on initcalls placed on the same level is to modify their order within Makefile, so that linker puts them in the "init" section with addresses in desired order. This doesn't seem to be an option for upstream. Do you have any clue how to solve this problem? Best Regards, Jan pon., 26 wrz 2022 o 16:45 Borislav Petkov <bp@suse.de> napisał(a): > > On Mon, Sep 26, 2022 at 02:49:12PM +0200, Jan Dąbroś wrote: > > What do you think about this? > > I don't mind as long as it is tested properly so that nothing breaks > - I can help out on my end as I have a lot of AMD hw - and then > properly documented in a comment above it *why* it needs to be > arch_initcall_sync(). > > Thx. > > -- > Regards/Gruss, > Boris. > > SUSE Software Solutions Germany GmbH > GF: Ivo Totev, Andrew Myers, Andrew McDonald, Martje Boudien Moerman > (HRB 36809, AG Nürnberg) ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2022-10-28 8:32 ` Jan Dąbroś @ 2023-01-09 11:12 ` Borislav Petkov 2023-01-09 17:10 ` Limonciello, Mario 2023-01-16 10:19 ` Jan Dąbroś 0 siblings, 2 replies; 30+ messages in thread From: Borislav Petkov @ 2023-01-09 11:12 UTC (permalink / raw) To: Jan Dąbroś, Limonciello, Mario Cc: Borislav Petkov, Hans de Goede, Andy Shevchenko, linux-kernel, linux-i2c, jarkko.nikula, wsa, rrangel, upstream, Muralidhara M K, Naveen Krishna Chatradhi, Yazen Ghannam Another forgotten thread... ;-\ + Yazen. On Fri, Oct 28, 2022 at 10:32:20AM +0200, Jan Dąbroś wrote: > So to summarize everything, I would like below order: > > acpi_init() -> init_amd_nbs() -> dw_i2c_init_driver() > ^--subsys_initcall ^--fs_initicall ^--subsys_initcall > > but I don't have a clear idea how to achieve this in a clean way. > > The only option seems to be to register init_amd_nbs() as > subsys_initcall and force it to execute after acpi_init() and before > dw_i2c_init_drvier(). However the only option (if I'm not mistaken) > for forcing order on initcalls placed on the same level is to modify > their order within Makefile, so that linker puts them in the "init" > section with addresses in desired order. This doesn't seem to be an > option for upstream. > > Do you have any clue how to solve this problem? Make init_amd_nbs() arch_initcall_sync() so that it executes after PCI init. By the time subsys_initcalls come, they'll have all the facilities they need, prepared for them... Along with big fat comment why. Btw, note to myself as I keep wondering about it each time: the sync calls come after the regular ones, in link order if you look at preprocessed linker script arch/x86/kernel/vmlinux.lds: __initcall_start = .; KEEP(*(.initcallearly.init)) __initcall0_start = .; KEEP(*(.initcall0.init)) KEEP(*(.initcall0s.init)) __initcall1_start = .; KEEP(*(.initcall1.init)) KEEP(*(.initcall1s.init)) __initcall2_start = .; KEEP(*(.initcall2.init)) KEEP(*(.initcall2s.init)) __initcall3_start = .; KEEP(*(.initcall3.init)) KEEP(*(.initcall3s.init)) __initcall4_start = .; KEEP(*(.initcall4.init)) KEEP(*(.initcall4s.init)) __initcall5_start = .; KEEP(*(.initcall5.init)) KEEP(*(.initcall5s.init)) __initcallrootfs_start = .; KEEP(*(.initcallrootfs.init)) KEEP(*(.initcallrootfss.init)) __initcall6_start = .; KEEP(*(.initcall6.init)) KEEP(*(.initcall6s.init)) __initcall7_start = .; KEEP(*(.initcall7.init)) KEEP(*(.initcall7s.init)) __initcall_end = .; Mario, is that something that would work for what you wanna do too? Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2023-01-09 11:12 ` Borislav Petkov @ 2023-01-09 17:10 ` Limonciello, Mario 2023-01-16 10:19 ` Jan Dąbroś 1 sibling, 0 replies; 30+ messages in thread From: Limonciello, Mario @ 2023-01-09 17:10 UTC (permalink / raw) To: Grzegorz Bernacki, Borislav Petkov, Jan Dąbroś Cc: Borislav Petkov, Hans de Goede, Andy Shevchenko, linux-kernel, linux-i2c, jarkko.nikula, wsa, rrangel, upstream, M K, Muralidhara, Chatradhi, Naveen Krishna, Ghannam, Yazen, Mukunda, Vijendar [Public] +Grzegorz who Jan mentioned is taking over some of this. +Vijendar for sound/soc/amd > -----Original Message----- > From: Borislav Petkov <bp@alien8.de> > Sent: Monday, January 9, 2023 05:12 > To: Jan Dąbroś <jsd@semihalf.com>; Limonciello, Mario > <Mario.Limonciello@amd.com> > Cc: Borislav Petkov <bp@suse.de>; Hans de Goede > <hdegoede@redhat.com>; Andy Shevchenko > <andriy.shevchenko@linux.intel.com>; linux-kernel@vger.kernel.org; linux- > i2c@vger.kernel.org; jarkko.nikula@linux.intel.com; wsa@kernel.org; > rrangel@chromium.org; upstream@semihalf.com; M K, Muralidhara > <Muralidhara.MK@amd.com>; Chatradhi, Naveen Krishna > <NaveenKrishna.Chatradhi@amd.com>; Ghannam, Yazen > <Yazen.Ghannam@amd.com> > Subject: Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO > access to SMN access > > Another forgotten thread... ;-\ > > + Yazen. > > On Fri, Oct 28, 2022 at 10:32:20AM +0200, Jan Dąbroś wrote: > > So to summarize everything, I would like below order: > > > > acpi_init() -> init_amd_nbs() -> dw_i2c_init_driver() > > ^--subsys_initcall ^--fs_initicall ^--subsys_initcall > > > > but I don't have a clear idea how to achieve this in a clean way. > > > > The only option seems to be to register init_amd_nbs() as > > subsys_initcall and force it to execute after acpi_init() and before > > dw_i2c_init_drvier(). However the only option (if I'm not mistaken) > > for forcing order on initcalls placed on the same level is to modify > > their order within Makefile, so that linker puts them in the "init" > > section with addresses in desired order. This doesn't seem to be an > > option for upstream. > > > > Do you have any clue how to solve this problem? > > Make init_amd_nbs() arch_initcall_sync() so that it executes after PCI init. > > By the time subsys_initcalls come, they'll have all the facilities they need, > prepared for them... > > Along with big fat comment why. > > Btw, note to myself as I keep wondering about it each time: the sync calls > come > after the regular ones, in link order if you look at preprocessed linker script > arch/x86/kernel/vmlinux.lds: > > __initcall_start = .; > KEEP(*(.initcallearly.init)) __initcall0_start = .; > KEEP(*(.initcall0.init)) KEEP(*(.initcall0s.init)) __initcall1_start = .; > KEEP(*(.initcall1.init)) KEEP(*(.initcall1s.init)) __initcall2_start = .; > KEEP(*(.initcall2.init)) KEEP(*(.initcall2s.init)) __initcall3_start = .; > KEEP(*(.initcall3.init)) KEEP(*(.initcall3s.init)) __initcall4_start = .; > KEEP(*(.initcall4.init)) KEEP(*(.initcall4s.init)) __initcall5_start = .; > KEEP(*(.initcall5.init)) KEEP(*(.initcall5s.init)) __initcallrootfs_start = .; > KEEP(*(.initcallrootfs.init)) KEEP(*(.initcallrootfss.init)) __initcall6_start = .; > KEEP(*(.initcall6.init)) KEEP(*(.initcall6s.init)) __initcall7_start = .; > KEEP(*(.initcall7.init)) KEEP(*(.initcall7s.init)) __initcall_end = .; > > Mario, is that something that would work for what you wanna do too? > Yeah I guess as long as the I2C init comes after arch_initcall_sync this should work. If it works for Jan & Grzegorz, then I think we can also adjust the "effectively local copy" of amd_smn_write/amd_smn_read that got added into sound/soc/amd to use this too now and it should work for what I wanted to do too. Jan/Grzegorz can you please have a try on your system and comment? > Thx. > > -- > Regards/Gruss, > Boris. > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeo > ple.kernel.org%2Ftglx%2Fnotes-about- > netiquette&data=05%7C01%7Cmario.limonciello%40amd.com%7C56e3280fc > 1cd416bd7ae08daf23262d0%7C3dd8961fe4884e608e11a82d994e183d%7C0% > 7C0%7C638088595520420341%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4 > wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7 > C%7C%7C&sdata=9t7J4u3W0kxXu717gzIyBJpAvt6YtQOKqtK9AcjWOvg%3D&r > eserved=0 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2023-01-09 11:12 ` Borislav Petkov 2023-01-09 17:10 ` Limonciello, Mario @ 2023-01-16 10:19 ` Jan Dąbroś 2023-01-16 12:38 ` Andy Shevchenko 2023-01-16 21:59 ` Borislav Petkov 1 sibling, 2 replies; 30+ messages in thread From: Jan Dąbroś @ 2023-01-16 10:19 UTC (permalink / raw) To: Borislav Petkov Cc: Limonciello, Mario, Borislav Petkov, Hans de Goede, Andy Shevchenko, linux-kernel, linux-i2c, jarkko.nikula, wsa, rrangel, upstream, Muralidhara M K, Naveen Krishna Chatradhi, Yazen Ghannam Hi Borislav, > Make init_amd_nbs() arch_initcall_sync() so that it executes after PCI init. I described earlier in this thread why such option is not working - let me quote myself: It's not enough for running init_amd_nbs() to have only pci_arch_init() done. We need the pci bus to be created and registered with all devices found on the bus. We are traversing through them and trying to find northbridge VID/DID. Due to the above, we need to run init_amd_nbs() only after acpi_scan_init() that is invoked from acpi_init() which is registered as subsys_initcall. That's why the trick with switching init_amd_nbs() to arch_initcall_sync will not work. We have a kind of chicken-and-egg problem here. Or is there something I missed? I wonder if there is upstreamable option to control order of the drivers' init by forcing link order? Best Regards, Jan ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2023-01-16 10:19 ` Jan Dąbroś @ 2023-01-16 12:38 ` Andy Shevchenko 2023-01-16 16:22 ` Limonciello, Mario 2023-01-16 21:59 ` Borislav Petkov 1 sibling, 1 reply; 30+ messages in thread From: Andy Shevchenko @ 2023-01-16 12:38 UTC (permalink / raw) To: Jan Dąbroś Cc: Borislav Petkov, Limonciello, Mario, Borislav Petkov, Hans de Goede, linux-kernel, linux-i2c, jarkko.nikula, wsa, rrangel, upstream, Muralidhara M K, Naveen Krishna Chatradhi, Yazen Ghannam On Mon, Jan 16, 2023 at 11:19:00AM +0100, Jan Dąbroś wrote: > Hi Borislav, > > > Make init_amd_nbs() arch_initcall_sync() so that it executes after PCI init. > > I described earlier in this thread why such option is not working - > let me quote myself: > > It's not enough for running init_amd_nbs() to have only > pci_arch_init() done. We need the pci bus to be created and registered > with all devices found on the bus. We are traversing through them and > trying to find northbridge VID/DID. Due to the above, we need to run > init_amd_nbs() only after acpi_scan_init() that is invoked from > acpi_init() which is registered as subsys_initcall. That's why the > trick with switching init_amd_nbs() to arch_initcall_sync will not > work. > > We have a kind of chicken-and-egg problem here. Or is there something I missed? > > I wonder if there is upstreamable option to control order of the > drivers' init by forcing link order? But what exactly do you need from North Bridge? Is it only its existence or do you need to have fully instantiated PCI device (if so, why?)? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2023-01-16 12:38 ` Andy Shevchenko @ 2023-01-16 16:22 ` Limonciello, Mario 2023-01-16 17:48 ` Andy Shevchenko 0 siblings, 1 reply; 30+ messages in thread From: Limonciello, Mario @ 2023-01-16 16:22 UTC (permalink / raw) To: Andy Shevchenko, Jan Dąbroś Cc: Borislav Petkov, Borislav Petkov, Hans de Goede, linux-kernel, linux-i2c, jarkko.nikula, wsa, rrangel, upstream, M K, Muralidhara, Chatradhi, Naveen Krishna, Ghannam, Yazen [Public] > -----Original Message----- > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Sent: Monday, January 16, 2023 06:39 > To: Jan Dąbroś <jsd@semihalf.com> > Cc: Borislav Petkov <bp@alien8.de>; Limonciello, Mario > <Mario.Limonciello@amd.com>; Borislav Petkov <bp@suse.de>; Hans de > Goede <hdegoede@redhat.com>; linux-kernel@vger.kernel.org; linux- > i2c@vger.kernel.org; jarkko.nikula@linux.intel.com; wsa@kernel.org; > rrangel@chromium.org; upstream@semihalf.com; M K, Muralidhara > <Muralidhara.MK@amd.com>; Chatradhi, Naveen Krishna > <NaveenKrishna.Chatradhi@amd.com>; Ghannam, Yazen > <Yazen.Ghannam@amd.com> > Subject: Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO > access to SMN access > > On Mon, Jan 16, 2023 at 11:19:00AM +0100, Jan Dąbroś wrote: > > Hi Borislav, > > > > > Make init_amd_nbs() arch_initcall_sync() so that it executes after PCI > init. > > > > I described earlier in this thread why such option is not working - > > let me quote myself: > > > > It's not enough for running init_amd_nbs() to have only > > pci_arch_init() done. We need the pci bus to be created and registered > > with all devices found on the bus. We are traversing through them and > > trying to find northbridge VID/DID. Due to the above, we need to run > > init_amd_nbs() only after acpi_scan_init() that is invoked from > > acpi_init() which is registered as subsys_initcall. That's why the > > trick with switching init_amd_nbs() to arch_initcall_sync will not > > work. > > > > We have a kind of chicken-and-egg problem here. Or is there something I > missed? > > > > I wonder if there is upstreamable option to control order of the > > drivers' init by forcing link order? > > But what exactly do you need from North Bridge? Is it only its existence or > do you need to have fully instantiated PCI device (if so, why?)? > There is a need to be able to write and read PCI config space. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2023-01-16 16:22 ` Limonciello, Mario @ 2023-01-16 17:48 ` Andy Shevchenko 2023-01-16 17:54 ` Andy Shevchenko 0 siblings, 1 reply; 30+ messages in thread From: Andy Shevchenko @ 2023-01-16 17:48 UTC (permalink / raw) To: Limonciello, Mario Cc: Jan Dąbroś, Borislav Petkov, Borislav Petkov, Hans de Goede, linux-kernel, linux-i2c, jarkko.nikula, wsa, rrangel, upstream, M K, Muralidhara, Chatradhi, Naveen Krishna, Ghannam, Yazen On Mon, Jan 16, 2023 at 04:22:09PM +0000, Limonciello, Mario wrote: > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Sent: Monday, January 16, 2023 06:39 > > To: Jan Dąbroś <jsd@semihalf.com> > > On Mon, Jan 16, 2023 at 11:19:00AM +0100, Jan Dąbroś wrote: > > > > Make init_amd_nbs() arch_initcall_sync() so that it executes after PCI > > init. > > > > > > I described earlier in this thread why such option is not working - > > > let me quote myself: > > > > > > It's not enough for running init_amd_nbs() to have only > > > pci_arch_init() done. We need the pci bus to be created and registered > > > with all devices found on the bus. We are traversing through them and > > > trying to find northbridge VID/DID. Due to the above, we need to run > > > init_amd_nbs() only after acpi_scan_init() that is invoked from > > > acpi_init() which is registered as subsys_initcall. That's why the > > > trick with switching init_amd_nbs() to arch_initcall_sync will not > > > work. > > > > > > We have a kind of chicken-and-egg problem here. Or is there something I > > missed? > > > > > > I wonder if there is upstreamable option to control order of the > > > drivers' init by forcing link order? > > > > But what exactly do you need from North Bridge? Is it only its existence or > > do you need to have fully instantiated PCI device (if so, why?)? > > There is a need to be able to write and read PCI config space. So, it's available even on early stages, are there some specifics why it can't be done using the respective APIs? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2023-01-16 17:48 ` Andy Shevchenko @ 2023-01-16 17:54 ` Andy Shevchenko 0 siblings, 0 replies; 30+ messages in thread From: Andy Shevchenko @ 2023-01-16 17:54 UTC (permalink / raw) To: Limonciello, Mario Cc: Jan Dąbroś, Borislav Petkov, Borislav Petkov, Hans de Goede, linux-kernel, linux-i2c, jarkko.nikula, wsa, rrangel, upstream, M K, Muralidhara, Chatradhi, Naveen Krishna, Ghannam, Yazen On Mon, Jan 16, 2023 at 07:48:10PM +0200, Andy Shevchenko wrote: > On Mon, Jan 16, 2023 at 04:22:09PM +0000, Limonciello, Mario wrote: > > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > Sent: Monday, January 16, 2023 06:39 > > > To: Jan Dąbroś <jsd@semihalf.com> > > > On Mon, Jan 16, 2023 at 11:19:00AM +0100, Jan Dąbroś wrote: > > > > > > Make init_amd_nbs() arch_initcall_sync() so that it executes after PCI > > > init. > > > > > > > > I described earlier in this thread why such option is not working - > > > > let me quote myself: > > > > > > > > It's not enough for running init_amd_nbs() to have only > > > > pci_arch_init() done. We need the pci bus to be created and registered > > > > with all devices found on the bus. We are traversing through them and > > > > trying to find northbridge VID/DID. Due to the above, we need to run > > > > init_amd_nbs() only after acpi_scan_init() that is invoked from > > > > acpi_init() which is registered as subsys_initcall. That's why the > > > > trick with switching init_amd_nbs() to arch_initcall_sync will not > > > > work. > > > > > > > > We have a kind of chicken-and-egg problem here. Or is there something I > > > missed? > > > > > > > > I wonder if there is upstreamable option to control order of the > > > > drivers' init by forcing link order? > > > > > > But what exactly do you need from North Bridge? Is it only its existence or > > > do you need to have fully instantiated PCI device (if so, why?)? > > > > There is a need to be able to write and read PCI config space. > > So, it's available even on early stages, are there some specifics why it can't > be done using the respective APIs? I think I understood the problem with the above. You probably don't know where NB can be located in the topology and that's why you can't simply access a _fixed_ BDF. So you want to have the topology being scanned beforehand. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2023-01-16 10:19 ` Jan Dąbroś 2023-01-16 12:38 ` Andy Shevchenko @ 2023-01-16 21:59 ` Borislav Petkov 1 sibling, 0 replies; 30+ messages in thread From: Borislav Petkov @ 2023-01-16 21:59 UTC (permalink / raw) To: Jan Dąbroś, Limonciello, Mario Cc: Hans de Goede, Andy Shevchenko, linux-kernel, linux-i2c, jarkko.nikula, wsa, rrangel, upstream, Muralidhara M K, Naveen Krishna Chatradhi, Yazen Ghannam On Mon, Jan 16, 2023 at 11:19:00AM +0100, Jan Dąbroś wrote: > It's not enough for running init_amd_nbs() to have only > pci_arch_init() done. We need the pci bus to be created and registered > with all devices found on the bus. And when is that done and ready? pci_scan_bus()? Lemme see if I understand the ordering correctly: 1. PCI needs to be initialized and all devices on the bus registered 2. AMD NB needs to run and enumerate the NBs so that the SMN access which you need, can work. 3. acpi_scan_init() 4. i2c-designware-platdrv registration Close? Now, Mario, remind me again, pls, why can't they use the MSR to get the PSP MMIO base. It has changed but why? Maybe we should talk offlist first. Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2022-09-16 13:18 ` [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access Jan Dabros 2022-09-16 17:38 ` Limonciello, Mario @ 2022-09-19 13:59 ` Andy Shevchenko 2022-09-20 16:27 ` Jan Dąbroś 1 sibling, 1 reply; 30+ messages in thread From: Andy Shevchenko @ 2022-09-19 13:59 UTC (permalink / raw) To: Jan Dabros Cc: linux-kernel, linux-i2c, jarkko.nikula, wsa, rrangel, upstream, mario.limonciello On Fri, Sep 16, 2022 at 03:18:53PM +0200, Jan Dabros wrote: > Due to a change in silicon compared to Cezanne, in future revisions MSR > access can't be used to get the base address of the PSP MMIO region that > contains the PSP mailbox interface. > > Modify driver to use SMN access also for Cezanne platforms (it is > working there) in order to simplify codebase when adding support for new > SoC versions. > > Export amd_cache_northbridges() which was unexported by > e1907d3: "x86/amd_nb: Unexport amd_cache_northbridges()" Please, use standard format of referring to the commits in the history (basically the same as for Fixes tags). > since function which registers i2c-designware-platdrv is a > subsys_initcall that is executed before fs_initcall (when enumeration of > NB descriptors occurs). Thus in order to use SMN accesses it's necessary > to explicitly call amd_cache_northrbidges() from within this driver. Also it doesn't clarify if this commit a full revert of that (rebased for new kernel versions) or partial or functional? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access 2022-09-19 13:59 ` Andy Shevchenko @ 2022-09-20 16:27 ` Jan Dąbroś 0 siblings, 0 replies; 30+ messages in thread From: Jan Dąbroś @ 2022-09-20 16:27 UTC (permalink / raw) To: Andy Shevchenko Cc: linux-kernel, linux-i2c, jarkko.nikula, wsa, rrangel, upstream, mario.limonciello Hi Andy, pon., 19 wrz 2022 o 15:59 Andy Shevchenko <andriy.shevchenko@linux.intel.com> napisał(a): > > On Fri, Sep 16, 2022 at 03:18:53PM +0200, Jan Dabros wrote: > > Due to a change in silicon compared to Cezanne, in future revisions MSR > > access can't be used to get the base address of the PSP MMIO region that > > contains the PSP mailbox interface. > > > > Modify driver to use SMN access also for Cezanne platforms (it is > > working there) in order to simplify codebase when adding support for new > > SoC versions. > > > > Export amd_cache_northbridges() which was unexported by > > > e1907d3: "x86/amd_nb: Unexport amd_cache_northbridges()" > > Please, use standard format of referring to the commits in the history > (basically the same as for Fixes tags). Sure. > > > since function which registers i2c-designware-platdrv is a > > subsys_initcall that is executed before fs_initcall (when enumeration of > > NB descriptors occurs). Thus in order to use SMN accesses it's necessary > > to explicitly call amd_cache_northrbidges() from within this driver. > > Also it doesn't clarify if this commit a full revert of that (rebased for > new kernel versions) or partial or functional? This is a partial revert, I will mention this in the commit msg. Best Regards, Jan ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH -next 2/2] i2c: designware: Add support for new SoCs in AMDPSP driver 2022-09-16 13:18 [PATCH -next 0/2] Add i2c arbitration support for new SoCs Jan Dabros 2022-09-16 13:18 ` [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access Jan Dabros @ 2022-09-16 13:18 ` Jan Dabros 2022-09-19 13:59 ` Andy Shevchenko 2023-03-17 19:18 ` Mark Hasemeyer 2022-09-19 13:59 ` [PATCH -next 0/2] Add i2c arbitration support for new SoCs Andy Shevchenko 2 siblings, 2 replies; 30+ messages in thread From: Jan Dabros @ 2022-09-16 13:18 UTC (permalink / raw) To: linux-kernel, linux-i2c, jarkko.nikula, andriy.shevchenko Cc: wsa, rrangel, upstream, mario.limonciello, jsd New AMD SoCs are using different algorithm for x86-PSP communication, thus need to modify I2C arbitration driver. Since possible future revisions should have follow new approach, mark functions used only for Cezanne with "czn_" prefix. While at it, remove redundant check by modifying psp_wait_cmd() to only check for MBOX_READY bit, since MBOX_CMD field being zero is verified by czn_psp_check_mbox_sts() later on in the sequence. Signed-off-by: Jan Dabros <jsd@semihalf.com> --- drivers/i2c/busses/i2c-designware-amdpsp.c | 145 ++++++++++++++------- 1 file changed, 97 insertions(+), 48 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-amdpsp.c b/drivers/i2c/busses/i2c-designware-amdpsp.c index 1d467fc83f59..4395a2ae960a 100644 --- a/drivers/i2c/busses/i2c-designware-amdpsp.c +++ b/drivers/i2c/busses/i2c-designware-amdpsp.c @@ -4,6 +4,7 @@ #include <linux/bits.h> #include <linux/i2c.h> #include <linux/io-64-nonatomic-lo-hi.h> +#include <linux/pci.h> #include <linux/psp-sev.h> #include <linux/types.h> #include <linux/workqueue.h> @@ -30,9 +31,13 @@ #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 +#define CZN_PSP_MBOX_CMD_OFFSET 0x3810570 +#define CZN_PSP_MBOX_BUFFER_L_OFFSET 0x3810574 +#define CZN_PSP_MBOX_BUFFER_H_OFFSET 0x3810578 +#define PSP_MBOX_CMD_OFFSET 0x3810A40 +#define PSP_MBOX_DOORBELL_OFFSET 0x3810A24 + +#define AMD_CPU_ID_CZN 0x1630 struct psp_req_buffer_hdr { u32 total_size; @@ -55,6 +60,7 @@ static unsigned long psp_i2c_sem_acquired; static u32 psp_i2c_access_count; static bool psp_i2c_mbox_fail; static struct device *psp_i2c_dev; +static unsigned short cpu_id; /* * Implementation of PSP-x86 i2c-arbitration mailbox introduced for AMD Cezanne @@ -63,6 +69,17 @@ static struct device *psp_i2c_dev; static int psp_mbox_probe(void) { + struct pci_dev *rdev; + + rdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0)); + if (!rdev) { + dev_err(psp_i2c_dev, "Failed to get host bridge device\n"); + return -ENODEV; + } + + cpu_id = rdev->device; + pci_dev_put(rdev); + /* * Explicitly initialize system management network interface here, since * usual init happens only after PCI subsystem is ready. This is too late @@ -81,80 +98,76 @@ static int psp_smn_read(u32 smn_addr, u32 *value) return amd_smn_read(0, smn_addr, value); } -/* Recovery field should be equal 0 to start sending commands */ -static int psp_check_mbox_recovery(void) +static int psp_mbox_ready(u32 smn_addr) { u32 tmp; - int status; - - status = psp_smn_read(PSP_MBOX_CMD_OFFSET, &tmp); - if (status) - return status; - - return FIELD_GET(PSP_MBOX_FIELDS_RECOVERY, tmp); -} - -static int psp_wait_cmd(void) -{ - u32 tmp, expected; int ret, status; - /* Expect mbox_cmd to be cleared and ready bit to be set by PSP */ - expected = FIELD_PREP(PSP_MBOX_FIELDS_READY, 1); - /* * Check for readiness of PSP mailbox in a tight loop in order to * process further as soon as command was consumed. */ ret = read_poll_timeout(psp_smn_read, status, - (status < 0) || (tmp == expected), 0, - PSP_CMD_TIMEOUT_US, 0, PSP_MBOX_CMD_OFFSET, - &tmp); + (status < 0) || (tmp & PSP_MBOX_FIELDS_READY), + 0, PSP_CMD_TIMEOUT_US, 0, smn_addr, &tmp); if (status) ret = status; return ret; } +/* Recovery field should be equal 0 to start sending commands */ +static int czn_psp_check_mbox_recovery(void) +{ + u32 tmp; + int status; + + status = psp_smn_read(CZN_PSP_MBOX_CMD_OFFSET, &tmp); + if (status) + return status; + + return FIELD_GET(PSP_MBOX_FIELDS_RECOVERY, tmp); +} + /* Status equal to 0 means that PSP succeed processing command */ -static int psp_check_mbox_sts(void) +static u32 czn_psp_check_mbox_sts(void) { u32 cmd_reg; int status; - status = psp_smn_read(PSP_MBOX_CMD_OFFSET, &cmd_reg); + status = psp_smn_read(CZN_PSP_MBOX_CMD_OFFSET, &cmd_reg); if (status) return status; return FIELD_GET(PSP_MBOX_FIELDS_STS, cmd_reg); } -static int psp_wr_mbox_buffer(phys_addr_t buf) +static int czn_psp_wr_mbox_buffer(phys_addr_t buf) { u32 buf_addr_h = upper_32_bits(buf); u32 buf_addr_l = lower_32_bits(buf); int status; - status = psp_smn_write(PSP_MBOX_BUFFER_H_OFFSET, buf_addr_h); + status = psp_smn_write(CZN_PSP_MBOX_BUFFER_H_OFFSET, buf_addr_h); if (status) return status; - status = psp_smn_write(PSP_MBOX_BUFFER_L_OFFSET, buf_addr_l); + status = psp_smn_write(CZN_PSP_MBOX_BUFFER_L_OFFSET, buf_addr_l); if (status) return status; return 0; } -static int psp_send_cmd(struct psp_i2c_req *req) +static int czn_psp_send_cmd(struct psp_i2c_req *req) { phys_addr_t req_addr; u32 cmd_reg; - if (psp_check_mbox_recovery()) + if (czn_psp_check_mbox_recovery()) return -EIO; - if (psp_wait_cmd()) + if (psp_mbox_ready(CZN_PSP_MBOX_CMD_OFFSET)) return -EBUSY; /* @@ -163,18 +176,18 @@ static int psp_send_cmd(struct psp_i2c_req *req) * PSP. Use physical address of buffer, since PSP will map this region. */ req_addr = __psp_pa((void *)req); - if (psp_wr_mbox_buffer(req_addr)) + if (czn_psp_wr_mbox_buffer(req_addr)) return -EIO; /* Write command register to trigger processing */ cmd_reg = FIELD_PREP(PSP_MBOX_FIELDS_CMD, PSP_I2C_REQ_BUS_CMD); - if (psp_smn_write(PSP_MBOX_CMD_OFFSET, cmd_reg)) + if (psp_smn_write(CZN_PSP_MBOX_CMD_OFFSET, cmd_reg)) return -EIO; - if (psp_wait_cmd()) + if (psp_mbox_ready(CZN_PSP_MBOX_CMD_OFFSET)) return -ETIMEDOUT; - if (psp_check_mbox_sts()) + if (czn_psp_check_mbox_sts()) return -EIO; return 0; @@ -185,8 +198,13 @@ static int check_i2c_req_sts(struct psp_i2c_req *req) { u32 status; - /* Status field in command-response buffer is updated by PSP */ - status = READ_ONCE(req->hdr.status); + if (req) { + /* Status field in command-response buffer is updated by PSP */ + status = READ_ONCE(req->hdr.status); + } else { + status = psp_smn_read(PSP_MBOX_CMD_OFFSET, &status); + status &= ~PSP_MBOX_FIELDS_READY; + } switch (status) { case PSP_I2C_REQ_STS_OK: @@ -199,8 +217,31 @@ static int check_i2c_req_sts(struct psp_i2c_req *req) } } -static int psp_send_check_i2c_req(struct psp_i2c_req *req) +static int psp_send_cmd(enum psp_i2c_req_type i2c_req_type) +{ + int ret; + + ret = psp_mbox_ready(PSP_MBOX_CMD_OFFSET); + if (ret) + return ret; + + psp_smn_write(PSP_MBOX_CMD_OFFSET, i2c_req_type); + + /* Ring the Doorbell for PSP by writing a non-zero value */ + psp_smn_write(PSP_MBOX_DOORBELL_OFFSET, 0x1); + + ret = psp_mbox_ready(PSP_MBOX_CMD_OFFSET); + if (ret) + return ret; + + return 0; +} + +static int psp_send_check_i2c_req(struct psp_i2c_req *req, + enum psp_i2c_req_type i2c_req_type) { + int ret; + /* * Errors in x86-PSP i2c-arbitration protocol may occur at two levels: * 1. mailbox communication - PSP is not operational or some IO errors @@ -208,10 +249,15 @@ static int psp_send_check_i2c_req(struct psp_i2c_req *req) * 2. i2c-requests - PSP refuses to grant i2c arbitration to x86 for too * long. * In order to distinguish between these two in error handling code, all - * errors on the first level (returned by psp_send_cmd) are shadowed by + * errors on the first level (returned by *psp_send_cmd) are shadowed by * -EIO. */ - if (psp_send_cmd(req)) + if (req) + ret = czn_psp_send_cmd(req); + else + ret = psp_send_cmd(i2c_req_type); + + if (ret) return -EIO; return check_i2c_req_sts(req); @@ -219,24 +265,27 @@ static int psp_send_check_i2c_req(struct psp_i2c_req *req) static int psp_send_i2c_req(enum psp_i2c_req_type i2c_req_type) { - struct psp_i2c_req *req; + struct psp_i2c_req *req = NULL; unsigned long start; int status, ret; - /* Allocate command-response buffer */ - req = kzalloc(sizeof(*req), GFP_KERNEL); - if (!req) - return -ENOMEM; + /* Allocate command-response buffer for Cezanne platforms */ + if (cpu_id == AMD_CPU_ID_CZN) { + req = kzalloc(sizeof(*req), GFP_KERNEL); + if (!req) + return -ENOMEM; - req->hdr.total_size = sizeof(*req); - req->type = i2c_req_type; + req->hdr.total_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); + 0, req, i2c_req_type); + if (ret) { dev_err(psp_i2c_dev, "Timed out waiting for PSP to %s I2C bus\n", (i2c_req_type == PSP_I2C_REQ_ACQUIRE) ? -- 2.37.3.968.ga6b4b080e4-goog ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH -next 2/2] i2c: designware: Add support for new SoCs in AMDPSP driver 2022-09-16 13:18 ` [PATCH -next 2/2] i2c: designware: Add support for new SoCs in AMDPSP driver Jan Dabros @ 2022-09-19 13:59 ` Andy Shevchenko 2023-03-17 19:18 ` Mark Hasemeyer 1 sibling, 0 replies; 30+ messages in thread From: Andy Shevchenko @ 2022-09-19 13:59 UTC (permalink / raw) To: Jan Dabros Cc: linux-kernel, linux-i2c, jarkko.nikula, wsa, rrangel, upstream, mario.limonciello On Fri, Sep 16, 2022 at 03:18:54PM +0200, Jan Dabros wrote: > New AMD SoCs are using different algorithm for x86-PSP communication, > thus need to modify I2C arbitration driver. Since possible future > revisions should have follow new approach, mark functions used only for > Cezanne with "czn_" prefix. > > While at it, remove redundant check by modifying psp_wait_cmd() to only > check for MBOX_READY bit, since MBOX_CMD field being zero is verified by > czn_psp_check_mbox_sts() later on in the sequence. ... > -#define PSP_MBOX_CMD_OFFSET 0x3810570 > -#define PSP_MBOX_BUFFER_L_OFFSET 0x3810574 > -#define PSP_MBOX_BUFFER_H_OFFSET 0x3810578 > +#define CZN_PSP_MBOX_CMD_OFFSET 0x3810570 > +#define CZN_PSP_MBOX_BUFFER_L_OFFSET 0x3810574 > +#define CZN_PSP_MBOX_BUFFER_H_OFFSET 0x3810578 Can we avoid this renaming noise by putting the proper names in the first place (in the first patch)? Respectively move the corresponding commit message piece to there. Or looking into the below maybe even split the renaming to a separate non-functional change? ... > + if (req) > + ret = czn_psp_send_cmd(req); > + else > + ret = psp_send_cmd(i2c_req_type); > + Unnecessary blank line. > + if (ret) > return -EIO; Why you can't return the actual error code here? ... > 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); > + 0, req, i2c_req_type); > + Stray blank line addition. > if (ret) { > dev_err(psp_i2c_dev, "Timed out waiting for PSP to %s I2C bus\n", > (i2c_req_type == PSP_I2C_REQ_ACQUIRE) ? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH -next 2/2] i2c: designware: Add support for new SoCs in AMDPSP driver 2022-09-16 13:18 ` [PATCH -next 2/2] i2c: designware: Add support for new SoCs in AMDPSP driver Jan Dabros 2022-09-19 13:59 ` Andy Shevchenko @ 2023-03-17 19:18 ` Mark Hasemeyer 2023-03-20 0:16 ` Mario Limonciello 1 sibling, 1 reply; 30+ messages in thread From: Mark Hasemeyer @ 2023-03-17 19:18 UTC (permalink / raw) To: jsd Cc: andriy.shevchenko, jarkko.nikula, linux-i2c, linux-kernel, mario.limonciello, rrangel, upstream, wsa, dabros - /* Status field in command-response buffer is updated by PSP */ - status = READ_ONCE(req->hdr.status); + if (req) { + /* Status field in command-response buffer is updated by PSP */ + status = READ_ONCE(req->hdr.status); + } else { + status = psp_smn_read(PSP_MBOX_CMD_OFFSET, &status); + status &= ~PSP_MBOX_FIELDS_READY; + } The value of the mbox cmd register is getting clobbered by the return value from psp_smn_read. This can cause bus arbitration to fail as the driver will think it can grab the bus when it's actually in use by the PSP. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 2/2] i2c: designware: Add support for new SoCs in AMDPSP driver 2023-03-17 19:18 ` Mark Hasemeyer @ 2023-03-20 0:16 ` Mario Limonciello 0 siblings, 0 replies; 30+ messages in thread From: Mario Limonciello @ 2023-03-20 0:16 UTC (permalink / raw) To: Mark Hasemeyer, jsd Cc: andriy.shevchenko, jarkko.nikula, linux-i2c, linux-kernel, rrangel, upstream, wsa, dabros On 3/17/23 14:18, Mark Hasemeyer wrote: > - /* Status field in command-response buffer is updated by PSP */ > - status = READ_ONCE(req->hdr.status); > + if (req) { > + /* Status field in command-response buffer is updated by PSP */ > + status = READ_ONCE(req->hdr.status); > + } else { > + status = psp_smn_read(PSP_MBOX_CMD_OFFSET, &status); > + status &= ~PSP_MBOX_FIELDS_READY; > + } > > The value of the mbox cmd register is getting clobbered by the return value > from psp_smn_read. This can cause bus arbitration to fail as the driver will > think it can grab the bus when it's actually in use by the PSP. Mark - FYI this approach is being superseded by another patch series that instead uses MMIO from the CCP driver. It would be better to comment on that series. https://lore.kernel.org/linux-i2c/20230310211954.2490-9-mario.limonciello@amd.com/ ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH -next 0/2] Add i2c arbitration support for new SoCs 2022-09-16 13:18 [PATCH -next 0/2] Add i2c arbitration support for new SoCs Jan Dabros 2022-09-16 13:18 ` [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access Jan Dabros 2022-09-16 13:18 ` [PATCH -next 2/2] i2c: designware: Add support for new SoCs in AMDPSP driver Jan Dabros @ 2022-09-19 13:59 ` Andy Shevchenko 2 siblings, 0 replies; 30+ messages in thread From: Andy Shevchenko @ 2022-09-19 13:59 UTC (permalink / raw) To: Jan Dabros Cc: linux-kernel, linux-i2c, jarkko.nikula, wsa, rrangel, upstream, mario.limonciello On Fri, Sep 16, 2022 at 03:18:52PM +0200, Jan Dabros wrote: > This patchset comprises changes into i2c-designware-amdpsp.c module > which aims to add support for new SoCs, while keep backward > compatibility with Cezanne platforms. > > Beside new algorithm introduced for the PSP-x86 communication, it also > switches from MSR/MMIO access to SMN (System Management Network) since > only the latter is working on both old new revisions of SoCs. Which one is better performance/energy/etc wise? If it's not the SMN, can we keep better in place? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2023-03-20 0:17 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-16 13:18 [PATCH -next 0/2] Add i2c arbitration support for new SoCs Jan Dabros 2022-09-16 13:18 ` [PATCH -next 1/2] i2c: designware: Switch from using MMIO access to SMN access Jan Dabros 2022-09-16 17:38 ` Limonciello, Mario 2022-09-20 16:24 ` Jan Dąbroś 2022-09-21 20:12 ` Borislav Petkov 2022-09-21 20:19 ` Limonciello, Mario 2022-09-21 20:50 ` Borislav Petkov 2022-09-22 9:49 ` Andy Shevchenko 2022-09-22 9:51 ` Andy Shevchenko 2022-09-22 13:48 ` Hans de Goede 2022-09-22 14:04 ` Borislav Petkov 2022-09-22 14:29 ` Hans de Goede 2022-09-26 12:49 ` Jan Dąbroś 2022-09-26 14:44 ` Borislav Petkov 2022-10-28 8:32 ` Jan Dąbroś 2023-01-09 11:12 ` Borislav Petkov 2023-01-09 17:10 ` Limonciello, Mario 2023-01-16 10:19 ` Jan Dąbroś 2023-01-16 12:38 ` Andy Shevchenko 2023-01-16 16:22 ` Limonciello, Mario 2023-01-16 17:48 ` Andy Shevchenko 2023-01-16 17:54 ` Andy Shevchenko 2023-01-16 21:59 ` Borislav Petkov 2022-09-19 13:59 ` Andy Shevchenko 2022-09-20 16:27 ` Jan Dąbroś 2022-09-16 13:18 ` [PATCH -next 2/2] i2c: designware: Add support for new SoCs in AMDPSP driver Jan Dabros 2022-09-19 13:59 ` Andy Shevchenko 2023-03-17 19:18 ` Mark Hasemeyer 2023-03-20 0:16 ` Mario Limonciello 2022-09-19 13:59 ` [PATCH -next 0/2] Add i2c arbitration support for new SoCs Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).