Commit d1b1110f authored by David S. Miller's avatar David S. Miller

Merge branch 'mvpp2-Armada-7k-8k-PP2-ACPI-support'

Marcin Wojtas says:

====================
Armada 7k/8k PP2 ACPI support

I quickly resend the series, thanks to Antoine Tenart's remark,
who spotted !CONFIG_ACPI compilation issue after introducing
the new fwnode_irq_get() routine. Please see the details in the changelog
below and the 3/7 commit log.

mvpp2 driver can work with the ACPI representation, as exposed
on a public branch:
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/marvell-armada-wip
It was compiled together with the most recent Tianocore EDK2 revision.
Please refer to the firmware build instruction on MacchiatoBin board:
http://wiki.macchiatobin.net/tiki-index.php?page=Build+from+source+-+UEFI+EDK+II

ACPI representation of PP2 controllers (withouth PHY support) can
be viewed in the github:
* MacchiatoBin:
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/71ae395da1661374b0f07d1602afb1eee56e9794/Platforms/Marvell/Armada/AcpiTables/Armada80x0McBin/Dsdt.asl#L201

* Armada 7040 DB:
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/71ae395da1661374b0f07d1602afb1eee56e9794/Platforms/Marvell/Armada/AcpiTables/Armada70x0/Dsdt.asl#L131

I will appreciate any comments or remarks.

Best regards,
Marcin

Changelog:
v3 -> v4:
* 3/7
    - add new macro (ACPI_HANDLE_FWNODE) and fix
      compilation with !CONFIG_ACPI
    - extend commit log and mention usability of fwnode_irq_get
      for the child nodes as well

v2 -> v3:
* 1/7, 2/7
    - Add Rafael's Acked-by's
* 3/7, 4/7
    - New patches
* 6/7, 7/7
    - Update driver with new helper routines usage
    - Improve commit log.

v1 -> v2:
* Remove MDIO patches
* Use PP2 ports only with link interrupts
* Release second region resources in mvpp2 driver (code moved from
  mvmdio), as explained in details in 5/5 commit message.
