Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 09 Dec 2022 15:27:55 +0200
From:   Vadym Kochan <vadym.kochan@plvision.eu>
Subject: Re: [PATCH v3 3/3] mmc: xenon: Fix 2G limitation on AC5 SoC
To:     Adrian Hunter <adrian.hunter@intel.com>,
        Hu Ziji <huziji@marvell.com>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-mmc@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     Elad Nachman <enachman@marvell.com>,
        Chris Packham <chris.packham@alliedtelesis.co.nz>
In-Reply-To: <69dd8714-4be1-6b1b-fa07-04c790a6c6fc@intel.com>
References: <20221205105931.410686-1-vadym.kochan@plvision.eu>
        <20221205105931.410686-4-vadym.kochan@plvision.eu>
        <18cf4197-adce-3e47-7802-80b0d078368b@intel.com>
        <VI1P190MB0317641905664AFF51F9F4EA951C9@VI1P190MB0317.EURP190.PROD.OUTLOOK.COM>
        <ce870974-3e4f-107f-2047-89dcaebff1a2@intel.com>
        <VI1P190MB0317A616976EC99EA0C44F47951C9@VI1P190MB0317.EURP190.PROD.OUTLOOK.COM>
        <69dd8714-4be1-6b1b-fa07-04c790a6c6fc@intel.com>
Content-Type: text/plain
Message-ID: <VI1P190MB0317DADE7450282444BFED32951C9@VI1P190MB0317.EURP190.PROD.OUTLOOK.COM>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?c+419U3syErs58fwV5FfGy7J8kbYl9HRyCO3KJeYFVaPWD9JaX1iXIr0Tj2d?=
 =?us-ascii?Q?PrzryJhCfZxwhOYta+ISF+smJoOWGc5xNYeakcxwE2Jecl6MSRuZAhWV/2N+?=
 =?us-ascii?Q?ynkU4aNzon+ETB9znJfyOGzUW24wEFcBQ6L0dTfxU2dpMhLevOpOwT0A7mmX?=
 =?us-ascii?Q?ippbs+BxTFlk4vHwOTy4T4H+K2y3mqtuD2YW7rtaev1pEf8UXgNlaDiOS+ge?=
 =?us-ascii?Q?SV5PXJ0OpAdj2sQxI0DFyWgoqE1dnxPXhX8N57LLnjj5YUR8RWH0sW4YYHyh?=
 =?us-ascii?Q?5gH9B6gyCbdtvNKChnxi6/0yo6KuLVk347MR2HJXbgRZbUiRv8OP8KZwoKef?=
 =?us-ascii?Q?FXIfhsXoY1Ls4JVjsjUM8Bfb6c8OeYdKoR4F5TnDNXVeb89VC/Z8ZRPDmldP?=
 =?us-ascii?Q?r5jqu3BVIrsfw03GJyso3aaRqIZck07AAET72rXSNkmrAuFXYwWmnkCk+oDe?=
 =?us-ascii?Q?pCn6ljpT5xSuQmAS8ImRspKAYT97Or5jtXHS3/uMxUpKQegQnD4Hvo+gxIbu?=
 =?us-ascii?Q?qQ4swldN6vQTnfNB8kQlUswc90VuVSHQPqL6LeI4wpZBg0YCSaeYDL+8Pr29?=
 =?us-ascii?Q?WaIlEnWqLKpMHGLPgMatjI4a09YFVfrjU90u60vJcAr0uIiUnXqfIZYioWGz?=
 =?us-ascii?Q?1Tyg47ol2K49HaJ9nqvrd/xzZZ/WWMpAJ+Ytx6vLGYoDU0G/nZZJ025vLyvW?=
 =?us-ascii?Q?0uyJMneCU6vQyJv6JY1d1IjvLoUzCCWqO0Yb/4FGfGxEBdZ94njkwk23jDSl?=
 =?us-ascii?Q?ZwBYBSjHd+GZOxqotgyc4oIJgMl7hZEWAJWw71j8HREyOcb+2KSIb3uovDGz?=
 =?us-ascii?Q?wAtlSzuyFCEBfo++aMK5efBwLPlL2sSeIB4K2dGCiHE/1uvEb3qxHlNKC0gh?=
 =?us-ascii?Q?JdZ2mZPEUOYr0as6DvO6fv+z/R384R5GiiQ3H9ES4C+giyp2ojwbWijvbrn3?=
 =?us-ascii?Q?1linsGabV7Kirb0FPDhxT8bwVWM212MC1ce0f4vSFQ6pgOafpEadKYFP2V/2?=
 =?us-ascii?Q?nQnyP/jo+ZKJ9QMYSeo4vfyWehHpEBirU72bL2hxqIQXXe1WH4CTL+VF8eI9?=
 =?us-ascii?Q?Rl1bq3IXo3tsAF9ViJ/QQN4xFLP57yS8Hvz7eurlzxZ3h7I59YGuLgCfXndl?=
 =?us-ascii?Q?Vo8uehatkhjYTC7p7PIgQJ7xa0qo0CA+gKx8XUiu3ACRbesmaReiaoYvWEZQ?=
 =?us-ascii?Q?bnqWbJRyiOQh1HjGInudDe1XKqdam8giV0ENA49B8iyY6UxbWutWFQ6tZWS7?=
 =?us-ascii?Q?Vbo/3WtDwr6jMONXct9GUe0imb/cHv7fIRZR064akPn9B236RWjKLogWD/cv?=
 =?us-ascii?Q?YLz06KJF1+XIrKg+pYTruqybPqAXYDb/Bbg0PLMo5/A2aIh9uGYxcdz9CPn2?=
 =?us-ascii?Q?fUeeb2L/QE4H+u0Nv5aXdxqcs5yEWCCI5T8kuBCDWA+mEr7WitrHfsQVqW6S?=
 =?us-ascii?Q?J2KMtjcopKdYemHLG/mraapJr1iM0wSy0UreQfl0N6Gx09R/2TxM+1EE/JXZ?=
 =?us-ascii?Q?DlxrG8il6KHr8m0fYG4k1STyTSPMXqsprU6J7wLTdFtjecozgbAQ8ssS7CF/?=
 =?us-ascii?Q?zM8zvKG9c7z7DQYToSzjPwWTd5Ioqa9tMnxv0Fs7g7cG0XuF21tp/bbkm4Jo?=
 =?us-ascii?Q?yQ=3D=3D?=
