Commit 3f5ae09d authored by Linus Torvalds's avatar Linus Torvalds

Forward-port PIRQ table updates from 2.4.x

This separates out the PIRQ table parsing to vendor-specific
code, which allows us to handle specific vendor quirks. In
particular, SiS has a really funky notion of what PCI device
ID's are meant to be.

Some hardware designers seem to be hitting the recreational
drugs a bit too heavily. Tssk, tssk.

The Sis96x irq routing update is confirmed to fix at least
one laptop.
parent d11cad54
......@@ -44,6 +44,11 @@ struct irq_router {
int (*set)(struct pci_dev *router, struct pci_dev *dev, int pirq, int new);
};
struct irq_router_handler {
u16 vendor;
int (*probe)(struct irq_router *r, struct pci_dev *router, u16 device);
};
int (*pcibios_enable_irq)(struct pci_dev *dev) = NULL;
/*
......@@ -258,111 +263,220 @@ static int pirq_cyrix_set(struct pci_dev *router, struct pci_dev *dev, int pirq,
}
/*
* PIRQ routing for SiS 85C503 router used in several SiS chipsets
* According to the SiS 5595 datasheet (preliminary V1.0, 12/24/1997)
* the related registers work as follows:
*
* general: one byte per re-routable IRQ,
* PIRQ routing for SiS 85C503 router used in several SiS chipsets.
* We have to deal with the following issues here:
* - vendors have different ideas about the meaning of link values
* - some onboard devices (integrated in the chipset) have special
* links and are thus routed differently (i.e. not via PCI INTA-INTD)
* - different revision of the router have a different layout for
* the routing registers, particularly for the onchip devices
*
* For all routing registers the common thing is we have one byte
* per routeable link which is defined as:
* bit 7 IRQ mapping enabled (0) or disabled (1)
* bits [6:4] reserved
* bits [6:4] reserved (sometimes used for onchip devices)
* bits [3:0] IRQ to map to
* allowed: 3-7, 9-12, 14-15
* reserved: 0, 1, 2, 8, 13
*
* individual registers in device config space:
* The config-space registers located at 0x41/0x42/0x43/0x44 are
* always used to route the normal PCI INT A/B/C/D respectively.
* Apparently there are systems implementing PCI routing table using
* link values 0x01-0x04 and others using 0x41-0x44 for PCI INTA..D.
* We try our best to handle both link mappings.
*
* Currently (2003-05-21) it appears most SiS chipsets follow the
* definition of routing registers from the SiS-5595 southbridge.
* According to the SiS 5595 datasheets the revision id's of the
* router (ISA-bridge) should be 0x01 or 0xb0.
*
* 0x41/0x42/0x43/0x44: PCI INT A/B/C/D - bits as in general case
* Furthermore we've also seen lspci dumps with revision 0x00 and 0xb1.
* Looks like these are used in a number of SiS 5xx/6xx/7xx chipsets.
* They seem to work with the current routing code. However there is
* some concern because of the two USB-OHCI HCs (original SiS 5595
* had only one). YMMV.
*
* 0x61: IDEIRQ: bits as in general case - but:
* bits [6:5] must be written 01
* bit 4 channel-select primary (0), secondary (1)
* Onchip routing for router rev-id 0x01/0xb0 and probably 0x00/0xb1:
*
* 0x62: USBIRQ: bits as in general case - but:
* bit 4 OHCI function disabled (0), enabled (1)
* 0x61: IDEIRQ:
* bits [6:5] must be written 01
* bit 4 channel-select primary (0), secondary (1)
*
* 0x62: USBIRQ:
* bit 6 OHCI function disabled (0), enabled (1)
*
* 0x6a: ACPI/SCI IRQ - bits as in general case
* 0x6a: ACPI/SCI IRQ: bits 4-6 reserved
*
* 0x7e: Data Acq. Module IRQ - bits 4-6 reserved
*
* We support USBIRQ (in addition to INTA-INTD) and keep the
* IDE, ACPI and DAQ routing untouched as set by the BIOS.
*
* Currently the only reported exception is the new SiS 65x chipset
* which includes the SiS 69x southbridge. Here we have the 85C503
* router revision 0x04 and there are changes in the register layout
* mostly related to the different USB HCs with USB 2.0 support.
*
* 0x7e: Data Acq. Module IRQ - bits as in general case
* Onchip routing for router rev-id 0x04 (try-and-error observation)
*
* Apparently there are systems implementing PCI routing table using both
* link values 0x01-0x04 and 0x41-0x44 for PCI INTA..D, but register offsets
* like 0x62 as link values for USBIRQ e.g. So there is no simple
* "register = offset + pirq" relation.
* Currently we support PCI INTA..D and USBIRQ and try our best to handle
* both link mappings.
* IDE/ACPI/DAQ mapping is currently unsupported (left untouched as set by BIOS).
* 0x60/0x61/0x62/0x63: 1xEHCI and 3xOHCI (companion) USB-HCs
* bit 6-4 are probably unused, not like 5595
*/
static int pirq_sis_get(struct pci_dev *router, struct pci_dev *dev, int pirq)
#define PIRQ_SIS_IRQ_MASK 0x0f
#define PIRQ_SIS_IRQ_DISABLE 0x80
#define PIRQ_SIS_USB_ENABLE 0x40
/* return value:
* -1 on error
* 0 for PCI INTA-INTD
* 0 or enable bit mask to check or set for onchip functions
*/
static inline int pirq_sis5595_onchip(int pirq, int *reg)
{
u8 x;
int reg = pirq;
int ret = -1;
*reg = pirq;
switch(pirq) {
case 0x01:
case 0x02:
case 0x03:
case 0x04:
reg += 0x40;
case 0x41:
case 0x42:
case 0x43:
case 0x44:
case 0x62:
pci_read_config_byte(router, reg, &x);
if (reg != 0x62)
break;
if (!(x & 0x40))
return 0;
break;
case 0x61:
case 0x6a:
case 0x7e:
printk(KERN_INFO "SiS pirq: advanced IDE/ACPI/DAQ mapping not yet implemented\n");
return 0;
default:
printk(KERN_INFO "SiS router pirq escape (%d)\n", pirq);
return 0;
case 0x01:
case 0x02:
case 0x03:
case 0x04:
*reg += 0x40;
case 0x41:
case 0x42:
case 0x43:
case 0x44:
ret = 0;
break;
case 0x62:
ret = PIRQ_SIS_USB_ENABLE; /* documented for 5595 */
break;
case 0x61:
case 0x6a:
case 0x7e:
printk(KERN_INFO "SiS pirq: IDE/ACPI/DAQ mapping not implemented: (%u)\n",
(unsigned) pirq);
/* fall thru */
default:
printk(KERN_INFO "SiS router unknown request: (%u)\n",
(unsigned) pirq);
break;
}
return (x & 0x80) ? 0 : (x & 0x0f);
}
return ret;
}
static int pirq_sis_set(struct pci_dev *router, struct pci_dev *dev, int pirq, int irq)
/* return value:
* -1 on error
* 0 for PCI INTA-INTD
* 0 or enable bit mask to check or set for onchip functions
*/
static inline int pirq_sis96x_onchip(int pirq, int *reg)
{
u8 x;
int reg = pirq;
int ret = -1;
*reg = pirq;
switch(pirq) {
case 0x01:
case 0x02:
case 0x03:
case 0x04:
reg += 0x40;
case 0x41:
case 0x42:
case 0x43:
case 0x44:
case 0x62:
x = (irq&0x0f) ? (irq&0x0f) : 0x80;
if (reg != 0x62)
break;
/* always mark OHCI enabled, as nothing else knows about this */
x |= 0x40;
break;
case 0x61:
case 0x6a:
case 0x7e:
printk(KERN_INFO "advanced SiS pirq mapping not yet implemented\n");
return 0;
default:
printk(KERN_INFO "SiS router pirq escape (%d)\n", pirq);
return 0;
case 0x01:
case 0x02:
case 0x03:
case 0x04:
*reg += 0x40;
case 0x41:
case 0x42:
case 0x43:
case 0x44:
case 0x60:
case 0x61:
case 0x62:
case 0x63:
ret = 0;
break;
default:
printk(KERN_INFO "SiS router unknown request: (%u)\n",
(unsigned) pirq);
break;
}
return ret;
}
static int pirq_sis5595_get(struct pci_dev *router, struct pci_dev *dev, int pirq)
{
u8 x;
int reg, check;
check = pirq_sis5595_onchip(pirq, &reg);
if (check < 0)
return 0;
pci_read_config_byte(router, reg, &x);
if (check != 0 && !(x & check))
return 0;
return (x & PIRQ_SIS_IRQ_DISABLE) ? 0 : (x & PIRQ_SIS_IRQ_MASK);
}
static int pirq_sis96x_get(struct pci_dev *router, struct pci_dev *dev, int pirq)
{
u8 x;
int reg, check;
check = pirq_sis96x_onchip(pirq, &reg);
if (check < 0)
return 0;
pci_read_config_byte(router, reg, &x);
if (check != 0 && !(x & check))
return 0;
return (x & PIRQ_SIS_IRQ_DISABLE) ? 0 : (x & PIRQ_SIS_IRQ_MASK);
}
static int pirq_sis5595_set(struct pci_dev *router, struct pci_dev *dev, int pirq, int irq)
{
u8 x;
int reg, set;
set = pirq_sis5595_onchip(pirq, &reg);
if (set < 0)
return 0;
x = (irq & PIRQ_SIS_IRQ_MASK);
if (x == 0)
x = PIRQ_SIS_IRQ_DISABLE;
else
x |= set;
pci_write_config_byte(router, reg, x);
return 1;
}
static int pirq_sis96x_set(struct pci_dev *router, struct pci_dev *dev, int pirq, int irq)
{
u8 x;
int reg, set;
set = pirq_sis96x_onchip(pirq, &reg);
if (set < 0)
return 0;
x = (irq & PIRQ_SIS_IRQ_MASK);
if (x == 0)
x = PIRQ_SIS_IRQ_DISABLE;
else
x |= set;
pci_write_config_byte(router, reg, x);
return 1;
}
/*
* VLSI: nibble offset 0x74 - educated guess due to routing table and
* config space of VLSI 82C534 PCI-bridge/router (1004:0102)
......@@ -455,96 +569,252 @@ static int pirq_bios_set(struct pci_dev *router, struct pci_dev *dev, int pirq,
return pcibios_set_irq_routing(bridge, pin, irq);
}
static struct irq_router pirq_bios_router =
{ "BIOS", 0, 0, NULL, pirq_bios_set };
#endif
static __init int intel_router_probe(struct irq_router *r, struct pci_dev *router, u16 device)
{
#if 0 /* Let's see what chip this is supposed to be ... */
/* We must not touch 440GX even if we have tables. 440GX has
different IRQ routing weirdness */
if (pci_find_device(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82440GX, NULL))
return 0;
#endif
static struct irq_router pirq_routers[] = {
{ "PIIX", PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371FB_0, pirq_piix_get, pirq_piix_set },
{ "PIIX", PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371SB_0, pirq_piix_get, pirq_piix_set },
{ "PIIX", PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_0, pirq_piix_get, pirq_piix_set },
{ "PIIX", PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371MX, pirq_piix_get, pirq_piix_set },
{ "PIIX", PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82443MX_0, pirq_piix_get, pirq_piix_set },
{ "PIIX", PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801AA_0, pirq_piix_get, pirq_piix_set },
{ "PIIX", PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801AB_0, pirq_piix_get, pirq_piix_set },
{ "PIIX", PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801BA_0, pirq_piix_get, pirq_piix_set },
{ "PIIX", PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801BA_10, pirq_piix_get, pirq_piix_set },
{ "PIIX", PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801CA_0, pirq_piix_get, pirq_piix_set },
{ "PIIX", PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801CA_12, pirq_piix_get, pirq_piix_set },
{ "PIIX", PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801DB_0, pirq_piix_get, pirq_piix_set },
{ "PIIX", PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801E_0, pirq_piix_get, pirq_piix_set },
{ "PIIX", PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801EB_0, pirq_piix_get, pirq_piix_set },
{ "PIIX", PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ESB_0, pirq_piix_get, pirq_piix_set },
{ "ALI", PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M1533, pirq_ali_get, pirq_ali_set },
{ "ITE", PCI_VENDOR_ID_ITE, PCI_DEVICE_ID_ITE_IT8330G_0, pirq_ite_get, pirq_ite_set },
{ "VIA", PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C586_0, pirq_via_get, pirq_via_set },
{ "VIA", PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C596, pirq_via_get, pirq_via_set },
{ "VIA", PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C686, pirq_via_get, pirq_via_set },
{ "OPTI", PCI_VENDOR_ID_OPTI, PCI_DEVICE_ID_OPTI_82C700, pirq_opti_get, pirq_opti_set },
{ "NatSemi", PCI_VENDOR_ID_CYRIX, PCI_DEVICE_ID_CYRIX_5520, pirq_cyrix_get, pirq_cyrix_set },
{ "SIS", PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_503, pirq_sis_get, pirq_sis_set },
{ "VLSI 82C534", PCI_VENDOR_ID_VLSI, PCI_DEVICE_ID_VLSI_82C534, pirq_vlsi_get, pirq_vlsi_set },
{ "ServerWorks", PCI_VENDOR_ID_SERVERWORKS, PCI_DEVICE_ID_SERVERWORKS_OSB4,
pirq_serverworks_get, pirq_serverworks_set },
{ "ServerWorks", PCI_VENDOR_ID_SERVERWORKS, PCI_DEVICE_ID_SERVERWORKS_CSB5,
pirq_serverworks_get, pirq_serverworks_set },
{ "AMD756 VIPER", PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_VIPER_740B,
pirq_amd756_get, pirq_amd756_set },
{ "AMD766", PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_VIPER_7413,
pirq_amd756_get, pirq_amd756_set },
{ "AMD768", PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_OPUS_7443,
pirq_amd756_get, pirq_amd756_set },
{ "default", 0, 0, NULL, NULL }
};
switch(device)
{
case PCI_DEVICE_ID_INTEL_82371FB_0:
case PCI_DEVICE_ID_INTEL_82371SB_0:
case PCI_DEVICE_ID_INTEL_82371AB_0:
case PCI_DEVICE_ID_INTEL_82371MX:
case PCI_DEVICE_ID_INTEL_82443MX_0:
case PCI_DEVICE_ID_INTEL_82801AA_0:
case PCI_DEVICE_ID_INTEL_82801AB_0:
case PCI_DEVICE_ID_INTEL_82801BA_0:
case PCI_DEVICE_ID_INTEL_82801BA_10:
case PCI_DEVICE_ID_INTEL_82801CA_0:
case PCI_DEVICE_ID_INTEL_82801CA_12:
case PCI_DEVICE_ID_INTEL_82801DB_0:
case PCI_DEVICE_ID_INTEL_82801E_0:
case PCI_DEVICE_ID_INTEL_82801EB_0:
case PCI_DEVICE_ID_INTEL_ESB_0:
r->name = "PIIX/ICH";
r->get = pirq_piix_get;
r->set = pirq_piix_set;
return 1;
}
return 0;
}
static struct irq_router *pirq_router;
static __init int via_router_probe(struct irq_router *r, struct pci_dev *router, u16 device)
{
/* FIXME: We should move some of the quirk fixup stuff here */
switch(device)
{
case PCI_DEVICE_ID_VIA_82C586_0:
case PCI_DEVICE_ID_VIA_82C596:
case PCI_DEVICE_ID_VIA_82C686:
case PCI_DEVICE_ID_VIA_8231:
/* FIXME: add new ones for 8233/5 */
r->name = "VIA";
r->get = pirq_via_get;
r->set = pirq_via_set;
return 1;
}
return 0;
}
static __init int vlsi_router_probe(struct irq_router *r, struct pci_dev *router, u16 device)
{
switch(device)
{
case PCI_DEVICE_ID_VLSI_82C534:
r->name = "VLSI 82C534";
r->get = pirq_vlsi_get;
r->set = pirq_vlsi_set;
return 1;
}
return 0;
}
static __init int serverworks_router_probe(struct irq_router *r, struct pci_dev *router, u16 device)
{
switch(device)
{
case PCI_DEVICE_ID_SERVERWORKS_OSB4:
case PCI_DEVICE_ID_SERVERWORKS_CSB5:
r->name = "ServerWorks";
r->get = pirq_serverworks_get;
r->set = pirq_serverworks_set;
return 1;
}
return 0;
}
static __init int sis_router_probe(struct irq_router *r, struct pci_dev *router, u16 device)
{
if (device != PCI_DEVICE_ID_SI_503)
return 0;
/*
* In case of SiS south bridge, we need to detect the two
* kinds of routing tables we have seen so far (5595 and 96x).
*
* The 96x tends to still come with routing tables that claim
* to be 503's.. Silly thing. Check the actual router chip.
*/
if ((router->device & 0xfff0) == 0x0960) {
r->name = "SIS96x";
r->get = pirq_sis96x_get;
r->set = pirq_sis96x_set;
DBG("PCI: Detecting SiS router at %02x:%02x : SiS096x detected\n",
rt->rtr_bus, rt->rtr_devfn);
} else {
r->name = "SIS5595";
r->get = pirq_sis5595_get;
r->set = pirq_sis5595_set;
DBG("PCI: Detecting SiS router at %02x:%02x : SiS5595 detected\n",
rt->rtr_bus, rt->rtr_devfn);
}
return 1;
}
static __init int cyrix_router_probe(struct irq_router *r, struct pci_dev *router, u16 device)
{
switch(device)
{
case PCI_DEVICE_ID_CYRIX_5520:
r->name = "NatSemi";
r->get = pirq_cyrix_get;
r->set = pirq_cyrix_set;
return 1;
}
return 0;
}
static __init int opti_router_probe(struct irq_router *r, struct pci_dev *router, u16 device)
{
switch(device)
{
case PCI_DEVICE_ID_OPTI_82C700:
r->name = "OPTI";
r->get = pirq_opti_get;
r->set = pirq_opti_set;
return 1;
}
return 0;
}
static __init int ite_router_probe(struct irq_router *r, struct pci_dev *router, u16 device)
{
switch(device)
{
case PCI_DEVICE_ID_ITE_IT8330G_0:
r->name = "ITE";
r->get = pirq_ite_get;
r->set = pirq_ite_set;
return 1;
}
return 0;
}
static __init int ali_router_probe(struct irq_router *r, struct pci_dev *router, u16 device)
{
switch(device)
{
case PCI_DEVICE_ID_AL_M1533:
r->name = "ALI";
r->get = pirq_ali_get;
r->set = pirq_ali_set;
return 1;
/* Should add 156x some day */
}
return 0;
}
static __init int amd_router_probe(struct irq_router *r, struct pci_dev *router, u16 device)
{
switch(device)
{
case PCI_DEVICE_ID_AMD_VIPER_740B:
r->name = "AMD756";
break;
case PCI_DEVICE_ID_AMD_VIPER_7413:
r->name = "AMD766";
break;
case PCI_DEVICE_ID_AMD_VIPER_7443:
r->name = "AMD768";
break;
default:
return 0;
}
r->get = pirq_amd756_get;
r->set = pirq_amd756_set;
return 1;
}
static __initdata struct irq_router_handler pirq_routers[] = {
{ PCI_VENDOR_ID_INTEL, intel_router_probe },
{ PCI_VENDOR_ID_AL, ali_router_probe },
{ PCI_VENDOR_ID_ITE, ite_router_probe },
{ PCI_VENDOR_ID_VIA, via_router_probe },
{ PCI_VENDOR_ID_OPTI, opti_router_probe },
{ PCI_VENDOR_ID_SI, sis_router_probe },
{ PCI_VENDOR_ID_CYRIX, cyrix_router_probe },
{ PCI_VENDOR_ID_VLSI, vlsi_router_probe },
{ PCI_VENDOR_ID_SERVERWORKS, serverworks_router_probe },
{ PCI_VENDOR_ID_AMD, amd_router_probe },
/* Someone with docs needs to add the ATI Radeon IGP */
{ 0, NULL }
};
static struct irq_router pirq_router;
static struct pci_dev *pirq_router_dev;
static void __init pirq_find_router(void)
/*
* FIXME: should we have an option to say "generic for
* chipset" ?
*/
static void __init pirq_find_router(struct irq_router *r)
{
struct irq_routing_table *rt = pirq_table;
struct irq_router *r;
struct irq_router_handler *h;
#ifdef CONFIG_PCI_BIOS
if (!rt->signature) {
printk(KERN_INFO "PCI: Using BIOS for IRQ routing\n");
pirq_router = &pirq_bios_router;
r->set = pirq_bios_set;
r->name = "BIOS";
return;
}
#endif
/* Default unless a driver reloads it */
r->name = "default";
r->get = NULL;
r->set = NULL;
DBG("PCI: Attempting to find IRQ router for %04x:%04x\n",
rt->rtr_vendor, rt->rtr_device);
/* fall back to default router if nothing else found */
pirq_router = &pirq_routers[ARRAY_SIZE(pirq_routers) - 1];
pirq_router_dev = pci_find_slot(rt->rtr_bus, rt->rtr_devfn);
if (!pirq_router_dev) {
DBG("PCI: Interrupt router not found at %02x:%02x\n", rt->rtr_bus, rt->rtr_devfn);
return;
}
for(r=pirq_routers; r->vendor; r++) {
/* Exact match against router table entry? Use it! */
if (r->vendor == rt->rtr_vendor && r->device == rt->rtr_device) {
pirq_router = r;
for( h = pirq_routers; h->vendor; h++) {
/* First look for a router match */
if (rt->rtr_vendor == h->vendor && h->probe(r, pirq_router_dev, rt->rtr_device))
break;
/* Fall back to a device match */
if (pirq_router_dev->vendor == h->vendor && h->probe(r, pirq_router_dev, pirq_router_dev->device))
break;
}
/* Match against router device entry? Use it as a fallback */
if (r->vendor == pirq_router_dev->vendor && r->device == pirq_router_dev->device) {
pirq_router = r;
}
}
printk(KERN_INFO "PCI: Using IRQ router %s [%04x/%04x] at %s\n",
pirq_router->name,
pirq_router.name,
pirq_router_dev->vendor,
pirq_router_dev->device,
pci_name(pirq_router_dev));
......@@ -574,7 +844,7 @@ static int pcibios_lookup_irq(struct pci_dev *dev, int assign)
int i, pirq, newirq;
int irq = 0;
u32 mask;
struct irq_router *r = pirq_router;
struct irq_router *r = &pirq_router;
struct pci_dev *dev2 = NULL;
char *msg = NULL;
......@@ -775,7 +1045,7 @@ static int __init pcibios_irq_init(void)
#endif
if (pirq_table) {
pirq_peer_trick();
pirq_find_router();
pirq_find_router(&pirq_router);
if (pirq_table->exclusive_irqs) {
int i;
for (i=0; i<16; i++)
......
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