====================
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parents e0b4ed01 a75edc7c
......@@ -16,6 +16,7 @@
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/of_graph.h>
#include <linux/of_irq.h>
#include <linux/property.h>
#include <linux/etherdevice.h>
#include <linux/phy.h>
......@@ -996,6 +997,32 @@ fwnode_get_next_child_node(const struct fwnode_handle *fwnode,
}
EXPORT_SYMBOL_GPL(fwnode_get_next_child_node);
/**
* fwnode_get_next_available_child_node - Return the next
* available child node handle for a node
* @fwnode: Firmware node to find the next child node for.
* @child: Handle to one of the node's child nodes or a %NULL handle.
*/
struct fwnode_handle *
fwnode_get_next_available_child_node(const struct fwnode_handle *fwnode,
struct fwnode_handle *child)
{
struct fwnode_handle *next_child = child;
if (!fwnode)
return NULL;
do {
next_child = fwnode_get_next_child_node(fwnode, next_child);
if (!next_child || fwnode_device_is_available(next_child))
break;
} while (next_child);
return next_child;
}
EXPORT_SYMBOL_GPL(fwnode_get_next_available_child_node);
/**
* device_get_next_child_node - Return the next child node handle for a device
* @dev: Device to find the next child node for.
......@@ -1126,21 +1153,21 @@ enum dev_dma_attr device_get_dma_attr(struct device *dev)
EXPORT_SYMBOL_GPL(device_get_dma_attr);
/**
* device_get_phy_mode - Get phy mode for given device
* @dev: Pointer to the given device
* fwnode_get_phy_mode - Get phy mode for given firmware node
* @fwnode: Pointer to the given node
*
* The function gets phy interface string from property 'phy-mode' or
* 'phy-connection-type', and return its index in phy_modes table, or errno in
* error case.
*/
int device_get_phy_mode(struct device *dev)
int fwnode_get_phy_mode(struct fwnode_handle *fwnode)
{
const char *pm;
int err, i;
err = device_property_read_string(dev, "phy-mode", &pm);
err = fwnode_property_read_string(fwnode, "phy-mode", &pm);
if (err < 0)
err = device_property_read_string(dev,
err = fwnode_property_read_string(fwnode,
"phy-connection-type", &pm);
if (err < 0)
return err;
......@@ -1151,13 +1178,27 @@ int device_get_phy_mode(struct device *dev)
return -ENODEV;
}
EXPORT_SYMBOL_GPL(fwnode_get_phy_mode);
/**
* device_get_phy_mode - Get phy mode for given device
* @dev: Pointer to the given device
*
* The function gets phy interface string from property 'phy-mode' or
* 'phy-connection-type', and return its index in phy_modes table, or errno in
* error case.
*/
int device_get_phy_mode(struct device *dev)
{
return fwnode_get_phy_mode(dev_fwnode(dev));
}
EXPORT_SYMBOL_GPL(device_get_phy_mode);
static void *device_get_mac_addr(struct device *dev,
static void *fwnode_get_mac_addr(struct fwnode_handle *fwnode,
const char *name, char *addr,
int alen)
{
int ret = device_property_read_u8_array(dev, name, addr, alen);
int ret = fwnode_property_read_u8_array(fwnode, name, addr, alen);
if (ret == 0 && alen == ETH_ALEN && is_valid_ether_addr(addr))
return addr;
......@@ -1165,8 +1206,8 @@ static void *device_get_mac_addr(struct device *dev,
}
/**
* device_get_mac_address - Get the MAC for a given device
* @dev: Pointer to the device
* fwnode_get_mac_address - Get the MAC from the firmware node
* @fwnode: Pointer to the firmware node
* @addr: Address of buffer to store the MAC in
* @alen: Length of the buffer pointed to by addr, should be ETH_ALEN
*
......@@ -1187,22 +1228,59 @@ static void *device_get_mac_addr(struct device *dev,
* In this case, the real MAC is in 'local-mac-address', and 'mac-address'
* exists but is all zeros.
*/
void *device_get_mac_address(struct device *dev, char *addr, int alen)
void *fwnode_get_mac_address(struct fwnode_handle *fwnode, char *addr, int alen)
{
char *res;
res = device_get_mac_addr(dev, "mac-address", addr, alen);
res = fwnode_get_mac_addr(fwnode, "mac-address", addr, alen);
if (res)
return res;
res = device_get_mac_addr(dev, "local-mac-address", addr, alen);
res = fwnode_get_mac_addr(fwnode, "local-mac-address", addr, alen);
if (res)
return res;
return device_get_mac_addr(dev, "address", addr, alen);
return fwnode_get_mac_addr(fwnode, "address", addr, alen);
}
EXPORT_SYMBOL(fwnode_get_mac_address);
/**
* device_get_mac_address - Get the MAC for a given device
* @dev: Pointer to the device
* @addr: Address of buffer to store the MAC in
* @alen: Length of the buffer pointed to by addr, should be ETH_ALEN
*/
void *device_get_mac_address(struct device *dev, char *addr, int alen)
{
return fwnode_get_mac_address(dev_fwnode(dev), addr, alen);
}
EXPORT_SYMBOL(device_get_mac_address);
/**
* fwnode_irq_get - Get IRQ directly from a fwnode
* @fwnode: Pointer to the firmware node
* @index: Zero-based index of the IRQ
*
* Returns Linux IRQ number on success. Other values are determined
* accordingly to acpi_/of_ irq_get() operation.
*/
int fwnode_irq_get(struct fwnode_handle *fwnode, unsigned int index)
{
struct device_node *of_node = to_of_node(fwnode);
struct resource res;
int ret;
if (IS_ENABLED(CONFIG_OF) && of_node)
return of_irq_get(of_node, index);
ret = acpi_irq_get(ACPI_HANDLE_FWNODE(fwnode), index, &res);
if (ret)
return ret;
return res.start;
}
EXPORT_SYMBOL(fwnode_irq_get);
/**
* device_graph_get_next_endpoint - Get next endpoint firmware node
* @fwnode: Pointer to the parent firmware node
......
......@@ -10,6 +10,7 @@
* warranty of any kind, whether express or implied.
*/
#include <linux/acpi.h>
#include <linux/kernel.h>
#include <linux/netdevice.h>
#include <linux/etherdevice.h>
......@@ -865,7 +866,7 @@ struct mvpp2 {
/* List of pointers to port structures */
int port_count;
struct mvpp2_port **port_list;
struct mvpp2_port *port_list[MVPP2_MAX_PORTS];
/* Aggregated TXQs */
struct mvpp2_tx_queue *aggr_txqs;
......@@ -932,6 +933,9 @@ struct mvpp2_port {
struct mvpp2 *priv;
/* Firmware node associated to the port */
struct fwnode_handle *fwnode;
/* Per-port registers' base address */
void __iomem *base;
void __iomem *stats_base;
......@@ -7499,7 +7503,10 @@ static int mvpp2_multi_queue_vectors_init(struct mvpp2_port *port,
strncpy(irqname, "rx-shared", sizeof(irqname));
}
if (port_node)
v->irq = of_irq_get_byname(port_node, irqname);
else
v->irq = fwnode_irq_get(port->fwnode, i);
if (v->irq <= 0) {
ret = -EINVAL;
goto err;
......@@ -7711,17 +7718,16 @@ static bool mvpp2_port_has_tx_irqs(struct mvpp2 *priv,
}
static void mvpp2_port_copy_mac_addr(struct net_device *dev, struct mvpp2 *priv,
struct device_node *port_node,
struct fwnode_handle *fwnode,
char **mac_from)
{
struct mvpp2_port *port = netdev_priv(dev);
char hw_mac_addr[ETH_ALEN] = {0};
const char *dt_mac_addr;
char fw_mac_addr[ETH_ALEN];
dt_mac_addr = of_get_mac_address(port_node);
if (dt_mac_addr && is_valid_ether_addr(dt_mac_addr)) {
*mac_from = "device tree";
ether_addr_copy(dev->dev_addr, dt_mac_addr);
if (fwnode_get_mac_address(fwnode, fw_mac_addr, ETH_ALEN)) {
*mac_from = "firmware node";
ether_addr_copy(dev->dev_addr, fw_mac_addr);
return;
}
......@@ -7740,13 +7746,14 @@ static void mvpp2_port_copy_mac_addr(struct net_device *dev, struct mvpp2 *priv,
/* Ports initialization */
static int mvpp2_port_probe(struct platform_device *pdev,
struct device_node *port_node,
struct mvpp2 *priv, int index)
struct fwnode_handle *port_fwnode,
struct mvpp2 *priv)
{
struct device_node *phy_node;
struct phy *comphy;
struct phy *comphy = NULL;
struct mvpp2_port *port;
struct mvpp2_port_pcpu *port_pcpu;
struct device_node *port_node = to_of_node(port_fwnode);
struct net_device *dev;
struct resource *res;
char *mac_from = "";
......@@ -7757,7 +7764,12 @@ static int mvpp2_port_probe(struct platform_device *pdev,
int phy_mode;
int err, i, cpu;
if (port_node) {
has_tx_irqs = mvpp2_port_has_tx_irqs(priv, port_node);
} else {
has_tx_irqs = true;
queue_mode = MVPP2_QDIST_MULTI_MODE;
}
if (!has_tx_irqs)
queue_mode = MVPP2_QDIST_SINGLE_MODE;
......@@ -7772,14 +7784,19 @@ static int mvpp2_port_probe(struct platform_device *pdev,
if (!dev)
return -ENOMEM;
if (port_node)
phy_node = of_parse_phandle(port_node, "phy", 0);
phy_mode = of_get_phy_mode(port_node);
else
phy_node = NULL;
phy_mode = fwnode_get_phy_mode(port_fwnode);
if (phy_mode < 0) {
dev_err(&pdev->dev, "incorrect phy mode\n");
err = phy_mode;
goto err_free_netdev;
}
if (port_node) {
comphy = devm_of_phy_get(&pdev->dev, port_node, NULL);
if (IS_ERR(comphy)) {
if (PTR_ERR(comphy) == -EPROBE_DEFER) {
......@@ -7788,8 +7805,9 @@ static int mvpp2_port_probe(struct platform_device *pdev,
}
comphy = NULL;
}
}
if (of_property_read_u32(port_node, "port-id", &id)) {
if (fwnode_property_read_u32(port_fwnode, "port-id", &id)) {
err = -EINVAL;
dev_err(&pdev->dev, "missing port-id value\n");
goto err_free_netdev;
......@@ -7802,6 +7820,7 @@ static int mvpp2_port_probe(struct platform_device *pdev,
port = netdev_priv(dev);
port->dev = dev;
port->fwnode = port_fwnode;
port->ntxqs = ntxqs;
port->nrxqs = nrxqs;
port->priv = priv;
......@@ -7811,7 +7830,10 @@ static int mvpp2_port_probe(struct platform_device *pdev,
if (err)
goto err_free_netdev;
if (port_node)
port->link_irq = of_irq_get_byname(port_node, "link");
else
port->link_irq = fwnode_irq_get(port_fwnode, port->nqvecs + 1);
if (port->link_irq == -EPROBE_DEFER) {
err = -EPROBE_DEFER;
goto err_deinit_qvecs;
......@@ -7820,7 +7842,7 @@ static int mvpp2_port_probe(struct platform_device *pdev,
/* the link irq is optional */
port->link_irq = 0;
if (of_property_read_bool(port_node, "marvell,loopback"))
if (fwnode_property_read_bool(port_fwnode, "marvell,loopback"))
port->flags |= MVPP2_F_LOOPBACK;
port->id = id;
......@@ -7845,7 +7867,7 @@ static int mvpp2_port_probe(struct platform_device *pdev,
MVPP21_MIB_COUNTERS_OFFSET +
port->gop_id * MVPP21_MIB_COUNTERS_PORT_SZ;
} else {
if (of_property_read_u32(port_node, "gop-port-id",
if (fwnode_property_read_u32(port_fwnode, "gop-port-id",
&port->gop_id)) {
err = -EINVAL;
dev_err(&pdev->dev, "missing gop-port-id value\n");
......@@ -7876,7 +7898,7 @@ static int mvpp2_port_probe(struct platform_device *pdev,
mutex_init(&port->gather_stats_lock);
INIT_DELAYED_WORK(&port->stats_work, mvpp2_gather_hw_statistics);
mvpp2_port_copy_mac_addr(dev, priv, port_node, &mac_from);
mvpp2_port_copy_mac_addr(dev, priv, port_fwnode, &mac_from);
port->tx_ring_size = MVPP2_MAX_TXD_DFLT;
port->rx_ring_size = MVPP2_MAX_RXD_DFLT;
......@@ -7934,7 +7956,8 @@ static int mvpp2_port_probe(struct platform_device *pdev,
}
netdev_info(dev, "Using %s mac address %pM\n", mac_from, dev->dev_addr);
priv->port_list[index] = port;
priv->port_list[priv->port_count++] = port;
return 0;
err_free_port_pcpu:
......@@ -8193,8 +8216,9 @@ static int mvpp2_init(struct platform_device *pdev, struct mvpp2 *priv)
static int mvpp2_probe(struct platform_device *pdev)
{
struct device_node *dn = pdev->dev.of_node;
struct device_node *port_node;
const struct acpi_device_id *acpi_id;
struct fwnode_handle *fwnode = pdev->dev.fwnode;
struct fwnode_handle *port_fwnode;
struct mvpp2 *priv;
struct resource *res;
void __iomem *base;
......@@ -8205,8 +8229,14 @@ static int mvpp2_probe(struct platform_device *pdev)
if (!priv)
return -ENOMEM;
if (has_acpi_companion(&pdev->dev)) {
acpi_id = acpi_match_device(pdev->dev.driver->acpi_match_table,
&pdev->dev);
priv->hw_version = (unsigned long)acpi_id->driver_data;
} else {
priv->hw_version =
(unsigned long)of_device_get_match_data(&pdev->dev);
}
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
base = devm_ioremap_resource(&pdev->dev, res);
......@@ -8220,10 +8250,23 @@ static int mvpp2_probe(struct platform_device *pdev)
return PTR_ERR(priv->lms_base);
} else {
res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
if (has_acpi_companion(&pdev->dev)) {
/* In case the MDIO memory region is declared in
* the ACPI, it can already appear as 'in-use'
* in the OS. Because it is overlapped by second
* region of the network controller, make
* sure it is released, before requesting it again.
* The care is taken by mvpp2 driver to avoid
* concurrent access to this memory region.
*/
release_resource(res);
}
priv->iface_base = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(priv->iface_base))
return PTR_ERR(priv->iface_base);
}
if (priv->hw_version == MVPP22 && dev_of_node(&pdev->dev)) {
priv->sysctrl_base =
syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
"marvell,system-controller");
......@@ -8249,6 +8292,7 @@ static int mvpp2_probe(struct platform_device *pdev)
else
priv->max_port_rxqs = 32;
if (dev_of_node(&pdev->dev)) {
priv->pp_clk = devm_clk_get(&pdev->dev, "pp_clk");
if (IS_ERR(priv->pp_clk))
return PTR_ERR(priv->pp_clk);
......@@ -8275,6 +8319,7 @@ static int mvpp2_probe(struct platform_device *pdev)
err = clk_prepare_enable(priv->mg_clk);
if (err < 0)
goto err_gop_clk;
}
priv->axi_clk = devm_clk_get(&pdev->dev, "axi_clk");
if (IS_ERR(priv->axi_clk)) {
......@@ -8287,10 +8332,14 @@ static int mvpp2_probe(struct platform_device *pdev)
if (err < 0)
goto err_gop_clk;
}
}
/* Get system's tclk rate */
priv->tclk = clk_get_rate(priv->pp_clk);
} else if (device_property_read_u32(&pdev->dev, "clock-frequency",
&priv->tclk)) {
dev_err(&pdev->dev, "missing clock-frequency value\n");
return -EINVAL;
}
if (priv->hw_version == MVPP22) {
err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(40));
......@@ -8313,30 +8362,19 @@ static int mvpp2_probe(struct platform_device *pdev)
goto err_mg_clk;
}
priv->port_count = of_get_available_child_count(dn);
/* Initialize ports */
fwnode_for_each_available_child_node(fwnode, port_fwnode) {
err = mvpp2_port_probe(pdev, port_fwnode, priv);
if (err < 0)
goto err_port_probe;
}
if (priv->port_count == 0) {
dev_err(&pdev->dev, "no ports enabled\n");
err = -ENODEV;
goto err_mg_clk;
}
priv->port_list = devm_kcalloc(&pdev->dev, priv->port_count,
sizeof(*priv->port_list),
GFP_KERNEL);
if (!priv->port_list) {
err = -ENOMEM;
goto err_mg_clk;
}
/* Initialize ports */
i = 0;
for_each_available_child_of_node(dn, port_node) {
err = mvpp2_port_probe(pdev, port_node, priv, i);
if (err < 0)
goto err_port_probe;
i++;
}
/* Statistics must be gathered regularly because some of them (like
* packets counters) are 32-bit registers and could overflow quite
* quickly. For instance, a 10Gb link used at full bandwidth with the
......@@ -8357,7 +8395,7 @@ static int mvpp2_probe(struct platform_device *pdev)
err_port_probe:
i = 0;
for_each_available_child_of_node(dn, port_node) {
fwnode_for_each_available_child_node(fwnode, port_fwnode) {
if (priv->port_list[i])
mvpp2_port_remove(priv->port_list[i]);
i++;
......@@ -8376,14 +8414,14 @@ static int mvpp2_probe(struct platform_device *pdev)
static int mvpp2_remove(struct platform_device *pdev)
{
struct mvpp2 *priv = platform_get_drvdata(pdev);
struct device_node *dn = pdev->dev.of_node;
struct device_node *port_node;
struct fwnode_handle *fwnode = pdev->dev.fwnode;
struct fwnode_handle *port_fwnode;
int i = 0;
flush_workqueue(priv->stats_queue);
destroy_workqueue(priv->stats_queue);
for_each_available_child_of_node(dn, port_node) {
fwnode_for_each_available_child_node(fwnode, port_fwnode) {
if (priv->port_list[i]) {
mutex_destroy(&priv->port_list[i]->gather_stats_lock);
mvpp2_port_remove(priv->port_list[i]);
......@@ -8406,6 +8444,9 @@ static int mvpp2_remove(struct platform_device *pdev)
aggr_txq->descs_dma);
}
if (is_acpi_node(port_fwnode))
return 0;
clk_disable_unprepare(priv->axi_clk);
clk_disable_unprepare(priv->mg_clk);
clk_disable_unprepare(priv->pp_clk);
......@@ -8427,12 +8468,19 @@ static const struct of_device_id mvpp2_match[] = {
};
MODULE_DEVICE_TABLE(of, mvpp2_match);
static const struct acpi_device_id mvpp2_acpi_match[] = {
{ "MRVL0110", MVPP22 },
{ },
};
MODULE_DEVICE_TABLE(acpi, mvpp2_acpi_match);
static struct platform_driver mvpp2_driver = {
.probe = mvpp2_probe,
.remove = mvpp2_remove,
.driver = {
.name = MVPP2_DRIVER_NAME,
.of_match_table = mvpp2_match,
.acpi_match_table = ACPI_PTR(mvpp2_acpi_match),
},
};
......
......@@ -56,6 +56,8 @@ static inline acpi_handle acpi_device_handle(struct acpi_device *adev)
#define ACPI_COMPANION_SET(dev, adev) set_primary_fwnode(dev, (adev) ? \
acpi_fwnode_handle(adev) : NULL)
#define ACPI_HANDLE(dev) acpi_device_handle(ACPI_COMPANION(dev))
#define ACPI_HANDLE_FWNODE(fwnode) \
acpi_device_handle(to_acpi_device_node(fwnode))
static inline struct fwnode_handle *acpi_alloc_fwnode_static(void)
{
......@@ -626,6 +628,7 @@ int acpi_arch_timer_mem_init(struct arch_timer_mem *timer_mem, int *timer_count)
#define ACPI_COMPANION(dev) (NULL)
#define ACPI_COMPANION_SET(dev, adev) do { } while (0)
#define ACPI_HANDLE(dev) (NULL)
#define ACPI_HANDLE_FWNODE(fwnode) (NULL)
#define ACPI_DEVICE_CLASS(_cls, _msk) .cls = (0), .cls_msk = (0),
struct fwnode_handle;
......
......@@ -83,11 +83,17 @@ struct fwnode_handle *fwnode_get_next_parent(
struct fwnode_handle *fwnode);
struct fwnode_handle *fwnode_get_next_child_node(
const struct fwnode_handle *fwnode, struct fwnode_handle *child);
struct fwnode_handle *fwnode_get_next_available_child_node(
const struct fwnode_handle *fwnode, struct fwnode_handle *child);
#define fwnode_for_each_child_node(fwnode, child) \
for (child = fwnode_get_next_child_node(fwnode, NULL); child; \
child = fwnode_get_next_child_node(fwnode, child))
#define fwnode_for_each_available_child_node(fwnode, child) \
for (child = fwnode_get_next_available_child_node(fwnode, NULL); child;\
child = fwnode_get_next_available_child_node(fwnode, child))
struct fwnode_handle *device_get_next_child_node(
struct device *dev, struct fwnode_handle *child);
......@@ -103,6 +109,8 @@ struct fwnode_handle *device_get_named_child_node(struct device *dev,
struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);
void fwnode_handle_put(struct fwnode_handle *fwnode);
int fwnode_irq_get(struct fwnode_handle *fwnode, unsigned int index);
unsigned int device_get_child_node_count(struct device *dev);
static inline bool device_property_read_bool(struct device *dev,
......@@ -279,6 +287,9 @@ int device_get_phy_mode(struct device *dev);
void *device_get_mac_address(struct device *dev, char *addr, int alen);
int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
void *fwnode_get_mac_address(struct fwnode_handle *fwnode,
char *addr, int alen);
struct fwnode_handle *fwnode_graph_get_next_endpoint(
const struct fwnode_handle *fwnode, struct fwnode_handle *prev);
struct fwnode_handle *
......
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