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=-15.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 995E6C64E7C for ; Wed, 2 Dec 2020 07:03:35 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (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 11B19221FB for ; Wed, 2 Dec 2020 07:03:35 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 11B19221FB Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=jms.id.au Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From:In-Reply-To: References:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=F/ipMlhOrsH4RI0sRjSZXu9lbDSgEDaJjbxHDKHHSjE=; b=DykN2AqT/a9s3BRL4PfRxK/bC xmZMLjQayDEDw2/Zdq6ot1J5TU9fru1atou0kD838Ith4ACGbX8jvd2Mj8iJqLqRC3uju1DkjsGdF WYVKSkxBLxkFvQ6a+hHHZ7trori07T0c33ZGTyydmZ57BwLg06B4JHBgNwt+twuBJVzhNGXtfS3L8 duzqnOMbz3PUDeI/45cAbUinsRKuK89efNS06QdQgMcJ6A6+2MESQAlUNHMQ3QvZ4oFMYL1mhMgtm WVOADsiSSU1BZC0GlnZf5va4aSHbU/y96qaJCoRn4n4ENGHAQ9lzGdJff01pfPF5gN/ADVC9EkSAg FnNWWWi5A==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kkM97-0001vr-8v; Wed, 02 Dec 2020 07:01:49 +0000 Received: from mail-qv1-xf42.google.com ([2607:f8b0:4864:20::f42]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kkM93-0001v1-Sa for linux-arm-kernel@lists.infradead.org; Wed, 02 Dec 2020 07:01:47 +0000 Received: by mail-qv1-xf42.google.com with SMTP id y11so258000qvu.10 for ; Tue, 01 Dec 2020 23:01:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=jms.id.au; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=MA2zJo9bj/EIZzJG0XmMx/IZDBJAL5UetxtUGnb1JcE=; b=aBFCXZNYzdc6s0fqQRq03Fap97IXfUONqVQg+WwwECxF143wymeGH5G8zL9AWOZCSZ 3scwloDVjjHtmgPlPSgWZdGhGAY+5nUQxQDMQ44aMQDEWts2u7mQ/TKbz6rFL3pQNcjb jWXLMnNqZHtxnkuWGlsg25PRob8WZcqoB24zE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=MA2zJo9bj/EIZzJG0XmMx/IZDBJAL5UetxtUGnb1JcE=; b=Gbq+NEt8uWqi3UIeCZA4V2/L41+zdrU09A/pqEKqsPQKOkMN4APUTps38qRe/gC1qY i+Uvih3lauq7TSqbcBVqJOfVbIxFzVsIhXSMCIO7hvi1ZwZU8A/PPSq87sOrI299S6Kb htwrBgh4TlMCLua8SuKB6IzYxvopoU+EtnPSaQSUlF4bXEP2eJjh6+NIEAOyueZL4LRK 3nJ4Ai8IOYAEljBQ+iLghSfhc46c89OJpNDiF/u/kH85wUssiKpn8idW48E4jFr3vYgH 4me8oIMVd/W4Ew5nYf+xW6wXsMo0pLKHNLUwoXdwI+OzpHdp7wAgy1a1wdiNRjJqz1Z5 7qPA== X-Gm-Message-State: AOAM530DQaUS/9HHQyplt2RW080QvSw3TzAmOKTdGS2RrXyx3j4dmcXG g+YHKzF7wAzQvE6Z6KH0RTaIXD1mhDIaMXLezv4= X-Google-Smtp-Source: ABdhPJwr1ikAABYLva9tQV6P42ObH2nWpxzGr7QZQK5w1rB8Fb6Ep334b2uhmyLtfItMHk49IniPr/zAGK29uoOqoyg= X-Received: by 2002:ad4:5687:: with SMTP id bc7mr1039099qvb.61.1606892503587; Tue, 01 Dec 2020 23:01:43 -0800 (PST) MIME-Version: 1.0 References: <20201202063612.21241-1-troy_lee@aspeedtech.com> <20201202063612.21241-3-troy_lee@aspeedtech.com> In-Reply-To: <20201202063612.21241-3-troy_lee@aspeedtech.com> From: Joel Stanley Date: Wed, 2 Dec 2020 07:01:31 +0000 Message-ID: Subject: Re: [PATCH v2 3/3] edac: Supporting AST2400 and AST2600 edac driver To: Troy Lee X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201202_020145_954302_D5339A9C X-CRM114-Status: GOOD ( 32.18 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , Tony Luck , Ryan Chen , James Morse , "moderated list:ARM/ASPEED MACHINE SUPPORT" , Andrew Jeffery , open list , Robert Richter , leetroy@gmail.com, Rob Herring , Borislav Petkov , Stefan Schaeckeler , Mauro Carvalho Chehab , "moderated list:ARM/ASPEED MACHINE SUPPORT" , "open list:EDAC-CORE" Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, 2 Dec 2020 at 06:37, Troy Lee wrote: > > Adding AST2400 and AST2600 edac driver support. > > Signed-off-by: Troy Lee > --- > Change since v1: > 1. Removing SoC specific code > 2. Changing numerical representation of memory sizing > --- > drivers/edac/Kconfig | 6 +-- > drivers/edac/aspeed_edac.c | 103 +++++++++++++++++++++++++++++-------- > 2 files changed, 85 insertions(+), 24 deletions(-) > > diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig > index fc30f2ef9782..8ea70746d0bf 100644 > --- a/drivers/edac/Kconfig > +++ b/drivers/edac/Kconfig > @@ -508,10 +508,10 @@ config EDAC_QCOM > health, you should probably say 'Y' here. > > config EDAC_ASPEED > - tristate "Aspeed AST 2500 SoC" > - depends on MACH_ASPEED_G5 > + tristate "Aspeed AST BMC SoC" > + depends on (MACH_ASPEED_G4 || MACH_ASPEED_G5 || MACH_ASPEED_G6) Change this to ARCH_ASPEED. > help > - Support for error detection and correction on the Aspeed AST 2500 SoC. > + Support for error detection and correction on the Aspeed AST BMC SoC. > > First, ECC must be configured in the bootloader. Then, this driver > will expose error counters via the EDAC kernel framework. > diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c > index fbec28dc661d..b8c8d6a05f91 100644 > --- a/drivers/edac/aspeed_edac.c > +++ b/drivers/edac/aspeed_edac.c > @@ -14,12 +14,11 @@ > #include > #include "edac_module.h" > > - These whitespace cleanups are ok, but can probably go in a different patch so they aren't mixed up with the functional changes. > #define DRV_NAME "aspeed-edac" > > - > #define ASPEED_MCR_PROT 0x00 /* protection key register */ > #define ASPEED_MCR_CONF 0x04 /* configuration register */ > +#define ASPEED_MCR_REQ 0x08 /* Graphics Memory Protection register */ > #define ASPEED_MCR_INTR_CTRL 0x50 /* interrupt control/status register */ > #define ASPEED_MCR_ADDR_UNREC 0x58 /* address of first un-recoverable error */ > #define ASPEED_MCR_ADDR_REC 0x5c /* address of last recoverable error */ > @@ -29,15 +28,17 @@ > #define ASPEED_MCR_PROT_PASSWD 0xfc600309 > #define ASPEED_MCR_CONF_DRAM_TYPE BIT(4) > #define ASPEED_MCR_CONF_ECC BIT(7) > +#define ASPEED_MCR_CONF_DRAM_CTRL_TYPE (BIT(28)|BIT(29)|BIT(30)|BIT(31)) We can use GENMASK here: GENMASK(31, 28) > #define ASPEED_MCR_INTR_CTRL_CLEAR BIT(31) > #define ASPEED_MCR_INTR_CTRL_CNT_REC GENMASK(23, 16) > #define ASPEED_MCR_INTR_CTRL_CNT_UNREC GENMASK(15, 12) > #define ASPEED_MCR_INTR_CTRL_ENABLE (BIT(0) | BIT(1)) > > +#define ASPEED_MCR_CONF_DRAM_CTRL_TYPE_AST2500 0x01 > +#define ASPEED_MCR_CONF_DRAM_CTRL_TYPE_AST2600 0x03 > > static struct regmap *aspeed_regmap; > > - > static int regmap_reg_write(void *context, unsigned int reg, unsigned int val) > { > void __iomem *regs = (void __iomem *)context; > @@ -53,7 +54,6 @@ static int regmap_reg_write(void *context, unsigned int reg, unsigned int val) > return 0; > } > > - > static int regmap_reg_read(void *context, unsigned int reg, unsigned int *val) > { > void __iomem *regs = (void __iomem *)context; > @@ -63,6 +63,79 @@ static int regmap_reg_read(void *context, unsigned int reg, unsigned int *val) > return 0; > } > > +extern void aspeed_sdmc_disable_mem_protection(u8 req) > +{ > + u32 req_val = 0; > + > + regmap_read(aspeed_regmap, ASPEED_MCR_REQ, &req_val); > + > + req_val &= ~BIT(req); > + > + regmap_write(aspeed_regmap, ASPEED_MCR_REQ, req_val); This is open coding regmap_update_bits: regmap_update_bits(aspeed_regmap, ASPEED_MCR_REQ, BIT(req), BIT(req)); > +} > +EXPORT_SYMBOL(aspeed_sdmc_disable_mem_protection); I don't think any of these functions need to be exported. The same comment applies to the functions below this one; you can make them all static and drop the extern and the EXPORT_SYMBOL. > + > +static const u32 ast2400_dram_table[] = { > + 64 << 20, > + 128 << 20, > + 256 << 20, > + 512 << 20, > +}; > + > +static const u32 ast2500_dram_table[] = { > + 128 << 20, > + 256 << 20, > + 512 << 20, > + 1024 << 20, > +}; > + > +static const u32 ast2600_dram_table[] = { > + 256 << 20, > + 512 << 20, > + 1024 << 20, > + 2048 << 20, > +}; > + > +extern u32 aspeed_get_dram_size(void) > +{ > + u32 reg04; > + u32 size; > + u8 type; > + > + regmap_read(aspeed_regmap, ASPEED_MCR_CONF, ®04); > + > + type = (reg04 & ASPEED_MCR_CONF_DRAM_CTRL_TYPE) >> 28; > + > + if (type == ASPEED_MCR_CONF_DRAM_CTRL_TYPE_AST2600) > + size = ast2600_dram_table[reg04 & 0x3]; > + else if (type == ASPEED_MCR_CONF_DRAM_CTRL_TYPE_AST2500) > + size = ast2500_dram_table[reg04 & 0x3]; > + else > + size = ast2400_dram_table[reg04 & 0x3]; > + > + return size; > +} > +EXPORT_SYMBOL(aspeed_get_dram_size); > + > +static const u32 aspeed_vga_table[] = { > + 8 << 20, > + 16 << 20, > + 32 << 20, > + 64 << 20, > +}; > + > +extern u32 aspeed_get_vga_size(void) > +{ > + u32 reg04; > + u32 size; > + > + regmap_read(aspeed_regmap, ASPEED_MCR_CONF, ®04); > + > + size = aspeed_vga_table[((reg04 & 0xC) >> 2)]; > + return size; If you do this: return aspeed_vga_table[((reg04 & 0xC) >> 2)] You can drop 'size'. > +} > +EXPORT_SYMBOL(aspeed_get_vga_size); > + > static bool regmap_is_volatile(struct device *dev, unsigned int reg) > { > switch (reg) { > @@ -227,7 +300,6 @@ static int config_irq(void *ctx, struct platform_device *pdev) > return 0; > } > > - > static int init_csrows(struct mem_ctl_info *mci) > { > struct csrow_info *csrow = mci->csrows[0]; > @@ -239,7 +311,7 @@ static int init_csrows(struct mem_ctl_info *mci) > int rc; > > /* retrieve info about physical memory from device tree */ > - np = of_find_node_by_path("/memory"); > + np = of_find_node_by_name(NULL, "memory"); > if (!np) { > dev_err(mci->pdev, "dt: missing /memory node\n"); > return -ENODEV; > @@ -275,14 +347,12 @@ static int init_csrows(struct mem_ctl_info *mci) > return 0; > } > > - > static int aspeed_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct edac_mc_layer layers[2]; > struct mem_ctl_info *mci; > void __iomem *regs; > - u32 reg04; > int rc; > > regs = devm_platform_ioremap_resource(pdev, 0); > @@ -294,13 +364,6 @@ static int aspeed_probe(struct platform_device *pdev) > if (IS_ERR(aspeed_regmap)) > return PTR_ERR(aspeed_regmap); > > - /* bail out if ECC mode is not configured */ > - regmap_read(aspeed_regmap, ASPEED_MCR_CONF, ®04); > - if (!(reg04 & ASPEED_MCR_CONF_ECC)) { > - dev_err(&pdev->dev, "ECC mode is not configured in u-boot\n"); > - return -EPERM; > - } > - > edac_op_state = EDAC_OPSTATE_INT; > > /* allocate & init EDAC MC data structure */ > @@ -373,12 +436,13 @@ static int aspeed_remove(struct platform_device *pdev) > return 0; > } > > - > static const struct of_device_id aspeed_of_match[] = { > + { .compatible = "aspeed,ast2400-sdram-edac" }, > { .compatible = "aspeed,ast2500-sdram-edac" }, > + { .compatible = "aspeed,ast2600-sdram-edac" }, > {}, > }; > - > +MODULE_DEVICE_TABLE(of, aspeed_of_match); > > static struct platform_driver aspeed_driver = { > .driver = { > @@ -395,18 +459,15 @@ static int __init aspeed_init(void) > return platform_driver_register(&aspeed_driver); > } > > - > static void __exit aspeed_exit(void) > { > platform_driver_unregister(&aspeed_driver); > } > > - > module_init(aspeed_init); > module_exit(aspeed_exit); Unrelated, you can clean up the above 6 lines with module_platform_driver(). Actually, looking at the kernel tree this was already done in v5.9. Please send your next version based on the latest kernel, eg v5.10-rc1. Cheers, Joel > > - > MODULE_LICENSE("GPL"); > MODULE_AUTHOR("Stefan Schaeckeler "); > -MODULE_DESCRIPTION("Aspeed AST2500 EDAC driver"); > +MODULE_DESCRIPTION("Aspeed EDAC driver"); > MODULE_VERSION("1.0"); > -- > 2.17.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel