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 81044C25B78 for ; Sat, 25 May 2024 05:39:17 +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=AxjUhXuoPtwPWwf5qIiTWpHXEyhpyLqahdvUzgQ0llE=; b=eJLNsxJO7RAOnv mFp9hXzqo2s1UfTLBPLwhgeFrpq6qz6oR/3kOM793fer9D8dJ0lWA14kCBzT204zABjRaAI+nyj0J I6yVooasJo1qm1pQyUfU3zyPOnIF2GCToz16oP/R6Fwzt8+xsEktsRlcZBCfY+GWfDUvPPJuS1X04 neAKg33pDj76HUDJGj5iQV0lpqfe+6jlmJHclMnNH4pncoTqXJyOK3vQR3mOBYKw3pe23RK5a1bME nK1tf01a0NP21HAFQMs3T953VYgoSXZ+j8iWW5VOmIesAbMz/TafvKhj16uNn9WtaayoI0Hy/l7C1 IDU7ojw3ustoJVFvYS4Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sAk7S-0000000ASDY-0EBd; Sat, 25 May 2024 05:39:02 +0000 Received: from mail-lf1-x12a.google.com ([2a00:1450:4864:20::12a]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sAk7L-0000000ASCL-1nZq for linux-arm-kernel@lists.infradead.org; Sat, 25 May 2024 05:38:57 +0000 Received: by mail-lf1-x12a.google.com with SMTP id 2adb3069b0e04-5238b5c07efso9126001e87.3 for ; Fri, 24 May 2024 22:38:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1716615531; x=1717220331; 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=2jTk6DCmjqMaCgQaVye3JQ9uLLmyyQnmkCOnFVG81v8=; b=CfmDuroF3sFBsIVWzGz/y1ClecUwk9MEB1X7AuICmoDA5sPddDgkO3wiaitSHohSiu vzXFM6hkeoaZCT6t4JDLcF1/HbjnEwjMDjjjAmtKP9tVtOGgbIKcHkIUVz6p+KP0D2Ky 49EQUhoMiMedmMmjPwQdaMrMCj2oMlusNRGlpZaqNQyhkmBReto583xtf+AZSFTWakew e8lmvn61SIKBgLUoU7nRZImov56PjB8ns+8viFUmfC92rJx6WXkFO9/NzPfuFAYRfV7X 1qw/TF1h1ESHiI6Msp5KH6U1oc9AmRFo8jWnG50iRtaQHCAOnwslw3vRwQ+lw0hdSfJ4 sufw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1716615531; x=1717220331; 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=2jTk6DCmjqMaCgQaVye3JQ9uLLmyyQnmkCOnFVG81v8=; b=Q/5YQuWrxKLEhaVJ5HYNoToN6RBW7WjiqmBHX+7mb1JhUbYQCMOlkJGNwfyi8UKm2x FjCEEFrOBaPwnfOS3w6FNocb/5RK9TpM+SXeBl8p1z5xVHeOdAX+zn3hxIr7m6Oh+LmW cMk4oWMFPrRLUyXl3YoNR1i65dXcxdhyd90GSVi4K1b1tboTtv0D+yxwt73LsPBFKszw sI8my7cQTG+6Jpxp9HSi1TGhm4J7TONafmHl2gZatLgc54ivjYC4gI6wb5BoEQBuoN92 7YnqeDhqsWzTbW4dZN4XkWw2RRhR9u09ONo4Mcr4KwBqAlbu9QFX1dchWJPWqWSEjbZD 6LYA== X-Forwarded-Encrypted: i=1; AJvYcCWxH/rscJlCdadVrRg6Nkzu5TYPOsxhVuWvqNdZtuQJM+5IdvZ3miqgOxxg0rHVpfIcIjYzSq2HnH0lrETgyvuKnsZjCJxaI7QrM+ODEa+2b88MoWU= X-Gm-Message-State: AOJu0YzxDSCSLFHIyT4IQD2XDsv1y3PQbxK3TkpXKoQI7IqBMXmAjb2o sOdqd6nwk0gsxKJr0B3XZlbi+kDF3NT6NbV70LpcVGFKaQa7rYhQYqslKGLyJYA= X-Google-Smtp-Source: AGHT+IEh302x5sOYeNXiDr3hNiH3etY5DH1v9PpLLFJxpQCbGJwqK/y+Z6vehXDwU5zTkK6XFrzXXQ== X-Received: by 2002:a19:ca50:0:b0:523:6354:881e with SMTP id 2adb3069b0e04-52964ca8dd1mr2256091e87.39.1716615530576; Fri, 24 May 2024 22:38:50 -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-5785238bb59sm2734909a12.30.2024.05.24.22.38.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 24 May 2024 22:38:50 -0700 (PDT) From: Andrea della Porta X-Google-Original-From: Andrea della Porta Date: Sat, 25 May 2024 07:39:01 +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: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240524_223855_487238_EC58BB90 X-CRM114-Status: GOOD ( 44.04 ) 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 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 current= ly > > include that feature yet. > > Based on downstream driver by raspberry foundation maintained kernel. > > = > > 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/sdhci-= 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_8V) > > +/* 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_SDR104) > > + > > 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(struct= 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(pltfm_ho= st); > > + 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->caps2 &= 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 * 1= 000), 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_74165b0 = =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_74165b0= =3D { > > }; > > = > > static const struct of_device_id __maybe_unused sdhci_brcm_of_match[]= =3D { > > + { .compatible =3D "brcm,bcm2712-sdhci", .data =3D &match_priv_2712 }, > > { .compatible =3D "brcm,bcm7425-sdhci", .data =3D &match_priv_7425 }, > > { .compatible =3D "brcm,bcm7445-sdhci", .data =3D &match_priv_7445 }, > > { .compatible =3D "brcm,bcm7216-sdhci", .data =3D &match_priv_7216 }, > > @@ -370,6 +432,9 @@ static int sdhci_brcmstb_probe(struct platform_devi= ce *pdev) > > (host->mmc->caps2 & MMC_CAP2_HS400_ES)) > > host->mmc_host_ops.hs400_enhanced_strobe =3D match_priv->hs400es; > > = > > + 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 this > is intended. I've tried to interpret the meaning of those two clocks since unfortunately= I don't own the datasheet for any of the platforms involved, so please take the fol= lowing as the result of my own (possibly wrong) intuition and (mostly wild) guessi= ng. The main clock is 'sw_sdio' while 'sdio_freq' is optional and the latter se= ems to be orthogonal to the former. While sw_sdio is mostly used for SD storage card, sdio_freq seems more rela= ted to SDIO family of cards (wifi, gps, camera, etc) for which you could specify a= particular (and higher) base frequency. Unfortunately I wasn't able to find any reference to sdio_freq in current d= evicetree so it's probably only specific to custom appliances: to be honest I'm not e= ven sure that BCM2712 is supporting that improved clock source. Also, from the foll= owing 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 * 1000), = 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 that it= relates to some Command Queue (timeout?) setting so it's probably only important if= CQE is enabled specifying 'supports-cqe' property, which is not in current devicet= ree (nor in downstream one). If this is the case it's mostly a performance improvem= ent, and as such something that we are not necessarily interested in right now since= 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 understanding o= f that specific register and/if the cqe support will be enabled. As a matter of fa= ct 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 muc= h earlier. The result is that right now the value set ito SDIO_CFG_CQ_CAPABILITY regis= ter is always equal to 1MHz. Further testing reveals that it is indeed working fine even = with those lines dropped, so I would deem that code as unnecessary for this early patc= hset. Is it a viable solution? 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