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=-14.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,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 A6B0DC47096 for ; Sun, 6 Jun 2021 17:16:06 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id F1B8D613C8 for ; Sun, 6 Jun 2021 17:16:05 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org F1B8D613C8 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id A6B6A81660; Sun, 6 Jun 2021 19:16:03 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="WizCBg/j"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id A87B18283E; Sun, 6 Jun 2021 19:16:01 +0200 (CEST) Received: from mail-qk1-x730.google.com (mail-qk1-x730.google.com [IPv6:2607:f8b0:4864:20::730]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id AECCE80C92 for ; Sun, 6 Jun 2021 19:15:57 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=seanga2@gmail.com Received: by mail-qk1-x730.google.com with SMTP id i67so14502784qkc.4 for ; Sun, 06 Jun 2021 10:15:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=UBQIgn+k7cNs6kwkdc8Jipxeu0cJw9GDwlpqfcBlp0Y=; b=WizCBg/jphMEJeh9EVltrl7NRoOYZeSaXBA3C/R8GRXsD4JpPNY9y0C3VP2Z+G3B7w Q2OYY53x2grpDTx1H5iZy+CW8ozM1h1A1ySVtwGQQ+8pMWxpRVkBluRSyn9bng0BLKuD JjEDSvz8POU9UCa1k75HmbymyjG2mAPQ9rKHQWBkifdSW6xrJrsTtdmY33n8xUTjpnnE H5Ybyz4MogL4ot8UTW1Uvm0g+nFY6LMyzagl5t+a6RzcOeCm+8HT1KYfM3n1Mc990mRF t1Kv97zPp1sZyQ0TxTNT7n//H66SNPhJddC4MtAo6iRE0NS2TupHJL8HqE8a+sZvKisA 2VWw== 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-language :content-transfer-encoding; bh=UBQIgn+k7cNs6kwkdc8Jipxeu0cJw9GDwlpqfcBlp0Y=; b=Uy35ucfom23yHG9jAVV/04g952eeSNbdMP+SiqOJ46636lh75ZYsomocgTMKxYI/PV E38VmVYT7SIjuwgN8/LjgFky1bqM2iiTV1rZLV4a224fyb7HUk9VBZeQU0b02p2yuuJw w1hRWvYiSoq6joLoBoQKDCHvZQJn0PQ09l+8OjLUXtjB6lek+irIzdYRL+PaPAWWyMuy wD/tV+Cr9aAKZejbp21TgCbWT9h7Wv+8HuIQ+JsxTRCrPykY+vxqrbcl99zi4okKQQ1Q a7IhV7GgS4Cfp5not1lxvMBktyGBx0E1Afcu1Wce8ZAIakHbUFjn7tH+3Qos1GULX5sr OvkA== X-Gm-Message-State: AOAM533Qo+h8p2ZyXMaXzaQsrf1aa9bQuK+kThEo9Hf2StkvpT2jad+S /pCgwJ3QZx+LxY3S6f6RujY= X-Google-Smtp-Source: ABdhPJzKudNk/n8uBDEK2+nsX+xoj9DSQ8zpc/k4Mht2bbgeLH/z9FTZsMevd8iuSTtRFkV/PpUcBQ== X-Received: by 2002:a37:4694:: with SMTP id t142mr13533050qka.265.1622999756583; Sun, 06 Jun 2021 10:15:56 -0700 (PDT) Received: from [192.168.1.201] (pool-74-96-87-9.washdc.fios.verizon.net. [74.96.87.9]) by smtp.googlemail.com with ESMTPSA id p2sm8116417qkj.94.2021.06.06.10.15.55 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 06 Jun 2021 10:15:56 -0700 (PDT) Subject: Re: [PATCH V5 1/2] mmc: add OpenPiton mmc support To: Tianrui Wei , u-boot@lists.denx.de Cc: ycliang@andestech.com, rick@andestech.com, peng.fan@nxp.com, jh80.chung@samsung.com, jbalkind@ucsb.edu, bmeng.cn@gmail.com, xypron.glpk@gmx.de References: <3b2fc708-b9ec-b49f-08e3-69ef79fbcb39@gmail.com> From: Sean Anderson Message-ID: <1454c57c-332b-43d5-c838-e8a3f0b156a4@gmail.com> Date: Sun, 6 Jun 2021 13:15:55 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.102.4 at phobos.denx.de X-Virus-Status: Clean On 6/6/21 12:57 PM, Tianrui Wei wrote: > Dear Sean, > > Many thanks for taking the time and energy to review our patches ( again ) > And sorry about not responding or not implementing them. I'll go through > them one by one and try to doing so in the future :P > > On 6/7/21 12:34 AM, Sean Anderson wrote: >> On 6/4/21 12:52 AM, Tianrui Wei wrote: >>> From: Tianrui Wei >>> Date: Fri, 4 June 2021 12:45:29 +0800 >>> Subject: [PATCH v5 1/2] mmc: add OpenPiton mmc support >>> >>> This patch adds mmc support for OpenPiton board. >>> >>> Changelog: >>> >>> v5: move definitions around, changed some incompatible type information >>> >>> Signed-off-by: Tianrui Wei >>> Signed-off-by: Jonathan Balkind >>> --- >>> >>> drivers/mmc/Kconfig | 7 + >>> drivers/mmc/Makefile | 1 + >>> drivers/mmc/piton_mmc.c | 174 +++++ >>> 3 files changed, 182 insertions(+) >>> create mode 100644 drivers/mmc/piton_mmc.c >>> >>> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig >>> index 14d79139..1038800f 100644 >>> --- a/drivers/mmc/Kconfig >>> +++ b/drivers/mmc/Kconfig >>> @@ -707,6 +707,13 @@ config MMC_SUNXI_HAS_MODE_SWITCH >>> bool >>> depends on MMC_SUNXI >>> +config MMC_PITON >>> + bool "MMC support for openpiton SoC" >>> + depends on DM_MMC && BLK >>> + help >>> + This driver enables SD card support in U-Boot port for >> >> We already know it's for U-Boot ;) > > :P > >> >>> + OpenPiton SoC >> >> Please add a bit more than this. In particular, it would be useful to >> note that this is an "emulated" device where the host does all of the >> work. > > Will do :P > >> >>> + >>> config GENERIC_ATMEL_MCI >>> bool "Atmel Multimedia Card Interface support" >>> depends on DM_MMC && BLK && ARCH_AT91 >>> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile >>> index 1c849cba..698dfe05 100644 >>> --- a/drivers/mmc/Makefile >>> +++ b/drivers/mmc/Makefile >>> @@ -71,6 +71,7 @@ obj-$(CONFIG_MMC_SDHCI_XENON) += xenon_sdhci.o >>> obj-$(CONFIG_MMC_SDHCI_ZYNQ) += zynq_sdhci.o >>> obj-$(CONFIG_MMC_SUNXI) += sunxi_mmc.o >>> +obj-$(CONFIG_MMC_PITON) += piton_mmc.o >>> obj-$(CONFIG_MMC_UNIPHIER) += tmio-common.o uniphier-sd.o >>> obj-$(CONFIG_RENESAS_SDHI) += tmio-common.o renesas-sdhi.o >>> obj-$(CONFIG_MMC_BCM2835) += bcm2835_sdhost.o >>> diff --git a/drivers/mmc/piton_mmc.c b/drivers/mmc/piton_mmc.c >>> new file mode 100644 >>> index 00000000..2b2d5756 >>> --- /dev/null >>> +++ b/drivers/mmc/piton_mmc.c >>> @@ -0,0 +1,174 @@ >>> +// SPDX-License-Identifier: GPL-2.0+ >>> +/* >>> + * (C) Copyright 2009 SAMSUNG Electronics >>> + * Minkyu Kang >>> + * Jaehoon Chung >>> + * Portions Copyright 2011-2019 NVIDIA Corporation >>> + * Portions Copyright 2021 Tianrui Wei >>> + * This file is adapted from tegra_mmc.c >>> + * Tianrui Wei >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +struct piton_mmc_plat { >>> + struct mmc_config cfg; >>> + struct mmc mmc; >>> +}; >>> + >>> +struct piton_mmc_priv { >>> + u64 piton_mmc_base_addr; /* peripheral id */ >> >> Again, please use void __iomem *. > > Sorry, forgot this change :( > >> >>> +}; >>> + >>> +/* >>> + * see mmc_read_blocks to see how it is used. >>> + * start block is hidden at cmd->arg >> >> Hidden? > > I wrote this comment a long time ago, I'll comment it out. > >> >>> + * also, initialize the block size at init >>> + */ >>> +static int piton_mmc_send_cmd(struct udevice *dev, struct mmc_cmd *cmd, >>> + struct mmc_data *data) >> >> Nit: alignment > > I'm not sure how to align this? Should I align the struct in the first line > and the second line? It appears so in my email client :P In your original email, this second line has 7 tabs, whereas the line above it has 29 characters before the `(`. I'm not sure why it's aligned in my reply. You can see the misaligned issue if you view your patch in patchwork. > >> >>> +{ >>> + /* check first if this is a pure command */ >>> + if (!data) >>> + return 0; >> >> Variable declarations come before code. > > Will do :P > >> >>> + >>> + u64 byte_cnt, start_block, start_addr; >> >> I think these should be size_t, since you use it for pointer >> arithmetic. > > Will do :P > >> >>> + >>> + struct piton_mmc_priv *priv = dev_get_priv(dev); >>> + unsigned long *buff = (unsigned long *)data->dest; >>> + >>> + start_addr = priv->piton_mmc_base_addr + (start_block); >>> + byte_cnt = data->blocks * data->blocksize; >> >> What should happen if (byte_cnt & 3)? > > The 1th or 3rd byte will be read into buf, Ah, disregard this; presumably blocksize will always be a (large enough) power of two. > >> >>> + start_block = cmd->cmdarg; >>> + >>> + /* if there is a read */ >>> + if (data->flags & MMC_DATA_READ) { >>> + for (u64 i = 0; i < byte_cnt; i += 4) { >> >> Declare i with the rest of the variables. >> >>> + *(buff) = readl((void *)(start_addr + i)); >> >> I think this could be rewritten as >> >> size_t byte_cnt; >> u32 *buff, *start_addr; >> >> /* ... */ >> >> for (byte_cnt = /* ... */; byte_cnt; byte_cnt -= sizeof(u32)); >> *buff++ = readl(start_addr++); >> >> Which more closely follows the model of memcpy. Speaking of which, I >> thought you were going to use it: > > Thanks for the style suggestion :P I'll use that > >> >>> Scratch the previous email, I stand corrected. You're right about >>> memcpy could work, thanks for the great suggestion :P >> >> What happened to that? > > I checked with Professor Balkind regarding memcpy, and there were some > issues with it because it's accessing an I/O device we need to maintain > particular alignment which memcpy might not do, so we're sticking with > the old implementation. Ok. > >> >>> + buff++; >>> + } >>> + } else { >>> + /* else there is a write >>> + * we don't handle write, so error right away >>> + */ >>> + return -ENODEV; >> >> Please use -ENOSYS. See [1] for details. > > Will do :P > >> >> In general, several of my comments on your previous patch are still >> unaddressed. I would like if you could either respond to them with your >> reasoning for not addressing them, or implement them. > > Will definitely do that in the future :P > > Tianrui > >> >> --Sean >> >> >> [1] https://apac01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fu-boot.readthedocs.io%2Fen%2Flatest%2Fdevelop%2Fdriver-model%2Fdesign.html%23error-codes&data=04%7C01%7C%7Cf72fe8fce39746732fdd08d92908e60b%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637585940462351512%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ncVph6yZoK4UaDnH2f3js3dsxtwNWih1fhI%2BwCq6jfw%3D&reserved=0 >> >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static int piton_mmc_ofdata_to_platdata(struct udevice *dev) >>> +{ >>> + struct piton_mmc_priv *priv = dev_get_priv(dev); >>> + struct piton_mmc_plat *plat = dev_get_platdata(dev); >>> + struct mmc_config *cfg; >>> + struct mmc *mmc; >>> + /* fill in device description */ >>> + struct blk_desc *bdesc; >>> + >>> + priv->piton_mmc_base_addr = dev_read_addr(dev); >>> + cfg = &plat->cfg; >>> + cfg->name = "PITON MMC"; >>> + cfg->host_caps = MMC_MODE_8BIT; >>> + cfg->f_max = 100000; >>> + cfg->f_min = 400000; >>> + cfg->voltages = MMC_VDD_21_22; >>> + >>> + mmc = &plat->mmc; >>> + mmc->read_bl_len = MMC_MAX_BLOCK_LEN; >>> + mmc->capacity_user = 0x100000000; >>> + mmc->capacity_user *= mmc->read_bl_len; >>> + mmc->capacity_boot = 0; >>> + mmc->capacity_rpmb = 0; >>> + for (int i = 0; i < 4; i++) >>> + mmc->capacity_gp[i] = 0; >>> + mmc->capacity = 0x2000000000ULL; >>> + mmc->has_init = 1; >>> + >>> + bdesc = mmc_get_blk_desc(mmc); >>> + bdesc->lun = 0; >>> + bdesc->hwpart = 0; >>> + bdesc->type = 0; >>> + bdesc->blksz = mmc->read_bl_len; >>> + bdesc->log2blksz = LOG2(bdesc->blksz); >>> + bdesc->lba = lldiv(mmc->capacity, mmc->read_bl_len); >>> + >>> + return 0; >>> +} >>> + >>> +/* test if piton has the micro mmc card present >>> + * always return 1, which means present >>> + */ >>> +static int piton_mmc_getcd(struct udevice *dev) >>> +{ >>> + /* >>> + * always return 1 >>> + */ >>> + return 1; >>> +} >>> + >>> +/* dummy function, piton_mmc don't need initialization >>> + * in hw >>> + */ >>> +static const struct dm_mmc_ops piton_mmc_ops = { >>> + .send_cmd = piton_mmc_send_cmd, >>> + .get_cd = piton_mmc_getcd, >>> +}; >>> + >>> +static int piton_mmc_probe(struct udevice *dev) >>> +{ >>> + struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); >>> + struct piton_mmc_plat *plat = dev_get_platdata(dev); >>> + struct mmc_config *cfg = &plat->cfg; >>> + >>> + cfg->name = dev->name; >>> + upriv->mmc = &plat->mmc; >>> + upriv->mmc->has_init = 1; >>> + upriv->mmc->capacity = 0x2000000000ULL; >>> + upriv->mmc->read_bl_len = MMC_MAX_BLOCK_LEN; >>> + >>> + return 0; >>> +} >>> + >>> +static int piton_mmc_bind(struct udevice *dev) >>> +{ >>> + struct piton_mmc_plat *plat = dev_get_platdata(dev); >>> + struct mmc_config *cfg = &plat->cfg; >>> + >>> + cfg->name = dev->name; >>> + cfg->host_caps = MMC_MODE_HS_52MHz | MMC_MODE_HS | MMC_MODE_8BIT; >>> + cfg->voltages = MMC_VDD_165_195 | MMC_VDD_32_33 | MMC_VDD_33_34; >>> + cfg->f_min = 1000000; >>> + cfg->f_max = 52000000; >>> + cfg->b_max = U32_MAX; >>> + >>> + return mmc_bind(dev, &plat->mmc, cfg); >>> +} >>> + >>> +static const struct udevice_id piton_mmc_ids[] = { >>> + {.compatible = "openpiton,piton-mmc"}, >>> + { /* sentinel */ } >>> +}; >>> + >>> +U_BOOT_DRIVER(piton_mmc_drv) = { >>> + .name = "piton_mmc", >>> + .id = UCLASS_MMC, >>> + .of_match = piton_mmc_ids, >>> + .ofdata_to_platdata = piton_mmc_ofdata_to_platdata, >>> + .bind = piton_mmc_bind, >>> + .probe = piton_mmc_probe, >>> + .ops = &piton_mmc_ops, >>> + .platdata_auto_alloc_size = sizeof(struct piton_mmc_plat), >>> + .priv_auto_alloc_size = sizeof(struct piton_mmc_priv), >>> +}; >>> >>