From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SIGNED_OFF_BY,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BD1A9C433E0 for ; Sat, 1 Aug 2020 15:36:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8939C2071E for ; Sat, 1 Aug 2020 15:36:20 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="gtkMX+3e" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726709AbgHAPf6 (ORCPT ); Sat, 1 Aug 2020 11:35:58 -0400 Received: from us-smtp-1.mimecast.com ([205.139.110.61]:21270 "EHLO us-smtp-delivery-1.mimecast.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1725841AbgHAPf6 (ORCPT ); Sat, 1 Aug 2020 11:35:58 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1596296153; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=K1oyKpN0hdQN8GePYwf2hKupytlxU0CEt9H//w+Cw2k=; b=gtkMX+3elUNIaF6B8hkFf+fu19vZ8orpbh6ObLukv2Ua4E8cy/w+g7sreDc6YFKiee/bD4 Din3bS9CTXe0wirejZguh310RhlbhIe2plVe+DDyYvSxbG0iF5Y4cfOUavujhNnKWf/xDZ +ZNTaC7vaa0kA3S99OVPUw+LVV1bt3k= Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-284-Dr9vygnpMpuilbRrAo206Q-1; Sat, 01 Aug 2020 11:35:50 -0400 X-MC-Unique: Dr9vygnpMpuilbRrAo206Q-1 Received: by mail-qk1-f198.google.com with SMTP id 3so23070012qkv.13 for ; Sat, 01 Aug 2020 08:35:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding :content-language; bh=K1oyKpN0hdQN8GePYwf2hKupytlxU0CEt9H//w+Cw2k=; b=JPBJYYQsxqbJpePsClRPqs/WLsyP2SbnozEMAXKoBcO6vSeoaDU2OM8O/OsGt8ttyM IUGfwTnwqNNEwEX4qZZxsZYmZWBJqmGgrojGzQWte+kHaqoUJ79B5+6fZ+LfJ8WyFbuL LD/rLH3ILYxrZ3yB1CMNk6GAMwd6yIB07mR4Gdx3iU8YeZfP9ZIw8J8m2gzlptzo45L5 UpU8ufR/DpCGc/cfNzZ0xihwYa50NHWigAyYdbQraVIyZ3KO9UvMUs1iP641mfO307jm 7E1Bot8hUTVXsse4QpvqDrd321XbJM1DTstvDkBdw/glWIfFKev90SMUyKf4VhpVhQn7 WGlw== X-Gm-Message-State: AOAM532pi/LMTwkI+ZVZVdpGJrvckAL42kusr7fO50qrQ2phc9C3SwFe 2J2o5dapuewxRrYtb6+iKDTBY148vlSs/DqDNcS8snppW3D0reex2Kw/6Hi3PUFzHUodGL2o7eg k7IBfq3oxVVOJPk5/Iw4ABiXP X-Received: by 2002:a05:6214:724:: with SMTP id c4mr9349302qvz.0.1596296145777; Sat, 01 Aug 2020 08:35:45 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxVTofFH4piqGp0TN9P/9W3daCh+kF/hIJnYcd57kquBeWH9Cy5cEwb/KauWFGG9h/sK3nBUA== X-Received: by 2002:a05:6214:724:: with SMTP id c4mr9349265qvz.0.1596296145189; Sat, 01 Aug 2020 08:35:45 -0700 (PDT) Received: from trix.remote.csb (075-142-250-213.res.spectrum.com. [75.142.250.213]) by smtp.gmail.com with ESMTPSA id h13sm13306758qtu.7.2020.08.01.08.35.43 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 01 Aug 2020 08:35:44 -0700 (PDT) Subject: Re: [PATCH v2 4/4] fpga: dfl: add support for N3000 nios private feature To: Xu Yilun Cc: mdf@kernel.org, linux-fpga@vger.kernel.org, linux-kernel@vger.kernel.org, lgoncalv@redhat.com, Wu Hao , Matthew Gerlach , Russ Weight References: <1595556555-9903-1-git-send-email-yilun.xu@intel.com> <1595556555-9903-5-git-send-email-yilun.xu@intel.com> <20200731085609.GF1781@yilunxu-OptiPlex-7050> From: Tom Rix Message-ID: <3ea2e6a0-c982-0ea4-ac6c-2a65a6b01b7d@redhat.com> Date: Sat, 1 Aug 2020 08:35:42 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: <20200731085609.GF1781@yilunxu-OptiPlex-7050> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org See below. On 7/31/20 1:56 AM, Xu Yilun wrote: > On Sat, Jul 25, 2020 at 07:53:59AM -0700, Tom Rix wrote: >> Mostly little stuff. >> >> The polling on the int in ns_bus_poll_stat_timeout should be refactored. >> >> Tom >> >> On 7/23/20 7:09 PM, Xu Yilun wrote: >>> This patch adds support for the nios handshake private feature on Intel >>> N3000 FPGA Card. This private feature provides a handshake interface to >>> FPGA NIOS firmware, which receives retimer configuration command from host >>> and executes via an internal SPI master. When nios finished the >>> configuration, host takes over the ownership of the SPI master to control >>> an Intel MAX10 BMC Chip on the SPI bus. >>> >>> For NIOS firmware handshake part, this driver requests the retimer >>> configuration for NIOS with parameters from module param, and adds some >>> sysfs nodes for user to query NIOS state. >>> >>> For SPI part, this driver adds a spi-altera platform device as well as >>> the MAX10 BMC spi slave info. A spi-altera driver will be matched to >>> handle following the SPI work. >>> >>> Signed-off-by: Xu Yilun >>> Signed-off-by: Wu Hao >>> Signed-off-by: Matthew Gerlach >>> Signed-off-by: Russ Weight >>> --- >>> .../ABI/testing/sysfs-bus-dfl-devices-n3000-nios | 16 + >>> drivers/fpga/Kconfig | 12 + >>> drivers/fpga/Makefile | 2 + >>> drivers/fpga/dfl-n3000-nios.c | 483 +++++++++++++++++++++ >>> 4 files changed, 513 insertions(+) >>> create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios >>> create mode 100644 drivers/fpga/dfl-n3000-nios.c >>> >>> diff --git a/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios b/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios >>> new file mode 100644 >>> index 0000000..8346b74 >>> --- /dev/null >>> +++ b/Documentation/ABI/testing/sysfs-bus-dfl-devices-n3000-nios >>> @@ -0,0 +1,16 @@ >>> +What: /sys/bus/dfl/devices/dfl-fme.X.X/fec_mode >>> +Date: July 2020 >>> +KernelVersion: 5.9 >>> +Contact: Xu Yilun >>> +Description: Read-only. It returns the FEC mode of the ethernet retimer >>> + configured by NIOS firmware. "rs" for RS FEC mode, "kr" for >>> + KR FEC mode, "no" FOR NO FEC mode. >>> + Format: string >>> + >>> +What: /sys/bus/dfl/devices/dfl-fme.X.X/nios_fw_version >>> +Date: July 2020 >>> +KernelVersion: 5.9 >>> +Contact: Xu Yilun >>> +Description: Read-only. It returns the NIOS firmware version in FPGA. Its >>> + format is "major.minor.patch". >>> + Format: %x.%x.%x >>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig >>> index b2408a7..fa5f3ab 100644 >>> --- a/drivers/fpga/Kconfig >>> +++ b/drivers/fpga/Kconfig >>> @@ -191,6 +191,18 @@ config FPGA_DFL_AFU >>> to the FPGA infrastructure via a Port. There may be more than one >>> Port/AFU per DFL based FPGA device. >>> >>> +config FPGA_DFL_N3000_NIOS >>> + tristate "FPGA DFL N3000 NIOS Driver" >>> + depends on FPGA_DFL >>> + select REGMAP >> depends on SPI_MASTER ? > I think we don't have to add this. The driver doesn't call any API > provided by SPI core, just fills the spi_board_info structure. ok > >>> + help >>> + This is the driver for the nios handshake private feature on Intel >>> + N3000 FPGA Card. This private feature provides a handshake interface >>> + to FPGA NIOS firmware, which receives retimer configuration command >>> + from host and executes via an internal SPI master. When nios finished >>> + the configuration, host takes over the ownership of the SPI master to >>> + control an Intel MAX10 BMC Chip on the SPI bus. >>> + >>> config FPGA_DFL_PCI >>> tristate "FPGA DFL PCIe Device Driver" >>> depends on PCI && FPGA_DFL >>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile >>> index d8e21df..27f20f2 100644 >>> --- a/drivers/fpga/Makefile >>> +++ b/drivers/fpga/Makefile >>> @@ -44,5 +44,7 @@ dfl-fme-objs += dfl-fme-perf.o >>> dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o >>> dfl-afu-objs += dfl-afu-error.o >>> >>> +obj-$(CONFIG_FPGA_DFL_N3000_NIOS) += dfl-n3000-nios.o >>> + >>> # Drivers for FPGAs which implement DFL >>> obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o >>> diff --git a/drivers/fpga/dfl-n3000-nios.c b/drivers/fpga/dfl-n3000-nios.c >>> new file mode 100644 >>> index 0000000..d098a10 >>> --- /dev/null >>> +++ b/drivers/fpga/dfl-n3000-nios.c >>> @@ -0,0 +1,483 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Driver for DFL N3000 NIOS private feature >>> + * >>> + * Copyright (C) 2019-2020 Intel Corporation, Inc. >>> + * >>> + * Authors: >>> + * Wu Hao >>> + * Xu Yilun >>> + */ >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#include "dfl.h" >>> + >>> +static char *fec_mode = "rs"; >>> +module_param(fec_mode, charp, 0444); >>> +MODULE_PARM_DESC(child, "FEC mode"); >> Should document what the valid values are and what they mean. >> >> ex/ what is 'rs' ? >> >> Maybe append something in the sysfs doc about fec_mode being settable. > Yes, I'll add the documents. > >>> + >>> +/* N3000 NIOS private feature registers */ >>> +#define NIOS_SPI_PARAM 0x8 >>> +#define SHIFT_MODE BIT_ULL(1) >>> +#define SHIFT_MODE_MSB 0 >>> +#define SHIFT_MODE_LSB 1 >>> +#define DATA_WIDTH GENMASK_ULL(7, 2) >>> +#define NUM_CHIPSELECT GENMASK_ULL(13, 8) >>> +#define CLK_POLARITY BIT_ULL(14) >>> +#define CLK_PHASE BIT_ULL(15) >>> +#define PERIPHERAL_ID GENMASK_ULL(47, 32) >>> + >>> +#define NIOS_SPI_CTRL 0x10 >>> +#define CTRL_WR_DATA GENMASK_ULL(31, 0) >>> +#define CTRL_ADDR GENMASK_ULL(44, 32) >>> +#define CTRL_CMD GENMASK_ULL(63, 62) >>> +#define CMD_NOP 0 >>> +#define CMD_RD 1 >>> +#define CMD_WR 2 >> These are fairly generic #define names, consider adding a prefix. > Yes. > >>> + >>> +#define NIOS_SPI_STAT 0x18 >>> +#define STAT_RD_DATA GENMASK_ULL(31, 0) >>> +#define STAT_RW_VAL BIT_ULL(32) >>> + >>> +/* nios handshake registers, indirect access */ >>> +#define NIOS_INIT 0x1000 >>> +#define NIOS_INIT_DONE BIT(0) >>> +#define NIOS_INIT_START BIT(1) >>> +/* Mode for PKVL A, link 0, the same below */ >>> +#define REQ_FEC_MODE_A0 GENMASK(9, 8) >>> +#define REQ_FEC_MODE_A1 GENMASK(11, 10) >>> +#define REQ_FEC_MODE_A2 GENMASK(13, 12) >>> +#define REQ_FEC_MODE_A3 GENMASK(15, 14) >>> +#define REQ_FEC_MODE_B0 GENMASK(17, 16) >>> +#define REQ_FEC_MODE_B1 GENMASK(19, 18) >>> +#define REQ_FEC_MODE_B2 GENMASK(21, 20) >>> +#define REQ_FEC_MODE_B3 GENMASK(23, 22) >>> +#define FEC_MODE_NO 0x0 >>> +#define FEC_MODE_KR 0x1 >>> +#define FEC_MODE_RS 0x2 >>> + >>> +#define NIOS_FW_VERSION 0x1004 >>> +#define NIOS_FW_VERSION_PATCH GENMASK(23, 20) >>> +#define NIOS_FW_VERSION_MINOR GENMASK(27, 24) >>> +#define NIOS_FW_VERSION_MAJOR GENMASK(31, 28) >>> + >>> +#define PKVL_A_MODE_STS 0x1020 >>> +#define PKVL_B_MODE_STS 0x1024 >>> + >>> +#define NS_REGBUS_WAIT_TIMEOUT 10000 /* loop count */ >>> +#define NIOS_INIT_TIMEOUT 10000000 /* usec */ >>> +#define NIOS_INIT_TIME_INTV 100000 /* usec */ >>> + >>> +struct dfl_n3000_nios { >>> + void __iomem *base; >>> + struct regmap *regmap; >>> + struct device *dev; >>> + struct platform_device *altr_spi; >> Make this element more meaningful, expand to 'altera_spi' > Yes. > >>> +}; >>> + >>> +static int n3000_nios_writel(struct dfl_n3000_nios *ns, unsigned int reg, >>> + unsigned int val) >>> +{ >>> + int ret; >>> + >>> + ret = regmap_write(ns->regmap, reg, val); >>> + if (ret) >>> + dev_err(ns->dev, "fail to write reg 0x%x val 0x%x: %d\n", >>> + reg, val, ret); >>> + >>> + return ret; >>> +} >>> + >>> +static int n3000_nios_readl(struct dfl_n3000_nios *ns, unsigned int reg, >>> + unsigned int *val) >>> +{ >>> + int ret; >>> + >>> + ret = regmap_read(ns->regmap, reg, val); >>> + if (ret) >>> + dev_err(ns->dev, "fail to read reg 0x%x: %d\n", reg, ret); >>> + >>> + return ret; >>> +} >>> + >>> +static ssize_t nios_fw_version_show(struct device *dev, >>> + struct device_attribute *attr, char *buf) >>> +{ >>> + struct dfl_n3000_nios *ns = dev_get_drvdata(dev); >>> + unsigned int val; >>> + int ret; >> This and similar could be >> >> ssize_t ret. > I think it may not be necessary. It is not reasonable n3000_nios_readl() > has the return type ssize_t. So no matter how you define the ret, the > cast is always needed. > > I see the general handling of this case in drivers is "int ret". Ok. > >>> + >>> + ret = n3000_nios_readl(ns, NIOS_FW_VERSION, &val); >>> + if (ret) >>> + return ret; >>> + >>> + return sprintf(buf, "%x.%x.%x\n", >>> + (u8)FIELD_GET(NIOS_FW_VERSION_MAJOR, val), >>> + (u8)FIELD_GET(NIOS_FW_VERSION_MINOR, val), >>> + (u8)FIELD_GET(NIOS_FW_VERSION_PATCH, val)); >>> +} >>> +static DEVICE_ATTR_RO(nios_fw_version); >>> + >>> +static ssize_t fec_mode_show(struct device *dev, >>> + struct device_attribute *attr, char *buf) >>> +{ >>> + struct dfl_n3000_nios *ns = dev_get_drvdata(dev); >>> + unsigned int val, mode; >>> + int ret; >>> + >>> + ret = n3000_nios_readl(ns, NIOS_INIT, &val); >>> + if (ret) >>> + return ret; >>> + >>> + /* >>> + * FEC mode should always be the same for all links, as we set them >>> + * in this way. >>> + */ >>> + mode = FIELD_GET(REQ_FEC_MODE_A0, val); >>> + if (mode != FIELD_GET(REQ_FEC_MODE_A1, val) || >>> + mode != FIELD_GET(REQ_FEC_MODE_A2, val) || >>> + mode != FIELD_GET(REQ_FEC_MODE_A3, val) || >>> + mode != FIELD_GET(REQ_FEC_MODE_B0, val) || >>> + mode != FIELD_GET(REQ_FEC_MODE_B1, val) || >>> + mode != FIELD_GET(REQ_FEC_MODE_B2, val) || >>> + mode != FIELD_GET(REQ_FEC_MODE_B3, val)) >>> + return -EFAULT; >>> + >>> + switch (mode) { >>> + case FEC_MODE_NO: >>> + return sprintf(buf, "no\n"); >>> + case FEC_MODE_KR: >>> + return sprintf(buf, "kr\n"); >>> + case FEC_MODE_RS: >>> + return sprintf(buf, "rs\n"); >>> + } >>> + >>> + return -EFAULT; >>> +} >>> +static DEVICE_ATTR_RO(fec_mode); >>> + >>> +static struct attribute *n3000_nios_attrs[] = { >>> + &dev_attr_nios_fw_version.attr, >>> + &dev_attr_fec_mode.attr, >>> + NULL, >>> +}; >>> +ATTRIBUTE_GROUPS(n3000_nios); >>> + >>> +static int init_error_detected(struct dfl_n3000_nios *ns) >> returning true/false, the return type should be boolean > Yes. > >>> +{ >>> + unsigned int val; >>> + >>> + if (n3000_nios_readl(ns, PKVL_A_MODE_STS, &val)) >>> + return true; >>> + >>> + if (val >= 0x100) >> A magic number like 0x100 should be #define-ed. >> >> similar below > Yes, I could improve this. > >>> + return true; >>> + >>> + if (n3000_nios_readl(ns, PKVL_B_MODE_STS, &val)) >>> + return true; >>> + >>> + if (val >= 0x100) >>> + return true; >>> + >>> + return false; >>> +} >>> + >>> +static void dump_error_stat(struct dfl_n3000_nios *ns) >>> +{ >>> + unsigned int val; >>> + >>> + if (n3000_nios_readl(ns, PKVL_A_MODE_STS, &val)) >>> + return; >> Not being able to read the register is itself and error. >> >> Should add something like 'Could not read register PKVL_A_MODE_STS' >> >> similar below > Yes, I could add the error log. > >>> + >>> + dev_info(ns->dev, "PKVL_A_MODE_STS %x\n", val); >>> + >>> + if (n3000_nios_readl(ns, PKVL_B_MODE_STS, &val)) >>> + return; >>> + >>> + dev_info(ns->dev, "PKVL_B_MODE_STS %x\n", val); >>> +} >>> + >>> +static int n3000_nios_init_done_check(struct dfl_n3000_nios *ns) >>> +{ >>> + struct device *dev = ns->dev; >>> + unsigned int val, mode; >>> + int ret; >>> + >>> + /* >>> + * this SPI is shared by NIOS core inside FPGA, NIOS will use this SPI >>> + * master to do some one time initialization after power up, and then >>> + * release the control to OS. driver needs to poll on INIT_DONE to >>> + * see when driver could take the control. >>> + * >>> + * Please note that after 3.x.x version, INIT_START is introduced, so >>> + * driver needs to trigger START firstly and then check INIT_DONE. >>> + */ >>> + >>> + ret = n3000_nios_readl(ns, NIOS_FW_VERSION, &val); >>> + if (ret) >>> + return ret; >>> + >>> + /* >>> + * If NIOS version register is totally uninitialized(== 0x0), then the >>> + * NIOS firmware is missing. So host could take control of SPI master >>> + * safely, but initialization work for NIOS is not done. This is an >>> + * issue of FPGA image. We didn't error out because we need SPI master >>> + * to reprogram a new image. >>> + */ >>> + if (val == 0) { >>> + dev_warn(dev, "NIOS version reg = 0x%x, skip INIT_DONE check, but PKVL may be uninitialized\n", >>> + val); >>> + return 0; >>> + } >>> + >>> + if (FIELD_GET(NIOS_FW_VERSION_MAJOR, val) >= 3) { >>> + /* read NIOS_INIT to check if PKVL INIT done or not */ >>> + ret = n3000_nios_readl(ns, NIOS_INIT, &val); >>> + if (ret) >>> + return ret; >>> + >>> + /* check if PKVLs are initialized already */ >>> + if (val & NIOS_INIT_DONE || val & NIOS_INIT_START) >>> + goto nios_init_done; >>> + >>> + /* configure FEC mode per module param */ >>> + val = NIOS_INIT_START; >>> + >>> + /* FEC mode will be ignored by hardware in 10G mode */ >>> + if (!strcmp(fec_mode, "no")) >> strncmp is safer but would lead to 'no123' being valid. > We do use strcmp here, so are we OK here? Yes. >>> + mode = FEC_MODE_NO; >>> + else if (!strcmp(fec_mode, "kr")) >>> + mode = FEC_MODE_KR; >>> + else if (!strcmp(fec_mode, "rs")) >>> + mode = FEC_MODE_RS; >>> + else >>> + return -EINVAL; >>> + >>> + /* set the same FEC mode for all links */ >>> + val |= FIELD_PREP(REQ_FEC_MODE_A0, mode) | >>> + FIELD_PREP(REQ_FEC_MODE_A1, mode) | >>> + FIELD_PREP(REQ_FEC_MODE_A2, mode) | >>> + FIELD_PREP(REQ_FEC_MODE_A3, mode) | >>> + FIELD_PREP(REQ_FEC_MODE_B0, mode) | >>> + FIELD_PREP(REQ_FEC_MODE_B1, mode) | >>> + FIELD_PREP(REQ_FEC_MODE_B2, mode) | >>> + FIELD_PREP(REQ_FEC_MODE_B3, mode); >>> + >>> + ret = n3000_nios_writel(ns, NIOS_INIT, val); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> +nios_init_done: >>> + /* polls on NIOS_INIT_DONE */ >>> + ret = regmap_read_poll_timeout(ns->regmap, NIOS_INIT, val, >>> + val & NIOS_INIT_DONE, >>> + NIOS_INIT_TIME_INTV, >>> + NIOS_INIT_TIMEOUT); >>> + if (ret) { >>> + dev_err(dev, "NIOS_INIT_DONE %s\n", >>> + (ret == -ETIMEDOUT) ? "timed out" : "check error"); >>> + goto dump_sts; >>> + } >>> + >>> + /* >>> + * after INIT_DONE is detected, it still needs to check if any ERR >>> + * detected. >>> + * We won't error out here even if error detected. Nios will release >>> + * spi controller when INIT_DONE is set, so driver could continue to >>> + * initialize spi controller device. >>> + */ >>> + if (init_error_detected(ns)) { >>> + dev_warn(dev, "NIOS_INIT_DONE OK, but err found during init\n"); >>> + goto dump_sts; >>> + } >>> + return 0; >>> + >>> +dump_sts: >>> + dump_error_stat(ns); >>> + >>> + return ret; >>> +} >>> + >>> +struct spi_board_info m10_n3000_info = { >>> + .modalias = "m10-n3000", >>> + .max_speed_hz = 12500000, >>> + .bus_num = 0, >>> + .chip_select = 0, >>> +}; >>> + >>> +static int create_altr_spi_controller(struct dfl_n3000_nios *ns) >> Also here, altr -> altera > Yes > >>> +{ >>> + struct altera_spi_platform_data pdata = { 0 }; >>> + struct platform_device_info pdevinfo = { 0 }; >>> + void __iomem *base = ns->base; >>> + u64 v; >>> + >>> + v = readq(base + NIOS_SPI_PARAM); >>> + >>> + pdata.mode_bits = SPI_CS_HIGH; >>> + if (FIELD_GET(CLK_POLARITY, v)) >>> + pdata.mode_bits |= SPI_CPOL; >>> + if (FIELD_GET(CLK_PHASE, v)) >>> + pdata.mode_bits |= SPI_CPHA; >>> + >>> + pdata.num_chipselect = FIELD_GET(NUM_CHIPSELECT, v); >>> + pdata.bits_per_word_mask = >>> + SPI_BPW_RANGE_MASK(1, FIELD_GET(DATA_WIDTH, v)); >>> + >>> + pdata.num_devices = 1; >>> + pdata.devices = &m10_n3000_info; >>> + >>> + dev_dbg(ns->dev, "%s cs %u bpm 0x%x mode 0x%x\n", __func__, >>> + pdata.num_chipselect, pdata.bits_per_word_mask, >>> + pdata.mode_bits); >>> + >>> + pdevinfo.name = "subdev_spi_altera"; >>> + pdevinfo.id = PLATFORM_DEVID_AUTO; >>> + pdevinfo.parent = ns->dev; >>> + pdevinfo.data = &pdata; >>> + pdevinfo.size_data = sizeof(pdata); >>> + >>> + ns->altr_spi = platform_device_register_full(&pdevinfo); >>> + return PTR_ERR_OR_ZERO(ns->altr_spi); >>> +} >>> + >>> +static void destroy_altr_spi_controller(struct dfl_n3000_nios *ns) >>> +{ >>> + platform_device_unregister(ns->altr_spi); >>> +} >>> + >>> +static int ns_bus_poll_stat_timeout(void __iomem *base, u64 *v) >>> +{ >>> + int loops = NS_REGBUS_WAIT_TIMEOUT; >> Would not this be better as time based timeout ? >> >> Looping over int is only good if you know the clock freq etc. > Time based timeout is more reasonable, but it will drop the performance: > 1. The indirect bus is accessed so frequently. It is on the critical > path of FPGA image flashing. > 2. The state change time is very short, usually in 1 or 2 loops. > > We've profiled that if we add time check here then, FPGA image flash > time would be several minutes longer. > > Anyway, 10000 times loop is large enough. As this is unusual, please add a comment with this explanation. >> Could this polling be switch to interrupt ? > We don't have interrupt source here. > >>> + >>> + /* >>> + * Usually the state changes in few loops, so we try to be simple here >>> + * for performance. >>> + */ >>> + do { >>> + *v = readq(base + NIOS_SPI_STAT); >>> + if (*v & STAT_RW_VAL) >>> + break; >>> + cpu_relax(); >>> + } while (--loops); >>> + >>> + return loops ? 0 : -ETIMEDOUT; >>> +} >>> + >>> +static int ns_bus_reg_write(void *context, unsigned int reg, unsigned int val) >> what is 'ns_' ? > It is the abbreviation of nios_spi, is that OK? > I don't want a long prefix for everything, but still want a namespace. Ok, then add a comment 'ns = nios_spi' >> Maybe this should be 'nios_spi' or just drop the prefix. >> >>> +{ >>> + void __iomem *base = context; >>> + u64 v = 0; >>> + >>> + v |= FIELD_PREP(CTRL_CMD, CMD_WR); >>> + v |= FIELD_PREP(CTRL_ADDR, reg); >>> + v |= FIELD_PREP(CTRL_WR_DATA, val); >>> + writeq(v, base + NIOS_SPI_CTRL); >>> + >>> + return ns_bus_poll_stat_timeout(base, &v); >>> +} >>> + >>> +static int ns_bus_reg_read(void *context, unsigned int reg, unsigned int *val) >>> +{ >>> + void __iomem *base = context; >>> + u64 v = 0; >>> + int ret; >>> + >>> + v |= FIELD_PREP(CTRL_CMD, CMD_RD); >>> + v |= FIELD_PREP(CTRL_ADDR, reg); >>> + writeq(v, base + NIOS_SPI_CTRL); >>> + >>> + ret = ns_bus_poll_stat_timeout(base, &v); >>> + if (!ret) >>> + *val = FIELD_GET(STAT_RD_DATA, v); >>> + >>> + return ret; >>> +} >>> + >>> +static const struct regmap_config ns_regbus_cfg = { >>> + .reg_bits = 32, >>> + .reg_stride = 4, >>> + .val_bits = 32, >>> + .fast_io = true, >>> + >>> + .reg_write = ns_bus_reg_write, >>> + .reg_read = ns_bus_reg_read, >>> +}; >>> + >>> +static int dfl_n3000_nios_probe(struct dfl_device *dfl_dev) >>> +{ >>> + struct device *dev = &dfl_dev->dev; >>> + struct dfl_n3000_nios *ns; >>> + int ret; >>> + >>> + ns = devm_kzalloc(dev, sizeof(*ns), GFP_KERNEL); >>> + if (!ns) >>> + return -ENOMEM; >>> + >>> + dev_set_drvdata(&dfl_dev->dev, ns); >>> + >>> + ns->dev = dev; >>> + >>> + ns->base = devm_ioremap_resource(&dfl_dev->dev, &dfl_dev->mmio_res); >>> + if (IS_ERR(ns->base)) { >>> + dev_err(dev, "get mem resource fail!\n"); >>> + return PTR_ERR(ns->base); >>> + } >>> + >>> + ns->regmap = devm_regmap_init(dev, NULL, ns->base, &ns_regbus_cfg); >>> + if (IS_ERR(ns->regmap)) >>> + return PTR_ERR(ns->regmap); >>> + >>> + ret = n3000_nios_init_done_check(ns); >>> + if (ret) >>> + return ret; >>> + >>> + ret = create_altr_spi_controller(ns); >>> + if (ret) >>> + dev_err(dev, "altr spi controller create failed: %d\n", ret); >>> + >>> + return ret; >>> +} >>> + >>> +static void dfl_n3000_nios_remove(struct dfl_device *dfl_dev) >>> +{ >>> + struct dfl_n3000_nios *ns = dev_get_drvdata(&dfl_dev->dev); >>> + >>> + destroy_altr_spi_controller(ns); >>> +} >>> + >>> +#define FME_FEATURE_ID_N3000_NIOS 0xd >> To minimize collisions, maybe collect these to a new common header like 'dfl-features-ids.h' or just add to some other visible existing header. > I'm going to abstract the dfl-bus related stuff in a new head file, > maybe we could change it when more dfl devices are added. Yes. that is fine. Reviewed-by: Tom Rix > > Thanks, > Yilun. > >>> + >>> +static const struct dfl_device_id dfl_n3000_nios_ids[] = { >>> + { FME_ID, FME_FEATURE_ID_N3000_NIOS }, >>> + { } >>> +}; >>> + >>> +static struct dfl_driver dfl_n3000_nios_driver = { >>> + .drv = { >>> + .name = "dfl-n3000-nios", >>> + .dev_groups = n3000_nios_groups, >>> + }, >>> + .id_table = dfl_n3000_nios_ids, >>> + .probe = dfl_n3000_nios_probe, >>> + .remove = dfl_n3000_nios_remove, >>> +}; >>> + >>> +module_dfl_driver(dfl_n3000_nios_driver); >>> + >>> +MODULE_DEVICE_TABLE(dfl, dfl_n3000_nios_ids); >>> +MODULE_DESCRIPTION("DFL N3000 NIOS driver"); >>> +MODULE_AUTHOR("Intel Corporation"); >>> +MODULE_LICENSE("GPL v2");