Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210304034141.7062-1-brad@pensando.io> <20210304034141.7062-6-brad@pensando.io>
In-Reply-To: <20210304034141.7062-6-brad@pensando.io>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Thu, 4 Mar 2021 10:41:13 +0100
Message-ID: <CAK8P3a2mQ9Tk+xeL-kdpQh3oqgNQeT6c-_KwNLQQ7mEc21fDvg@mail.gmail.com>
Subject: Re: [PATCH 5/8] mmc: sdhci-cadence: Add Pensando Elba SoC support
To:     Brad Larson <brad@pensando.io>
Cc:     Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        Mark Brown <broonie@kernel.org>,
        Serge Semin <fancer.lancer@gmail.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        Olof Johansson <olof@lixom.net>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        linux-spi <linux-spi@vger.kernel.org>,
        linux-mmc <linux-mmc@vger.kernel.org>,
        DTML <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Mar 4, 2021 at 4:41 AM Brad Larson <brad@pensando.io> wrote:

> +
> +static void elba_write_l(struct sdhci_host *host, u32 val, int reg)
> +{
> +       struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&priv->wrlock, flags);
> +       writel(0x78, priv->ctl_addr);
> +       writel(val, host->ioaddr + reg);
> +       spin_unlock_irqrestore(&priv->wrlock, flags);
> +}

Please be aware that the spinlock does not actually guarantee serializing
a series of mmio writes unless the MMIO mapping is non-posted, or
you follow it with a readl() from the same device before the spin_unlock().

> @@ -453,8 +441,14 @@ static const struct dev_pm_ops sdhci_cdns_pm_ops = {
>  static const struct of_device_id sdhci_cdns_match[] = {
>         {
>                 .compatible = "socionext,uniphier-sd4hc",
> -               .data = &sdhci_cdns_uniphier_pltfm_data,
> +               .data = &sdhci_cdns_uniphier_drv_data,
>         },
> +#ifdef CONFIG_MMC_SDHCI_CADENCE_ELBA
> +       {
> +               .compatible = "pensando,elba-emmc",
> +               .data = &sdhci_elba_drv_data
> +       },
> +#endif
>         { .compatible = "cdns,sd4hc" },
>         { /* sentinel */ }
>  };

This introduces a reverse dependency between the modules, which will cause
problems at link time depending on how you configure it. There are two ways
to avoid this:

a) the simple method is to always link every backend into the driver module,
usually leaving them all enabled at compile time.

b) once this gets out of hand because there are too many variants, or the
differences between them are too big, you refactor the common code into
a library module that just exports a functions but has no module_init()
itself, plus a front-end driver for each variant, which now calls into
the common
code rather than being called by the common code.

      Arnd