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.1 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FORGED_MUA_MOZILLA,FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,MSGID_FROM_MTA_HEADER,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 BAF8FC48BDF for ; Mon, 14 Jun 2021 00:28:33 +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 A2B3E610CD for ; Mon, 14 Jun 2021 00:28:32 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A2B3E610CD Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=outlook.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 E3EAB80C58; Mon, 14 Jun 2021 02:28:29 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=outlook.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=outlook.com header.i=@outlook.com header.b="B2sqpexs"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 664A380EC6; Mon, 14 Jun 2021 02:28:27 +0200 (CEST) Received: from APC01-SG2-obe.outbound.protection.outlook.com (mail-sg2apc01olkn0818.outbound.protection.outlook.com [IPv6:2a01:111:f400:febd::818]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 85B7F80412 for ; Mon, 14 Jun 2021 02:28:19 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=outlook.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=tianrui-wei@outlook.com ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=fKbq0Vqpk4cry7kQTkDedSxVNFHNyZVarSWEyB5bY5Ua99qw2NZv7wEN/5TYNXp+NWm3bSuoKpTiJIzI0wV6amhTfUG0x3aLr4QOE8R+4iqV225w+gndzX8e1Pr7CpXVWe5AmR28TUnT1D0c2DBDCF/ooWSfvwVCglGYSMLEye1cCW4g3boe08uSR3e8kPqPoasli8VPuGw4X9KLRb7GIBtvzvEekKiN92oPC3OevygNSYVJ/5CHI2wZH9VQkAS/2sMFURa9UtCoJneC2AfImhYMxVMz5eLmLO9njLPxmGfCIZuSloe4/vWPT5RFQqwN8gzIPI1UsDdbfG+o/s2HXg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=xBw6Yo2I5h1AlKZRbJinFwLPrDXFBK0w8oDprYIMsLI=; b=JDWh66QDkQjE/Nump863VcNWiELthVEMHTOw7ZRjncbY9EjTqKFi6SD19vsDfB/V+rmaqO4+Gyx3tWj5e3vgg1BC7NWsjDVXLesjXeNZoYRHUUY2U2l/klj83LYli0OtgaG5UW4hD4a0aj/ceZCnMvp3Ko3t3VXvnehjj2JsD06F8euPjg65ZqUQkJkTzarI1eSiaHLLrr4nTmN87saEDQpoi5ThSXmpnNwCTZ8XLtWld3IVcNQ7S3OK40eh40dtx87xkBFWQLmZBgcRWLMyTMfVeXxknjtGA6zA1kvg7YjMxsxiBbj1fNf389pWgVFr5T/6q4iHOU/pmzcmFcDbmg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outlook.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=xBw6Yo2I5h1AlKZRbJinFwLPrDXFBK0w8oDprYIMsLI=; b=B2sqpexsP8vBgZR+Kzu5UEtvlkVMb+mVaiEi2MY8xL+K7G3z80isVjzLoOkFT/o3iT0vZl4ybfX7HS0EzDIZzvFfqO2+TurH6oIpqOpaUayOUBnfchtW9PBwcwWmLuH3EdGMxzt+wBQZbp3EWcn7iGDjZZFkUfWPn7zbcPOOfGt4PgpSml8DPvFxOtZFVxKQnCo2Go+7/wyqFi1tOSSmXfgpURzaibBrsHMTv6KiONHWJMrPwm/B4r+ix1OIKXzRRNBjZ1bmGT1MtZjzhXX33/8CjqFJbnbq656UyAp66hacWfdUMAAFqNTNpMH0TlZwtmHMeidiuSJeuFsRPeYFNA== Received: from SG2PR06CA0125.apcprd06.prod.outlook.com (2603:1096:1:1d::27) by SG2PR03MB4280.apcprd03.prod.outlook.com (2603:1096:4:2b::17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4242.13; Mon, 14 Jun 2021 00:28:15 +0000 Received: from SG2APC01FT019.eop-APC01.prod.protection.outlook.com (2603:1096:1:1d:cafe::8a) by SG2PR06CA0125.outlook.office365.com (2603:1096:1:1d::27) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4219.21 via Frontend Transport; Mon, 14 Jun 2021 00:28:15 +0000 Received: from SY4PR01MB6798.ausprd01.prod.outlook.com (10.152.250.51) by SG2APC01FT019.mail.protection.outlook.com (10.152.250.121) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4219.21 via Frontend Transport; Mon, 14 Jun 2021 00:28:15 +0000 X-IncomingTopHeaderMarker: OriginalChecksum:C3F00489C48499143B1F178B392BAEF8F5798E80DB9C24BA902F597140FE097C; UpperCasedChecksum:C4EEDF28CC1DA587D57D6A4F6BCA496045302B8445C1517BFA0638844EE90803; SizeAsReceived:9014; Count:48 Received: from SY4PR01MB6798.ausprd01.prod.outlook.com ([fe80::5476:5394:7bb7:6941]) by SY4PR01MB6798.ausprd01.prod.outlook.com ([fe80::5476:5394:7bb7:6941%2]) with mapi id 15.20.4219.025; Mon, 14 Jun 2021 00:28:13 +0000 Subject: Re: [RESEND PATCH v6 2/2] mmc: openpiton: add piton_mmc driver To: Jaehoon Chung , u-boot@lists.denx.de Cc: ycliang@andestech.com, rick@andestech.com, peng.fan@nxp.com, jbalkind@ucsb.edu, seanga2@gmail.com, bmeng.cn@gmail.com References: <20210612171650.7247-1-tianrui-wei@outlook.com> From: Tianrui Wei Message-ID: Date: Mon, 14 Jun 2021 08:28:08 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-TMN: [kbA9/Rdf+e5LimtKFgUW8ZmuwmyhvqV5] X-ClientProxiedBy: HK2PR02CA0155.apcprd02.prod.outlook.com (2603:1096:201:1f::15) To SY4PR01MB6798.ausprd01.prod.outlook.com (2603:10c6:10:137::12) X-Microsoft-Original-Message-ID: <3a2c6704-1f9c-f944-e254-202d843b2474@outlook.com> MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from [192.168.3.161] (180.160.51.170) by HK2PR02CA0155.apcprd02.prod.outlook.com (2603:1096:201:1f::15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4219.21 via Frontend Transport; Mon, 14 Jun 2021 00:28:11 +0000 X-MS-PublicTrafficType: Email X-IncomingHeaderCount: 48 X-EOPAttributedMessage: 0 X-MS-Office365-Filtering-Correlation-Id: 27373e53-60cf-4279-be4b-08d92ecb4b99 X-MS-TrafficTypeDiagnostic: SG2PR03MB4280: X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: j80K9zrNdp99c6QR2nqwEOTHRXyf0DQMJqYTUb0NjZTCrQX71qnJuW9LlpJF7LlWzq6u5xSGdJ20rA7NGzPu3lG2VJ57TQo5Zc9NxAwFO7NUpKdFYl4MKeHJGngdm+jXSHxroUhfT2eCKus637l7IouWJrvTWUjDABHJ/x/TysfjkzwTNDVPN7D9vCC49fP+3y0U1vG4j9L9wlmqaHIGsO7IA8SRU8KFxQw7oSeDr0EOPxFHnvJ+A3xeAF86p2LMBk0fcH9DKK3EROb6lKkxqo8yYidHRUqpdhMffzHdcY35F9yxqi9zJHSTZqwo860KHSLvTgUxfFkv4SGOrxte9/oWmO64f/JaUj2vwj3KIxtMA9axlSiKB1TRh1tXtIegoiuQ+igKEXAJXM4ZKSQgtw== X-MS-Exchange-AntiSpam-MessageData: T0PMJgVFc3TUXKabjn9P0kQpvq/1/TxfXHCa5xQM1SOUs4a6fcpSkcvC+Z9hlwRUwtZwSOPJxtBAV7fTJf62uD9qQlAuR6vmUNHRa5C6f1zQh897qQvdeFOgEh2XARX4XZWP/pe5ca4OUBfl/KrrLQ== X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 27373e53-60cf-4279-be4b-08d92ecb4b99 X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jun 2021 00:28:13.4128 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-CrossTenant-AuthSource: SG2APC01FT019.eop-APC01.prod.protection.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: Internet X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-Transport-CrossTenantHeadersStamped: SG2PR03MB4280 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.103.2 at phobos.denx.de X-Virus-Status: Clean Hi Jaehoon, Many thanks for taking the time to review our patches :P On 6/14/2021 6:09 AM, Jaehoon Chung wrote: > Dear Tianrui, > > On 6/13/21 2:16 AM, Tianrui Wei wrote: >> This commit adds support to piton_mmc driver for OpenPiton-riscv64 >> In particular, this driver >> >> - V6 >> . change type of address >> . move declarations ahead >> . loop style update >> >> Signed-off-by: Tianrui Wei >> Signed-off-by: Jonathan Balkind >> --- >> drivers/mmc/Kconfig | 9 +++ >> drivers/mmc/Makefile | 1 + >> drivers/mmc/piton_mmc.c | 169 ++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 179 insertions(+) >> create mode 100644 drivers/mmc/piton_mmc.c >> >> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig >> index 8901456967..096d6a930c 100644 >> --- a/drivers/mmc/Kconfig >> +++ b/drivers/mmc/Kconfig >> @@ -727,6 +727,15 @@ 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 selects support for the SD host controller on >> + OpenPiton SoC. Note that this SD controller directly >> + exposes the contents of the SD card as memory mapped, >> + so there is no manual configuration required > Fix indentation. Will do > >> + >> 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 89d6af3db3..cc08b41d0d 100644 >> --- a/drivers/mmc/Makefile >> +++ b/drivers/mmc/Makefile >> @@ -72,6 +72,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 > Ditto. > >> 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 0000000000..32671d1a89 >> --- /dev/null >> +++ b/drivers/mmc/piton_mmc.c >> @@ -0,0 +1,169 @@ >> +// 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 { >> + void __iomem *piton_mmc_base_addr; /* peripheral id */ >> +}; >> + >> +/* >> + * see mmc_read_blocks to see how it is used. >> + * start block is hidden at cmd->arg >> + * also, initialize the block size at init >> + */ >> +static int piton_mmc_send_cmd(struct udevice *dev, struct mmc_cmd *cmd, >> + struct mmc_data *data) > Ditto. Will do > >> +{ >> + /* check first if this is a pure command */ >> + if (!data) >> + return 0; >> + >> + struct piton_mmc_priv *priv = dev_get_priv(dev); >> + u32 *buff, *start_addr; >> + size_t byte_cnt, start_block; >> + >> + buff = (u32 *)data->dest; >> + start_block = cmd->cmdarg; >> + start_addr = priv->piton_mmc_base_addr + start_block; >> + >> + /* if there is a read */ >> + if (data->flags & MMC_DATA_READ) { >> + for (byte_cnt = data->blocks * data->blocksize; byte_cnt; >> + byte_cnt -= sizeof(u32)) { >> + *buff++ = readl(start_addr++); >> + } >> + } else { >> + return -ENOSYS; > Is is right to return -ENOSYS? MMC_DATA_WRITE doesn't support? Will change to support it > >> + } >> + >> + 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 = (void *)dev_read_addr(dev); >> + cfg = &plat->cfg; >> + cfg->name = "PITON MMC"; >> + cfg->host_caps = MMC_MODE_8BIT; >> + cfg->f_max = 100000; > f_max is 100K? I don't think so. We don't use f_max anywhere, so we're setting dummy values :P > >> + 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; > Use macro. It's not readable. What's 0x2000000000ULL? Will do > >> + mmc->has_init = 1; > Right? has_init will be set in mmc.c. > Why it's set to 1 in driver? Our mmc controller directly maps the contents of the SD card into a memory region, so there're no configuration registers avaiable. We're setting these values to avoid problems with mmc.c > >> + >> + 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 the micro sd card is 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; > Ditto. Ditto > >> + upriv->mmc->capacity = 0x2000000000ULL; > Ditto. Ditto > >> + upriv->mmc->read_bl_len = MMC_MAX_BLOCK_LEN; > readl_blk_len will be set in mmc.c. > Your driver doesn't use mmc.c? Why do you set to some values in your driver? Ditto > >> + 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; > Use a readable value instead of U32_MAX. CONFIG_SYS_MMC_MAX_BLK_COUNT or other. Will do. Thanks for your review and helpful comments. We're very keen to get this upstream so I will address your issues ASAP and get a new version submitted. Regarding readl_blk_len, our MMC is a bit of a special case in that it's directly memory mapped under the hood. Can we go ahead with this version or might you have another suggestion? Best regards, Tianrui > > Best Regards, > Jaehoon Chung > >> + >> + 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), >> +}; >>