X-OriginatorOrg: plvision.eu
X-MS-Exchange-CrossTenant-Network-Message-Id: fffa2bdc-93e4-4b23-bf6a-08dad9e92ee6
X-MS-Exchange-CrossTenant-AuthSource: VI1P190MB0317.EURP190.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Dec 2022 13:27:56.5546
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 03707b74-30f3-46b6-a0e0-ff0a7438c9c4
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 35q7e6hRndha6E9NO2V4iF4J6M0vHnNOD+H0Dftj0PHUKRPo8+/jJobtF8BlNQlfV4RtvghLZtu1sOy04MkNJF8aZswWo1YOaIn7m477QOo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU0P190MB1827
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, 9 Dec 2022 14:13:06 +0200, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 9/12/22 14:10, Vadym Kochan wrote:
> > Hi Adrian,
> > 
> > On Fri, 9 Dec 2022 13:53:58 +0200, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >> On 9/12/22 13:39, Vadym Kochan wrote:
> >>> Hi Adrian,
> >>>
> >>> On Fri, 9 Dec 2022 09:23:05 +0200, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>>> On 5/12/22 12:59, Vadym Kochan wrote:
> >>>>> There is a limitation on AC5 SoC that mmc controller
> >>>>> can't have DMA access over 2G memory, so use SDMA with
> >>>>> a bounce buffer. Swiotlb can't help because on arm64 arch
> >>>>> it reserves memblock's at the end of the memory.
> >>>>>
> >>>>> Additionally set mask to 34 bit since on AC5 SoC RAM starts
> >>>>> at 0x2_00000000.
> >>>>
> >>>> Can you explain more about how a 34-bit DMA mask works when
> >>>> SDMA only supports 32-bit addresses?
> >>>>
> >>>
> >>> So, after I set
> >>>
> >>>>> +		host->flags &= ~SDHCI_USE_64_BIT_DMA;
> >>>
> >>> then sdhc core sets mask to 32 bit, but then dma_map fails to map
> >>> bounce buffer because the base address is higher than 32bit - 0x2_00000000,
> >>> and 34bit mask fixed it.
> >>
> >> What happens if the bounce buffer gets mapped in the range
> >> 0x1_00000000 to 0x1_ffffffff ?
> >>
> > 
> > From my understanding, on the AC5 SoC RAM starts at 0x2_00000000 so the bounce
> > buffer can be mapped in the range 0x2_00000000..0x2_ffffffff
> 
> Right but I guess I meant what about 0x3_00000000..0x3_ffffffff ?
> Isn't that also in DMA_BIT_MASK(34)

Yes, you are right.

> 
> > 
> >>>
> >>>>>
> >>>>> Co-developed-by: Elad Nachman <enachman@marvell.com>
> >>>>> Signed-off-by: Elad Nachman <enachman@marvell.com>
> >>>>> Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
> >>>>> ---
> >>>>>  drivers/mmc/host/sdhci-xenon.c | 38 ++++++++++++++++++++++++++++++++++
> >>>>>  drivers/mmc/host/sdhci-xenon.h |  3 ++-
> >>>>>  2 files changed, 40 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c
> >>>>> index 08e838400b52..5f3db0425674 100644
> >>>>> --- a/drivers/mmc/host/sdhci-xenon.c
> >>>>> +++ b/drivers/mmc/host/sdhci-xenon.c
> >>>>> @@ -13,7 +13,9 @@
> >>>>>  
> >>>>>  #include <linux/acpi.h>
> >>>>>  #include <linux/delay.h>
> >>>>> +#include <linux/dma-mapping.h>
> >>>>>  #include <linux/ktime.h>
> >>>>> +#include <linux/mm.h>
> >>>>>  #include <linux/module.h>
> >>>>>  #include <linux/of.h>
> >>>>>  #include <linux/pm.h>
> >>>>> @@ -253,6 +255,22 @@ static unsigned int xenon_get_max_clock(struct sdhci_host *host)
> >>>>>  		return pltfm_host->clock;
> >>>>>  }
> >>>>>  
> >>>>> +static int xenon_set_dma_mask(struct sdhci_host *host)
> >>>>> +{
> >>>>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >>>>> +	struct xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> >>>>> +	struct mmc_host *mmc = host->mmc;
> >>>>> +	struct device *dev = mmc_dev(mmc);
> >>>>> +
> >>>>> +	if (priv->hw_version == XENON_AC5) {
> >>>>> +		host->flags &= ~SDHCI_USE_64_BIT_DMA;
> >>>>> +
> >>>>> +		return dma_set_mask_and_coherent(dev, DMA_BIT_MASK(34));
> >>>>> +	}
> >>>>> +
> >>>>> +	return sdhci_set_dma_mask(host);
> >>>>> +}
> >>>>> +
> >>>>>  static const struct sdhci_ops sdhci_xenon_ops = {
> >>>>>  	.voltage_switch		= xenon_voltage_switch,
> >>>>>  	.set_clock		= sdhci_set_clock,
> >>>>> @@ -261,6 +279,7 @@ static const struct sdhci_ops sdhci_xenon_ops = {
> >>>>>  	.reset			= xenon_reset,
> >>>>>  	.set_uhs_signaling	= xenon_set_uhs_signaling,
> >>>>>  	.get_max_clock		= xenon_get_max_clock,
> >>>>> +	.set_dma_mask		= xenon_set_dma_mask,
> >>>>>  };
> >>>>>  
> >>>>>  static const struct sdhci_pltfm_data sdhci_xenon_pdata = {
> >>>>> @@ -486,6 +505,18 @@ static void xenon_sdhc_unprepare(struct sdhci_host *host)
> >>>>>  	xenon_disable_sdhc(host, sdhc_id);
> >>>>>  }
> >>>>>  
> >>>>> +static int xenon_ac5_probe(struct sdhci_host *host)
> >>>>> +{
> >>>>> +	struct sysinfo si;
> >>>>> +
> >>>>> +	si_meminfo(&si);
> >>>>> +
> >>>>> +	if ((si.totalram * si.mem_unit) > SZ_2G)
> >>>>> +		host->quirks |= SDHCI_QUIRK_BROKEN_ADMA;
> >>>>> +
> >>>>> +	return 0;
> >>>>> +}
> >>>>> +
> >>>>>  static int xenon_probe(struct platform_device *pdev)
> >>>>>  {
> >>>>>  	struct sdhci_pltfm_host *pltfm_host;
> >>>>> @@ -533,6 +564,12 @@ static int xenon_probe(struct platform_device *pdev)
> >>>>>  		}
> >>>>>  	}
> >>>>>  
> >>>>> +	if (priv->hw_version == XENON_AC5) {
> >>>>> +		err = xenon_ac5_probe(host);
> >>>>> +		if (err)
> >>>>> +			goto err_clk_axi;
> >>>>> +	}
> >>>>> +
> >>>>>  	err = mmc_of_parse(host->mmc);
> >>>>>  	if (err)
> >>>>>  		goto err_clk_axi;
> >>>>> @@ -682,6 +719,7 @@ static const struct of_device_id sdhci_xenon_dt_ids[] = {
> >>>>>  	{ .compatible = "marvell,armada-ap807-sdhci", .data = (void *)XENON_AP807},
> >>>>>  	{ .compatible = "marvell,armada-cp110-sdhci", .data =  (void *)XENON_CP110},
> >>>>>  	{ .compatible = "marvell,armada-3700-sdhci", .data =  (void *)XENON_A3700},
> >>>>> +	{ .compatible = "marvell,ac5-sdhci", .data = (void *)XENON_AC5},
> >>>>>  	{}
> >>>>>  };
> >>>>>  MODULE_DEVICE_TABLE(of, sdhci_xenon_dt_ids);
> >>>>> diff --git a/drivers/mmc/host/sdhci-xenon.h b/drivers/mmc/host/sdhci-xenon.h
> >>>>> index 3e9c6c908a79..0460d97aad26 100644
> >>>>> --- a/drivers/mmc/host/sdhci-xenon.h
> >>>>> +++ b/drivers/mmc/host/sdhci-xenon.h
> >>>>> @@ -57,7 +57,8 @@ enum xenon_variant {
> >>>>>  	XENON_A3700,
> >>>>>  	XENON_AP806,
> >>>>>  	XENON_AP807,
> >>>>> -	XENON_CP110
> >>>>> +	XENON_CP110,
> >>>>> +	XENON_AC5
> >>>>>  };
> >>>>>  
> >>>>>  struct xenon_priv {
> >>>>
> >>>
> >>> Regards,
> >>
>