Commit 54ef3fe6 authored by Jason Cooper's avatar Jason Cooper

Merge branch 'mvebu/soc-cpuidle' into mvebu/soc

Conflicts:
	arch/arm/mach-mvebu/pmsu.c
parents b03e119f e53b1fd4
......@@ -6,5 +6,15 @@ following property:
Required root node property:
- compatible: must contain either "marvell,armada380" or
"marvell,armada385" depending on the variant of the SoC being used.
- compatible: must contain "marvell,armada380"
In addition, boards using the Marvell Armada 385 SoC shall have the
following property before the previous one:
Required root node property:
compatible: must contain "marvell,armada385"
Example:
compatible = "marvell,a385-rd", "marvell,armada385", "marvell,armada380";
......@@ -16,7 +16,7 @@
/ {
model = "Marvell Armada 380 family SoC";
compatible = "marvell,armada380", "marvell,armada38x";
compatible = "marvell,armada380";
cpus {
#address-cells = <1>;
......
......@@ -16,7 +16,7 @@
/ {
model = "Marvell Armada 385 Development Board";
compatible = "marvell,a385-db", "marvell,armada385", "marvell,armada38x";
compatible = "marvell,a385-db", "marvell,armada385", "marvell,armada380";
chosen {
bootargs = "console=ttyS0,115200 earlyprintk";
......
......@@ -17,7 +17,7 @@
/ {
model = "Marvell Armada 385 Reference Design";
compatible = "marvell,a385-rd", "marvell,armada385", "marvell,armada38x";
compatible = "marvell,a385-rd", "marvell,armada385", "marvell,armada380";
chosen {
bootargs = "console=ttyS0,115200 earlyprintk";
......
......@@ -16,7 +16,7 @@
/ {
model = "Marvell Armada 385 family SoC";
compatible = "marvell,armada385", "marvell,armada38x";
compatible = "marvell,armada385", "marvell,armada380";
cpus {
#address-cells = <1>;
......
......@@ -20,7 +20,7 @@
/ {
model = "Marvell Armada 38x family SoC";
compatible = "marvell,armada38x";
compatible = "marvell,armada380";
aliases {
gpio0 = &gpio0;
......
......@@ -105,7 +105,6 @@ ethphy0: ethernet-phy@0 {
compatible = "ethernet-phy-id0141.0cb0",
"ethernet-phy-ieee802.3-c22";
reg = <0>;
phy-connection-type = "rgmii-id";
};
ethphy1: ethernet-phy@1 {
......@@ -113,7 +112,6 @@ ethphy1: ethernet-phy@1 {
compatible = "ethernet-phy-id0141.0cb0",
"ethernet-phy-ieee802.3-c22";
reg = <1>;
phy-connection-type = "rgmii-id";
};
};
......@@ -121,6 +119,7 @@ &eth0 {
status = "okay";
ethernet0-port@0 {
phy-handle = <&ethphy0>;
phy-connection-type = "rgmii-id";
};
};
......@@ -128,5 +127,6 @@ &eth1 {
status = "okay";
ethernet1-port@0 {
phy-handle = <&ethphy1>;
phy-connection-type = "rgmii-id";
};
};
......@@ -10,6 +10,7 @@ config ARCH_MVEBU
select ZONE_DMA if ARM_LPAE
select ARCH_REQUIRE_GPIOLIB
select PCI_QUIRKS if PCI
select OF_ADDRESS_PCI
if ARCH_MVEBU
......@@ -19,6 +20,7 @@ config MACH_MVEBU_V7
bool
select ARMADA_370_XP_TIMER
select CACHE_L2X0
select ARM_CPU_SUSPEND
config MACH_ARMADA_370
bool "Marvell Armada 370 boards" if ARCH_MULTI_V7
......
......@@ -7,7 +7,7 @@ CFLAGS_pmsu.o := -march=armv7-a
obj-y += system-controller.o mvebu-soc-id.o
ifeq ($(CONFIG_MACH_MVEBU_V7),y)
obj-y += cpu-reset.o board-v7.o coherency.o coherency_ll.o pmsu.o
obj-y += cpu-reset.o board-v7.o coherency.o coherency_ll.o pmsu.o pmsu_ll.o
obj-$(CONFIG_SMP) += platsmp.o headsmp.o platsmp-a9.o headsmp-a9.o
endif
......
......@@ -25,6 +25,5 @@ extern struct smp_operations armada_xp_smp_ops;
#endif
int armada_370_xp_pmsu_idle_enter(unsigned long deepidle);
void armada_370_xp_pmsu_idle_exit(void);
#endif /* __MACH_ARMADA_370_XP_H */
......@@ -23,6 +23,7 @@
#include <linux/mbus.h>
#include <linux/signal.h>
#include <linux/slab.h>
#include <linux/irqchip.h>
#include <asm/hardware/cache-l2x0.h>
#include <asm/mach/arch.h>
#include <asm/mach/map.h>
......@@ -33,14 +34,14 @@
#include "coherency.h"
#include "mvebu-soc-id.h"
static void __iomem *scu_base;
/*
* Enables the SCU when available. Obviously, this is only useful on
* Cortex-A based SOCs, not on PJ4B based ones.
*/
static void __init mvebu_scu_enable(void)
{
void __iomem *scu_base;
struct device_node *np =
of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
if (np) {
......@@ -50,6 +51,11 @@ static void __init mvebu_scu_enable(void)
}
}
void __iomem *mvebu_get_scu_base(void)
{
return scu_base;
}
/*
* Early versions of Armada 375 SoC have a bug where the BootROM
* leaves an external data abort pending. The kernel is hit by this
......@@ -71,17 +77,23 @@ static int armada_375_external_abort_wa(unsigned long addr, unsigned int fsr,
return 1;
}
static void __init mvebu_timer_and_clk_init(void)
static void __init mvebu_init_irq(void)
{
of_clk_init(NULL);
clocksource_of_init();
irqchip_init();
mvebu_scu_enable();
coherency_init();
BUG_ON(mvebu_mbus_dt_init(coherency_available()));
}
if (of_machine_is_compatible("marvell,armada375"))
hook_fault_code(16 + 6, armada_375_external_abort_wa, SIGBUS, 0,
"imprecise external abort");
static void __init external_abort_quirk(void)
{
u32 dev, rev;
if (mvebu_get_soc_id(&dev, &rev) == 0 && rev > ARMADA_375_Z1_REV)
return;
hook_fault_code(16 + 6, armada_375_external_abort_wa, SIGBUS, 0,
"imprecise external abort");
}
static void __init i2c_quirk(void)
......@@ -178,8 +190,10 @@ static void __init mvebu_dt_init(void)
{
if (of_machine_is_compatible("plathome,openblocks-ax3-4"))
i2c_quirk();
if (of_machine_is_compatible("marvell,a375-db"))
if (of_machine_is_compatible("marvell,a375-db")) {
external_abort_quirk();
thermal_quirk();
}
of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
}
......@@ -194,7 +208,7 @@ DT_MACHINE_START(ARMADA_370_XP_DT, "Marvell Armada 370/XP (Device Tree)")
.l2c_aux_mask = ~0,
.smp = smp_ops(armada_xp_smp_ops),
.init_machine = mvebu_dt_init,
.init_time = mvebu_timer_and_clk_init,
.init_irq = mvebu_init_irq,
.restart = mvebu_restart,
.dt_compat = armada_370_xp_dt_compat,
MACHINE_END
......@@ -207,7 +221,7 @@ static const char * const armada_375_dt_compat[] = {
DT_MACHINE_START(ARMADA_375_DT, "Marvell Armada 375 (Device Tree)")
.l2c_aux_val = 0,
.l2c_aux_mask = ~0,
.init_time = mvebu_timer_and_clk_init,
.init_irq = mvebu_init_irq,
.init_machine = mvebu_dt_init,
.restart = mvebu_restart,
.dt_compat = armada_375_dt_compat,
......@@ -222,7 +236,7 @@ static const char * const armada_38x_dt_compat[] = {
DT_MACHINE_START(ARMADA_38X_DT, "Marvell Armada 380/385 (Device Tree)")
.l2c_aux_val = 0,
.l2c_aux_mask = ~0,
.init_time = mvebu_timer_and_clk_init,
.init_irq = mvebu_init_irq,
.restart = mvebu_restart,
.dt_compat = armada_38x_dt_compat,
MACHINE_END
......@@ -292,6 +292,10 @@ static struct notifier_block mvebu_hwcc_nb = {
.notifier_call = mvebu_hwcc_notifier,
};
static struct notifier_block mvebu_hwcc_pci_nb = {
.notifier_call = mvebu_hwcc_notifier,
};
static void __init armada_370_coherency_init(struct device_node *np)
{
struct resource res;
......@@ -427,7 +431,7 @@ static int __init coherency_pci_init(void)
{
if (coherency_available())
bus_register_notifier(&pci_bus_type,
&mvebu_hwcc_nb);
&mvebu_hwcc_pci_nb);
return 0;
}
......
......@@ -23,4 +23,6 @@ void mvebu_pmsu_set_cpu_boot_addr(int hw_cpu, void *boot_addr);
void mvebu_system_controller_set_cpu_boot_addr(void *boot_addr);
int mvebu_system_controller_get_soc_id(u32 *dev, u32 *rev);
void __iomem *mvebu_get_scu_base(void);
#endif
......@@ -15,20 +15,12 @@
#include <linux/linkage.h>
#include <linux/init.h>
__CPUINIT
#define CPU_RESUME_ADDR_REG 0xf10182d4
.global armada_375_smp_cpu1_enable_code_start
.global armada_375_smp_cpu1_enable_code_end
#include <asm/assembler.h>
armada_375_smp_cpu1_enable_code_start:
ldr r0, [pc, #4]
ldr r1, [r0]
mov pc, r1
.word CPU_RESUME_ADDR_REG
armada_375_smp_cpu1_enable_code_end:
__CPUINIT
ENTRY(mvebu_cortex_a9_secondary_startup)
ARM_BE8(setend be)
bl v7_invalidate_l1
b secondary_startup
ENDPROC(mvebu_cortex_a9_secondary_startup)
......@@ -20,33 +20,8 @@
#include <asm/smp_scu.h>
#include <asm/smp_plat.h>
#include "common.h"
#include "mvebu-soc-id.h"
#include "pmsu.h"
#define CRYPT0_ENG_ID 41
#define CRYPT0_ENG_ATTR 0x1
#define SRAM_PHYS_BASE 0xFFFF0000
#define BOOTROM_BASE 0xFFF00000
#define BOOTROM_SIZE 0x100000
extern unsigned char armada_375_smp_cpu1_enable_code_end;
extern unsigned char armada_375_smp_cpu1_enable_code_start;
static void armada_375_smp_cpu1_enable_wa(void)
{
void __iomem *sram_virt_base;
mvebu_mbus_del_window(BOOTROM_BASE, BOOTROM_SIZE);
mvebu_mbus_add_window_by_id(CRYPT0_ENG_ID, CRYPT0_ENG_ATTR,
SRAM_PHYS_BASE, SZ_64K);
sram_virt_base = ioremap(SRAM_PHYS_BASE, SZ_64K);
memcpy(sram_virt_base, &armada_375_smp_cpu1_enable_code_start,
&armada_375_smp_cpu1_enable_code_end
- &armada_375_smp_cpu1_enable_code_start);
}
extern void mvebu_cortex_a9_secondary_startup(void);
static int __cpuinit mvebu_cortex_a9_boot_secondary(unsigned int cpu,
......@@ -63,21 +38,10 @@ static int __cpuinit mvebu_cortex_a9_boot_secondary(unsigned int cpu,
* address.
*/
hw_cpu = cpu_logical_map(cpu);
if (of_machine_is_compatible("marvell,armada375")) {
u32 dev, rev;
if (mvebu_get_soc_id(&dev, &rev) == 0 &&
rev == ARMADA_375_Z1_REV)
armada_375_smp_cpu1_enable_wa();
if (of_machine_is_compatible("marvell,armada375"))
mvebu_system_controller_set_cpu_boot_addr(mvebu_cortex_a9_secondary_startup);
}
else {
mvebu_pmsu_set_cpu_boot_addr(hw_cpu,
mvebu_cortex_a9_secondary_startup);
}
else
mvebu_pmsu_set_cpu_boot_addr(hw_cpu, mvebu_cortex_a9_secondary_startup);
smp_wmb();
ret = mvebu_cpu_reset_deassert(hw_cpu);
if (ret) {
......
......@@ -109,7 +109,7 @@ static int armada_xp_boot_secondary(unsigned int cpu, struct task_struct *idle)
*/
static void armada_xp_secondary_init(unsigned int cpu)
{
armada_370_xp_pmsu_idle_exit();
mvebu_v7_pmsu_idle_exit();
}
static void __init armada_xp_smp_init_cpus(void)
......
This diff is collapsed.
......@@ -12,5 +12,10 @@
#define __MACH_MVEBU_PMSU_H
int armada_xp_boot_cpu(unsigned int cpu_id, void *phys_addr);
int mvebu_setup_boot_addr_wa(unsigned int crypto_eng_target,
unsigned int crypto_eng_attribute,
phys_addr_t resume_addr_reg);
void mvebu_v7_pmsu_idle_exit(void);
#endif /* __MACH_370_XP_PMSU_H */
/*
* Copyright (C) 2014 Marvell
*
* Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
* Gregory Clement <gregory.clement@free-electrons.com>
*
* This file is licensed under the terms of the GNU General Public
* License version 2. This program is licensed "as is" without any
* warranty of any kind, whether express or implied.
*/
#include <linux/linkage.h>
#include <asm/assembler.h>
/*
* This is the entry point through which CPUs exiting cpuidle deep
* idle state are going.
*/
ENTRY(armada_370_xp_cpu_resume)
ARM_BE8(setend be ) @ go BE8 if entered LE
bl ll_add_cpu_to_smp_group
bl ll_enable_coherency
b cpu_resume
ENDPROC(armada_370_xp_cpu_resume)
ENTRY(armada_38x_cpu_resume)
/* do we need it for Armada 38x*/
ARM_BE8(setend be ) @ go BE8 if entered LE
bl v7_invalidate_l1
mrc p15, 4, r1, c15, c0 @ get SCU base address
orr r1, r1, #0x8 @ SCU CPU Power Status Register
mrc 15, 0, r0, cr0, cr0, 5 @ get the CPU ID
and r0, r0, #15
add r1, r1, r0
mov r0, #0x0
strb r0, [r1] @ switch SCU power state to Normal mode
b cpu_resume
ENDPROC(armada_38x_cpu_resume)
.global mvebu_boot_wa_start
.global mvebu_boot_wa_end
/* The following code will be executed from SRAM */
ENTRY(mvebu_boot_wa_start)
mvebu_boot_wa_start:
ARM_BE8(setend be)
adr r0, 1f
ldr r0, [r0] @ load the address of the
@ resume register
ldr r0, [r0] @ load the value in the
@ resume register
ARM_BE8(rev r0, r0) @ the value is stored LE
mov pc, r0 @ jump to this value
/*
* the last word of this piece of code will be filled by the physical
* address of the boot address register just after being copied in SRAM
*/
1:
.long .
mvebu_boot_wa_end:
ENDPROC(mvebu_boot_wa_end)
......@@ -28,8 +28,14 @@
#include <linux/io.h>
#include <linux/reboot.h>
#include "common.h"
#include "mvebu-soc-id.h"
#include "pmsu.h"
#define ARMADA_375_CRYPT0_ENG_TARGET 41
#define ARMADA_375_CRYPT0_ENG_ATTR 1
static void __iomem *system_controller_base;
static phys_addr_t system_controller_phys_base;
struct mvebu_system_controller {
u32 rstoutn_mask_offset;
......@@ -121,10 +127,32 @@ int mvebu_system_controller_get_soc_id(u32 *dev, u32 *rev)
}
#ifdef CONFIG_SMP
void mvebu_armada375_smp_wa_init(void)
{
u32 dev, rev;
phys_addr_t resume_addr_reg;
if (mvebu_get_soc_id(&dev, &rev) != 0)
return;
if (rev != ARMADA_375_Z1_REV)
return;
resume_addr_reg = system_controller_phys_base +
mvebu_sc->resume_boot_addr;
mvebu_setup_boot_addr_wa(ARMADA_375_CRYPT0_ENG_TARGET,
ARMADA_375_CRYPT0_ENG_ATTR,
resume_addr_reg);
}
void mvebu_system_controller_set_cpu_boot_addr(void *boot_addr)
{
BUG_ON(system_controller_base == NULL);
BUG_ON(mvebu_sc->resume_boot_addr == 0);
if (of_machine_is_compatible("marvell,armada375"))
mvebu_armada375_smp_wa_init();
writel(virt_to_phys(boot_addr), system_controller_base +
mvebu_sc->resume_boot_addr);
}
......@@ -138,7 +166,10 @@ static int __init mvebu_system_controller_init(void)
np = of_find_matching_node_and_match(NULL, of_system_controller_table,
&match);
if (np) {
struct resource res;
system_controller_base = of_iomap(np, 0);
of_address_to_resource(np, 0, &res);
system_controller_phys_base = res.start;
mvebu_sc = (struct mvebu_system_controller *)match->data;
of_node_put(np);
}
......
#
# ARM CPU Idle drivers
#
config ARM_ARMADA_370_XP_CPUIDLE
bool "CPU Idle Driver for Armada 370/XP family processors"
depends on ARCH_MVEBU
help
Select this to enable cpuidle on Armada 370/XP processors.
config ARM_BIG_LITTLE_CPUIDLE
bool "Support for ARM big.LITTLE processors"
depends on ARCH_VEXPRESS_TC2_PM
......@@ -61,3 +55,9 @@ config ARM_EXYNOS_CPUIDLE
depends on ARCH_EXYNOS
help
Select this to enable cpuidle for Exynos processors
config ARM_MVEBU_V7_CPUIDLE
bool "CPU Idle Driver for mvebu v7 family processors"
depends on ARCH_MVEBU
help
Select this to enable cpuidle on Armada 370, 38x and XP processors.
......@@ -7,7 +7,7 @@ obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
##################################################################################
# ARM SoC drivers
obj-$(CONFIG_ARM_ARMADA_370_XP_CPUIDLE) += cpuidle-armada-370-xp.o
obj-$(CONFIG_ARM_MVEBU_V7_CPUIDLE) += cpuidle-mvebu-v7.o
obj-$(CONFIG_ARM_BIG_LITTLE_CPUIDLE) += cpuidle-big_little.o
obj-$(CONFIG_ARM_CLPS711X_CPUIDLE) += cpuidle-clps711x.o
obj-$(CONFIG_ARM_HIGHBANK_CPUIDLE) += cpuidle-calxeda.o
......
/*
* Marvell Armada 370 and Armada XP SoC cpuidle driver
* Marvell Armada 370, 38x and XP SoC cpuidle driver
*
* Copyright (C) 2014 Marvell
*
......@@ -21,12 +21,11 @@
#include <linux/platform_device.h>
#include <asm/cpuidle.h>
#define ARMADA_370_XP_MAX_STATES 3
#define ARMADA_370_XP_FLAG_DEEP_IDLE 0x10000
#define MVEBU_V7_FLAG_DEEP_IDLE 0x10000
static int (*armada_370_xp_cpu_suspend)(int);
static int (*mvebu_v7_cpu_suspend)(int);
static int armada_370_xp_enter_idle(struct cpuidle_device *dev,
static int mvebu_v7_enter_idle(struct cpuidle_device *dev,
struct cpuidle_driver *drv,
int index)
{
......@@ -34,10 +33,10 @@ static int armada_370_xp_enter_idle(struct cpuidle_device *dev,
bool deepidle = false;
cpu_pm_enter();
if (drv->states[index].flags & ARMADA_370_XP_FLAG_DEEP_IDLE)
if (drv->states[index].flags & MVEBU_V7_FLAG_DEEP_IDLE)
deepidle = true;
ret = armada_370_xp_cpu_suspend(deepidle);
ret = mvebu_v7_cpu_suspend(deepidle);
if (ret)
return ret;
......@@ -46,11 +45,11 @@ static int armada_370_xp_enter_idle(struct cpuidle_device *dev,
return index;
}
static struct cpuidle_driver armada_370_xp_idle_driver = {
.name = "armada_370_xp_idle",
static struct cpuidle_driver armadaxp_idle_driver = {
.name = "armada_xp_idle",
.states[0] = ARM_CPUIDLE_WFI_STATE,
.states[1] = {
.enter = armada_370_xp_enter_idle,
.enter = mvebu_v7_enter_idle,
.exit_latency = 10,
.power_usage = 50,
.target_residency = 100,
......@@ -59,35 +58,93 @@ static struct cpuidle_driver armada_370_xp_idle_driver = {
.desc = "CPU power down",
},
.states[2] = {
.enter = armada_370_xp_enter_idle,
.enter = mvebu_v7_enter_idle,
.exit_latency = 100,
.power_usage = 5,
.target_residency = 1000,
.flags = CPUIDLE_FLAG_TIME_VALID |
ARMADA_370_XP_FLAG_DEEP_IDLE,
MVEBU_V7_FLAG_DEEP_IDLE,
.name = "MV CPU DEEP IDLE",
.desc = "CPU and L2 Fabric power down",
},
.state_count = ARMADA_370_XP_MAX_STATES,
.state_count = 3,
};
static int armada_370_xp_cpuidle_probe(struct platform_device *pdev)
static struct cpuidle_driver armada370_idle_driver = {
.name = "armada_370_idle",
.states[0] = ARM_CPUIDLE_WFI_STATE,
.states[1] = {
.enter = mvebu_v7_enter_idle,
.exit_latency = 100,
.power_usage = 5,
.target_residency = 1000,
.flags = (CPUIDLE_FLAG_TIME_VALID |
MVEBU_V7_FLAG_DEEP_IDLE),
.name = "Deep Idle",
.desc = "CPU and L2 Fabric power down",
},
.state_count = 2,
};
static struct cpuidle_driver armada38x_idle_driver = {
.name = "armada_38x_idle",
.states[0] = ARM_CPUIDLE_WFI_STATE,
.states[1] = {
.enter = mvebu_v7_enter_idle,
.exit_latency = 10,
.power_usage = 5,
.target_residency = 100,
.flags = CPUIDLE_FLAG_TIME_VALID,
.name = "Idle",
.desc = "CPU and SCU power down",
},
.state_count = 2,
};
static int mvebu_v7_cpuidle_probe(struct platform_device *pdev)
{
mvebu_v7_cpu_suspend = pdev->dev.platform_data;
armada_370_xp_cpu_suspend = (void *)(pdev->dev.platform_data);
return cpuidle_register(&armada_370_xp_idle_driver, NULL);
if (!strcmp(pdev->dev.driver->name, "cpuidle-armada-xp"))
return cpuidle_register(&armadaxp_idle_driver, NULL);
else if (!strcmp(pdev->dev.driver->name, "cpuidle-armada-370"))
return cpuidle_register(&armada370_idle_driver, NULL);
else if (!strcmp(pdev->dev.driver->name, "cpuidle-armada-38x"))
return cpuidle_register(&armada38x_idle_driver, NULL);
else
return -EINVAL;
}
static struct platform_driver armada_370_xp_cpuidle_plat_driver = {
static struct platform_driver armadaxp_cpuidle_plat_driver = {
.driver = {
.name = "cpuidle-armada-xp",
.owner = THIS_MODULE,
},
.probe = mvebu_v7_cpuidle_probe,
};
module_platform_driver(armadaxp_cpuidle_plat_driver);
static struct platform_driver armada370_cpuidle_plat_driver = {
.driver = {
.name = "cpuidle-armada-370",
.owner = THIS_MODULE,
},
.probe = mvebu_v7_cpuidle_probe,
};
module_platform_driver(armada370_cpuidle_plat_driver);
static struct platform_driver armada38x_cpuidle_plat_driver = {
.driver = {
.name = "cpuidle-armada-370-xp",
.name = "cpuidle-armada-38x",
.owner = THIS_MODULE,
},
.probe = armada_370_xp_cpuidle_probe,
.probe = mvebu_v7_cpuidle_probe,
};
module_platform_driver(armada_370_xp_cpuidle_plat_driver);
module_platform_driver(armada38x_cpuidle_plat_driver);
MODULE_AUTHOR("Gregory CLEMENT <gregory.clement@free-electrons.com>");
MODULE_DESCRIPTION("Armada 370/XP cpu idle driver");
MODULE_DESCRIPTION("Marvell EBU v7 cpuidle driver");
MODULE_LICENSE("GPL");
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