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=-7.0 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED 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 14CF9C43381 for ; Tue, 26 Feb 2019 10:59:46 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D217A20842 for ; Tue, 26 Feb 2019 10:59:45 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="loOhW4ij"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=ti.com header.i=@ti.com header.b="kjwlFYNI" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D217A20842 Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=ti.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-mtd-bounces+linux-mtd=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date: Message-ID:From:References:To:Subject:Reply-To:Content-ID:Content-Description :Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=+JeNEfuTI0zBk4LrpTXuDKgk1LVyij9M55zRAZj6i0E=; b=loOhW4iju8rOFV WQoU6nAI08qF/tuU9zeTjZ2kKMB6BbguFJYcMMxdvwKvqWFxOQzpRWteYxY1KaGS+5yMCcmLxckes yttrgs4nhrS+q+JNtWPas2SCmzdT5xQoo7RI+2coJSVX6FmKBjVGvl/buLG+RCMejckjzzV0UU9Ey wZofCwUq5NhhzEH42bbdJmPzyvv2zeslN2qRg/dD+HJRJmEIYX5o6j3UzcpuM4s9nv1h8KlRAIF3H BL8nBwfExgBssHhjFU6PX7bHqzN4PXq7J+vuE6kvXMC/rH4aVi/UpAkN8oIGH/OesXUWXfE17Xk0A VEqIGsQ/BkFqMNhu6isg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1gyaSd-0003J4-Sv; Tue, 26 Feb 2019 10:59:43 +0000 Received: from fllv0015.ext.ti.com ([198.47.19.141]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1gyaSZ-0003Ie-QK; Tue, 26 Feb 2019 10:59:42 +0000 Received: from lelv0266.itg.ti.com ([10.180.67.225]) by fllv0015.ext.ti.com (8.15.2/8.15.2) with ESMTP id x1QAxThK114173; Tue, 26 Feb 2019 04:59:29 -0600 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1551178769; bh=xu8wrs2N42x599628JA5mvXzDvF/M2T/HAGD+1mUjL8=; h=Subject:To:CC:References:From:Date:In-Reply-To; b=kjwlFYNI+r0QLt9Es4I7p8ObYC0sK6tKBfeluNOApobJos/8X2Wa0kxbaL6AawOyi HVCqwLRL+yrP8r3mLuIDBi1TGAMjrAuV/bHi8PfhIHNwpT6yRuKIWOgkZEXyOTw9Rx G1Z5OyniDFDCN1jFgeaLDDUmbSJ1V3o/d+RmnUy8= Received: from DLEE109.ent.ti.com (dlee109.ent.ti.com [157.170.170.41]) by lelv0266.itg.ti.com (8.15.2/8.15.2) with ESMTPS id x1QAxT8T118117 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 26 Feb 2019 04:59:29 -0600 Received: from DLEE105.ent.ti.com (157.170.170.35) by DLEE109.ent.ti.com (157.170.170.41) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1591.10; Tue, 26 Feb 2019 04:59:29 -0600 Received: from dlep33.itg.ti.com (157.170.170.75) by DLEE105.ent.ti.com (157.170.170.35) with Microsoft SMTP Server (version=TLS1_0, cipher=TLS_RSA_WITH_AES_256_CBC_SHA) id 15.1.1591.10 via Frontend Transport; Tue, 26 Feb 2019 04:59:29 -0600 Received: from [172.24.190.89] (ileax41-snat.itg.ti.com [10.172.224.153]) by dlep33.itg.ti.com (8.14.3/8.13.8) with ESMTP id x1QAxO3M005249; Tue, 26 Feb 2019 04:59:25 -0600 Subject: Re: [RFC PATCH 3/5] mtd: Add support for Hyperbus memory devices To: Sergei Shtylyov , David Woodhouse , Brian Norris , Boris Brezillon , Marek Vasut , Richard Weinberger , Rob Herring References: <20190219063607.29949-1-vigneshr@ti.com> <20190219063607.29949-4-vigneshr@ti.com> <42d0fd7b-42e0-605c-70ee-6e308908fc90@cogentembedded.com> From: Vignesh R Message-ID: Date: Tue, 26 Feb 2019 16:30:25 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: <42d0fd7b-42e0-605c-70ee-6e308908fc90@cogentembedded.com> Content-Language: en-US X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190226_025939_939219_46090E15 X-CRM114-Status: GOOD ( 32.09 ) X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "devicetree@vger.kernel.org" , Arnd Bergmann , "tudor.ambarus@microchip.com" , Greg Kroah-Hartman , "Nori, Sekhar" , "linux-kernel@vger.kernel.org" , "linux-mtd@lists.infradead.org" , "linux-arm-kernel@lists.infradead.org" Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-mtd" Errors-To: linux-mtd-bounces+linux-mtd=archiver.kernel.org@lists.infradead.org On 24/02/19 1:49 AM, Sergei Shtylyov wrote: > Hello! > > On 02/19/2019 09:36 AM, Vignesh R (by way of Boris Brezillon ) wrote: > >> Cypress HyperBus is Low Signal Count, High Performance Double Data Rate Bus >> interface between a host system master and one or more slave interfaces. >> HyperBus is used to connect microprocessor, microcontroller, or ASIC >> devices with random access NOR flash memory(called HyperFlash) or > > Need space before (. > >> self refresh DRAM(called HyperRAM). >> >> Its a 8-bit data bus (DQ[7:0]) with Read-Write Data Strobe (RWDS) >> signal and either Single-ended clock(3.0V parts) or Differential clock >> (1.8V parts). It uses ChipSelect lines to select b/w multiple slaves. >> At bus level, it follows a separate protocol described in HyperBus >> specification[1]. >> >> HyperFlash follows CFI AMD/Fujitsu Extended Command Set (0x0002) similar >> to that of existing parallel NORs. Since Hyperbus is x8 DDR bus, >> its equivalent to x16 parallel NOR flash wrt bits per clk. But Hyperbus >> operates at >166MHz frequencies. >> HyperRAM provides direct random read/write access to flash memory >> array. >> >> But, Hyperbus memory controllers seem to abstract implementation details >> and expose a simple MMIO interface to access connected flash. >> >> Add support for registering HyperFlash devices with MTD framework. MTD >> maps framework along with CFI chip support framework are used to support >> communicate with flash. > > Communicating. > >> Framework is modelled along the lines of spi-nor framework. HyperBus >> memory controller(HBMC) drivers call hb_register_device() to register a > > Again, space needed before (. > >> single HyperFlash device. HyperFlash core parses MMIO access >> information from DT, sets up the map_info struct, probes CFI flash and >> registers it with MTD framework. >> >> Some HBMC masters need calibration/training sequence[3] to be carried >> out, in order for DLL inside the controller to lock, by reading a known >> string/pattern. This is done by repeatedly reading CFI Query >> Identification String. Calibration needs to be done before try to detect > > Trying. > >> flash as part of CFI flash probe. >> >> HyperRAM is not supported atm. > > ATM? > >> HyperBus specification can be found at[1] >> HyperFlash datasheet can be found at[2] >> >> [1] https://www.cypress.com/file/213356/download >> [2] https://www.cypress.com/file/213346/download >> [3] http://www.ti.com/lit/ug/spruid7b/spruid7b.pdf >> Table 12-5741. HyperFlash Access Sequence >> >> Signed-off-by: Vignesh R > [...] >> diff --git a/drivers/mtd/hyperbus/Kconfig b/drivers/mtd/hyperbus/Kconfig >> new file mode 100644 >> index 000000000000..02c38afc5c50 >> --- /dev/null >> +++ b/drivers/mtd/hyperbus/Kconfig >> @@ -0,0 +1,23 @@ >> +menuconfig MTD_HYPERBUS >> + tristate "Hyperbus support" >> + select MTD_CFI >> + select MTD_MAP_BANK_WIDTH_2 >> + select MTD_CFI_AMDSTD >> + select MTD_COMPLEX_MAPPINGS >> + help >> + This is the framework for the Hyperbus which can be used by >> + the Hyperbus Controller driver to commmunicate with > ^^^ > Too many m's. :-) > >> + Hyperflash. See Cypress Hyperbus specification for more >> + details >> + >> + >> +if MTD_HYPERBUS >> + >> +config HBMC_AM654 >> + tristate "Hyperbus controller driver for AM65x SoC" >> + help >> + This is the driver for Hyperbus controller on TI's AM65x and >> + other SoCs >> + >> +endif # MTD_HYPERBUS > > The above clearly belongs to patch #5. > >> + > > No empty lines at end of file, please... > >> diff --git a/drivers/mtd/hyperbus/Makefile b/drivers/mtd/hyperbus/Makefile >> new file mode 100644 >> index 000000000000..64e377d7f636 >> --- /dev/null >> +++ b/drivers/mtd/hyperbus/Makefile >> @@ -0,0 +1,4 @@ >> +# SPDX-License-Identifier: GPL-2.0 >> + >> +obj-$(CONFIG_MTD_HYPERBUS) += core.o >> +obj-$(CONFIG_HBMC_AM654) += hbmc_am654.o > > The above line clearly belongs to patch #5 as well... > >> diff --git a/drivers/mtd/hyperbus/core.c b/drivers/mtd/hyperbus/core.c >> new file mode 100644 >> index 000000000000..d3d44aab7503 >> --- /dev/null >> +++ b/drivers/mtd/hyperbus/core.c >> @@ -0,0 +1,167 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// >> +// Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/ >> +// Author: Vignesh R >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#define HB_CALIB_COUNT 25 > > Isn't this controller specific? > I can convert this to be a field in hb_device struct that controller driver can populate. But, I believe above count can still serve as conservative default. > [...] >> +/* Calibrate HBMC by repeatedly reading "QRY" string from CFI space */ >> +static int hb_calibrate(struct hb_device *hbdev) > > s/int/bool/, perhaps? > > [...] >> +int hb_register_device(struct hb_device *hbdev) >> +{ >> + struct resource res; >> + struct device *dev; >> + struct device_node *np; >> + struct map_info *map; >> + struct hb_ops *ops; >> + int err; >> + >> + if (!hbdev || !hbdev->dev || !hbdev->np) { >> + pr_err("hyperbus: please fill all the necessary fields!\n"); >> + return -EINVAL; >> + } >> + >> + np = hbdev->np; >> + if (!of_device_is_compatible(np, "cypress,hyperflash")) >> + return -ENODEV; >> + >> + hbdev->memtype = HYPERFLASH; >> + >> + if (of_address_to_resource(np, 0, &res)) > > Isn't the direct mapping property of the HF controller, not of HyperFlash > itself? > >> + return -EINVAL; >> + >> + dev = hbdev->dev; >> + map = &hbdev->map; >> + map->size = resource_size(&res); >> + map->virt = devm_ioremap_resource(dev, &res); >> + if (IS_ERR(map->virt)) >> + return PTR_ERR(map->virt); >> + >> + map->name = dev_name(dev); >> + map->bankwidth = 2; >> + >> + simple_map_init(map); > > It's not that simple, I'm afraid -- e.g. Renesas RPC-IF has read and write > mappings in the separate memory resources. > > [...] >> + if (hbdev->needs_calib) { >> + err = hb_calibrate(hbdev); >> + if (!err) { > > Why call this variable 'err' when it indicates successful calibration? > >> + dev_err(hbdev->dev, "Calibration failed\n"); >> + return -ENODEV; >> + } >> + } >> + >> + hbdev->mtd = do_map_probe("cfi_probe", map); >> + if (!hbdev->mtd) { >> + dev_err(hbdev->dev, "probing failed\n"); > > "map probe", perhaps? > >> + return -ENXIO; >> + } >> + >> + hbdev->mtd->dev.parent = hbdev->dev; >> + mtd_set_of_node(hbdev->mtd, np); >> + >> + err = mtd_device_register(hbdev->mtd, NULL, 0); >> + if (err) { >> + dev_err(hbdev->dev, "failed to register mtd device\n"); >> + goto err_destroy; >> + } >> + hbdev->registered = true; >> + >> + return 0; >> + >> +err_destroy: > > The label and the code below doesn't seem necessary. Just do it above > instead of *goto*. > >> + map_destroy(hbdev->mtd); >> + return err; >> +} > [...] >> diff --git a/include/linux/mtd/hyperbus.h b/include/linux/mtd/hyperbus.h >> new file mode 100644 >> index 000000000000..0aa11458c424 >> --- /dev/null >> +++ b/include/linux/mtd/hyperbus.h >> @@ -0,0 +1,73 @@ >> +/* SPDX-License-Identifier: GPL-2.0 >> + * >> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/ >> + */ >> + >> +#ifndef __LINUX_MTD_HYPERBUS_H__ >> +#define __LINUX_MTD_HYPERBUS_H__ >> + >> +#include >> + >> +enum hb_memtype { > > I'm for the full hyperbus_ prefixes, it's not that long and seems clearer. > > [...] > > MBR, Sergei > -- Regards Vignesh ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/