Hi,
Thanks for your patch. There're some comments bellow.
On 2022/1/19 14:57, Chen Baozi wrote:
> The system would hang up when the Phytium S2500 communicates with
> some BMCs after several rounds of transactions, unless we reset
> the controller timeout counter manually by calling firmware through
> SMC.
>
> Signed-off-by: Wang Yinfeng <wangyinfeng(a)phytium.com.cn>
> Signed-off-by: Chen Baozi <chenbaozi(a)phytium.com.cn>
> ---
> drivers/char/ipmi/ipmi_si_mem_io.c | 117 ++++++++++++++++++++++++++++-
> 1 file changed, 113 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_si_mem_io.c b/drivers/char/ipmi/ipmi_si_mem_io.c
> index 75583612ab10..73a3d6979eaa 100644
> --- a/drivers/char/ipmi/ipmi_si_mem_io.c
> +++ b/drivers/char/ipmi/ipmi_si_mem_io.c
> @@ -3,6 +3,105 @@
> #include <linux/io.h>
> #include "ipmi_si.h"
>
> +#ifdef CONFIG_ARM_GIC_PHYTIUM_2500
> +#include <linux/arm-smccc.h>
> +
> +#define CTL_RST_FUNC_ID 0xC2000011
> +
> +static void ctl_smc(unsigned long arg0, unsigned long arg1,
> + unsigned long arg2, unsigned long arg3)
> +{
> + struct arm_smccc_res res;
> +
> + arm_smccc_smc(arg0, arg1, arg2, arg3, 0, 0, 0, 0, &res);
> + if (res.a0 != 0)
> + pr_err("Error: Firmware call SMC reset Failed: %d, addr: 0x%lx\n",
> + (int)res.a0, arg2);
> +}
> +
> +static void ctl_timeout_reset(void)
> +{
> + ctl_smc(CTL_RST_FUNC_ID, 0x1, 0x28100208, 0x1);
> + ctl_smc(CTL_RST_FUNC_ID, 0x1, 0x2810020C, 0x1);
> +}
> +
> +static unsigned char intf_mem_inb_2500(const struct si_sm_io *io,
> + unsigned int offset)
> +{
> + ctl_timeout_reset();
> +
> + return readb((io->addr)+(offset * io->regspacing));
与 intf_mem_inb() 代码重复,下同。
> +}
> +
> +static unsigned char intf_mem_inw_2500(const struct si_sm_io *io,
> + unsigned int offset)
> +{
> + ctl_timeout_reset();
> +
> + return (readw((io->addr)+(offset * io->regspacing)) >> io->regshift)
> + & 0xff;
> +}
> +
> +static unsigned char intf_mem_inl_2500(const struct si_sm_io *io,
> + unsigned int offset)
> +{
> + ctl_timeout_reset();
> +
> + return (readl((io->addr)+(offset * io->regspacing)) >> io->regshift)
> + & 0xff;
> +}
> +
> +static unsigned char mem_inq_2500(const struct si_sm_io *io,
> + unsigned int offset)
> +{
> + ctl_timeout_reset();
> +
> + return (readq((io->addr)+(offset * io->regspacing)) >> io->regshift)
> + & 0xff;
> +}
> +#else
> +#define intf_mem_inb_2500 intf_mem_inb
> +#define intf_mem_inw_2500 intf_mem_inw
> +#define intf_mem_inl_2500 intf_mem_inl
> +#define mem_inq_2500 mem_inq
> +#endif
> +
> +static bool apply_phytium2500_workaround;
这个变量全局存在的,其实只在 phytium2500 平台上才有意义。
> +
> +#ifdef CONFIG_ACPI
> +struct ipmi_workaround_oem_info {
> + char oem_id[ACPI_OEM_ID_SIZE + 1];
> +};
> +
> +static struct ipmi_workaround_oem_info wa_info[] = {
> + {
> + .oem_id = "KPSVVJ",
> + }
> +};
> +
> +static void ipmi_check_phytium_workaround(void)
> +{
> + struct acpi_table_header tbl;
> + int i;
> +
> + if (ACPI_FAILURE(acpi_get_table_header(ACPI_SIG_DSDT, 0, &tbl)))
> + return;
> +
> + for (i = 0; i < ARRAY_SIZE(wa_info); i++) {
> + if (strncmp(wa_info[i].oem_id, tbl.oem_id, ACPI_OEM_ID_SIZE))
> + continue;
> +
> + apply_phytium2500_workaround = true;
> + break;
> + }
> +}
这个 CONFIG_ACPI 和 PHYTIUM 没有依赖,即便不是 PHYTIUM 平台,仍然会做这些没必要的检查。
而且在非 ARM64 架构上,这个 apply_phytium2500_workaround 仍然会全局存在。
是不是类似这样比较好:
#ifdef CONFIG_ARM_GIC_PHYTIUM_2500
static bool apply_phytium2500_workaround;
static void ipmi_check_phytium_workaround(void)
{
#ifdef CONFIG_ACPI
...
#endif
}
...
static inline void ipmi_phytium2500_workaround(void)
{
if (apply_phytium2500_workaround)
ctl_timeout_reset();
}
static unsigned char intf_mem_inb(const struct si_sm_io *io,
unsigned int offset)
{
+ ipmi_phytium2500_workaround();
return readb((io->addr)+(offset * io->regspacing));
}
static unsigned char intf_mem_inw(const struct si_sm_io *io,
unsigned int offset)
{
+ ipmi_phytium2500_workaround();
return (readw((io->addr)+(offset * io->regspacing)) >> io->regshift)
& 0xff;
}
...
#else
static inline void ipmi_phytium2500_workaround() {}
static inline void ipmi_check_phytium_workaround(void) {}
#endif
> +#else
> +static void ipmi_check_phytium_workaround(void)
> +{
> + apply_phytium2500_workaround = false;
> +}
> +#endif
> +
> static unsigned char intf_mem_inb(const struct si_sm_io *io,
> unsigned int offset)
> {
> @@ -81,26 +180,36 @@ int ipmi_si_mem_setup(struct si_sm_io *io)
> if (!addr)
> return -ENODEV;
>
> + ipmi_check_phytium_workaround();
> +
> /*
> * Figure out the actual readb/readw/readl/etc routine to use based
> * upon the register size.
> */
> switch (io->regsize) {
> case 1:
> - io->inputb = intf_mem_inb;
> + io->inputb = apply_phytium2500_workaround ?
> + intf_mem_inb_2500 :
> + intf_mem_inb;
> io->outputb = intf_mem_outb;
> break;
> case 2:
> - io->inputb = intf_mem_inw;
> + io->inputb = apply_phytium2500_workaround ?
> + intf_mem_inw_2500 :
> + intf_mem_inw;
> io->outputb = intf_mem_outw;
> break;
> case 4:
> - io->inputb = intf_mem_inl;
> + io->inputb = apply_phytium2500_workaround ?
> + intf_mem_inl_2500 :
> + intf_mem_inl;
> io->outputb = intf_mem_outl;
> break;
> #ifdef readq
> case 8:
> - io->inputb = mem_inq;
> + io->inputb = apply_phytium2500_workaround ?
> + mem_inq_2500 :
> + mem_inq;
> io->outputb = mem_outq;
> break;
> #endif
>