Commit 16e47624 authored by Hariprasad Shenai's avatar Hariprasad Shenai Committed by David S. Miller

cxgb4: Add new scheme to update T4/T5 firmware

Signed-off-by: default avatarHariprasad Shenai <hariprasad@chelsio.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent 70ee3666
......@@ -49,13 +49,15 @@
#include <asm/io.h>
#include "cxgb4_uld.h"
#define FW_VERSION_MAJOR 1
#define FW_VERSION_MINOR 4
#define FW_VERSION_MICRO 0
#define T4FW_VERSION_MAJOR 0x01
#define T4FW_VERSION_MINOR 0x06
#define T4FW_VERSION_MICRO 0x18
#define T4FW_VERSION_BUILD 0x00
#define FW_VERSION_MAJOR_T5 0
#define FW_VERSION_MINOR_T5 0
#define FW_VERSION_MICRO_T5 0
#define T5FW_VERSION_MAJOR 0x01
#define T5FW_VERSION_MINOR 0x08
#define T5FW_VERSION_MICRO 0x1C
#define T5FW_VERSION_BUILD 0x00
#define CH_WARN(adap, fmt, ...) dev_warn(adap->pdev_dev, fmt, ## __VA_ARGS__)
......@@ -287,6 +289,23 @@ struct adapter_params {
unsigned int ofldq_wr_cred;
};
#include "t4fw_api.h"
#define FW_VERSION(chip) ( \
FW_HDR_FW_VER_MAJOR_GET(chip##FW_VERSION_MAJOR) | \
FW_HDR_FW_VER_MINOR_GET(chip##FW_VERSION_MINOR) | \
FW_HDR_FW_VER_MICRO_GET(chip##FW_VERSION_MICRO) | \
FW_HDR_FW_VER_BUILD_GET(chip##FW_VERSION_BUILD))
#define FW_INTFVER(chip, intf) (FW_HDR_INTFVER_##intf)
struct fw_info {
u8 chip;
char *fs_name;
char *fw_mod_name;
struct fw_hdr fw_hdr;
};
struct trace_params {
u32 data[TRACE_LEN / 4];
u32 mask[TRACE_LEN / 4];
......@@ -901,7 +920,11 @@ int get_vpd_params(struct adapter *adapter, struct vpd_params *p);
int t4_load_fw(struct adapter *adapter, const u8 *fw_data, unsigned int size);
unsigned int t4_flash_cfg_addr(struct adapter *adapter);
int t4_load_cfg(struct adapter *adapter, const u8 *cfg_data, unsigned int size);
int t4_check_fw_version(struct adapter *adapter);
int t4_get_fw_version(struct adapter *adapter, u32 *vers);
int t4_get_tp_version(struct adapter *adapter, u32 *vers);
int t4_prep_fw(struct adapter *adap, struct fw_info *fw_info,
const u8 *fw_data, unsigned int fw_size,
struct fw_hdr *card_fw, enum dev_state state, int *reset);
int t4_prep_adapter(struct adapter *adapter);
int t4_port_init(struct adapter *adap, int mbox, int pf, int vf);
void t4_fatal_err(struct adapter *adapter);
......
......@@ -276,9 +276,9 @@ static DEFINE_PCI_DEVICE_TABLE(cxgb4_pci_tbl) = {
{ 0, }
};
#define FW_FNAME "cxgb4/t4fw.bin"
#define FW4_FNAME "cxgb4/t4fw.bin"
#define FW5_FNAME "cxgb4/t5fw.bin"
#define FW_CFNAME "cxgb4/t4-config.txt"
#define FW4_CFNAME "cxgb4/t4-config.txt"
#define FW5_CFNAME "cxgb4/t5-config.txt"
MODULE_DESCRIPTION(DRV_DESC);
......@@ -286,7 +286,7 @@ MODULE_AUTHOR("Chelsio Communications");
MODULE_LICENSE("Dual BSD/GPL");
MODULE_VERSION(DRV_VERSION);
MODULE_DEVICE_TABLE(pci, cxgb4_pci_tbl);
MODULE_FIRMWARE(FW_FNAME);
MODULE_FIRMWARE(FW4_FNAME);
MODULE_FIRMWARE(FW5_FNAME);
/*
......@@ -1070,72 +1070,6 @@ freeout: t4_free_sge_resources(adap);
return 0;
}
/*
* Returns 0 if new FW was successfully loaded, a positive errno if a load was
* started but failed, and a negative errno if flash load couldn't start.
*/
static int upgrade_fw(struct adapter *adap)
{
int ret;
u32 vers, exp_major;
const struct fw_hdr *hdr;
const struct firmware *fw;
struct device *dev = adap->pdev_dev;
char *fw_file_name;
switch (CHELSIO_CHIP_VERSION(adap->params.chip)) {
case CHELSIO_T4:
fw_file_name = FW_FNAME;
exp_major = FW_VERSION_MAJOR;
break;
case CHELSIO_T5:
fw_file_name = FW5_FNAME;
exp_major = FW_VERSION_MAJOR_T5;
break;
default:
dev_err(dev, "Unsupported chip type, %x\n", adap->params.chip);
return -EINVAL;
}
ret = request_firmware(&fw, fw_file_name, dev);
if (ret < 0) {
dev_err(dev, "unable to load firmware image %s, error %d\n",
fw_file_name, ret);
return ret;
}
hdr = (const struct fw_hdr *)fw->data;
vers = ntohl(hdr->fw_ver);
if (FW_HDR_FW_VER_MAJOR_GET(vers) != exp_major) {
ret = -EINVAL; /* wrong major version, won't do */
goto out;
}
/*
* If the flash FW is unusable or we found something newer, load it.
*/
if (FW_HDR_FW_VER_MAJOR_GET(adap->params.fw_vers) != exp_major ||
vers > adap->params.fw_vers) {
dev_info(dev, "upgrading firmware ...\n");
ret = t4_fw_upgrade(adap, adap->mbox, fw->data, fw->size,
/*force=*/false);
if (!ret)
dev_info(dev,
"firmware upgraded to version %pI4 from %s\n",
&hdr->fw_ver, fw_file_name);
else
dev_err(dev, "firmware upgrade failed! err=%d\n", -ret);
} else {
/*
* Tell our caller that we didn't upgrade the firmware.
*/
ret = -EINVAL;
}
out: release_firmware(fw);
return ret;
}
/*
* Allocate a chunk of memory using kmalloc or, if that fails, vmalloc.
* The allocated memory is cleared.
......@@ -4668,8 +4602,10 @@ static int adap_init0_config(struct adapter *adapter, int reset)
const struct firmware *cf;
unsigned long mtype = 0, maddr = 0;
u32 finiver, finicsum, cfcsum;
int ret, using_flash;
int ret;
int config_issued = 0;
char *fw_config_file, fw_config_file_path[256];
char *config_name = NULL;
/*
* Reset device if necessary.
......@@ -4688,7 +4624,7 @@ static int adap_init0_config(struct adapter *adapter, int reset)
*/
switch (CHELSIO_CHIP_VERSION(adapter->params.chip)) {
case CHELSIO_T4:
fw_config_file = FW_CFNAME;
fw_config_file = FW4_CFNAME;
break;
case CHELSIO_T5:
fw_config_file = FW5_CFNAME;
......@@ -4702,13 +4638,16 @@ static int adap_init0_config(struct adapter *adapter, int reset)
ret = request_firmware(&cf, fw_config_file, adapter->pdev_dev);
if (ret < 0) {
using_flash = 1;
config_name = "On FLASH";
mtype = FW_MEMTYPE_CF_FLASH;
maddr = t4_flash_cfg_addr(adapter);
} else {
u32 params[7], val[7];
using_flash = 0;
sprintf(fw_config_file_path,
"/lib/firmware/%s", fw_config_file);
config_name = fw_config_file_path;
if (cf->size >= FLASH_CFG_MAX_SIZE)
ret = -ENOMEM;
else {
......@@ -4776,6 +4715,26 @@ static int adap_init0_config(struct adapter *adapter, int reset)
FW_LEN16(caps_cmd));
ret = t4_wr_mbox(adapter, adapter->mbox, &caps_cmd, sizeof(caps_cmd),
&caps_cmd);
/* If the CAPS_CONFIG failed with an ENOENT (for a Firmware
* Configuration File in FLASH), our last gasp effort is to use the
* Firmware Configuration File which is embedded in the firmware. A
* very few early versions of the firmware didn't have one embedded
* but we can ignore those.
*/
if (ret == -ENOENT) {
memset(&caps_cmd, 0, sizeof(caps_cmd));
caps_cmd.op_to_write =
htonl(FW_CMD_OP(FW_CAPS_CONFIG_CMD) |
FW_CMD_REQUEST |
FW_CMD_READ);
caps_cmd.cfvalid_to_len16 = htonl(FW_LEN16(caps_cmd));
ret = t4_wr_mbox(adapter, adapter->mbox, &caps_cmd,
sizeof(caps_cmd), &caps_cmd);
config_name = "Firmware Default";
}
config_issued = 1;
if (ret < 0)
goto bye;
......@@ -4816,7 +4775,6 @@ static int adap_init0_config(struct adapter *adapter, int reset)
if (ret < 0)
goto bye;
sprintf(fw_config_file_path, "/lib/firmware/%s", fw_config_file);
/*
* Return successfully and note that we're operating with parameters
* not supplied by the driver, rather than from hard-wired
......@@ -4824,11 +4782,8 @@ static int adap_init0_config(struct adapter *adapter, int reset)
*/
adapter->flags |= USING_SOFT_PARAMS;
dev_info(adapter->pdev_dev, "Successfully configured using Firmware "\
"Configuration File %s, version %#x, computed checksum %#x\n",
(using_flash
? "in device FLASH"
: fw_config_file_path),
finiver, cfcsum);
"Configuration File \"%s\", version %#x, computed checksum %#x\n",
config_name, finiver, cfcsum);
return 0;
/*
......@@ -4837,9 +4792,9 @@ static int adap_init0_config(struct adapter *adapter, int reset)
* want to issue a warning since this is fairly common.)
*/
bye:
if (ret != -ENOENT)
dev_warn(adapter->pdev_dev, "Configuration file error %d\n",
-ret);
if (config_issued && ret != -ENOENT)
dev_warn(adapter->pdev_dev, "\"%s\" configuration file error %d\n",
config_name, -ret);
return ret;
}
......@@ -5086,6 +5041,47 @@ static int adap_init0_no_config(struct adapter *adapter, int reset)
return ret;
}
static struct fw_info fw_info_array[] = {
{
.chip = CHELSIO_T4,
.fs_name = FW4_CFNAME,
.fw_mod_name = FW4_FNAME,
.fw_hdr = {
.chip = FW_HDR_CHIP_T4,
.fw_ver = __cpu_to_be32(FW_VERSION(T4)),
.intfver_nic = FW_INTFVER(T4, NIC),
.intfver_vnic = FW_INTFVER(T4, VNIC),
.intfver_ri = FW_INTFVER(T4, RI),
.intfver_iscsi = FW_INTFVER(T4, ISCSI),
.intfver_fcoe = FW_INTFVER(T4, FCOE),
},
}, {
.chip = CHELSIO_T5,
.fs_name = FW5_CFNAME,
.fw_mod_name = FW5_FNAME,
.fw_hdr = {
.chip = FW_HDR_CHIP_T5,
.fw_ver = __cpu_to_be32(FW_VERSION(T5)),
.intfver_nic = FW_INTFVER(T5, NIC),
.intfver_vnic = FW_INTFVER(T5, VNIC),
.intfver_ri = FW_INTFVER(T5, RI),
.intfver_iscsi = FW_INTFVER(T5, ISCSI),
.intfver_fcoe = FW_INTFVER(T5, FCOE),
},
}
};
static struct fw_info *find_fw_info(int chip)
{
int i;
for (i = 0; i < ARRAY_SIZE(fw_info_array); i++) {
if (fw_info_array[i].chip == chip)
return &fw_info_array[i];
}
return NULL;
}
/*
* Phase 0 of initialization: contact FW, obtain config, perform basic init.
*/
......@@ -5123,44 +5119,54 @@ static int adap_init0(struct adapter *adap)
* later reporting and B. to warn if the currently loaded firmware
* is excessively mismatched relative to the driver.)
*/
ret = t4_check_fw_version(adap);
/* The error code -EFAULT is returned by t4_check_fw_version() if
* firmware on adapter < supported firmware. If firmware on adapter
* is too old (not supported by driver) and we're the MASTER_PF set
* adapter state to DEV_STATE_UNINIT to force firmware upgrade
* and reinitialization.
*/
if ((adap->flags & MASTER_PF) && ret == -EFAULT)
state = DEV_STATE_UNINIT;
t4_get_fw_version(adap, &adap->params.fw_vers);
t4_get_tp_version(adap, &adap->params.tp_vers);
if ((adap->flags & MASTER_PF) && state != DEV_STATE_INIT) {
if (ret == -EINVAL || ret == -EFAULT || ret > 0) {
if (upgrade_fw(adap) >= 0) {
/*
* Note that the chip was reset as part of the
* firmware upgrade so we don't reset it again
* below and grab the new firmware version.
*/
reset = 0;
ret = t4_check_fw_version(adap);
} else
if (ret == -EFAULT) {
/*
* Firmware is old but still might
* work if we force reinitialization
* of the adapter. Ignoring FW upgrade
* failure.
struct fw_info *fw_info;
struct fw_hdr *card_fw;
const struct firmware *fw;
const u8 *fw_data = NULL;
unsigned int fw_size = 0;
/* This is the firmware whose headers the driver was compiled
* against
*/
dev_warn(adap->pdev_dev,
"Ignoring firmware upgrade "
"failure, and forcing driver "
"to reinitialize the "
"adapter.\n");
ret = 0;
fw_info = find_fw_info(CHELSIO_CHIP_VERSION(adap->params.chip));
if (fw_info == NULL) {
dev_err(adap->pdev_dev,
"unable to get firmware info for chip %d.\n",
CHELSIO_CHIP_VERSION(adap->params.chip));
return -EINVAL;
}
/* allocate memory to read the header of the firmware on the
* card
*/
card_fw = t4_alloc_mem(sizeof(*card_fw));
/* Get FW from from /lib/firmware/ */
ret = request_firmware(&fw, fw_info->fw_mod_name,
adap->pdev_dev);
if (ret < 0) {
dev_err(adap->pdev_dev,
"unable to load firmware image %s, error %d\n",
fw_info->fw_mod_name, ret);
} else {
fw_data = fw->data;
fw_size = fw->size;
}
/* upgrade FW logic */
ret = t4_prep_fw(adap, fw_info, fw_data, fw_size, card_fw,
state, &reset);
/* Cleaning up */
if (fw != NULL)
release_firmware(fw);
t4_free_mem(card_fw);
if (ret < 0)
return ret;
goto bye;
}
/*
......
......@@ -863,104 +863,169 @@ static int t4_write_flash(struct adapter *adapter, unsigned int addr,
}
/**
* get_fw_version - read the firmware version
* t4_get_fw_version - read the firmware version
* @adapter: the adapter
* @vers: where to place the version
*
* Reads the FW version from flash.
*/
static int get_fw_version(struct adapter *adapter, u32 *vers)
int t4_get_fw_version(struct adapter *adapter, u32 *vers)
{
return t4_read_flash(adapter, adapter->params.sf_fw_start +
offsetof(struct fw_hdr, fw_ver), 1, vers, 0);
return t4_read_flash(adapter, FLASH_FW_START +
offsetof(struct fw_hdr, fw_ver), 1,
vers, 0);
}
/**
* get_tp_version - read the TP microcode version
* t4_get_tp_version - read the TP microcode version
* @adapter: the adapter
* @vers: where to place the version
*
* Reads the TP microcode version from flash.
*/
static int get_tp_version(struct adapter *adapter, u32 *vers)
int t4_get_tp_version(struct adapter *adapter, u32 *vers)
{
return t4_read_flash(adapter, adapter->params.sf_fw_start +
return t4_read_flash(adapter, FLASH_FW_START +
offsetof(struct fw_hdr, tp_microcode_ver),
1, vers, 0);
}
/**
* t4_check_fw_version - check if the FW is compatible with this driver
* @adapter: the adapter
*
* Checks if an adapter's FW is compatible with the driver. Returns 0
* if there's exact match, a negative error if the version could not be
* read or there's a major version mismatch, and a positive value if the
* expected major version is found but there's a minor version mismatch.
/* Is the given firmware API compatible with the one the driver was compiled
* with?
*/
int t4_check_fw_version(struct adapter *adapter)
static int fw_compatible(const struct fw_hdr *hdr1, const struct fw_hdr *hdr2)
{
u32 api_vers[2];
int ret, major, minor, micro;
int exp_major, exp_minor, exp_micro;
ret = get_fw_version(adapter, &adapter->params.fw_vers);
if (!ret)
ret = get_tp_version(adapter, &adapter->params.tp_vers);
if (!ret)
ret = t4_read_flash(adapter, adapter->params.sf_fw_start +
offsetof(struct fw_hdr, intfver_nic),
2, api_vers, 1);
if (ret)
return ret;
/* short circuit if it's the exact same firmware version */
if (hdr1->chip == hdr2->chip && hdr1->fw_ver == hdr2->fw_ver)
return 1;
major = FW_HDR_FW_VER_MAJOR_GET(adapter->params.fw_vers);
minor = FW_HDR_FW_VER_MINOR_GET(adapter->params.fw_vers);
micro = FW_HDR_FW_VER_MICRO_GET(adapter->params.fw_vers);
#define SAME_INTF(x) (hdr1->intfver_##x == hdr2->intfver_##x)
if (hdr1->chip == hdr2->chip && SAME_INTF(nic) && SAME_INTF(vnic) &&
SAME_INTF(ri) && SAME_INTF(iscsi) && SAME_INTF(fcoe))
return 1;
#undef SAME_INTF
switch (CHELSIO_CHIP_VERSION(adapter->params.chip)) {
case CHELSIO_T4:
exp_major = FW_VERSION_MAJOR;
exp_minor = FW_VERSION_MINOR;
exp_micro = FW_VERSION_MICRO;
break;
case CHELSIO_T5:
exp_major = FW_VERSION_MAJOR_T5;
exp_minor = FW_VERSION_MINOR_T5;
exp_micro = FW_VERSION_MICRO_T5;
break;
default:
dev_err(adapter->pdev_dev, "Unsupported chip type, %x\n",
adapter->params.chip);
return -EINVAL;
}
return 0;
}
memcpy(adapter->params.api_vers, api_vers,
sizeof(adapter->params.api_vers));
/* The firmware in the filesystem is usable, but should it be installed?
* This routine explains itself in detail if it indicates the filesystem
* firmware should be installed.
*/
static int should_install_fs_fw(struct adapter *adap, int card_fw_usable,
int k, int c)
{
const char *reason;
if (major < exp_major || (major == exp_major && minor < exp_minor) ||
(major == exp_major && minor == exp_minor && micro < exp_micro)) {
dev_err(adapter->pdev_dev,
"Card has firmware version %u.%u.%u, minimum "
"supported firmware is %u.%u.%u.\n", major, minor,
micro, exp_major, exp_minor, exp_micro);
return -EFAULT;
if (!card_fw_usable) {
reason = "incompatible or unusable";
goto install;
}
if (major != exp_major) { /* major mismatch - fail */
dev_err(adapter->pdev_dev,
"card FW has major version %u, driver wants %u\n",
major, exp_major);
return -EINVAL;
if (k > c) {
reason = "older than the version supported with this driver";
goto install;
}
if (minor == exp_minor && micro == exp_micro)
return 0; /* perfect match */
return 0;
install:
dev_err(adap->pdev_dev, "firmware on card (%u.%u.%u.%u) is %s, "
"installing firmware %u.%u.%u.%u on card.\n",
FW_HDR_FW_VER_MAJOR_GET(c), FW_HDR_FW_VER_MINOR_GET(c),
FW_HDR_FW_VER_MICRO_GET(c), FW_HDR_FW_VER_BUILD_GET(c), reason,
FW_HDR_FW_VER_MAJOR_GET(k), FW_HDR_FW_VER_MINOR_GET(k),
FW_HDR_FW_VER_MICRO_GET(k), FW_HDR_FW_VER_BUILD_GET(k));
/* Minor/micro version mismatch. Report it but often it's OK. */
return 1;
}
int t4_prep_fw(struct adapter *adap, struct fw_info *fw_info,
const u8 *fw_data, unsigned int fw_size,
struct fw_hdr *card_fw, enum dev_state state,
int *reset)
{
int ret, card_fw_usable, fs_fw_usable;
const struct fw_hdr *fs_fw;
const struct fw_hdr *drv_fw;
drv_fw = &fw_info->fw_hdr;
/* Read the header of the firmware on the card */
ret = -t4_read_flash(adap, FLASH_FW_START,
sizeof(*card_fw) / sizeof(uint32_t),
(uint32_t *)card_fw, 1);
if (ret == 0) {
card_fw_usable = fw_compatible(drv_fw, (const void *)card_fw);
} else {
dev_err(adap->pdev_dev,
"Unable to read card's firmware header: %d\n", ret);
card_fw_usable = 0;
}
if (fw_data != NULL) {
fs_fw = (const void *)fw_data;
fs_fw_usable = fw_compatible(drv_fw, fs_fw);
} else {
fs_fw = NULL;
fs_fw_usable = 0;
}
if (card_fw_usable && card_fw->fw_ver == drv_fw->fw_ver &&
(!fs_fw_usable || fs_fw->fw_ver == drv_fw->fw_ver)) {
/* Common case: the firmware on the card is an exact match and
* the filesystem one is an exact match too, or the filesystem
* one is absent/incompatible.
*/
} else if (fs_fw_usable && state == DEV_STATE_UNINIT &&
should_install_fs_fw(adap, card_fw_usable,
be32_to_cpu(fs_fw->fw_ver),
be32_to_cpu(card_fw->fw_ver))) {
ret = -t4_fw_upgrade(adap, adap->mbox, fw_data,
fw_size, 0);
if (ret != 0) {
dev_err(adap->pdev_dev,
"failed to install firmware: %d\n", ret);
goto bye;
}
/* Installed successfully, update the cached header too. */
memcpy(card_fw, fs_fw, sizeof(*card_fw));
card_fw_usable = 1;
*reset = 0; /* already reset as part of load_fw */
}
if (!card_fw_usable) {
uint32_t d, c, k;
d = be32_to_cpu(drv_fw->fw_ver);
c = be32_to_cpu(card_fw->fw_ver);
k = fs_fw ? be32_to_cpu(fs_fw->fw_ver) : 0;
dev_err(adap->pdev_dev, "Cannot find a usable firmware: "
"chip state %d, "
"driver compiled with %d.%d.%d.%d, "
"card has %d.%d.%d.%d, filesystem has %d.%d.%d.%d\n",
state,
FW_HDR_FW_VER_MAJOR_GET(d), FW_HDR_FW_VER_MINOR_GET(d),
FW_HDR_FW_VER_MICRO_GET(d), FW_HDR_FW_VER_BUILD_GET(d),
FW_HDR_FW_VER_MAJOR_GET(c), FW_HDR_FW_VER_MINOR_GET(c),
FW_HDR_FW_VER_MICRO_GET(c), FW_HDR_FW_VER_BUILD_GET(c),
FW_HDR_FW_VER_MAJOR_GET(k), FW_HDR_FW_VER_MINOR_GET(k),
FW_HDR_FW_VER_MICRO_GET(k), FW_HDR_FW_VER_BUILD_GET(k));
ret = EINVAL;
goto bye;
}
/* We're using whatever's on the card and it's known to be good. */
adap->params.fw_vers = be32_to_cpu(card_fw->fw_ver);
adap->params.tp_vers = be32_to_cpu(card_fw->tp_microcode_ver);
bye:
return ret;
}
/**
* t4_flash_erase_sectors - erase a range of flash sectors
* @adapter: the adapter
......
......@@ -2157,7 +2157,7 @@ struct fw_debug_cmd {
struct fw_hdr {
u8 ver;
u8 reserved1;
u8 chip; /* terminator chip type */
__be16 len512; /* bin length in units of 512-bytes */
__be32 fw_ver; /* firmware version */
__be32 tp_microcode_ver;
......@@ -2176,6 +2176,11 @@ struct fw_hdr {
__be32 reserved6[23];
};
enum fw_hdr_chip {
FW_HDR_CHIP_T4,
FW_HDR_CHIP_T5
};
#define FW_HDR_FW_VER_MAJOR_GET(x) (((x) >> 24) & 0xff)
#define FW_HDR_FW_VER_MINOR_GET(x) (((x) >> 16) & 0xff)
#define FW_HDR_FW_VER_MICRO_GET(x) (((x) >> 8) & 0xff)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment