[{"id":1764985,"web_url":"http://patchwork.ozlabs.org/comment/1764985/","msgid":"<20170907223625.GW11248@lunn.ch>","list_archive_url":null,"date":"2017-09-07T22:36:25","subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":13608,"url":"http://patchwork.ozlabs.org/api/people/13608/","name":"Andrew Lunn","email":"andrew@lunn.ch"},"content":"On Thu, Sep 07, 2017 at 09:17:16PM +0000, Tristram.Ha@microchip.com wrote:\n> From: Tristram Ha \n> \n> Add KSZ8795 switch support with function code.\n> \n> Signed-off-by: Tristram Ha \n> ---\n> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c\n> new file mode 100644\n> index 0000000..e4d4e6a\n> --- /dev/null\n> +++ b/drivers/net/dsa/microchip/ksz8795.c\n> @@ -0,0 +1,2066 @@\n> +/*\n> + * Microchip KSZ8795 switch driver\n> + *\n> + * Copyright (C) 2017 Microchip Technology Inc.\n> + *\tTristram Ha \n> + *\n> + * Permission to use, copy, modify, and/or distribute this software for any\n> + * purpose with or without fee is hereby granted, provided that the above\n> + * copyright notice and this permission notice appear in all copies.\n> + *\n> + * THE SOFTWARE IS PROVIDED \"AS IS\" AND THE AUTHOR DISCLAIMS ALL WARRANTIES\n> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF\n> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR\n> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES\n> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN\n> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF\n> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"ksz_priv.h\"\n> +#include \"ksz8795.h\"\n> +\n> +/**\n> + * Some counters do not need to be read too often because they are less likely\n> + * to increase much.\n> + */\n\nWhat does comment mean? Are you caching statistics, and updating\ndifferent values at different rates?\n\n> +static const struct {\n> +\tint interval;\n> +\tchar string[ETH_GSTRING_LEN];\n> +} mib_names[TOTAL_SWITCH_COUNTER_NUM] = {\n> +\t{ 1, \"rx_hi\" },\n> +\t{ 4, \"rx_undersize\" },\n> +\t{ 4, \"rx_fragments\" },\n> +\t{ 4, \"rx_oversize\" },\n> +\t{ 4, \"rx_jabbers\" },\n> +\t{ 4, \"rx_symbol_err\" },\n> +\t{ 4, \"rx_crc_err\" },\n> +\t{ 4, \"rx_align_err\" },\n> +\t{ 4, \"rx_mac_ctrl\" },\n> +\t{ 1, \"rx_pause\" },\n> +\t{ 1, \"rx_bcast\" },\n> +\t{ 1, \"rx_mcast\" },\n> +\t{ 1, \"rx_ucast\" },\n> +\t{ 2, \"rx_64_or_less\" },\n> +\t{ 2, \"rx_65_127\" },\n> +\t{ 2, \"rx_128_255\" },\n> +\t{ 2, \"rx_256_511\" },\n> +\t{ 2, \"rx_512_1023\" },\n> +\t{ 2, \"rx_1024_1522\" },\n> +\t{ 2, \"rx_1523_2000\" },\n> +\t{ 2, \"rx_2001\" },\n> +\t{ 1, \"tx_hi\" },\n> +\t{ 4, \"tx_late_col\" },\n> +\t{ 1, \"tx_pause\" },\n> +\t{ 1, \"tx_bcast\" },\n> +\t{ 1, \"tx_mcast\" },\n> +\t{ 1, \"tx_ucast\" },\n> +\t{ 4, \"tx_deferred\" },\n> +\t{ 4, \"tx_total_col\" },\n> +\t{ 4, \"tx_exc_col\" },\n> +\t{ 4, \"tx_single_col\" },\n> +\t{ 4, \"tx_mult_col\" },\n> +\t{ 1, \"rx_total\" },\n> +\t{ 1, \"tx_total\" },\n> +\t{ 2, \"rx_discards\" },\n> +\t{ 2, \"tx_discards\" },\n> +};\n> +\n> +static void ksz_cfg(struct ksz_device *dev, u32 addr, u8 bits, bool set)\n> +{\n> +\tu8 data;\n> +\n> +\tksz_read8(dev, addr, &data);\n> +\tif (set)\n> +\t\tdata |= bits;\n> +\telse\n> +\t\tdata &= ~bits;\n> +\tksz_write8(dev, addr, data);\n> +}\n\nShouldn't this function be in the shared code? There is an exact copy\ncurrently in ksz_common.c.\n\n> +static void ksz_port_cfg(struct ksz_device *dev, int port, int offset, u8 bits,\n> +\t\t\t bool set)\n> +{\n> +\tu32 addr;\n> +\tu8 data;\n> +\n> +\taddr = PORT_CTRL_ADDR(port, offset);\n> +\tksz_read8(dev, addr, &data);\n> +\n> +\tif (set)\n> +\t\tdata |= bits;\n> +\telse\n> +\t\tdata &= ~bits;\n> +\n> +\tksz_write8(dev, addr, data);\n> +}\n\nAnd this also appears to be shared.\n\n> +static void port_r_mib_cnt(struct ksz_device *dev, int port, u16 addr, u64 *cnt)\n\nIt would be good to use the ksz8795_ prefix for functions specific to\nthe KSZ8795.\n\n> +{\n> +\tu32 data;\n> +\tu16 ctrl_addr;\n> +\tu8 check;\n> +\tint timeout;\n> +\n> +\tctrl_addr = addr + SWITCH_COUNTER_NUM * port;\n> +\n> +\tctrl_addr |= IND_ACC_TABLE(TABLE_MIB | TABLE_READ);\n> +\tmutex_lock(&dev->stats_mutex);\n> +\tksz_write16(dev, REG_IND_CTRL_0, ctrl_addr);\n> +\n> +\tfor (timeout = 1; timeout > 0; timeout--) {\n\nA rather short timeount!\n\n> +\t\tksz_read8(dev, REG_IND_MIB_CHECK, &check);\n> +\n> +\t\tif (check & MIB_COUNTER_VALID) {\n> +\t\t\tksz_read32(dev, REG_IND_DATA_LO, &data);\n> +\t\t\tif (check & MIB_COUNTER_OVERFLOW)\n> +\t\t\t\t*cnt += MIB_COUNTER_VALUE + 1;\n> +\t\t\t*cnt += data & MIB_COUNTER_VALUE;\n> +\t\t\tbreak;\n> +\t\t}\n\nShould there be a dev_err() here about timing out?\n\n\n> +\t}\n> +\tmutex_unlock(&dev->stats_mutex);\n> +}\n> +\n> +static int valid_dyn_entry(struct ksz_device *dev, u8 *data)\n> +{\n> +\tint timeout = 100;\n> +\n> +\tdo {\n> +\t\tksz_read8(dev, REG_IND_DATA_CHECK, data);\n> +\t\ttimeout--;\n> +\t} while ((*data & DYNAMIC_MAC_TABLE_NOT_READY) && timeout);\n> +\n> +\t/* Entry is not ready for accessing. */\n> +\tif (*data & DYNAMIC_MAC_TABLE_NOT_READY) {\n> +\t\treturn 1;\n\nUse standard error code. -ETIMEOUT\n\n> +\t/* Entry is ready for accessing. */\n> +\t} else {\n> +\t\tksz_read8(dev, REG_IND_DATA_8, data);\n> +\n> +\t\t/* There is no valid entry in the table. */\n> +\t\tif (*data & DYNAMIC_MAC_TABLE_MAC_EMPTY)\n> +\t\t\treturn 2;\n\n-EINVAL?\n\n> +\t}\n> +\treturn 0;\n> +}\n\nIt also looks like these return values get propagated further. So you\nreally should be using error code.\n\n> +static int ksz_r_dyn_mac_table(struct ksz_device *dev, u16 addr, u8 *mac_addr,\n> +\t\t\t u8 *fid, u8 *src_port, u8 *timestamp,\n> +\t\t\t u16 *entries)\n> +{\n> +\tu32 data_hi;\n> +\tu32 data_lo;\n> +\tu16 ctrl_addr;\n> +\tint rc;\n> +\tu8 data;\n> +\n> +\tctrl_addr = IND_ACC_TABLE(TABLE_DYNAMIC_MAC | TABLE_READ) | addr;\n> +\n> +\tksz_write16(dev, REG_IND_CTRL_0, ctrl_addr);\n> +\n> +\trc = valid_dyn_entry(dev, &data);\n> +\tif (rc == 1) {\n> +\t\tif (addr == 0)\n> +\t\t\t*entries = 0;\n> +\t} else if (rc == 2) {\n> +\t\t*entries = 0;\n> +\t/* At least one valid entry in the table. */\n> +\t} else {\n> +\t\tu64 buf;\n> +\n> +\t\tdev->ops->get(dev, REG_IND_DATA_HI, &buf, sizeof(buf));\n> +\t\tbuf = be64_to_cpu(buf);\n> +\t\tdata_hi = (u32)(buf >> 32);\n> +\t\tdata_lo = (u32)buf;\n> +\n> +\t\t/* Check out how many valid entry in the table. */\n> +\t\t*entries = (u16)(((((u16)\n> +\t\t\tdata & DYNAMIC_MAC_TABLE_ENTRIES_H) <<\n> +\t\t\tDYNAMIC_MAC_ENTRIES_H_S) |\n> +\t\t\t(((data_hi & DYNAMIC_MAC_TABLE_ENTRIES) >>\n> +\t\t\tDYNAMIC_MAC_ENTRIES_S))) + 1);\n\nWhen i see so many ((((( i think this needs splitting up to make it\nreadable.\n\n> +\n> +\t\t*fid = (u8)((data_hi & DYNAMIC_MAC_TABLE_FID) >>\n> +\t\t\tDYNAMIC_MAC_FID_S);\n> +\t\t*src_port = (u8)((data_hi & DYNAMIC_MAC_TABLE_SRC_PORT) >>\n> +\t\t\tDYNAMIC_MAC_SRC_PORT_S);\n> +\t\t*timestamp = (u8)((\n> +\t\t\tdata_hi & DYNAMIC_MAC_TABLE_TIMESTAMP) >>\n> +\t\t\tDYNAMIC_MAC_TIMESTAMP_S);\n\nAre all the casts needed? Please go through all the code and see about\nremoving all the unneeded casts.\n\n> +\n> +\treturn rc;\n> +}\n> +\n> +struct alu_struct {\n> +\tu8\tis_static:1;\n> +\tu8\tprio_age:3;\n> +\tu8\tis_override:1;\n> +\tu8\tis_use_fid:1;\n> +\tu8\tport_forward:5;\n> +\tu8\tfid:7;\n> +\tu8\tmac[ETH_ALEN];\n> +};\n> +\n> +static int ksz_r_sta_mac_table(struct ksz_device *dev, u16 addr,\n> +\t\t\t struct alu_struct *alu)\n> +{\n> +\tu64 data;\n> +\tu32 data_hi;\n> +\tu32 data_lo;\n> +\n> +\tksz_r_table_64(dev, TABLE_STATIC_MAC, addr, &data);\n> +\tdata_hi = data >> 32;\n> +\tdata_lo = (u32)data;\n> +\tif (data_hi & (STATIC_MAC_TABLE_VALID | STATIC_MAC_TABLE_OVERRIDE)) {\n> +\t\talu->mac[5] = (u8)data_lo;\n> +\t\talu->mac[4] = (u8)(data_lo >> 8);\n> +\t\talu->mac[3] = (u8)(data_lo >> 16);\n> +\t\talu->mac[2] = (u8)(data_lo >> 24);\n> +\t\talu->mac[1] = (u8)data_hi;\n> +\t\talu->mac[0] = (u8)(data_hi >> 8);\n> +\t\talu->port_forward =\n> +\t\t\t(u8)((data_hi & STATIC_MAC_TABLE_FWD_PORTS) >>\n> +\t\t\tSTATIC_MAC_FWD_PORTS_S);\n> +\t\talu->is_override =\n> +\t\t\t(data_hi & STATIC_MAC_TABLE_OVERRIDE) ? 1 : 0;\n> +\t\tdata_hi >>= 1;\n> +\t\talu->is_use_fid = (data_hi & STATIC_MAC_TABLE_USE_FID) ? 1 : 0;\n> +\t\talu->fid = (u8)((data_hi & STATIC_MAC_TABLE_FID) >>\n> +\t\t\tSTATIC_MAC_FID_S);\n> +\t\treturn 0;\n> +\t}\n> +\treturn -1;\n\nAnd what does -1 mean?\n\n> +static void ksz_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)\n> +{\n> +\tstruct ksz_port *port;\n> +\tu8 ctrl;\n> +\tu8 restart;\n> +\tu8 link;\n> +\tu8 speed;\n> +\tu8 force;\n> +\tu8 p = phy;\n> +\tu16 data = 0;\n> +\tint processed = true;\n> +\n> +\tport = &dev->ports[p];\n> +\tswitch (reg) {\n> +\tcase PHY_REG_CTRL:\n> +\t\tksz_pread8(dev, p, P_LOCAL_CTRL, &ctrl);\n> +\t\tksz_pread8(dev, p, P_NEG_RESTART_CTRL, &restart);\n> +\t\tksz_pread8(dev, p, P_SPEED_STATUS, &speed);\n> +\t\tksz_pread8(dev, p, P_FORCE_CTRL, &force);\n> +\t\tif (restart & PORT_PHY_LOOPBACK)\n> +\t\t\tdata |= PHY_LOOPBACK;\n> +\t\tif (force & PORT_FORCE_100_MBIT)\n> +\t\t\tdata |= PHY_SPEED_100MBIT;\n> +\t\tif (!(force & PORT_AUTO_NEG_DISABLE))\n> +\t\t\tdata |= PHY_AUTO_NEG_ENABLE;\n> +\t\tif (restart & PORT_POWER_DOWN)\n> +\t\t\tdata |= PHY_POWER_DOWN;\n\nSame questions i asked Pavel\n\nIs there a way to access the PHY registers over SPI? The datasheet\nsuggests there is, but it does not say how, as far as i can tell.\n\nIdeally, we want to use just a normal PHY driver.\n\n> +static int ksz_sset_count(struct dsa_switch *ds)\n> +{\n> +\treturn TOTAL_SWITCH_COUNTER_NUM;\n> +}\n> +\n> +static void ksz_get_strings(struct dsa_switch *ds, int port, uint8_t *buf)\n> +{\n> +\tint i;\n> +\n> +\tfor (i = 0; i < TOTAL_SWITCH_COUNTER_NUM; i++) {\n> +\t\tmemcpy(buf + i * ETH_GSTRING_LEN, mib_names[i].string,\n> +\t\t ETH_GSTRING_LEN);\n> +\t}\n> +}\n> +\n> +static void ksz_get_ethtool_stats(struct dsa_switch *ds, int port,\n> +\t\t\t\t uint64_t *buf)\n> +{\n> +\tstruct ksz_device *dev = ds->priv;\n> +\tstruct ksz_port_mib *mib;\n> +\tstruct ksz_port_mib_info *info;\n> +\tint i;\n> +\n> +\tmib = &dev->ports[port].mib;\n> +\n> +\tspin_lock(&dev->mib_read_lock);\n> +\tfor (i = 0; i < TOTAL_SWITCH_COUNTER_NUM; i++) {\n> +\t\tinfo = &mib->info[i];\n> +\t\tbuf[i] = info->counter;\n> +\t\tinfo->read_cnt += info->read_max;\n> +\t}\n> +\tspin_unlock(&dev->mib_read_lock);\n> +\n> +\tmib->ctrl = 1;\n> +\tschedule_work(&dev->mib_read);\n\nHumm. I want to take a closer look at this caching code...\n\n> +}\n> +\n> +static u8 STP_MULTICAST_ADDR[] = {\n> +\t0x01, 0x80, 0xC2, 0x00, 0x00, 0x00\n> +};\n\nThis could be const. And this is not python. Global variables don't\nneed to be all capitals.\n\n Andrew","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpFgV6bYCz9s78\n\tfor ;\n\tFri, 8 Sep 2017 08:36:54 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S932521AbdIGWg3 (ORCPT );\n\tThu, 7 Sep 2017 18:36:29 -0400","from vps0.lunn.ch ([178.209.37.122]:60177 \"EHLO vps0.lunn.ch\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1752689AbdIGWg2 (ORCPT );\n\tThu, 7 Sep 2017 18:36:28 -0400","from andrew by vps0.lunn.ch with local (Exim 4.84_2)\n\t(envelope-from )\n\tid 1dq5PN-00089p-I8; Fri, 08 Sep 2017 00:36:25 +0200"],"Date":"Fri, 8 Sep 2017 00:36:25 +0200","From":"Andrew Lunn ","To":"Tristram.Ha@microchip.com","Cc":"muvarov@gmail.com, pavel@ucw.cz, nathan.leigh.conrad@gmail.com,\n\tvivien.didelot@savoirfairelinux.com, f.fainelli@gmail.com,\n\tnetdev@vger.kernel.org, linux-kernel@vger.kernel.org,\n\tWoojung.Huh@microchip.com","Subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","Message-ID":"<20170907223625.GW11248@lunn.ch>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765181,"web_url":"http://patchwork.ozlabs.org/comment/1765181/","msgid":"<20170908091856.GB17738@amd>","list_archive_url":null,"date":"2017-09-08T09:18:56","subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":2109,"url":"http://patchwork.ozlabs.org/api/people/2109/","name":"Pavel Machek","email":"pavel@ucw.cz"},"content":"On Thu 2017-09-07 21:17:16, Tristram.Ha@microchip.com wrote:\n> From: Tristram Ha \n> \n> Add KSZ8795 switch support with function code.\n\nEnglish? \"Add KSZ8795 switch support.\" ?\n\n> Signed-off-by: Tristram Ha \n> ---\n> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c\n> new file mode 100644\n> index 0000000..e4d4e6a\n> --- /dev/null\n> +++ b/drivers/net/dsa/microchip/ksz8795.c\n> @@ -0,0 +1,2066 @@\n> +/*\n> + * Microchip KSZ8795 switch driver\n> + *\n> + * Copyright (C) 2017 Microchip Technology Inc.\n> + *\tTristram Ha \n> + *\n> + * Permission to use, copy, modify, and/or distribute this software for any\n> + * purpose with or without fee is hereby granted, provided that the above\n> + * copyright notice and this permission notice appear in all copies.\n> + *\n> + * THE SOFTWARE IS PROVIDED \"AS IS\" AND THE AUTHOR DISCLAIMS ALL WARRANTIES\n> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF\n> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR\n> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES\n> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN\n> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF\n> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.\n> + */\n\nThis is not exactly GPL, right? But tagging below says it is\nGPL. Please fix one.\n\n> +/**\n> + * Some counters do not need to be read too often because they are less likely\n> + * to increase much.\n> + */\n\nKerneldoc markup but this does not look like kerneldoc. Use plain /* instead?\n\n> +static void ksz_cfg(struct ksz_device *dev, u32 addr, u8 bits, bool set)\n> +{\n> +\tu8 data;\n> +\n> +\tksz_read8(dev, addr, &data);\n> +\tif (set)\n> +\t\tdata |= bits;\n> +\telse\n> +\t\tdata &= ~bits;\n> +\tksz_write8(dev, addr, data);\n> +}\n> +\n> +static void ksz_port_cfg(struct ksz_device *dev, int port, int offset, u8 bits,\n> +\t\t\t bool set)\n> +{\n> +\tu32 addr;\n> +\tu8 data;\n> +\n> +\taddr = PORT_CTRL_ADDR(port, offset);\n> +\tksz_read8(dev, addr, &data);\n> +\n> +\tif (set)\n> +\t\tdata |= bits;\n> +\telse\n> +\t\tdata &= ~bits;\n> +\n> +\tksz_write8(dev, addr, data);\n> +}\n\nOther drivers will need this code, right? Does it make sense to put it\ninto header?\n\n> +static int chk_last_port(struct ksz_device *dev, int p)\n> +{\n> +\tif (dev->last_port && p == dev->last_port)\n> +\t\tp = dev->cpu_port;\n> +\treturn p;\n> +}\n\nWe often prefix even static functions with common prefix. Up to you, I\nguess.\n\n> +static int ksz_reset_switch(struct ksz_device *dev)\n> +{\n> +\t/* reset switch */\n> +\tksz_write8(dev, REG_POWER_MANAGEMENT_1,\n> +\t\t SW_SOFTWARE_POWER_DOWN << SW_POWER_MANAGEMENT_MODE_S);\n> +\tksz_write8(dev, REG_POWER_MANAGEMENT_1, 0);\n> +\n> +\treturn 0;\n> +}\n\nThis is going to be same in other drivers, right?\n\n> +\n> +\tfor (timeout = 1; timeout > 0; timeout--) {\n> +\t\tksz_read8(dev, REG_IND_MIB_CHECK, &check);\n> +\n> +\t\tif (check & MIB_COUNTER_VALID) {\n> +\t\t\tksz_read32(dev, REG_IND_DATA_LO, &data);\n> +\t\t\tif (addr < 2) {\n> +\t\t\t\tu64 total;\n> +\n> +\t\t\t\ttotal = check & MIB_TOTAL_BYTES_H;\n> +\t\t\t\ttotal <<= 32;\n> +\t\t\t\t*cnt += total;\n> +\t\t\t\t*cnt += data;\n> +\t\t\t\tif (check & MIB_COUNTER_OVERFLOW) {\n> +\t\t\t\t\ttotal = MIB_TOTAL_BYTES_H + 1;\n> +\t\t\t\t\ttotal <<= 32;\n> +\t\t\t\t\t*cnt += total;\n> +\t\t\t\t}\n> +\t\t\t} else {\n> +\t\t\t\tif (check & MIB_COUNTER_OVERFLOW)\n> +\t\t\t\t\t*cnt += MIB_PACKET_DROPPED + 1;\n> +\t\t\t\t*cnt += data & MIB_PACKET_DROPPED;\n> +\t\t\t}\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n\nWhy do you need a loop here? This is quite strange code. (And you have\nsimilar strangeness elsewhere. Please fix.)\n\n> +static int valid_dyn_entry(struct ksz_device *dev, u8 *data)\n> +{\n> +\tint timeout = 100;\n> +\n> +\tdo {\n> +\t\tksz_read8(dev, REG_IND_DATA_CHECK, data);\n> +\t\ttimeout--;\n> +\t} while ((*data & DYNAMIC_MAC_TABLE_NOT_READY) && timeout);\n> +\n> +\t/* Entry is not ready for accessing. */\n> +\tif (*data & DYNAMIC_MAC_TABLE_NOT_READY) {\n> +\t\treturn 1;\n> +\t/* Entry is ready for accessing. */\n> +\t} else {\n> +\t\tksz_read8(dev, REG_IND_DATA_8, data);\n> +\n> +\t\t/* There is no valid entry in the table. */\n> +\t\tif (*data & DYNAMIC_MAC_TABLE_MAC_EMPTY)\n> +\t\t\treturn 2;\n> +\t}\n> +\treturn 0;\n> +}\n\nNormal calling convention is 0 / -ERROR, not 0,1,2.\n\n> +static void ksz_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)\n> +{\n> +\tstruct ksz_port *port;\n> +\tu8 ctrl;\n> +\tu8 restart;\n> +\tu8 link;\n> +\tu8 speed;\n> +\tu8 force;\n> +\tu8 p = phy;\n> +\tu16 data = 0;\n> +\tint processed = true;\n> +\n> +\tport = &dev->ports[p];\n> +\tswitch (reg) {\n> +\tcase PHY_REG_CTRL:\n> +\t\tksz_pread8(dev, p, P_LOCAL_CTRL, &ctrl);\n> +\t\tksz_pread8(dev, p, P_NEG_RESTART_CTRL, &restart);\n> +\t\tksz_pread8(dev, p, P_SPEED_STATUS, &speed);\n> +\t\tksz_pread8(dev, p, P_FORCE_CTRL, &force);\n> +\t\tif (restart & PORT_PHY_LOOPBACK)\n> +\t\t\tdata |= PHY_LOOPBACK;\n> +\t\tif (force & PORT_FORCE_100_MBIT)\n> +\t\t\tdata |= PHY_SPEED_100MBIT;\n> +\t\tif (!(force & PORT_AUTO_NEG_DISABLE))\n> +\t\t\tdata |= PHY_AUTO_NEG_ENABLE;\n> +\t\tif (restart & PORT_POWER_DOWN)\n> +\t\t\tdata |= PHY_POWER_DOWN;\n> +\t\tif (restart & PORT_AUTO_NEG_RESTART)\n> +\t\t\tdata |= PHY_AUTO_NEG_RESTART;\n> +\t\tif (force & PORT_FORCE_FULL_DUPLEX)\n> +\t\t\tdata |= PHY_FULL_DUPLEX;\n> +\t\tif (speed & PORT_HP_MDIX)\n> +\t\t\tdata |= PHY_HP_MDIX;\n> +\t\tif (restart & PORT_FORCE_MDIX)\n> +\t\t\tdata |= PHY_FORCE_MDIX;\n> +\t\tif (restart & PORT_AUTO_MDIX_DISABLE)\n> +\t\t\tdata |= PHY_AUTO_MDIX_DISABLE;\n> +\t\tif (restart & PORT_TX_DISABLE)\n> +\t\t\tdata |= PHY_TRANSMIT_DISABLE;\n> +\t\tif (restart & PORT_LED_OFF)\n> +\t\t\tdata |= PHY_LED_DISABLE;\n> +\t\tbreak;\n> +\tcase PHY_REG_STATUS:\n> +\t\tksz_pread8(dev, p, P_LINK_STATUS, &link);\n> +\t\tksz_pread8(dev, p, P_SPEED_STATUS, &speed);\n> +\t\tdata = PHY_100BTX_FD_CAPABLE |\n> +\t\t\tPHY_100BTX_CAPABLE |\n> +\t\t\tPHY_10BT_FD_CAPABLE |\n> +\t\t\tPHY_10BT_CAPABLE |\n> +\t\t\tPHY_AUTO_NEG_CAPABLE;\n> +\t\tif (link & PORT_AUTO_NEG_COMPLETE)\n> +\t\t\tdata |= PHY_AUTO_NEG_ACKNOWLEDGE;\n> +\t\tif (link & PORT_STAT_LINK_GOOD) {\n> +\t\t\tdata |= PHY_LINK_STATUS;\n> +\t\t\tif (!port->link_up) {\n> +\t\t\t\tport->link_up = 1;\n> +\t\t\t\tdev->live_ports |= (1 << phy) & dev->on_ports;\n> +\t\t\t}\n> +\t\t} else if (port->link_up) {\n> +\t\t\tport->link_down = 1;\n> +\t\t\tport->link_up = 0;\n> +\t\t\tdev->live_ports &= ~(1 << phy);\n> +\t\t}\n> +\t\tbreak;\n> +\tcase PHY_REG_ID_1:\n> +\t\tdata = KSZ8795_ID_HI;\n> +\t\tbreak;\n> +\tcase PHY_REG_ID_2:\n> +\t\tdata = KSZ8795_ID_LO;\n> +\t\tbreak;\n> +\tcase PHY_REG_AUTO_NEGOTIATION:\n> +\t\tksz_pread8(dev, p, P_LOCAL_CTRL, &ctrl);\n> +\t\tdata = PHY_AUTO_NEG_802_3;\n> +\t\tif (ctrl & PORT_AUTO_NEG_SYM_PAUSE)\n> +\t\t\tdata |= PHY_AUTO_NEG_SYM_PAUSE;\n> +\t\tif (ctrl & PORT_AUTO_NEG_100BTX_FD)\n> +\t\t\tdata |= PHY_AUTO_NEG_100BTX_FD;\n> +\t\tif (ctrl & PORT_AUTO_NEG_100BTX)\n> +\t\t\tdata |= PHY_AUTO_NEG_100BTX;\n> +\t\tif (ctrl & PORT_AUTO_NEG_10BT_FD)\n> +\t\t\tdata |= PHY_AUTO_NEG_10BT_FD;\n> +\t\tif (ctrl & PORT_AUTO_NEG_10BT)\n> +\t\t\tdata |= PHY_AUTO_NEG_10BT;\n> +\t\tbreak;\n> +\tcase PHY_REG_REMOTE_CAPABILITY:\n> +\t\tksz_pread8(dev, p, P_REMOTE_STATUS, &link);\n> +\t\tdata = PHY_AUTO_NEG_802_3;\n> +\t\tif (link & PORT_REMOTE_SYM_PAUSE)\n> +\t\t\tdata |= PHY_AUTO_NEG_SYM_PAUSE;\n> +\t\tif (link & PORT_REMOTE_100BTX_FD)\n> +\t\t\tdata |= PHY_AUTO_NEG_100BTX_FD;\n> +\t\tif (link & PORT_REMOTE_100BTX)\n> +\t\t\tdata |= PHY_AUTO_NEG_100BTX;\n> +\t\tif (link & PORT_REMOTE_10BT_FD)\n> +\t\t\tdata |= PHY_AUTO_NEG_10BT_FD;\n> +\t\tif (link & PORT_REMOTE_10BT)\n> +\t\t\tdata |= PHY_AUTO_NEG_10BT;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tprocessed = false;\n> +\t\tbreak;\n> +\t}\n> +\tif (processed)\n> +\t\t*val = data;\n> +}\n\nSimilar code will be needed by other drivers, right?\n\n> +static int ksz_port_bridge_join(struct dsa_switch *ds, int port,\n> +\t\t\t\tstruct net_device *br)\n> +{\n> +\tstruct ksz_device *dev = ds->priv;\n> +\n> +\tdev->br_member |= (1 << port);\n> +\n> +\t/**\n> +\t * port_stp_state_set() will be called after to put the port in\n> +\t * appropriate state so there is no need to do anything.\n> +\t */\n\n/** -> /*. Fix it elsewhere, too. This is not kerneldoc.\n\n> +\t/* Topology is changed. */\n> +\tif (forward != dev->member)\n\nhas changed?\n\nThanks for the patches,\n\n\t\t\t\t\t\t\t\t\tPavel","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpWwk5svDz9s82\n\tfor ;\n\tFri, 8 Sep 2017 19:19:18 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752229AbdIHJTH (ORCPT );\n\tFri, 8 Sep 2017 05:19:07 -0400","from atrey.karlin.mff.cuni.cz ([195.113.26.193]:34206 \"EHLO\n\tatrey.karlin.mff.cuni.cz\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752526AbdIHJTD (ORCPT\n\t); Fri, 8 Sep 2017 05:19:03 -0400","by atrey.karlin.mff.cuni.cz (Postfix, from userid 512)\n\tid 58472824EF; Fri, 8 Sep 2017 11:19:01 +0200 (CEST)"],"Date":"Fri, 8 Sep 2017 11:18:56 +0200","From":"Pavel Machek ","To":"Tristram.Ha@microchip.com","Cc":"andrew@lunn.ch, muvarov@gmail.com, nathan.leigh.conrad@gmail.com,\n\tvivien.didelot@savoirfairelinux.com, f.fainelli@gmail.com,\n\tnetdev@vger.kernel.org, linux-kernel@vger.kernel.org,\n\tWoojung.Huh@microchip.com","Subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","Message-ID":"<20170908091856.GB17738@amd>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"hHWLQfXTYDoKhP50\"","Content-Disposition":"inline","In-Reply-To":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765552,"web_url":"http://patchwork.ozlabs.org/comment/1765552/","msgid":"<93AF473E2DA327428DE3D46B72B1E9FD41121E3D@CHN-SV-EXMX02.mchp-main.com>","list_archive_url":null,"date":"2017-09-08T17:54:24","subject":"RE: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":72262,"url":"http://patchwork.ozlabs.org/api/people/72262/","name":"","email":"Tristram.Ha@microchip.com"},"content":"> -----Original Message-----\n> From: Pavel Machek [mailto:pavel@ucw.cz]\n> Sent: Friday, September 08, 2017 2:19 AM\n> To: Tristram Ha - C24268\n> Cc: andrew@lunn.ch; muvarov@gmail.com; nathan.leigh.conrad@gmail.com;\n> vivien.didelot@savoirfairelinux.com; f.fainelli@gmail.com;\n> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Woojung Huh - C21699\n> Subject: Re: [PATCH RFC 3/5] Add KSZ8795 switch driver\n> \n> On Thu 2017-09-07 21:17:16, Tristram.Ha@microchip.com wrote:\n> > From: Tristram Ha \n> >\n> > Add KSZ8795 switch support with function code.\n> \n> English? \"Add KSZ8795 switch support.\" ?\n> \n> > Signed-off-by: Tristram Ha \n> > ---\n> > diff --git a/drivers/net/dsa/microchip/ksz8795.c\n> b/drivers/net/dsa/microchip/ksz8795.c\n> > new file mode 100644\n> > index 0000000..e4d4e6a\n> > --- /dev/null\n> > +++ b/drivers/net/dsa/microchip/ksz8795.c\n> > @@ -0,0 +1,2066 @@\n> > +/*\n> > + * Microchip KSZ8795 switch driver\n> > + *\n> > + * Copyright (C) 2017 Microchip Technology Inc.\n> > + *\tTristram Ha \n> > + *\n> > + * Permission to use, copy, modify, and/or distribute this software for any\n> > + * purpose with or without fee is hereby granted, provided that the above\n> > + * copyright notice and this permission notice appear in all copies.\n> > + *\n> > + * THE SOFTWARE IS PROVIDED \"AS IS\" AND THE AUTHOR DISCLAIMS ALL\n> WARRANTIES\n> > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES\n> OF\n> > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE\n> LIABLE FOR\n> > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY\n> DAMAGES\n> > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER\n> IN AN\n> > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,\n> ARISING OUT OF\n> > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.\n> > + */\n> \n> This is not exactly GPL, right? But tagging below says it is\n> GPL. Please fix one.\n> \n\nThis boilerplate paragraph was copied from the KSZ9477 driver, although I did\nwonder why this was used.\n\n> > +static int ksz_reset_switch(struct ksz_device *dev)\n> > +{\n> > +\t/* reset switch */\n> > +\tksz_write8(dev, REG_POWER_MANAGEMENT_1,\n> > +\t\t SW_SOFTWARE_POWER_DOWN <<\n> SW_POWER_MANAGEMENT_MODE_S);\n> > +\tksz_write8(dev, REG_POWER_MANAGEMENT_1, 0);\n> > +\n> > +\treturn 0;\n> > +}\n> \n> This is going to be same in other drivers, right?\n> \n\nThe switch reset is different in each chip, although KSZ8795 and\nKSZ8895 use the same mechanism.\n\n> > +\n> > +\tfor (timeout = 1; timeout > 0; timeout--) {\n> > +\t\tksz_read8(dev, REG_IND_MIB_CHECK, &check);\n> > +\n> > +\t\tif (check & MIB_COUNTER_VALID) {\n> > +\t\t\tksz_read32(dev, REG_IND_DATA_LO, &data);\n> > +\t\t\tif (addr < 2) {\n> > +\t\t\t\tu64 total;\n> > +\n> > +\t\t\t\ttotal = check & MIB_TOTAL_BYTES_H;\n> > +\t\t\t\ttotal <<= 32;\n> > +\t\t\t\t*cnt += total;\n> > +\t\t\t\t*cnt += data;\n> > +\t\t\t\tif (check & MIB_COUNTER_OVERFLOW) {\n> > +\t\t\t\t\ttotal = MIB_TOTAL_BYTES_H + 1;\n> > +\t\t\t\t\ttotal <<= 32;\n> > +\t\t\t\t\t*cnt += total;\n> > +\t\t\t\t}\n> > +\t\t\t} else {\n> > +\t\t\t\tif (check & MIB_COUNTER_OVERFLOW)\n> > +\t\t\t\t\t*cnt += MIB_PACKET_DROPPED + 1;\n> > +\t\t\t\t*cnt += data & MIB_PACKET_DROPPED;\n> > +\t\t\t}\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> \n> Why do you need a loop here? This is quite strange code. (And you have\n> similar strangeness elsewhere. Please fix.)\n> \n\nThe MIB_COUNTER_VALID bit may be invalid on first read, although in slow\nSPI speed it never happens. The timeout value should be increased to 2.\n\n> > +static int valid_dyn_entry(struct ksz_device *dev, u8 *data)\n> > +{\n> > +\tint timeout = 100;\n> > +\n> > +\tdo {\n> > +\t\tksz_read8(dev, REG_IND_DATA_CHECK, data);\n> > +\t\ttimeout--;\n> > +\t} while ((*data & DYNAMIC_MAC_TABLE_NOT_READY) && timeout);\n> > +\n> > +\t/* Entry is not ready for accessing. */\n> > +\tif (*data & DYNAMIC_MAC_TABLE_NOT_READY) {\n> > +\t\treturn 1;\n> > +\t/* Entry is ready for accessing. */\n> > +\t} else {\n> > +\t\tksz_read8(dev, REG_IND_DATA_8, data);\n> > +\n> > +\t\t/* There is no valid entry in the table. */\n> > +\t\tif (*data & DYNAMIC_MAC_TABLE_MAC_EMPTY)\n> > +\t\t\treturn 2;\n> > +\t}\n> > +\treturn 0;\n> > +}\n> \n> Normal calling convention is 0 / -ERROR, not 0,1,2.\n>\n\nThis is an internal function that is not returning any error. It just reports\ndifferent conditions so the calling function decides what to do.\n \n> > +static void ksz_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)\n> > +{\n> > +\tstruct ksz_port *port;\n> > +\tu8 ctrl;\n> > +\tu8 restart;\n> > +\tu8 link;\n> > +\tu8 speed;\n> > +\tu8 force;\n> > +\tu8 p = phy;\n> > +\tu16 data = 0;\n> > +\tint processed = true;\n> > +\n> > +\tport = &dev->ports[p];\n> > +\tswitch (reg) {\n> > +\tcase PHY_REG_CTRL:\n> > +\t\tksz_pread8(dev, p, P_LOCAL_CTRL, &ctrl);\n> > +\t\tksz_pread8(dev, p, P_NEG_RESTART_CTRL, &restart);\n> > +\t\tksz_pread8(dev, p, P_SPEED_STATUS, &speed);\n> > +\t\tksz_pread8(dev, p, P_FORCE_CTRL, &force);\n> > +\t\tif (restart & PORT_PHY_LOOPBACK)\n> > +\t\t\tdata |= PHY_LOOPBACK;\n> > +\t\tif (force & PORT_FORCE_100_MBIT)\n> > +\t\t\tdata |= PHY_SPEED_100MBIT;\n> > +\t\tif (!(force & PORT_AUTO_NEG_DISABLE))\n> > +\t\t\tdata |= PHY_AUTO_NEG_ENABLE;\n> > +\t\tif (restart & PORT_POWER_DOWN)\n> > +\t\t\tdata |= PHY_POWER_DOWN;\n> > +\t\tif (restart & PORT_AUTO_NEG_RESTART)\n> > +\t\t\tdata |= PHY_AUTO_NEG_RESTART;\n> > +\t\tif (force & PORT_FORCE_FULL_DUPLEX)\n> > +\t\t\tdata |= PHY_FULL_DUPLEX;\n> > +\t\tif (speed & PORT_HP_MDIX)\n> > +\t\t\tdata |= PHY_HP_MDIX;\n> > +\t\tif (restart & PORT_FORCE_MDIX)\n> > +\t\t\tdata |= PHY_FORCE_MDIX;\n> > +\t\tif (restart & PORT_AUTO_MDIX_DISABLE)\n> > +\t\t\tdata |= PHY_AUTO_MDIX_DISABLE;\n> > +\t\tif (restart & PORT_TX_DISABLE)\n> > +\t\t\tdata |= PHY_TRANSMIT_DISABLE;\n> > +\t\tif (restart & PORT_LED_OFF)\n> > +\t\t\tdata |= PHY_LED_DISABLE;\n> > +\t\tbreak;\n> > +\tcase PHY_REG_STATUS:\n> > +\t\tksz_pread8(dev, p, P_LINK_STATUS, &link);\n> > +\t\tksz_pread8(dev, p, P_SPEED_STATUS, &speed);\n> > +\t\tdata = PHY_100BTX_FD_CAPABLE |\n> > +\t\t\tPHY_100BTX_CAPABLE |\n> > +\t\t\tPHY_10BT_FD_CAPABLE |\n> > +\t\t\tPHY_10BT_CAPABLE |\n> > +\t\t\tPHY_AUTO_NEG_CAPABLE;\n> > +\t\tif (link & PORT_AUTO_NEG_COMPLETE)\n> > +\t\t\tdata |= PHY_AUTO_NEG_ACKNOWLEDGE;\n> > +\t\tif (link & PORT_STAT_LINK_GOOD) {\n> > +\t\t\tdata |= PHY_LINK_STATUS;\n> > +\t\t\tif (!port->link_up) {\n> > +\t\t\t\tport->link_up = 1;\n> > +\t\t\t\tdev->live_ports |= (1 << phy) & dev->on_ports;\n> > +\t\t\t}\n> > +\t\t} else if (port->link_up) {\n> > +\t\t\tport->link_down = 1;\n> > +\t\t\tport->link_up = 0;\n> > +\t\t\tdev->live_ports &= ~(1 << phy);\n> > +\t\t}\n> > +\t\tbreak;\n> > +\tcase PHY_REG_ID_1:\n> > +\t\tdata = KSZ8795_ID_HI;\n> > +\t\tbreak;\n> > +\tcase PHY_REG_ID_2:\n> > +\t\tdata = KSZ8795_ID_LO;\n> > +\t\tbreak;\n> > +\tcase PHY_REG_AUTO_NEGOTIATION:\n> > +\t\tksz_pread8(dev, p, P_LOCAL_CTRL, &ctrl);\n> > +\t\tdata = PHY_AUTO_NEG_802_3;\n> > +\t\tif (ctrl & PORT_AUTO_NEG_SYM_PAUSE)\n> > +\t\t\tdata |= PHY_AUTO_NEG_SYM_PAUSE;\n> > +\t\tif (ctrl & PORT_AUTO_NEG_100BTX_FD)\n> > +\t\t\tdata |= PHY_AUTO_NEG_100BTX_FD;\n> > +\t\tif (ctrl & PORT_AUTO_NEG_100BTX)\n> > +\t\t\tdata |= PHY_AUTO_NEG_100BTX;\n> > +\t\tif (ctrl & PORT_AUTO_NEG_10BT_FD)\n> > +\t\t\tdata |= PHY_AUTO_NEG_10BT_FD;\n> > +\t\tif (ctrl & PORT_AUTO_NEG_10BT)\n> > +\t\t\tdata |= PHY_AUTO_NEG_10BT;\n> > +\t\tbreak;\n> > +\tcase PHY_REG_REMOTE_CAPABILITY:\n> > +\t\tksz_pread8(dev, p, P_REMOTE_STATUS, &link);\n> > +\t\tdata = PHY_AUTO_NEG_802_3;\n> > +\t\tif (link & PORT_REMOTE_SYM_PAUSE)\n> > +\t\t\tdata |= PHY_AUTO_NEG_SYM_PAUSE;\n> > +\t\tif (link & PORT_REMOTE_100BTX_FD)\n> > +\t\t\tdata |= PHY_AUTO_NEG_100BTX_FD;\n> > +\t\tif (link & PORT_REMOTE_100BTX)\n> > +\t\t\tdata |= PHY_AUTO_NEG_100BTX;\n> > +\t\tif (link & PORT_REMOTE_10BT_FD)\n> > +\t\t\tdata |= PHY_AUTO_NEG_10BT_FD;\n> > +\t\tif (link & PORT_REMOTE_10BT)\n> > +\t\t\tdata |= PHY_AUTO_NEG_10BT;\n> > +\t\tbreak;\n> > +\tdefault:\n> > +\t\tprocessed = false;\n> > +\t\tbreak;\n> > +\t}\n> > +\tif (processed)\n> > +\t\t*val = data;\n> > +}\n> \n> Similar code will be needed by other drivers, right?\n> \n\nAlthough KSZ8795 and KSZ8895 may use the same code, the other\nchips will have different code.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xplMY1Jqgz9sBW\n\tfor ;\n\tSat, 9 Sep 2017 03:54:49 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S932432AbdIHRyi convert rfc822-to-8bit (ORCPT\n\t);\n\tFri, 8 Sep 2017 13:54:38 -0400","from esa5.microchip.iphmx.com ([216.71.150.166]:41294 \"EHLO\n\tesa5.microchip.iphmx.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S932336AbdIHRyh (ORCPT\n\t); Fri, 8 Sep 2017 13:54:37 -0400","from exsmtp03.microchip.com (HELO email.microchip.com)\n\t([198.175.253.49])\n\tby esa5.microchip.iphmx.com with ESMTP/TLS/DHE-RSA-AES256-SHA;\n\t08 Sep 2017 10:54:26 -0700","from CHN-SV-EXMX02.mchp-main.com ([fe80::7dfe:3761:863e:3963]) by\n\tchn-sv-exch03.mchp-main.com ([fe80::9916:1afa:df82:7a64%13]) with\n\tmapi id 14.03.0352.000; Fri, 8 Sep 2017 10:54:24 -0700"],"X-IronPort-AV":"E=Sophos;i=\"5.42,362,1500966000\"; d=\"scan'208\";a=\"4544326\"","From":"","To":"","CC":", , ,\n\t, ,\n\t, ,\n\t","Subject":"RE: [PATCH RFC 3/5] Add KSZ8795 switch driver","Thread-Topic":"[PATCH RFC 3/5] Add KSZ8795 switch driver","Thread-Index":"AdMoGw7chwKpH5nHQDyZLhGSoYClOwAA0svAACf0VAAAAsLgkA==","Date":"Fri, 8 Sep 2017 17:54:24 +0000","Message-ID":"<93AF473E2DA327428DE3D46B72B1E9FD41121E3D@CHN-SV-EXMX02.mchp-main.com>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>\n\t<20170908091856.GB17738@amd>","In-Reply-To":"<20170908091856.GB17738@amd>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[10.10.76.4]","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765569,"web_url":"http://patchwork.ozlabs.org/comment/1765569/","msgid":"<20170908183227.GJ25219@lunn.ch>","list_archive_url":null,"date":"2017-09-08T18:32:27","subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":13608,"url":"http://patchwork.ozlabs.org/api/people/13608/","name":"Andrew Lunn","email":"andrew@lunn.ch"},"content":"> > > @@ -0,0 +1,2066 @@\n> > > +/*\n> > > + * Microchip KSZ8795 switch driver\n> > > + *\n> > > + * Copyright (C) 2017 Microchip Technology Inc.\n> > > + *\tTristram Ha \n> > > + *\n> > > + * Permission to use, copy, modify, and/or distribute this software for any\n> > > + * purpose with or without fee is hereby granted, provided that the above\n> > > + * copyright notice and this permission notice appear in all copies.\n> > > + *\n> > > + * THE SOFTWARE IS PROVIDED \"AS IS\" AND THE AUTHOR DISCLAIMS ALL\n> > WARRANTIES\n> > > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES\n> > OF\n> > > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE\n> > LIABLE FOR\n> > > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY\n> > DAMAGES\n> > > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER\n> > IN AN\n> > > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,\n> > ARISING OUT OF\n> > > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.\n> > > + */\n> > \n> > This is not exactly GPL, right? But tagging below says it is\n> > GPL. Please fix one.\n> > \n> \n> This boilerplate paragraph was copied from the KSZ9477 driver, although I did\n> wonder why this was used.\n\nHi Tristram\n\nPlease can you talk to your legal people and see if this can be\nreplaced with the standard GPL text?\n\n> > > +\tfor (timeout = 1; timeout > 0; timeout--) {\n> > > +\t\tksz_read8(dev, REG_IND_MIB_CHECK, &check);\n> > > +\n> > > +\t\tif (check & MIB_COUNTER_VALID) {\n> > > +\t\t\tksz_read32(dev, REG_IND_DATA_LO, &data);\n> > > +\t\t\tif (addr < 2) {\n> > > +\t\t\t\tu64 total;\n> > > +\n> > > +\t\t\t\ttotal = check & MIB_TOTAL_BYTES_H;\n> > > +\t\t\t\ttotal <<= 32;\n> > > +\t\t\t\t*cnt += total;\n> > > +\t\t\t\t*cnt += data;\n> > > +\t\t\t\tif (check & MIB_COUNTER_OVERFLOW) {\n> > > +\t\t\t\t\ttotal = MIB_TOTAL_BYTES_H + 1;\n> > > +\t\t\t\t\ttotal <<= 32;\n> > > +\t\t\t\t\t*cnt += total;\n> > > +\t\t\t\t}\n> > > +\t\t\t} else {\n> > > +\t\t\t\tif (check & MIB_COUNTER_OVERFLOW)\n> > > +\t\t\t\t\t*cnt += MIB_PACKET_DROPPED + 1;\n> > > +\t\t\t\t*cnt += data & MIB_PACKET_DROPPED;\n> > > +\t\t\t}\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t}\n> > \n> > Why do you need a loop here? This is quite strange code. (And you have\n> > similar strangeness elsewhere. Please fix.)\n> > \n> \n> The MIB_COUNTER_VALID bit may be invalid on first read, although in slow\n> SPI speed it never happens. The timeout value should be increased to 2.\n\nMaybe timeout is the wrong name? There is nothing to do with time\nhere.\n \n> > > +static int valid_dyn_entry(struct ksz_device *dev, u8 *data)\n> > > +{\n> > > +\tint timeout = 100;\n> > > +\n> > > +\tdo {\n> > > +\t\tksz_read8(dev, REG_IND_DATA_CHECK, data);\n> > > +\t\ttimeout--;\n> > > +\t} while ((*data & DYNAMIC_MAC_TABLE_NOT_READY) && timeout);\n> > > +\n> > > +\t/* Entry is not ready for accessing. */\n> > > +\tif (*data & DYNAMIC_MAC_TABLE_NOT_READY) {\n> > > +\t\treturn 1;\n> > > +\t/* Entry is ready for accessing. */\n> > > +\t} else {\n> > > +\t\tksz_read8(dev, REG_IND_DATA_8, data);\n> > > +\n> > > +\t\t/* There is no valid entry in the table. */\n> > > +\t\tif (*data & DYNAMIC_MAC_TABLE_MAC_EMPTY)\n> > > +\t\t\treturn 2;\n> > > +\t}\n> > > +\treturn 0;\n> > > +}\n> > \n> > Normal calling convention is 0 / -ERROR, not 0,1,2.\n> >\n> \n> This is an internal function that is not returning any error. It just reports\n> different conditions so the calling function decides what to do.\n\nStill, best practice is to use standard error codes.\n \n Andrew","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpmCK2Y15z9sBZ\n\tfor ;\n\tSat, 9 Sep 2017 04:32:45 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1756634AbdIHScd (ORCPT );\n\tFri, 8 Sep 2017 14:32:33 -0400","from vps0.lunn.ch ([178.209.37.122]:33511 \"EHLO vps0.lunn.ch\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1756210AbdIHScc (ORCPT );\n\tFri, 8 Sep 2017 14:32:32 -0400","from andrew by vps0.lunn.ch with local (Exim 4.84_2)\n\t(envelope-from )\n\tid 1dqO4p-0002lt-Q0; Fri, 08 Sep 2017 20:32:27 +0200"],"Date":"Fri, 8 Sep 2017 20:32:27 +0200","From":"Andrew Lunn ","To":"Tristram.Ha@microchip.com","Cc":"pavel@ucw.cz, muvarov@gmail.com, nathan.leigh.conrad@gmail.com,\n\tvivien.didelot@savoirfairelinux.com, f.fainelli@gmail.com,\n\tnetdev@vger.kernel.org, linux-kernel@vger.kernel.org,\n\tWoojung.Huh@microchip.com","Subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","Message-ID":"<20170908183227.GJ25219@lunn.ch>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>\n\t<20170908091856.GB17738@amd>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41121E3D@CHN-SV-EXMX02.mchp-main.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<93AF473E2DA327428DE3D46B72B1E9FD41121E3D@CHN-SV-EXMX02.mchp-main.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765572,"web_url":"http://patchwork.ozlabs.org/comment/1765572/","msgid":"<9235D6609DB808459E95D78E17F2E43D40B1C835@CHN-SV-EXMX02.mchp-main.com>","list_archive_url":null,"date":"2017-09-08T18:35:30","subject":"RE: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":66705,"url":"http://patchwork.ozlabs.org/api/people/66705/","name":"","email":"Woojung.Huh@microchip.com"},"content":"> > > > @@ -0,0 +1,2066 @@\n> > > > +/*\n> > > > + * Microchip KSZ8795 switch driver\n> > > > + *\n> > > > + * Copyright (C) 2017 Microchip Technology Inc.\n> > > > + *\tTristram Ha \n> > > > + *\n> > > > + * Permission to use, copy, modify, and/or distribute this software for\n> any\n> > > > + * purpose with or without fee is hereby granted, provided that the\n> above\n> > > > + * copyright notice and this permission notice appear in all copies.\n> > > > + *\n> > > > + * THE SOFTWARE IS PROVIDED \"AS IS\" AND THE AUTHOR DISCLAIMS\n> ALL\n> > > WARRANTIES\n> > > > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED\n> WARRANTIES\n> > > OF\n> > > > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR\n> BE\n> > > LIABLE FOR\n> > > > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR\n> ANY\n> > > DAMAGES\n> > > > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS,\n> WHETHER\n> > > IN AN\n> > > > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,\n> > > ARISING OUT OF\n> > > > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS\n> SOFTWARE.\n> > > > + */\n> > >\n> > > This is not exactly GPL, right? But tagging below says it is\n> > > GPL. Please fix one.\n> > >\n> >\n> > This boilerplate paragraph was copied from the KSZ9477 driver, although I\n> did\n> > wonder why this was used.\n> \n> Hi Tristram\n> \n> Please can you talk to your legal people and see if this can be\n> replaced with the standard GPL text?\nThis should be replaced to GPL. These text copied from drivers/net/dsa/b53/*.\nWill submit patches of drivers/net/dsa/microchip/*\n\n- Woojung","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpmHx6Sh1z9s82\n\tfor ;\n\tSat, 9 Sep 2017 04:36:45 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1756786AbdIHSgT convert rfc822-to-8bit (ORCPT\n\t);\n\tFri, 8 Sep 2017 14:36:19 -0400","from esa5.microchip.iphmx.com ([216.71.150.166]:45768 \"EHLO\n\tesa5.microchip.iphmx.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1756717AbdIHSgR (ORCPT\n\t); Fri, 8 Sep 2017 14:36:17 -0400","from exsmtp01.microchip.com (HELO email.microchip.com)\n\t([198.175.253.37])\n\tby esa5.microchip.iphmx.com with ESMTP/TLS/AES128-SHA;\n\t08 Sep 2017 11:35:31 -0700","from CHN-SV-EXMX02.mchp-main.com ([fe80::7dfe:3761:863e:3963]) by\n\tCHN-SV-EXCH01.mchp-main.com ([fe80::9840:ffdf:ec5:1335%29]) with\n\tmapi id 14.03.0352.000; Fri, 8 Sep 2017 11:35:30 -0700"],"X-IronPort-AV":"E=Sophos;i=\"5.42,363,1500966000\"; d=\"scan'208\";a=\"4545675\"","From":"","To":", ","CC":", , ,\n\t, ,\n\t, ,\n\t","Subject":"RE: [PATCH RFC 3/5] Add KSZ8795 switch driver","Thread-Topic":"[PATCH RFC 3/5] Add KSZ8795 switch driver","Thread-Index":"AdMoGw7chwKpH5nHQDyZLhGSoYClOwAA0svAACf0VAAAAsLgkAAQkfOAAA6iVMA=","Date":"Fri, 8 Sep 2017 18:35:30 +0000","Message-ID":"<9235D6609DB808459E95D78E17F2E43D40B1C835@CHN-SV-EXMX02.mchp-main.com>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>\n\t<20170908091856.GB17738@amd>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41121E3D@CHN-SV-EXMX02.mchp-main.com>\n\t<20170908183227.GJ25219@lunn.ch>","In-Reply-To":"<20170908183227.GJ25219@lunn.ch>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[10.10.76.4]","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765632,"web_url":"http://patchwork.ozlabs.org/comment/1765632/","msgid":"<20170908215735.GF27428@amd>","list_archive_url":null,"date":"2017-09-08T21:57:35","subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":2109,"url":"http://patchwork.ozlabs.org/api/people/2109/","name":"Pavel Machek","email":"pavel@ucw.cz"},"content":"Hi!\n\n> > > +\tdefault:\n> > > +\t\tprocessed = false;\n> > > +\t\tbreak;\n> > > +\t}\n> > > +\tif (processed)\n> > > +\t\t*val = data;\n> > > +}\n> > \n> > Similar code will be needed by other drivers, right?\n> \n> Although KSZ8795 and KSZ8895 may use the same code, the other\n> chips will have different code.\n\nOk, please make sure code is shared between these two.\n\nThansk,\n\t\t\t\t\t\t\t\t\tPavel","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xprlz4W9zz9sRY\n\tfor ;\n\tSat, 9 Sep 2017 07:57:51 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1757228AbdIHV5i (ORCPT );\n\tFri, 8 Sep 2017 17:57:38 -0400","from atrey.karlin.mff.cuni.cz ([195.113.26.193]:59657 \"EHLO\n\tatrey.karlin.mff.cuni.cz\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1756914AbdIHV5h (ORCPT\n\t); Fri, 8 Sep 2017 17:57:37 -0400","by atrey.karlin.mff.cuni.cz (Postfix, from userid 512)\n\tid 11EB2824B0; Fri, 8 Sep 2017 23:57:36 +0200 (CEST)"],"Date":"Fri, 8 Sep 2017 23:57:35 +0200","From":"Pavel Machek ","To":"Tristram.Ha@microchip.com","Cc":"andrew@lunn.ch, muvarov@gmail.com, nathan.leigh.conrad@gmail.com,\n\tvivien.didelot@savoirfairelinux.com, f.fainelli@gmail.com,\n\tnetdev@vger.kernel.org, linux-kernel@vger.kernel.org,\n\tWoojung.Huh@microchip.com","Subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","Message-ID":"<20170908215735.GF27428@amd>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>\n\t<20170908091856.GB17738@amd>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41121E3D@CHN-SV-EXMX02.mchp-main.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"+ts6NCQ4mrNQIV8p\"","Content-Disposition":"inline","In-Reply-To":"<93AF473E2DA327428DE3D46B72B1E9FD41121E3D@CHN-SV-EXMX02.mchp-main.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765679,"web_url":"http://patchwork.ozlabs.org/comment/1765679/","msgid":"<93AF473E2DA327428DE3D46B72B1E9FD41123010@CHN-SV-EXMX02.mchp-main.com>","list_archive_url":null,"date":"2017-09-09T01:44:37","subject":"RE: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":72262,"url":"http://patchwork.ozlabs.org/api/people/72262/","name":"","email":"Tristram.Ha@microchip.com"},"content":"> -----Original Message-----\n> From: Pavel Machek [mailto:pavel@ucw.cz]\n> Sent: Friday, September 08, 2017 2:58 PM\n> To: Tristram Ha - C24268\n> Cc: andrew@lunn.ch; muvarov@gmail.com; nathan.leigh.conrad@gmail.com;\n> vivien.didelot@savoirfairelinux.com; f.fainelli@gmail.com;\n> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Woojung Huh -\n> C21699\n> Subject: Re: [PATCH RFC 3/5] Add KSZ8795 switch driver\n> \n> Hi!\n> \n> > > > +\tdefault:\n> > > > +\t\tprocessed = false;\n> > > > +\t\tbreak;\n> > > > +\t}\n> > > > +\tif (processed)\n> > > > +\t\t*val = data;\n> > > > +}\n> > >\n> > > Similar code will be needed by other drivers, right?\n> >\n> > Although KSZ8795 and KSZ8895 may use the same code, the other\n> > chips will have different code.\n> \n> Ok, please make sure code is shared between these two.\n\nThe exact function probably cannot be shared between KSZ8795\nand KSZ8895 drivers because although the register constants look\nthe same but their exact locations are different in the 2 chips.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpxpq6L4Cz9t16\n\tfor ;\n\tSat, 9 Sep 2017 11:45:39 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1757309AbdIIBpY convert rfc822-to-8bit (ORCPT\n\t);\n\tFri, 8 Sep 2017 21:45:24 -0400","from esa4.microchip.iphmx.com ([68.232.154.123]:36699 \"EHLO\n\tesa4.microchip.iphmx.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1757209AbdIIBpX (ORCPT\n\t); Fri, 8 Sep 2017 21:45:23 -0400","from smtpout.microchip.com (HELO email.microchip.com)\n\t([198.175.253.82])\n\tby esa4.microchip.iphmx.com with ESMTP/TLS/DHE-RSA-AES256-SHA;\n\t08 Sep 2017 18:44:37 -0700","from CHN-SV-EXMX02.mchp-main.com ([fe80::7dfe:3761:863e:3963]) by\n\tCHN-SV-EXCH05.mchp-main.com ([fe80::c1bf:7679:c1f8:4560%15]) with\n\tmapi id 14.03.0352.000; Fri, 8 Sep 2017 18:44:37 -0700"],"X-IronPort-AV":"E=Sophos;i=\"5.42,364,1500966000\"; d=\"scan'208\";a=\"6667043\"","From":"","To":"","CC":", , ,\n\t, ,\n\t, ,\n\t","Subject":"RE: [PATCH RFC 3/5] Add KSZ8795 switch driver","Thread-Topic":"[PATCH RFC 3/5] Add KSZ8795 switch driver","Thread-Index":"AdMoGw7chwKpH5nHQDyZLhGSoYClOwAA0svAACf0VAAAAsLgkAAXu/yAAAb8AcA=","Date":"Sat, 9 Sep 2017 01:44:37 +0000","Message-ID":"<93AF473E2DA327428DE3D46B72B1E9FD41123010@CHN-SV-EXMX02.mchp-main.com>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>\n\t<20170908091856.GB17738@amd>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41121E3D@CHN-SV-EXMX02.mchp-main.com>\n\t<20170908215735.GF27428@amd>","In-Reply-To":"<20170908215735.GF27428@amd>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[10.10.76.4]","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765816,"web_url":"http://patchwork.ozlabs.org/comment/1765816/","msgid":"<20170909154534.GA19117@lunn.ch>","list_archive_url":null,"date":"2017-09-09T15:45:34","subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":13608,"url":"http://patchwork.ozlabs.org/api/people/13608/","name":"Andrew Lunn","email":"andrew@lunn.ch"},"content":"On Sat, Sep 09, 2017 at 01:44:37AM +0000, Tristram.Ha@microchip.com wrote:\n> > -----Original Message-----\n> > From: Pavel Machek [mailto:pavel@ucw.cz]\n> > Sent: Friday, September 08, 2017 2:58 PM\n> > To: Tristram Ha - C24268\n> > Cc: andrew@lunn.ch; muvarov@gmail.com; nathan.leigh.conrad@gmail.com;\n> > vivien.didelot@savoirfairelinux.com; f.fainelli@gmail.com;\n> > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Woojung Huh -\n> > C21699\n> > Subject: Re: [PATCH RFC 3/5] Add KSZ8795 switch driver\n> > \n> > Hi!\n> > \n> > > > > +\tdefault:\n> > > > > +\t\tprocessed = false;\n> > > > > +\t\tbreak;\n> > > > > +\t}\n> > > > > +\tif (processed)\n> > > > > +\t\t*val = data;\n> > > > > +}\n> > > >\n> > > > Similar code will be needed by other drivers, right?\n> > >\n> > > Although KSZ8795 and KSZ8895 may use the same code, the other\n> > > chips will have different code.\n> > \n> > Ok, please make sure code is shared between these two.\n> \n> The exact function probably cannot be shared between KSZ8795\n> and KSZ8895 drivers because although the register constants look\n> the same but their exact locations are different in the 2 chips.\n\nYou need to be careful with such functions. If they look identical,\nsomebody will try to consolidate them into one. So either you need to\ndo the consolidation yourself in order to get it right, or you need to\nadd a comment about how it differs.\n\n Andrew","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xqJSQ2LsLz9t16\n\tfor ;\n\tSun, 10 Sep 2017 01:45:58 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1757573AbdIIPpk (ORCPT );\n\tSat, 9 Sep 2017 11:45:40 -0400","from vps0.lunn.ch ([178.209.37.122]:34388 \"EHLO vps0.lunn.ch\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1753865AbdIIPpj (ORCPT );\n\tSat, 9 Sep 2017 11:45:39 -0400","from andrew by vps0.lunn.ch with local (Exim 4.84_2)\n\t(envelope-from )\n\tid 1dqhws-000567-89; Sat, 09 Sep 2017 17:45:34 +0200"],"Date":"Sat, 9 Sep 2017 17:45:34 +0200","From":"Andrew Lunn ","To":"Tristram.Ha@microchip.com","Cc":"pavel@ucw.cz, muvarov@gmail.com, nathan.leigh.conrad@gmail.com,\n\tvivien.didelot@savoirfairelinux.com, f.fainelli@gmail.com,\n\tnetdev@vger.kernel.org, linux-kernel@vger.kernel.org,\n\tWoojung.Huh@microchip.com","Subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","Message-ID":"<20170909154534.GA19117@lunn.ch>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>\n\t<20170908091856.GB17738@amd>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41121E3D@CHN-SV-EXMX02.mchp-main.com>\n\t<20170908215735.GF27428@amd>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41123010@CHN-SV-EXMX02.mchp-main.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<93AF473E2DA327428DE3D46B72B1E9FD41123010@CHN-SV-EXMX02.mchp-main.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1770472,"web_url":"http://patchwork.ozlabs.org/comment/1770472/","msgid":"<93AF473E2DA327428DE3D46B72B1E9FD41124D5A@CHN-SV-EXMX02.mchp-main.com>","list_archive_url":null,"date":"2017-09-18T20:27:13","subject":"RE: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":72262,"url":"http://patchwork.ozlabs.org/api/people/72262/","name":"","email":"Tristram.Ha@microchip.com"},"content":"> > +/**\n> > + * Some counters do not need to be read too often because they are less\n> likely\n> > + * to increase much.\n> > + */\n> \n> What does comment mean? Are you caching statistics, and updating\n> different values at different rates?\n> \n\nThere are 34 counters. In normal case using generic bus I/O or PCI to read them\nis very quick, but the switch is mostly accessed using SPI, or even I2C. As the SPI\naccess is very slow and cannot run in interrupt context I keep worrying reading\nthe MIB counters in a loop for 5 or more ports will prevent other critical hardware\naccess from executing soon enough. These accesses can be getting 1588 PTP\ntimestamps and opening/closing ports. (RSTP Conformance Test sends test traffic\nto port supposed to be closed/opened after receiving specific RSTP BPDU.)\n\n> > +static const struct {\n> > +\tint interval;\n> > +\tchar string[ETH_GSTRING_LEN];\n> > +} mib_names[TOTAL_SWITCH_COUNTER_NUM] = {\n> > +\t{ 1, \"rx_hi\" },\n> > +\t{ 4, \"rx_undersize\" },\n> > +\t{ 4, \"rx_fragments\" },\n> > +\t{ 4, \"rx_oversize\" },\n> > +\t{ 4, \"rx_jabbers\" },\n> > +\t{ 4, \"rx_symbol_err\" },\n> > +\t{ 4, \"rx_crc_err\" },\n> > +\t{ 4, \"rx_align_err\" },\n> > +\t{ 4, \"rx_mac_ctrl\" },\n> > +\t{ 1, \"rx_pause\" },\n> > +\t{ 1, \"rx_bcast\" },\n> > +\t{ 1, \"rx_mcast\" },\n> > +\t{ 1, \"rx_ucast\" },\n> > +\t{ 2, \"rx_64_or_less\" },\n> > +\t{ 2, \"rx_65_127\" },\n> > +\t{ 2, \"rx_128_255\" },\n> > +\t{ 2, \"rx_256_511\" },\n> > +\t{ 2, \"rx_512_1023\" },\n> > +\t{ 2, \"rx_1024_1522\" },\n> > +\t{ 2, \"rx_1523_2000\" },\n> > +\t{ 2, \"rx_2001\" },\n> > +\t{ 1, \"tx_hi\" },\n> > +\t{ 4, \"tx_late_col\" },\n> > +\t{ 1, \"tx_pause\" },\n> > +\t{ 1, \"tx_bcast\" },\n> > +\t{ 1, \"tx_mcast\" },\n> > +\t{ 1, \"tx_ucast\" },\n> > +\t{ 4, \"tx_deferred\" },\n> > +\t{ 4, \"tx_total_col\" },\n> > +\t{ 4, \"tx_exc_col\" },\n> > +\t{ 4, \"tx_single_col\" },\n> > +\t{ 4, \"tx_mult_col\" },\n> > +\t{ 1, \"rx_total\" },\n> > +\t{ 1, \"tx_total\" },\n> > +\t{ 2, \"rx_discards\" },\n> > +\t{ 2, \"tx_discards\" },\n> > +};\n\n> > +{\n> > +\tu32 data;\n> > +\tu16 ctrl_addr;\n> > +\tu8 check;\n> > +\tint timeout;\n> > +\n> > +\tctrl_addr = addr + SWITCH_COUNTER_NUM * port;\n> > +\n> > +\tctrl_addr |= IND_ACC_TABLE(TABLE_MIB | TABLE_READ);\n> > +\tmutex_lock(&dev->stats_mutex);\n> > +\tksz_write16(dev, REG_IND_CTRL_0, ctrl_addr);\n> > +\n> > +\tfor (timeout = 1; timeout > 0; timeout--) {\n> \n> A rather short timeount!\n> \n> > +\t\tksz_read8(dev, REG_IND_MIB_CHECK, &check);\n> > +\n> > +\t\tif (check & MIB_COUNTER_VALID) {\n> > +\t\t\tksz_read32(dev, REG_IND_DATA_LO, &data);\n> > +\t\t\tif (check & MIB_COUNTER_OVERFLOW)\n> > +\t\t\t\t*cnt += MIB_COUNTER_VALUE + 1;\n> > +\t\t\t*cnt += data & MIB_COUNTER_VALUE;\n> > +\t\t\tbreak;\n> > +\t\t}\n> \n> Should there be a dev_err() here about timing out?\n> \n\nThe timeout value should be at least 2. Hardware datasheet says the\nvalid bit should be checked, but in my experience it is always there.\nMaybe a very slow SPI access speed can make that happen, but it is\npointless to run in slow speed if the system can run in the fastest speed\npossible.\n \n> > +static void ksz_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)\n> > +{\n> > +\tstruct ksz_port *port;\n> > +\tu8 ctrl;\n> > +\tu8 restart;\n> > +\tu8 link;\n> > +\tu8 speed;\n> > +\tu8 force;\n> > +\tu8 p = phy;\n> > +\tu16 data = 0;\n> > +\tint processed = true;\n> > +\n> > +\tport = &dev->ports[p];\n> > +\tswitch (reg) {\n> > +\tcase PHY_REG_CTRL:\n> > +\t\tksz_pread8(dev, p, P_LOCAL_CTRL, &ctrl);\n> > +\t\tksz_pread8(dev, p, P_NEG_RESTART_CTRL, &restart);\n> > +\t\tksz_pread8(dev, p, P_SPEED_STATUS, &speed);\n> > +\t\tksz_pread8(dev, p, P_FORCE_CTRL, &force);\n> > +\t\tif (restart & PORT_PHY_LOOPBACK)\n> > +\t\t\tdata |= PHY_LOOPBACK;\n> > +\t\tif (force & PORT_FORCE_100_MBIT)\n> > +\t\t\tdata |= PHY_SPEED_100MBIT;\n> > +\t\tif (!(force & PORT_AUTO_NEG_DISABLE))\n> > +\t\t\tdata |= PHY_AUTO_NEG_ENABLE;\n> > +\t\tif (restart & PORT_POWER_DOWN)\n> > +\t\t\tdata |= PHY_POWER_DOWN;\n> \n> Same questions i asked Pavel\n> \n> Is there a way to access the PHY registers over SPI? The datasheet\n> suggests there is, but it does not say how, as far as i can tell.\n> \n> Ideally, we want to use just a normal PHY driver.\n>\n\nThe switch does not have actual PHY registers when used in SPI mode,\nso the PHY register access has to be simulated.\n\nA side-note is the PHY device id returned will pair the PHY with one of\nthe Micrel PHYs defined in the kernel. I just found out the new phylib\ncode removed the Asymmetric/Symmetric Pause support in the PHY\ndriver and let the MAC driver indicate the support. This is reasonable\nas the MAC is responsible for sending and processing PAUSE frames.\nHowever, the PHY in the switch port is not connected to the MAC.\nIs there a way to indicate this support in the DSA model?\n\nIdeally the switch should have control over which Pause mode should\nbe enabled. Generally Asymmetric Pause is used in Gigabit switch, and\nflow control is better disabled if some other traffic shaping is used.\n\n> > +static void ksz_get_ethtool_stats(struct dsa_switch *ds, int port,\n> > +\t\t\t\t uint64_t *buf)\n> > +{\n> > +\tstruct ksz_device *dev = ds->priv;\n> > +\tstruct ksz_port_mib *mib;\n> > +\tstruct ksz_port_mib_info *info;\n> > +\tint i;\n> > +\n> > +\tmib = &dev->ports[port].mib;\n> > +\n> > +\tspin_lock(&dev->mib_read_lock);\n> > +\tfor (i = 0; i < TOTAL_SWITCH_COUNTER_NUM; i++) {\n> > +\t\tinfo = &mib->info[i];\n> > +\t\tbuf[i] = info->counter;\n> > +\t\tinfo->read_cnt += info->read_max;\n> > +\t}\n> > +\tspin_unlock(&dev->mib_read_lock);\n> > +\n> > +\tmib->ctrl = 1;\n> > +\tschedule_work(&dev->mib_read);\n> \n> Humm. I want to take a closer look at this caching code...\n\nThe switch needs to read MIB counters periodically as the counters\nwill overflow when there are lots of traffic. The current\nimplementation is to read all ports every second but not all at the\nsame time as the operation may block other critical hardware\nregister access.\n\nThe idea is if the users check the MIB counters once they will check\nthem again very soon to note which counter has been increased.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xwyH43740z9s78\n\tfor ;\n\tTue, 19 Sep 2017 06:27:28 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751378AbdIRU1Q convert rfc822-to-8bit (ORCPT\n\t);\n\tMon, 18 Sep 2017 16:27:16 -0400","from esa5.microchip.iphmx.com ([216.71.150.166]:58346 \"EHLO\n\tesa5.microchip.iphmx.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750714AbdIRU1P (ORCPT\n\t); Mon, 18 Sep 2017 16:27:15 -0400","from exsmtp02.microchip.com (HELO email.microchip.com)\n\t([198.175.253.38])\n\tby esa5.microchip.iphmx.com with ESMTP/TLS/DHE-RSA-AES256-SHA;\n\t18 Sep 2017 13:27:15 -0700","from CHN-SV-EXMX02.mchp-main.com ([fe80::7dfe:3761:863e:3963]) by\n\tCHN-SV-EXCH02.mchp-main.com ([::1]) with mapi id 14.03.0352.000;\n\tMon, 18 Sep 2017 13:27:14 -0700"],"X-IronPort-AV":"E=Sophos;i=\"5.42,414,1500966000\"; d=\"scan'208\";a=\"4802394\"","From":"","To":"","CC":", , ,\n\t, ,\n\t, ,\n\t","Subject":"RE: [PATCH RFC 3/5] Add KSZ8795 switch driver","Thread-Topic":"[PATCH RFC 3/5] Add KSZ8795 switch driver","Thread-Index":"AdMoGw7chwKpH5nHQDyZLhGSoYClOwAA0svAABGDyIACFLHiAA==","Date":"Mon, 18 Sep 2017 20:27:13 +0000","Message-ID":"<93AF473E2DA327428DE3D46B72B1E9FD41124D5A@CHN-SV-EXMX02.mchp-main.com>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>\n\t<20170907223625.GW11248@lunn.ch>","In-Reply-To":"<20170907223625.GW11248@lunn.ch>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[10.10.215.90]","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1777102,"web_url":"http://patchwork.ozlabs.org/comment/1777102/","msgid":"<20170928152445.GC2482@amd>","list_archive_url":null,"date":"2017-09-28T15:24:45","subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":2109,"url":"http://patchwork.ozlabs.org/api/people/2109/","name":"Pavel Machek","email":"pavel@ucw.cz"},"content":"> > > > Similar code will be needed by other drivers, right?\n> > >\n> > > Although KSZ8795 and KSZ8895 may use the same code, the other\n> > > chips will have different code.\n> > \n> > Ok, please make sure code is shared between these two.\n> \n> The exact function probably cannot be shared between KSZ8795\n> and KSZ8895 drivers because although the register constants look\n> the same but their exact locations are different in the 2 chips.\n\nPut the code into header as static inline and include it from both\nplaces?\n\n\t\t\t\t\t\t\t\t\tPavel","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y2z5T5FlCz9tX5\n\tfor ;\n\tFri, 29 Sep 2017 01:25:01 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753321AbdI1PYt (ORCPT );\n\tThu, 28 Sep 2017 11:24:49 -0400","from atrey.karlin.mff.cuni.cz ([195.113.26.193]:48991 \"EHLO\n\tatrey.karlin.mff.cuni.cz\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752059AbdI1PYs (ORCPT\n\t); Thu, 28 Sep 2017 11:24:48 -0400","by atrey.karlin.mff.cuni.cz (Postfix, from userid 512)\n\tid 40990824A6; Thu, 28 Sep 2017 17:24:47 +0200 (CEST)"],"Date":"Thu, 28 Sep 2017 17:24:45 +0200","From":"Pavel Machek ","To":"Tristram.Ha@microchip.com","Cc":"andrew@lunn.ch, muvarov@gmail.com, nathan.leigh.conrad@gmail.com,\n\tvivien.didelot@savoirfairelinux.com, f.fainelli@gmail.com,\n\tnetdev@vger.kernel.org, linux-kernel@vger.kernel.org,\n\tWoojung.Huh@microchip.com","Subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","Message-ID":"<20170928152445.GC2482@amd>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>\n\t<20170908091856.GB17738@amd>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41121E3D@CHN-SV-EXMX02.mchp-main.com>\n\t<20170908215735.GF27428@amd>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41123010@CHN-SV-EXMX02.mchp-main.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"vOmOzSkFvhd7u8Ms\"","Content-Disposition":"inline","In-Reply-To":"<93AF473E2DA327428DE3D46B72B1E9FD41123010@CHN-SV-EXMX02.mchp-main.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1777229,"web_url":"http://patchwork.ozlabs.org/comment/1777229/","msgid":"<20170928184059.GA2825@amd>","list_archive_url":null,"date":"2017-09-28T18:40:59","subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":2109,"url":"http://patchwork.ozlabs.org/api/people/2109/","name":"Pavel Machek","email":"pavel@ucw.cz"},"content":"Hi!\n\nOn Mon 2017-09-18 20:27:13, Tristram.Ha@microchip.com wrote:\n> > > +/**\n> > > + * Some counters do not need to be read too often because they are less\n> > likely\n> > > + * to increase much.\n> > > + */\n> > \n> > What does comment mean? Are you caching statistics, and updating\n> > different values at different rates?\n> > \n> \n> There are 34 counters. In normal case using generic bus I/O or PCI to read them\n> is very quick, but the switch is mostly accessed using SPI, or even I2C. As the SPI\n> access is very slow and cannot run in interrupt context I keep worrying reading\n> the MIB counters in a loop for 5 or more ports will prevent other critical hardware\n> access from executing soon enough. These accesses can be getting 1588 PTP\n> timestamps and opening/closing ports. (RSTP Conformance Test sends test traffic\n> to port supposed to be closed/opened after receiving specific RSTP\n> BPDU.)\n\nHmm. Ok, interesting.\n\nI wonder how well this is going to work if userspace actively 'does\nsomething' with the switch.\n\nIt seems to me that even if your statistics code is careful not to do\n'a lot' of accesses at the same time, userspace can use other parts of\nthe driver to do the same, and thus cause same unwanted effects...\n\t\t\t\t\t\t\t\t\tPavel","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y33S90MPKz9tXP\n\tfor ;\n\tFri, 29 Sep 2017 04:41:29 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1750981AbdI1SlR (ORCPT );\n\tThu, 28 Sep 2017 14:41:17 -0400","from atrey.karlin.mff.cuni.cz ([195.113.26.193]:55844 \"EHLO\n\tatrey.karlin.mff.cuni.cz\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750763AbdI1SlQ (ORCPT\n\t); Thu, 28 Sep 2017 14:41:16 -0400","by atrey.karlin.mff.cuni.cz (Postfix, from userid 512)\n\tid 73720824A5; Thu, 28 Sep 2017 20:41:14 +0200 (CEST)"],"Date":"Thu, 28 Sep 2017 20:40:59 +0200","From":"Pavel Machek ","To":"Tristram.Ha@microchip.com","Cc":"andrew@lunn.ch, muvarov@gmail.com, nathan.leigh.conrad@gmail.com,\n\tvivien.didelot@savoirfairelinux.com, f.fainelli@gmail.com,\n\tnetdev@vger.kernel.org, linux-kernel@vger.kernel.org,\n\tWoojung.Huh@microchip.com","Subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","Message-ID":"<20170928184059.GA2825@amd>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>\n\t<20170907223625.GW11248@lunn.ch>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41124D5A@CHN-SV-EXMX02.mchp-main.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"HcAYCG3uE/tztfnV\"","Content-Disposition":"inline","In-Reply-To":"<93AF473E2DA327428DE3D46B72B1E9FD41124D5A@CHN-SV-EXMX02.mchp-main.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1777232,"web_url":"http://patchwork.ozlabs.org/comment/1777232/","msgid":"<329de4cb-f06a-89b7-3cbd-e67493ffb067@gmail.com>","list_archive_url":null,"date":"2017-09-28T18:45:11","subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":2800,"url":"http://patchwork.ozlabs.org/api/people/2800/","name":"Florian Fainelli","email":"f.fainelli@gmail.com"},"content":"On 09/28/2017 11:40 AM, Pavel Machek wrote:\n> Hi!\n> \n> On Mon 2017-09-18 20:27:13, Tristram.Ha@microchip.com wrote:\n>>>> +/**\n>>>> + * Some counters do not need to be read too often because they are less\n>>> likely\n>>>> + * to increase much.\n>>>> + */\n>>>\n>>> What does comment mean? Are you caching statistics, and updating\n>>> different values at different rates?\n>>>\n>>\n>> There are 34 counters. In normal case using generic bus I/O or PCI to read them\n>> is very quick, but the switch is mostly accessed using SPI, or even I2C. As the SPI\n>> access is very slow and cannot run in interrupt context I keep worrying reading\n>> the MIB counters in a loop for 5 or more ports will prevent other critical hardware\n>> access from executing soon enough. These accesses can be getting 1588 PTP\n>> timestamps and opening/closing ports. (RSTP Conformance Test sends test traffic\n>> to port supposed to be closed/opened after receiving specific RSTP\n>> BPDU.)\n> \n> Hmm. Ok, interesting.\n> \n> I wonder how well this is going to work if userspace actively 'does\n> something' with the switch.\n> \n> It seems to me that even if your statistics code is careful not to do\n> 'a lot' of accesses at the same time, userspace can use other parts of\n> the driver to do the same, and thus cause same unwanted effects...\n\nA few switches have a MIB snapshot feature that is implemented such that\naccessing the snapshot does not hog the remainder of the switch\nregisters, is this something possible on KSZ switches?\n\nTangential: net-next is currently open, so now would be a good time to\nsend a revised version of your patch series to target possibly 4.15 with\nan initial implementation. Please fix the cover-letter and patch\nthreading such that they look like the following:\n\n[PATCH 0/X]\n [PATCH 1/X]\n [PATCH 2/X]\n etc..\n\nRight now this shows up as separate emails/patches and this is very\nannoying to follow as a thread.\n\nThank you","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"HE23JxBV\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y33Xr5kLJz9tXp\n\tfor ;\n\tFri, 29 Sep 2017 04:45:32 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751346AbdI1SpV (ORCPT );\n\tThu, 28 Sep 2017 14:45:21 -0400","from mail-qk0-f182.google.com ([209.85.220.182]:49623 \"EHLO\n\tmail-qk0-f182.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750951AbdI1SpU (ORCPT\n\t); Thu, 28 Sep 2017 14:45:20 -0400","by mail-qk0-f182.google.com with SMTP id u67so2346460qkg.6;\n\tThu, 28 Sep 2017 11:45:19 -0700 (PDT)","from [10.112.156.244] ([192.19.255.250])\n\tby smtp.googlemail.com with ESMTPSA id\n\tt65sm1436781qke.12.2017.09.28.11.45.12\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tThu, 28 Sep 2017 11:45:17 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20161025;\n\th=subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=t1vgjVCxdppQPOAb0QdBCI2Jwes0gzdgkBZX1PGrE70=;\n\tb=HE23JxBVltIjGrV57YFpJydW1RALHAeFGbBcLfV9CgUeasev9WlzrY1t2FmlZuZ4x2\n\tUjTUi/7shCsWMfM6Uagl4+O5GU8fLupTkOYLm1j+pcTXOqNtO8qd33Y9YRS+ueDzEt99\n\tjfPqaR/Zcha8+DLrQxoSGicc/mwOTGiHkWdlBGWl2VWQgzzGQnWHFbosSlByDjfmmMkG\n\tJNvI+LUZm4URmm2e8ax8UCWHYr+bzTLe0arxP8jNsGaaMai0GyAzbYgEDwZprsy6Cif7\n\tnN3+bgwZve0tUIUsciIaRMaFVatbGOPLgqkgaWSspVdZQ/4koi937FkLEJ/ss8f60sMu\n\tL6Fw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:cc:references:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=t1vgjVCxdppQPOAb0QdBCI2Jwes0gzdgkBZX1PGrE70=;\n\tb=ENP4jht+bY3nCs9/vyt3VM0foMPFrNvNYwW4cRzuCT3kqvMCFGkxNlD8PA975403nH\n\tD6FZdWH0Hijcn61PGVmlyLFpelJoxVIhLlfBqvbUW2srzIhpmS95Io6JT5rFUV1bDBeC\n\tTMh+4eubZUOlfTvN6CaoRg+TAGyW3HrFSQUkWflWNyGW/3z95zez5RbBSpnt5fR0hEDG\n\tmyXTsSt6nDA2QI//cFlpT3dVMIE23q084oE08XSCaUKvLwdvLCwcyAcUS0lkRDybXxrV\n\tNzXe4BMoI2T9TD0rbcfS1iQ7A7FjuFEokV6tsAl9ocC0qRgzYvrxTDYhXwLGEXM5vrUk\n\tX4GQ==","X-Gm-Message-State":"AMCzsaXHrckeP1ryJi5we4tZrk37OzeFUKVCLsoFcwWnMyzcnsGKrEgp\n\tgsRYDd2T9/snRiEodsHK9hM=","X-Google-Smtp-Source":"AOwi7QCk6rAygyolDXZ9lZiQeAz6cuWvvTVMfn7wPhG1Cxz61nt4TXFI1566mNHQclUeaDzn8T6H6w==","X-Received":"by 10.55.197.194 with SMTP id k63mr2573354qkl.208.1506624319181; \n\tThu, 28 Sep 2017 11:45:19 -0700 (PDT)","Subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","To":"Pavel Machek , Tristram.Ha@microchip.com","Cc":"andrew@lunn.ch, muvarov@gmail.com, nathan.leigh.conrad@gmail.com,\n\tvivien.didelot@savoirfairelinux.com, netdev@vger.kernel.org,\n\tlinux-kernel@vger.kernel.org, Woojung.Huh@microchip.com","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>\n\t<20170907223625.GW11248@lunn.ch>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41124D5A@CHN-SV-EXMX02.mchp-main.com>\n\t<20170928184059.GA2825@amd>","From":"Florian Fainelli ","Message-ID":"<329de4cb-f06a-89b7-3cbd-e67493ffb067@gmail.com>","Date":"Thu, 28 Sep 2017 11:45:11 -0700","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170928184059.GA2825@amd>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1777251,"web_url":"http://patchwork.ozlabs.org/comment/1777251/","msgid":"<20170928193416.GH14940@lunn.ch>","list_archive_url":null,"date":"2017-09-28T19:34:16","subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":13608,"url":"http://patchwork.ozlabs.org/api/people/13608/","name":"Andrew Lunn","email":"andrew@lunn.ch"},"content":"On Mon, Sep 18, 2017 at 08:27:13PM +0000, Tristram.Ha@microchip.com wrote:\n> > > +/**\n> > > + * Some counters do not need to be read too often because they are less\n> > likely\n> > > + * to increase much.\n> > > + */\n> > \n> > What does comment mean? Are you caching statistics, and updating\n> > different values at different rates?\n> > \n> \n> There are 34 counters. In normal case using generic bus I/O or PCI to read them\n> is very quick, but the switch is mostly accessed using SPI, or even I2C. As the SPI\n> access is very slow.\n\nHow slow is it? The Marvell switches all use MDIO. It is probably a\nbit faster than I2C, but it is a lot slower than MMIO or PCI.\n\nethtool -S lan0 takes about 25ms.\n\nNo other driver does caching. So i'm hesitant to add one which does.\n\n> These accesses can be getting 1588 PTP timestamps and opening/closing ports.\n\nYou could drop the mutex between each statistic read, so allowing\nsomething else access to the switch. That should reduce the jitter PTP\nexperiences.\n\n\tAndrew","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y34dR63clz9sRm\n\tfor ;\n\tFri, 29 Sep 2017 05:34:35 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751403AbdI1TeX (ORCPT );\n\tThu, 28 Sep 2017 15:34:23 -0400","from vps0.lunn.ch ([185.16.172.187]:60391 \"EHLO vps0.lunn.ch\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1750977AbdI1TeV (ORCPT );\n\tThu, 28 Sep 2017 15:34:21 -0400","from andrew by vps0.lunn.ch with local (Exim 4.84_2)\n\t(envelope-from )\n\tid 1dxeZc-000631-9l; Thu, 28 Sep 2017 21:34:16 +0200"],"Date":"Thu, 28 Sep 2017 21:34:16 +0200","From":"Andrew Lunn ","To":"Tristram.Ha@microchip.com","Cc":"muvarov@gmail.com, pavel@ucw.cz, nathan.leigh.conrad@gmail.com,\n\tvivien.didelot@savoirfairelinux.com, f.fainelli@gmail.com,\n\tnetdev@vger.kernel.org, linux-kernel@vger.kernel.org,\n\tWoojung.Huh@microchip.com","Subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","Message-ID":"<20170928193416.GH14940@lunn.ch>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>\n\t<20170907223625.GW11248@lunn.ch>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41124D5A@CHN-SV-EXMX02.mchp-main.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<93AF473E2DA327428DE3D46B72B1E9FD41124D5A@CHN-SV-EXMX02.mchp-main.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1777436,"web_url":"http://patchwork.ozlabs.org/comment/1777436/","msgid":"<063D6719AE5E284EB5DD2968C1650D6DD0084EDC@AcuExch.aculab.com>","list_archive_url":null,"date":"2017-09-29T09:14:26","subject":"RE: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":6689,"url":"http://patchwork.ozlabs.org/api/people/6689/","name":"David Laight","email":"David.Laight@ACULAB.COM"},"content":"From: Andrew Lunn\n> Sent: 28 September 2017 20:34\n...\n> > There are 34 counters. In normal case using generic bus I/O or PCI to read them\n> > is very quick, but the switch is mostly accessed using SPI, or even I2C. As the SPI\n> > access is very slow.\n> \n> How slow is it? The Marvell switches all use MDIO. It is probably a\n> bit faster than I2C, but it is a lot slower than MMIO or PCI.\n> \n> ethtool -S lan0 takes about 25ms.\n\nIs the SPI access software bit-banged?\nDoing that with software delays isn't friendly to the rest of the system.\n(Hardware guys please note...)\n\nOne possibility is to rate-limit the stats reading.\nThen an application cannot completely 'hog' the SPI bandwidth.\n\n\tDavid","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y3Qqp3rBjz9t2Q\n\tfor ;\n\tFri, 29 Sep 2017 19:14:46 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752134AbdI2JOc convert rfc822-to-8bit (ORCPT\n\t);\n\tFri, 29 Sep 2017 05:14:32 -0400","from smtp-out6.electric.net ([192.162.217.192]:50247 \"EHLO\n\tsmtp-out6.electric.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752065AbdI2JOa (ORCPT\n\t); Fri, 29 Sep 2017 05:14:30 -0400","from 1dxrNK-0000h8-Tf by out6a.electric.net with emc1-ok (Exim\n\t4.87) (envelope-from )\n\tid 1dxrNL-0000ok-Ur; Fri, 29 Sep 2017 02:14:27 -0700","by emcmailer; Fri, 29 Sep 2017 02:14:27 -0700","from [156.67.243.126] (helo=AcuExch.aculab.com)\n\tby out6a.electric.net with esmtps (TLSv1:AES128-SHA:128)\n\t(Exim 4.87) (envelope-from )\n\tid 1dxrNK-0000h8-Tf; Fri, 29 Sep 2017 02:14:26 -0700","from ACUEXCH.Aculab.com ([::1]) by AcuExch.aculab.com ([::1]) with\n\tmapi id 14.03.0123.003; Fri, 29 Sep 2017 10:14:27 +0100"],"From":"David Laight ","To":"'Andrew Lunn' ,\n\t\"Tristram.Ha@microchip.com\" ","CC":"\"muvarov@gmail.com\" , \"pavel@ucw.cz\" ,\n\t\"nathan.leigh.conrad@gmail.com\" ,\n\t\"vivien.didelot@savoirfairelinux.com\" \n\t,\n\t\"f.fainelli@gmail.com\" ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\t\"Woojung.Huh@microchip.com\" ","Subject":"RE: [PATCH RFC 3/5] Add KSZ8795 switch driver","Thread-Topic":"[PATCH RFC 3/5] Add KSZ8795 switch driver","Thread-Index":"AQHTOJDPlP6ChEAuCUy7qr2HuLws76LLlACA","Date":"Fri, 29 Sep 2017 09:14:26 +0000","Message-ID":"<063D6719AE5E284EB5DD2968C1650D6DD0084EDC@AcuExch.aculab.com>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>\n\t<20170907223625.GW11248@lunn.ch>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41124D5A@CHN-SV-EXMX02.mchp-main.com>\n\t<20170928193416.GH14940@lunn.ch>","In-Reply-To":"<20170928193416.GH14940@lunn.ch>","Accept-Language":"en-GB, en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[10.202.99.200]","Content-Type":"text/plain; charset=\"Windows-1252\"","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","X-Outbound-IP":"156.67.243.126","X-Env-From":"David.Laight@ACULAB.COM","X-Proto":"esmtps","X-Revdns":"","X-HELO":"AcuExch.aculab.com","X-TLS":"TLSv1:AES128-SHA:128","X-Authenticated_ID":"","X-PolicySMART":"3396946, 3397078","X-Virus-Status":["Scanned by VirusSMART (c)","Scanned by VirusSMART (s)"],"Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1777512,"web_url":"http://patchwork.ozlabs.org/comment/1777512/","msgid":"<20170929121201.GD19710@lunn.ch>","list_archive_url":null,"date":"2017-09-29T12:12:01","subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":13608,"url":"http://patchwork.ozlabs.org/api/people/13608/","name":"Andrew Lunn","email":"andrew@lunn.ch"},"content":"On Fri, Sep 29, 2017 at 09:14:26AM +0000, David Laight wrote:\n> From: Andrew Lunn\n> > Sent: 28 September 2017 20:34\n> ...\n> > > There are 34 counters. In normal case using generic bus I/O or PCI to read them\n> > > is very quick, but the switch is mostly accessed using SPI, or even I2C. As the SPI\n> > > access is very slow.\n> > \n> > How slow is it? The Marvell switches all use MDIO. It is probably a\n> > bit faster than I2C, but it is a lot slower than MMIO or PCI.\n> > \n> > ethtool -S lan0 takes about 25ms.\n> \n> Is the SPI access software bit-banged?\n\nThat will depend on the board design. I've used mdio bit banging, and\nthat was painfully slow for stats.\n\nBut we should primarily think about average hardware. It is going to\nhave hardware SPI or I2C. If statistics reading with hardware I2C is\nreasonable, i would avoid caching, and just ensure other accesses are\npermitted between individual statistic reads.\n\nIt also requires Microchip also post new code. They have been very\nsilent for quite a while....\n\n\t Andrew","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y3Vn40FDHz9t3s\n\tfor ;\n\tFri, 29 Sep 2017 22:12:40 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751004AbdI2MM2 (ORCPT );\n\tFri, 29 Sep 2017 08:12:28 -0400","from vps0.lunn.ch ([185.16.172.187]:32839 \"EHLO vps0.lunn.ch\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1750715AbdI2MM1 (ORCPT );\n\tFri, 29 Sep 2017 08:12:27 -0400","from andrew by vps0.lunn.ch with local (Exim 4.84_2)\n\t(envelope-from )\n\tid 1dxu9B-0002tB-27; Fri, 29 Sep 2017 14:12:01 +0200"],"Date":"Fri, 29 Sep 2017 14:12:01 +0200","From":"Andrew Lunn ","To":"David Laight ","Cc":"\"Tristram.Ha@microchip.com\" ,\n\t\"muvarov@gmail.com\" , \"pavel@ucw.cz\" ,\n\t\"nathan.leigh.conrad@gmail.com\" ,\n\t\"vivien.didelot@savoirfairelinux.com\" \n\t,\n\t\"f.fainelli@gmail.com\" ,\n\t\"netdev@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\t\"Woojung.Huh@microchip.com\" ","Subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","Message-ID":"<20170929121201.GD19710@lunn.ch>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>\n\t<20170907223625.GW11248@lunn.ch>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41124D5A@CHN-SV-EXMX02.mchp-main.com>\n\t<20170928193416.GH14940@lunn.ch>\n\t<063D6719AE5E284EB5DD2968C1650D6DD0084EDC@AcuExch.aculab.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<063D6719AE5E284EB5DD2968C1650D6DD0084EDC@AcuExch.aculab.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1777709,"web_url":"http://patchwork.ozlabs.org/comment/1777709/","msgid":"<93AF473E2DA327428DE3D46B72B1E9FD4112CB89@CHN-SV-EXMX02.mchp-main.com>","list_archive_url":null,"date":"2017-09-29T18:24:36","subject":"RE: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":72262,"url":"http://patchwork.ozlabs.org/api/people/72262/","name":"","email":"Tristram.Ha@microchip.com"},"content":"The actual SPI access performance will depend on the SPI host controller.\nThe SPI access speed, ranging from 12 MHz to 50 MHz depending on the\nchip, is a factor but the performance of the SPI host controller is more\nimportant. Generally the SPI host controller scales down the clock by a\nfactor of 2. So if the maximum 50 Mhz does not work for the chip the next\nspeed is 25 Mhz. Most of the SPI host controllers work in range of 20-30 Mhz\nwith Microchip SPI switches.\n\nFor example, Raspberry Pi 2 has 2 SPI host controllers. The new code uses the\nnewer host controller which has better performance even when running in\nslower SPI speed.\n\nIt is hard to measure the actual SPI performance of the switch, but for SPI\nEthernet controller the performance can be viewed by running throughput test\nas SPI is responsible for passing network frames between system and device.\n\nThe ODROID C1 (A Raspberry Pi like SoC) has the best SPI performance, even not\nrunning in the highest SPI speed.\n\nTypical SPI register read takes about 120 microseconds.\n\nNow back to my concern about SPI access. It is not accessible in interrupt context.\nEven in a timer callback a work queue has to be scheduled to program the hardware\nregisters. My concern is if a task is already running with SPI access to a lot of registers\nlike reading the 32 MIB counters in every port of the switch, another register access\nhas to wait until they are finished. This normally does not pose a problem in\nregular switch operation, but there are some situations it will create a problem.\n\nOne of the situations is running RSTP Conformance Test. The test case sends a BPDU\nto open/close the port and then send traffic to test if the port is really opened/closed.\nFor software implementation which receives the BPDU and all network traffic it is\nreasonable to expect the software opens/closes the port and then can regulate the\nnetwork traffic whatever it wants, but for a hardware implementation which programs\na register to open/close the port then it is critical this register write can be executed as\nsoon as possible.\n\nAnother situation is getting the PTP transmit timestamp of a PTP event message.\nThe Microchip PTP switch uses registers to store the Sync, Delay_Req, and Pdelay_Resp\ntimestamps. If this register is not read as soon as possible and another message of the\nsame type is sent, the last timestamp is lost. Software can regulate the sending of\nthese messages and this situation does not happen in normal operation. But in a stress\ntest this PTP operation definitely cannot handle it.\n\nI know this MIB counter reading implementation cannot guarantee those urgent register\naccess to happen promptly, but it minimizes the chance of blocking those accesses in normal\noperation.\n\n\n> -----Original Message-----\n> From: Andrew Lunn [mailto:andrew@lunn.ch]\n> Sent: Friday, September 29, 2017 5:12 AM\n> To: David Laight\n> Cc: Tristram Ha - C24268; muvarov@gmail.com; pavel@ucw.cz;\n> nathan.leigh.conrad@gmail.com; vivien.didelot@savoirfairelinux.com;\n> f.fainelli@gmail.com; netdev@vger.kernel.org; linux-\n> kernel@vger.kernel.org; Woojung Huh - C21699\n> Subject: Re: [PATCH RFC 3/5] Add KSZ8795 switch driver\n> \n> On Fri, Sep 29, 2017 at 09:14:26AM +0000, David Laight wrote:\n> > From: Andrew Lunn\n> > > Sent: 28 September 2017 20:34\n> > ...\n> > > > There are 34 counters. In normal case using generic bus I/O or PCI to\n> read them\n> > > > is very quick, but the switch is mostly accessed using SPI, or even I2C.\n> As the SPI\n> > > > access is very slow.\n> > >\n> > > How slow is it? The Marvell switches all use MDIO. It is probably a\n> > > bit faster than I2C, but it is a lot slower than MMIO or PCI.\n> > >\n> > > ethtool -S lan0 takes about 25ms.\n> >\n> > Is the SPI access software bit-banged?\n> \n> That will depend on the board design. I've used mdio bit banging, and\n> that was painfully slow for stats.\n> \n> But we should primarily think about average hardware. It is going to\n> have hardware SPI or I2C. If statistics reading with hardware I2C is\n> reasonable, i would avoid caching, and just ensure other accesses are\n> permitted between individual statistic reads.\n> \n> It also requires Microchip also post new code. They have been very\n> silent for quite a while....\n> \n> \t Andrew","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y3g2X6ZWkz9t3C\n\tfor ;\n\tSat, 30 Sep 2017 04:24:52 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752412AbdI2SYk convert rfc822-to-8bit (ORCPT\n\t);\n\tFri, 29 Sep 2017 14:24:40 -0400","from esa1.microchip.iphmx.com ([68.232.147.91]:22010 \"EHLO\n\tesa1.microchip.iphmx.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752172AbdI2SYi (ORCPT\n\t); Fri, 29 Sep 2017 14:24:38 -0400","from smtpout.microchip.com (HELO email.microchip.com)\n\t([198.175.253.82])\n\tby esa1.microchip.iphmx.com with ESMTP/TLS/DHE-RSA-AES256-SHA;\n\t29 Sep 2017 11:24:38 -0700","from CHN-SV-EXMX02.mchp-main.com ([fe80::7dfe:3761:863e:3963]) by\n\tCHN-SV-EXCH05.mchp-main.com ([fe80::c1bf:7679:c1f8:4560%15]) with\n\tmapi id 14.03.0352.000; Fri, 29 Sep 2017 11:24:37 -0700"],"X-IronPort-AV":"E=Sophos;i=\"5.42,453,1500966000\"; d=\"scan'208\";a=\"7872736\"","From":"","To":", ","CC":", , ,\n\t, ,\n\t, ,\n\t","Subject":"RE: [PATCH RFC 3/5] Add KSZ8795 switch driver","Thread-Topic":"[PATCH RFC 3/5] Add KSZ8795 switch driver","Thread-Index":"AdMoGw7chwKpH5nHQDyZLhGSoYClOwAA0svAABGDyIACFLHiAAIFED0AAByk3AAABjO4gAAC/4Vw","Date":"Fri, 29 Sep 2017 18:24:36 +0000","Message-ID":"<93AF473E2DA327428DE3D46B72B1E9FD4112CB89@CHN-SV-EXMX02.mchp-main.com>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>\n\t<20170907223625.GW11248@lunn.ch>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41124D5A@CHN-SV-EXMX02.mchp-main.com>\n\t<20170928193416.GH14940@lunn.ch>\n\t<063D6719AE5E284EB5DD2968C1650D6DD0084EDC@AcuExch.aculab.com>\n\t<20170929121201.GD19710@lunn.ch>","In-Reply-To":"<20170929121201.GD19710@lunn.ch>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[10.10.76.4]","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1777714,"web_url":"http://patchwork.ozlabs.org/comment/1777714/","msgid":"<93AF473E2DA327428DE3D46B72B1E9FD4112CBC8@CHN-SV-EXMX02.mchp-main.com>","list_archive_url":null,"date":"2017-09-29T18:45:19","subject":"RE: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":72262,"url":"http://patchwork.ozlabs.org/api/people/72262/","name":"","email":"Tristram.Ha@microchip.com"},"content":"> > > > > Similar code will be needed by other drivers, right?\n> > > >\n> > > > Although KSZ8795 and KSZ8895 may use the same code, the other\n> > > > chips will have different code.\n> > >\n> > > Ok, please make sure code is shared between these two.\n> >\n> > The exact function probably cannot be shared between KSZ8795\n> > and KSZ8895 drivers because although the register constants look\n> > the same but their exact locations are different in the 2 chips.\n> \n> Put the code into header as static inline and include it from both\n> places?\n> \n\nAlthough KSZ8795 and KSZ8895 share the same code when simulating\nthe PHY register access, even though the exact registers are not the\nsame, this code needs a little modification for another chip. It also looks\ntoo large to put in a header.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y3gVN69Lrz9sPk\n\tfor ;\n\tSat, 30 Sep 2017 04:45:32 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752522AbdI2SpX convert rfc822-to-8bit (ORCPT\n\t);\n\tFri, 29 Sep 2017 14:45:23 -0400","from esa1.microchip.iphmx.com ([68.232.147.91]:56484 \"EHLO\n\tesa1.microchip.iphmx.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752334AbdI2SpW (ORCPT\n\t); Fri, 29 Sep 2017 14:45:22 -0400","from smtpout.microchip.com (HELO email.microchip.com)\n\t([198.175.253.82])\n\tby esa1.microchip.iphmx.com with ESMTP/TLS/DHE-RSA-AES256-SHA;\n\t29 Sep 2017 11:45:21 -0700","from CHN-SV-EXMX02.mchp-main.com ([fe80::7dfe:3761:863e:3963]) by\n\tCHN-SV-EXCH05.mchp-main.com ([fe80::c1bf:7679:c1f8:4560%15]) with\n\tmapi id 14.03.0352.000; Fri, 29 Sep 2017 11:45:21 -0700"],"X-IronPort-AV":"E=Sophos;i=\"5.42,453,1500966000\"; d=\"scan'208\";a=\"7873389\"","From":"","To":"","CC":", , ,\n\t, ,\n\t, ,\n\t","Subject":"RE: [PATCH RFC 3/5] Add KSZ8795 switch driver","Thread-Topic":"[PATCH RFC 3/5] Add KSZ8795 switch driver","Thread-Index":"AdMoGw7chwKpH5nHQDyZLhGSoYClOwAA0svAACf0VAAAAsLgkAAXu/yAAAb8AcAD2R/bgAAqcfQg","Date":"Fri, 29 Sep 2017 18:45:19 +0000","Message-ID":"<93AF473E2DA327428DE3D46B72B1E9FD4112CBC8@CHN-SV-EXMX02.mchp-main.com>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>\n\t<20170908091856.GB17738@amd>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41121E3D@CHN-SV-EXMX02.mchp-main.com>\n\t<20170908215735.GF27428@amd>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41123010@CHN-SV-EXMX02.mchp-main.com>\n\t<20170928152445.GC2482@amd>","In-Reply-To":"<20170928152445.GC2482@amd>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[10.10.76.4]","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1777716,"web_url":"http://patchwork.ozlabs.org/comment/1777716/","msgid":"<20170929185316.GB17713@lunn.ch>","list_archive_url":null,"date":"2017-09-29T18:53:16","subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":13608,"url":"http://patchwork.ozlabs.org/api/people/13608/","name":"Andrew Lunn","email":"andrew@lunn.ch"},"content":"> My concern is if a task is already running with SPI access to a lot\n> of registers like reading the 32 MIB counters in every port of the\n> switch, another register access has to wait until they are finished.\n\nWhy does it have to wait? Looking at the code in\nksz_get_ethtool_stats(), you don't take any mutex which will prevent\nothers from using the SPI bus. All there is is a mutex which prevents\ntwo sets of ksz_get_ethtool_stats() at the same time.\n\nSo a PTP read could happen in parallel, and will not be blocked by MIB\nreads. They should get interleaved access to the SPI bus.\n\n Andrew","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y3ghR1rzqz9t3k\n\tfor ;\n\tSat, 30 Sep 2017 04:54:15 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752553AbdI2SyI (ORCPT );\n\tFri, 29 Sep 2017 14:54:08 -0400","from vps0.lunn.ch ([185.16.172.187]:33781 \"EHLO vps0.lunn.ch\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1752351AbdI2SyF (ORCPT );\n\tFri, 29 Sep 2017 14:54:05 -0400","from andrew by vps0.lunn.ch with local (Exim 4.84_2)\n\t(envelope-from )\n\tid 1dy0PU-0005iW-El; Fri, 29 Sep 2017 20:53:16 +0200"],"Date":"Fri, 29 Sep 2017 20:53:16 +0200","From":"Andrew Lunn ","To":"Tristram.Ha@microchip.com","Cc":"David.Laight@ACULAB.COM, muvarov@gmail.com, pavel@ucw.cz,\n\tnathan.leigh.conrad@gmail.com, vivien.didelot@savoirfairelinux.com,\n\tf.fainelli@gmail.com, netdev@vger.kernel.org,\n\tlinux-kernel@vger.kernel.org, Woojung.Huh@microchip.com","Subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","Message-ID":"<20170929185316.GB17713@lunn.ch>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>\n\t<20170907223625.GW11248@lunn.ch>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41124D5A@CHN-SV-EXMX02.mchp-main.com>\n\t<20170928193416.GH14940@lunn.ch>\n\t<063D6719AE5E284EB5DD2968C1650D6DD0084EDC@AcuExch.aculab.com>\n\t<20170929121201.GD19710@lunn.ch>\n\t<93AF473E2DA327428DE3D46B72B1E9FD4112CB89@CHN-SV-EXMX02.mchp-main.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<93AF473E2DA327428DE3D46B72B1E9FD4112CB89@CHN-SV-EXMX02.mchp-main.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1777718,"web_url":"http://patchwork.ozlabs.org/comment/1777718/","msgid":"<93AF473E2DA327428DE3D46B72B1E9FD4112CBF0@CHN-SV-EXMX02.mchp-main.com>","list_archive_url":null,"date":"2017-09-29T18:56:31","subject":"RE: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":72262,"url":"http://patchwork.ozlabs.org/api/people/72262/","name":"","email":"Tristram.Ha@microchip.com"},"content":"> On Mon 2017-09-18 20:27:13, Tristram.Ha@microchip.com wrote:\n> > > > +/**\n> > > > + * Some counters do not need to be read too often because they are\n> less\n> > > likely\n> > > > + * to increase much.\n> > > > + */\n> > >\n> > > What does comment mean? Are you caching statistics, and updating\n> > > different values at different rates?\n> > >\n> >\n> > There are 34 counters. In normal case using generic bus I/O or PCI to read\n> them\n> > is very quick, but the switch is mostly accessed using SPI, or even I2C. As\n> the SPI\n> > access is very slow and cannot run in interrupt context I keep worrying\n> reading\n> > the MIB counters in a loop for 5 or more ports will prevent other critical\n> hardware\n> > access from executing soon enough. These accesses can be getting 1588\n> PTP\n> > timestamps and opening/closing ports. (RSTP Conformance Test sends test\n> traffic\n> > to port supposed to be closed/opened after receiving specific RSTP\n> > BPDU.)\n> \n> Hmm. Ok, interesting.\n> \n> I wonder how well this is going to work if userspace actively 'does\n> something' with the switch.\n> \n> It seems to me that even if your statistics code is careful not to do\n> 'a lot' of accesses at the same time, userspace can use other parts of\n> the driver to do the same, and thus cause same unwanted effects...\n\nIf the user calls \"ethtool -S\" in a tight loop the system will waste a lot of\nCPU time, but this is more like a user error.\nAnother solution is not to schedule to read the MIB counters in that\nfunction call. I think I was doing a favor to update the MIB counters\nsooner as the user probably wants to find out what is wrong with the\nswitch by reading the MIB counters and checking them several times.\nFor system tracking like SNMP I think it is likely a separate mechanism\nis used to gather those information. If I am wrong that function definitely\nneeds to be modified.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y3glJ6JLbz9t3k\n\tfor ;\n\tSat, 30 Sep 2017 04:56:44 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752512AbdI2S4e convert rfc822-to-8bit (ORCPT\n\t);\n\tFri, 29 Sep 2017 14:56:34 -0400","from esa4.microchip.iphmx.com ([68.232.154.123]:14797 \"EHLO\n\tesa4.microchip.iphmx.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752351AbdI2S4d (ORCPT\n\t); Fri, 29 Sep 2017 14:56:33 -0400","from exsmtp02.microchip.com (HELO email.microchip.com)\n\t([198.175.253.38])\n\tby esa4.microchip.iphmx.com with ESMTP/TLS/DHE-RSA-AES256-SHA;\n\t29 Sep 2017 11:56:32 -0700","from CHN-SV-EXMX02.mchp-main.com ([fe80::7dfe:3761:863e:3963]) by\n\tCHN-SV-EXCH02.mchp-main.com ([::1]) with mapi id 14.03.0352.000;\n\tFri, 29 Sep 2017 11:56:32 -0700"],"X-IronPort-AV":"E=Sophos;i=\"5.42,453,1500966000\"; d=\"scan'208\";a=\"7249863\"","From":"","To":"","CC":", , ,\n\t, ,\n\t, ,\n\t","Subject":"RE: [PATCH RFC 3/5] Add KSZ8795 switch driver","Thread-Topic":"[PATCH RFC 3/5] Add KSZ8795 switch driver","Thread-Index":"AdMoGw7chwKpH5nHQDyZLhGSoYClOwAA0svAABGDyIACFLHiAAIDM9mAACPJiPA=","Date":"Fri, 29 Sep 2017 18:56:31 +0000","Message-ID":"<93AF473E2DA327428DE3D46B72B1E9FD4112CBF0@CHN-SV-EXMX02.mchp-main.com>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>\n\t<20170907223625.GW11248@lunn.ch>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41124D5A@CHN-SV-EXMX02.mchp-main.com>\n\t<20170928184059.GA2825@amd>","In-Reply-To":"<20170928184059.GA2825@amd>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[10.10.76.4]","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1777730,"web_url":"http://patchwork.ozlabs.org/comment/1777730/","msgid":"<93AF473E2DA327428DE3D46B72B1E9FD4112CC1D@CHN-SV-EXMX02.mchp-main.com>","list_archive_url":null,"date":"2017-09-29T19:19:17","subject":"RE: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":72262,"url":"http://patchwork.ozlabs.org/api/people/72262/","name":"","email":"Tristram.Ha@microchip.com"},"content":"> > My concern is if a task is already running with SPI access to a lot\n> > of registers like reading the 32 MIB counters in every port of the\n> > switch, another register access has to wait until they are finished.\n> \n> Why does it have to wait? Looking at the code in\n> ksz_get_ethtool_stats(), you don't take any mutex which will prevent\n> others from using the SPI bus. All there is is a mutex which prevents\n> two sets of ksz_get_ethtool_stats() at the same time.\n> \n> So a PTP read could happen in parallel, and will not be blocked by MIB\n> reads. They should get interleaved access to the SPI bus.\n> \n\nThe MIB counters are read in the background. For multiple CPU cores 2\ntasks may run in the same time allowing SPI access one after another.\nFor single core I am not sure an SPI access like coming from an interrupt\nroutine can jump ahead from one in a background task.\n\nI know nowadays SoCs are powerful enough to do amazing things. It is\njust I spent a long time using a low-powered SoC doing switch driver\ndevelopment.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y3hFf6NbHz9t2Q\n\tfor ;\n\tSat, 30 Sep 2017 05:19:34 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752434AbdI2TTX convert rfc822-to-8bit (ORCPT\n\t);\n\tFri, 29 Sep 2017 15:19:23 -0400","from esa1.microchip.iphmx.com ([68.232.147.91]:12556 \"EHLO\n\tesa1.microchip.iphmx.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752294AbdI2TTV (ORCPT\n\t); Fri, 29 Sep 2017 15:19:21 -0400","from exsmtp01.microchip.com (HELO email.microchip.com)\n\t([198.175.253.37])\n\tby esa1.microchip.iphmx.com with ESMTP/TLS/AES128-SHA;\n\t29 Sep 2017 12:19:19 -0700","from CHN-SV-EXMX02.mchp-main.com ([fe80::7dfe:3761:863e:3963]) by\n\tCHN-SV-EXCH01.mchp-main.com ([fe80::9840:ffdf:ec5:1335%29]) with\n\tmapi id 14.03.0352.000; Fri, 29 Sep 2017 12:19:18 -0700"],"X-IronPort-AV":"E=Sophos;i=\"5.42,453,1500966000\"; d=\"scan'208\";a=\"7874464\"","From":"","To":"","CC":", , ,\n\t,\n\t, ,\n\t, ,\n\t","Subject":"RE: [PATCH RFC 3/5] Add KSZ8795 switch driver","Thread-Topic":"[PATCH RFC 3/5] Add KSZ8795 switch driver","Thread-Index":"AdMoGw7chwKpH5nHQDyZLhGSoYClOwAA0svAABGDyIACFLHiAAIFED0AAByk3AAABjO4gAAC/4VwAAsD8AAADnmeYA==","Date":"Fri, 29 Sep 2017 19:19:17 +0000","Message-ID":"<93AF473E2DA327428DE3D46B72B1E9FD4112CC1D@CHN-SV-EXMX02.mchp-main.com>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>\n\t<20170907223625.GW11248@lunn.ch>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41124D5A@CHN-SV-EXMX02.mchp-main.com>\n\t<20170928193416.GH14940@lunn.ch>\n\t<063D6719AE5E284EB5DD2968C1650D6DD0084EDC@AcuExch.aculab.com>\n\t<20170929121201.GD19710@lunn.ch>\n\t<93AF473E2DA327428DE3D46B72B1E9FD4112CB89@CHN-SV-EXMX02.mchp-main.com>\n\t<20170929185316.GB17713@lunn.ch>","In-Reply-To":"<20170929185316.GB17713@lunn.ch>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[10.10.76.4]","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1777788,"web_url":"http://patchwork.ozlabs.org/comment/1777788/","msgid":"<20170929203926.GD17713@lunn.ch>","list_archive_url":null,"date":"2017-09-29T20:39:26","subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":13608,"url":"http://patchwork.ozlabs.org/api/people/13608/","name":"Andrew Lunn","email":"andrew@lunn.ch"},"content":"On Fri, Sep 29, 2017 at 07:19:17PM +0000, Tristram.Ha@microchip.com wrote:\n> > > My concern is if a task is already running with SPI access to a lot\n> > > of registers like reading the 32 MIB counters in every port of the\n> > > switch, another register access has to wait until they are finished.\n> > \n> > Why does it have to wait? Looking at the code in\n> > ksz_get_ethtool_stats(), you don't take any mutex which will prevent\n> > others from using the SPI bus. All there is is a mutex which prevents\n> > two sets of ksz_get_ethtool_stats() at the same time.\n> > \n> > So a PTP read could happen in parallel, and will not be blocked by MIB\n> > reads. They should get interleaved access to the SPI bus.\n> > \n> \n> The MIB counters are read in the background. For multiple CPU cores 2\n> tasks may run in the same time allowing SPI access one after another.\n> For single core I am not sure an SPI access like coming from an interrupt\n> routine can jump ahead from one in a background task.\n\nThe SPI subsystem has a mutex per controller. When starting a\ntransfer, it takes the mutex and release it once the transfer has\ncompleted. There is also a reschedule point at the end of a\ntransfer. So even on your single core CPU, there can be multi tasking\ngoing on.\n\n Andrew","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y3k2w0bxmz9t2Q\n\tfor ;\n\tSat, 30 Sep 2017 06:40:24 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752501AbdI2UkN (ORCPT );\n\tFri, 29 Sep 2017 16:40:13 -0400","from vps0.lunn.ch ([185.16.172.187]:33885 \"EHLO vps0.lunn.ch\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1752003AbdI2UkM (ORCPT );\n\tFri, 29 Sep 2017 16:40:12 -0400","from andrew by vps0.lunn.ch with local (Exim 4.84_2)\n\t(envelope-from )\n\tid 1dy24E-0006Tc-Ao; Fri, 29 Sep 2017 22:39:26 +0200"],"Date":"Fri, 29 Sep 2017 22:39:26 +0200","From":"Andrew Lunn ","To":"Tristram.Ha@microchip.com","Cc":"David.Laight@ACULAB.COM, muvarov@gmail.com, pavel@ucw.cz,\n\tnathan.leigh.conrad@gmail.com, vivien.didelot@savoirfairelinux.com,\n\tf.fainelli@gmail.com, netdev@vger.kernel.org,\n\tlinux-kernel@vger.kernel.org, Woojung.Huh@microchip.com","Subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","Message-ID":"<20170929203926.GD17713@lunn.ch>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>\n\t<20170907223625.GW11248@lunn.ch>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41124D5A@CHN-SV-EXMX02.mchp-main.com>\n\t<20170928193416.GH14940@lunn.ch>\n\t<063D6719AE5E284EB5DD2968C1650D6DD0084EDC@AcuExch.aculab.com>\n\t<20170929121201.GD19710@lunn.ch>\n\t<93AF473E2DA327428DE3D46B72B1E9FD4112CB89@CHN-SV-EXMX02.mchp-main.com>\n\t<20170929185316.GB17713@lunn.ch>\n\t<93AF473E2DA327428DE3D46B72B1E9FD4112CC1D@CHN-SV-EXMX02.mchp-main.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<93AF473E2DA327428DE3D46B72B1E9FD4112CC1D@CHN-SV-EXMX02.mchp-main.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1777995,"web_url":"http://patchwork.ozlabs.org/comment/1777995/","msgid":"<20171001072151.GA14600@amd>","list_archive_url":null,"date":"2017-10-01T07:21:51","subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","submitter":{"id":2109,"url":"http://patchwork.ozlabs.org/api/people/2109/","name":"Pavel Machek","email":"pavel@ucw.cz"},"content":"On Fri 2017-09-29 18:45:19, Tristram.Ha@microchip.com wrote:\n> > > > > > Similar code will be needed by other drivers, right?\n> > > > >\n> > > > > Although KSZ8795 and KSZ8895 may use the same code, the other\n> > > > > chips will have different code.\n> > > >\n> > > > Ok, please make sure code is shared between these two.\n> > >\n> > > The exact function probably cannot be shared between KSZ8795\n> > > and KSZ8895 drivers because although the register constants look\n> > > the same but their exact locations are different in the 2 chips.\n> > \n> > Put the code into header as static inline and include it from both\n> > places?\n> \n> Although KSZ8795 and KSZ8895 share the same code when simulating\n> the PHY register access, even though the exact registers are not the\n> same, this code needs a little modification for another chip. It also looks\n> too large to put in a header.\n\nI believe putting it into a header is prefferable to having two (or\nthree) copies of the code. If it needs small changes, you can\nintroduce if () ..., gcc can probably optimize it out.\n\nThanks,\n\t\t\t\t\t\t\t\t\tPavel","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y4cDy2PFzz9sRm\n\tfor ;\n\tSun, 1 Oct 2017 18:22:10 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1750992AbdJAHVz (ORCPT );\n\tSun, 1 Oct 2017 03:21:55 -0400","from atrey.karlin.mff.cuni.cz ([195.113.26.193]:41533 \"EHLO\n\tatrey.karlin.mff.cuni.cz\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750849AbdJAHVy (ORCPT\n\t); Sun, 1 Oct 2017 03:21:54 -0400","by atrey.karlin.mff.cuni.cz (Postfix, from userid 512)\n\tid 45EFA82417; Sun, 1 Oct 2017 09:21:52 +0200 (CEST)"],"Date":"Sun, 1 Oct 2017 09:21:51 +0200","From":"Pavel Machek ","To":"Tristram.Ha@microchip.com","Cc":"andrew@lunn.ch, muvarov@gmail.com, nathan.leigh.conrad@gmail.com,\n\tvivien.didelot@savoirfairelinux.com, f.fainelli@gmail.com,\n\tnetdev@vger.kernel.org, linux-kernel@vger.kernel.org,\n\tWoojung.Huh@microchip.com","Subject":"Re: [PATCH RFC 3/5] Add KSZ8795 switch driver","Message-ID":"<20171001072151.GA14600@amd>","References":"<93AF473E2DA327428DE3D46B72B1E9FD41121A87@CHN-SV-EXMX02.mchp-main.com>\n\t<20170908091856.GB17738@amd>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41121E3D@CHN-SV-EXMX02.mchp-main.com>\n\t<20170908215735.GF27428@amd>\n\t<93AF473E2DA327428DE3D46B72B1E9FD41123010@CHN-SV-EXMX02.mchp-main.com>\n\t<20170928152445.GC2482@amd>\n\t<93AF473E2DA327428DE3D46B72B1E9FD4112CBC8@CHN-SV-EXMX02.mchp-main.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"FCuugMFkClbJLl1L\"","Content-Disposition":"inline","In-Reply-To":"<93AF473E2DA327428DE3D46B72B1E9FD4112CBC8@CHN-SV-EXMX02.mchp-main.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}}]