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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id AB1DBC25B78 for ; Mon, 27 May 2024 07:27:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:Date:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=vRK3pY4YJq78ZYxruQtieQO1p5qgN60ErBdsn/Aqk38=; b=pghL+sr8p7RNnu AUxUJbzolctZieC40zvlQLeTyw9jM9jayrQIAg02XOW+p0oESRzUy+XxW/hICStIOEO3sGanZL5hC JSynOSbbJm4B4AnSG/oj8T7K6NxtnoX2gI+05gRhjLQCcKcTOkDnHyzgO5TlkX0XMeWUyCnn5Cyi5 oOpNnjk6S76rqD6JyDTrQSPv72sjGlehGsuvCAbINnOj9uyxVM1TRy2no3rSJ9ORmvbCzg+Ad7wll SZe5TRQPwV4aex5pLktCwV07yGiD9Jvo27YS805gDbkE+u0dRnzzGDAipJaJB9GyIZnvEyNcT+Qzt KDFkL4bdc7YSCNEfy3Aw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sBUlg-0000000E3tA-0bNO; Mon, 27 May 2024 07:27:40 +0000 Received: from mail-ed1-x536.google.com ([2a00:1450:4864:20::536]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sBUlc-0000000E3s4-3zt2 for linux-arm-kernel@lists.infradead.org; Mon, 27 May 2024 07:27:38 +0000 Received: by mail-ed1-x536.google.com with SMTP id 4fb4d7f45d1cf-579d02657abso984895a12.3 for ; Mon, 27 May 2024 00:27:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1716794853; x=1717399653; darn=lists.infradead.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:mail-followup-to:message-id:subject:cc:to :date:from:from:to:cc:subject:date:message-id:reply-to; bh=Tq08Kk3nKg9qcVVrH+CQCgxy3zPqfMg+Tuq3n+0tlL4=; b=cLdnnydCzQyvraA59Q/3zEOZ68WgKPRjqV4JNFizgp263ZPW6FFMTCKeD9nDnCTCWU hw2ASS+nUMSiG8KCEYy5NAX8zN1x7UBa23JIyreDq1lgcoYEDBegvjE8ha8K08QtYUwg FKh7m5o0QDHIjTCI8w+SQxZ/BDpz/w0pL512A3Xph47ytAVF+fzy9C8mHIw/5YWqThw1 lmNvN7cgHrwemMGgEgLbSUySZEkzSqoSQCqkGFTw2LfWxD/74b06niCHsulB2EEUgKsH fhXcnyxHHuQw4XoIygt1y8OSUAq4cuIjVMXoMqcEbIq3jSfiLD1WEFMKnhxPRyYntt1F TZEg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1716794853; x=1717399653; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:mail-followup-to:message-id:subject:cc:to :date:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=Tq08Kk3nKg9qcVVrH+CQCgxy3zPqfMg+Tuq3n+0tlL4=; b=fTzxxWBmMO+ufAzKnqUoK5vjJAEixHzP2St496et/KT/4da2sQkdIGO63QSoCBpqS7 EhNne1mek0EWvnX5gWRjGufg6Ixaq/stiwZBp5nBn+BvHNOjCmxrfchrAUSMAmLvI2OL zuxuDyalEJkIHt5hW1u/nJ/c4Du+dUdEH1t+bkLt4HiLgKekyjulpeeGKhirg3VZcgtd qR/N/nmYSjMFGSxiHooANqwQxOmu2OD57xrO9MYxZCeZ8MP0nfHsLfXba7cAoYu0Rw9y zf5qP/aOp2cpdHPt422BabYk7ttFjyuOKaRjPfqECDurzH7ikVbv2PaevP1g4uNSSuAt A3vA== X-Forwarded-Encrypted: i=1; AJvYcCUwCEF4N6Yp6O3tznVwvueuRiQgWO8ACWpLbbgZj52SZoFZpJqjHxdAarYQgh3a/dyZDX0YQHvoYwN2+i4uil/Y8S3xhBxPvnwK9YnJSpC8ETSpV0Q= X-Gm-Message-State: AOJu0YxFY5K/cCuQrYerKAPxQf3HjSPJbiQkNEkmySJRF7cQ/54AiP0q 7uSxjDcZl7LNAN+RX/26u8y6bp79bMaQHg7S9nbkPfLmAYU1yisKqfd+xdZRcjk= X-Google-Smtp-Source: AGHT+IF/Ad/UrVqGCx4yFYh9J4k54L4ZIgvX2cg0dOunkhcbkkjAaaXiLkqInRLp/8vA3D0cOmTk6Q== X-Received: by 2002:a50:9518:0:b0:578:6438:be99 with SMTP id 4fb4d7f45d1cf-5786438bed5mr5978574a12.7.1716794853077; Mon, 27 May 2024 00:27:33 -0700 (PDT) Received: from localhost (host-87-18-209-253.retail.telecomitalia.it. [87.18.209.253]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-57852495de0sm5342116a12.61.2024.05.27.00.27.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 27 May 2024 00:27:32 -0700 (PDT) From: Andrea della Porta X-Google-Original-From: Andrea della Porta Date: Mon, 27 May 2024 09:27:45 +0200 To: Stefan Wahren Cc: Andrea della Porta , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Florian Fainelli , Ray Jui , Scott Branden , Marc Zyngier , Broadcom internal kernel review list , Ulf Hansson , Adrian Hunter , Kamal Dasu , Al Cooper , devicetree@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mmc@vger.kernel.org Subject: Re: [PATCH v3 3/4] mmc: sdhci-brcmstb: Add BCM2712 support Message-ID: Mail-Followup-To: Stefan Wahren , Andrea della Porta , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Florian Fainelli , Ray Jui , Scott Branden , Marc Zyngier , Broadcom internal kernel review list , Ulf Hansson , Adrian Hunter , Kamal Dasu , Al Cooper , devicetree@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mmc@vger.kernel.org References: <9ef2d8c2-621a-4e05-8afa-5d2b99a36caa@gmx.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <9ef2d8c2-621a-4e05-8afa-5d2b99a36caa@gmx.net> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240527_002737_095344_736589E4 X-CRM114-Status: GOOD ( 58.42 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Stefan, On 08:59 Sat 25 May , Stefan Wahren wrote: > Hi Andrea, > = > Am 25.05.24 um 07:39 schrieb Andrea della Porta: > > On 14:26 Tue 21 May , Stefan Wahren wrote: > > > Hi Andrea, > > > = > > > Am 21.05.24 um 10:35 schrieb Andrea della Porta: > > > > Broadcom BCM2712 SoC has an SDHCI card controller using the SDIO CFG > > > > register block present on other STB chips. Add support for BCM2712 > > > > SD capabilities of this chipset. > > > > The silicon is SD Express capable but this driver port does not cur= rently > > > > include that feature yet. > > > > Based on downstream driver by raspberry foundation maintained kerne= l. > > > > = > > > > Signed-off-by: Andrea della Porta > > > > --- > > > > drivers/mmc/host/sdhci-brcmstb.c | 65 ++++++++++++++++++++++++++= ++++++ > > > > 1 file changed, 65 insertions(+) > > > > = > > > > diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sd= hci-brcmstb.c > > > > index 9053526fa212..b349262da36e 100644 > > > > --- a/drivers/mmc/host/sdhci-brcmstb.c > > > > +++ b/drivers/mmc/host/sdhci-brcmstb.c > > > > @@ -30,6 +30,21 @@ > > > > = > > > > #define SDHCI_ARASAN_CQE_BASE_ADDR 0x200 > > > > = > > > > +#define SDIO_CFG_CQ_CAPABILITY 0x4c > > > > +#define SDIO_CFG_CQ_CAPABILITY_FMUL GENMASK(13, 12) > > > > + > > > > +#define SDIO_CFG_CTRL 0x0 > > > > +#define SDIO_CFG_CTRL_SDCD_N_TEST_EN BIT(31) > > > > +#define SDIO_CFG_CTRL_SDCD_N_TEST_LEV BIT(30) > > > > + > > > > +#define SDIO_CFG_MAX_50MHZ_MODE 0x1ac > > > > +#define SDIO_CFG_MAX_50MHZ_MODE_STRAP_OVERRIDE BIT(31) > > > > +#define SDIO_CFG_MAX_50MHZ_MODE_ENABLE BIT(0) > > > > + > > > > +#define MMC_CAP_HSE_MASK (MMC_CAP2_HSX00_1_2V | MMC_CAP2_HSX00_1_8= V) > > > > +/* Select all SD UHS type I SDR speed above 50MB/s */ > > > > +#define MMC_CAP_UHS_I_SDR_MASK (MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SD= R104) > > > > + > > > > struct sdhci_brcmstb_priv { > > > > void __iomem *cfg_regs; > > > > unsigned int flags; > > > > @@ -38,6 +53,7 @@ struct sdhci_brcmstb_priv { > > > > }; > > > > = > > > > struct brcmstb_match_priv { > > > > + void (*cfginit)(struct sdhci_host *host); > > > > void (*hs400es)(struct mmc_host *mmc, struct mmc_ios *ios); > > > > struct sdhci_ops *ops; > > > > const unsigned int flags; > > > > @@ -168,6 +184,38 @@ static void sdhci_brcmstb_set_uhs_signaling(st= ruct sdhci_host *host, > > > > sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2); > > > > } > > > > = > > > > +static void sdhci_brcmstb_cfginit_2712(struct sdhci_host *host) > > > > +{ > > > > + struct sdhci_pltfm_host *pltfm_host =3D sdhci_priv(host); > > > > + struct sdhci_brcmstb_priv *brcmstb_priv =3D sdhci_pltfm_priv(pltf= m_host); > > > > + u32 reg, base_clk_mhz; > > > > + > > > > + /* > > > > + * If we support a speed that requires tuning, > > > > + * then select the delay line PHY as the clock source. > > > > + */ > > > > + if ((host->mmc->caps & MMC_CAP_UHS_I_SDR_MASK) || (host->mmc->cap= s2 & MMC_CAP_HSE_MASK)) { > > > > + reg =3D readl(brcmstb_priv->cfg_regs + SDIO_CFG_MAX_50MHZ_MODE); > > > > + reg &=3D ~SDIO_CFG_MAX_50MHZ_MODE_ENABLE; > > > > + reg |=3D SDIO_CFG_MAX_50MHZ_MODE_STRAP_OVERRIDE; > > > > + writel(reg, brcmstb_priv->cfg_regs + SDIO_CFG_MAX_50MHZ_MODE); > > > > + } > > > > + > > > > + if ((host->mmc->caps & MMC_CAP_NONREMOVABLE) || > > > > + (host->mmc->caps & MMC_CAP_NEEDS_POLL)) { > > > > + /* Force presence */ > > > > + reg =3D readl(brcmstb_priv->cfg_regs + SDIO_CFG_CTRL); > > > > + reg &=3D ~SDIO_CFG_CTRL_SDCD_N_TEST_LEV; > > > > + reg |=3D SDIO_CFG_CTRL_SDCD_N_TEST_EN; > > > > + writel(reg, brcmstb_priv->cfg_regs + SDIO_CFG_CTRL); > > > > + } > > > > + > > > > + /* Guesstimate the timer frequency (controller base clock) */ > > > > + base_clk_mhz =3D max_t(u32, clk_get_rate(pltfm_host->clk) / (1000= * 1000), 1); > > > > + reg =3D SDIO_CFG_CQ_CAPABILITY_FMUL | base_clk_mhz; > > > > + writel(reg, brcmstb_priv->cfg_regs + SDIO_CFG_CQ_CAPABILITY); > > > This part assumes the clock isn't changed afterwards, see below ... > > > > +} > > > > + > > > > static void sdhci_brcmstb_dumpregs(struct mmc_host *mmc) > > > > { > > > > sdhci_dumpregs(mmc_priv(mmc)); > > > > @@ -200,6 +248,14 @@ static struct sdhci_ops sdhci_brcmstb_ops =3D { > > > > .set_uhs_signaling =3D sdhci_set_uhs_signaling, > > > > }; > > > > = > > > > +static struct sdhci_ops sdhci_brcmstb_ops_2712 =3D { > > > > + .set_clock =3D sdhci_set_clock, > > > > + .set_power =3D sdhci_set_power_and_bus_voltage, > > > > + .set_bus_width =3D sdhci_set_bus_width, > > > > + .reset =3D sdhci_reset, > > > > + .set_uhs_signaling =3D sdhci_set_uhs_signaling, > > > > +}; > > > > + > > > > static struct sdhci_ops sdhci_brcmstb_ops_7216 =3D { > > > > .set_clock =3D sdhci_brcmstb_set_clock, > > > > .set_bus_width =3D sdhci_set_bus_width, > > > > @@ -214,6 +270,11 @@ static struct sdhci_ops sdhci_brcmstb_ops_7416= 5b0 =3D { > > > > .set_uhs_signaling =3D sdhci_brcmstb_set_uhs_signaling, > > > > }; > > > > = > > > > +static const struct brcmstb_match_priv match_priv_2712 =3D { > > > > + .cfginit =3D sdhci_brcmstb_cfginit_2712, > > > > + .ops =3D &sdhci_brcmstb_ops_2712, > > > > +}; > > > > + > > > > static struct brcmstb_match_priv match_priv_7425 =3D { > > > > .flags =3D BRCMSTB_MATCH_FLAGS_NO_64BIT | > > > > BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT, > > > > @@ -238,6 +299,7 @@ static struct brcmstb_match_priv match_priv_741= 65b0 =3D { > > > > }; > > > > = > > > > static const struct of_device_id __maybe_unused sdhci_brcm_of_ma= tch[] =3D { > > > > + { .compatible =3D "brcm,bcm2712-sdhci", .data =3D &match_priv_271= 2 }, > > > > { .compatible =3D "brcm,bcm7425-sdhci", .data =3D &match_priv_7= 425 }, > > > > { .compatible =3D "brcm,bcm7445-sdhci", .data =3D &match_priv_7= 445 }, > > > > { .compatible =3D "brcm,bcm7216-sdhci", .data =3D &match_priv_7= 216 }, > > > > @@ -370,6 +432,9 @@ static int sdhci_brcmstb_probe(struct platform_= device *pdev) > > > > (host->mmc->caps2 & MMC_CAP2_HS400_ES)) > > > > host->mmc_host_ops.hs400_enhanced_strobe =3D match_priv->hs400= es; > > > > = > > > > + if (match_priv->cfginit) > > > > + match_priv->cfginit(host); > > > > + > > > I'm not sure this is the right place to call cfginit. > > > sdhci_brcmstb_cfginit_2712 retrives the current controller base clock, > > > but at the end of=A0 sdhci_brcmstb_probe this clock frequency could be > > > adjusted by the device property "clock-frequency". So i'm not sure th= is > > > is intended. > > I've tried to interpret the meaning of those two clocks since unfortuna= tely I don't > > own the datasheet for any of the platforms involved, so please take the= following > > as the result of my own (possibly wrong) intuition and (mostly wild) gu= essing. > > = > > The main clock is 'sw_sdio' while 'sdio_freq' is optional and the latte= r seems to be > > orthogonal to the former. > > While sw_sdio is mostly used for SD storage card, sdio_freq seems more = related to > > SDIO family of cards (wifi, gps, camera, etc) for which you could speci= fy a particular > > (and higher) base frequency. > > Unfortunately I wasn't able to find any reference to sdio_freq in curre= nt devicetree > > so it's probably only specific to custom appliances: to be honest I'm n= ot even sure > > that BCM2712 is supporting that improved clock source. Also, from the = following lines > > at the end of cfginit function: > > = > > /* Guesstimate the timer frequency (controller base clock) */ > > base_clk_mhz =3D max_t(u32, clk_get_rate(pltfm_host->clk) / (1000 * 100= 0), 1); > > reg =3D SDIO_CFG_CQ_CAPABILITY_FMUL | base_clk_mhz; > > writel(reg, brcmstb_priv->cfg_regs + SDIO_CFG_CQ_CAPABILITY); > > = > > judging from the name of SDIO_CFG_CQ_CAPABILITY register, I'd guess tha= t it relates > > to some Command Queue (timeout?) setting so it's probably only importan= t if CQE is > > enabled specifying 'supports-cqe' property, which is not in current dev= icetree (nor > > in downstream one). If this is the case it's mostly a performance impr= ovement, and > > as such something that we are not necessarily interested in right now s= ince this > > patchset adds just minimal boot support. I would then drop those lines,= as we could > > just reintroduce them if they need be once we have a better understandi= ng of that > > specific register and/if the cqe support will be enabled. As a matter o= f fact those > > lines are not working as expected in any case since pltfm_host->clk is = set at the > > very end of sdhci_brcmstb_probe() while the cfginit function is invoked= much earlier. > > The result is that right now the value set ito SDIO_CFG_CQ_CAPABILITY r= egister is always > > equal to 1MHz. Further testing reveals that it is indeed working fine e= ven with those > > lines dropped, so I would deem that code as unnecessary for this early = patchset. > > Is it a viable solution? > I don't have any knowledge about this hardware, so my opinion based on > your good investigations. But i would be fine with this. > = > Just to make it clear, this works with and without U-Boot in the bootchai= n? Yes, I confirm it works with both. Thanks, Andrea > = > Thanks > > = > > Many thanks, > > Andrea > > = > > > > /* > > > > * Supply the existing CAPS, but clear the UHS modes. This > > > > * will allow these modes to be specified by device tree > = _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel