Re: [PATCH v2 05/13] mmc: sdhci-cadence: Add Pensando Elba SoC support

Hi Uffe,

On Tue, Mar 30, 2021 at 3:31 AM Ulf Hansson [off-list ref] wrote:

+ Masahiro Yamada (main author of the driver)

On Mon, 29 Mar 2021 at 03:59, Brad Larson [off-list ref] wrote:

quoted

Add support for Pensando Elba SoC which explicitly controls
byte-lane enables on writes.  Refactor to allow platform
specific write ops.

Signed-off-by: Brad Larson <redacted>
---
 drivers/mmc/host/Kconfig              |  15 +++
 drivers/mmc/host/Makefile             |   1 +
 drivers/mmc/host/sdhci-cadence-elba.c | 137 ++++++++++++++++++++++++++

By looking at the amount of code changes that seem to be needed to
support the Pensando Elba variant, I don't think it's necessary to
split this into a separate file.

Unless Yamada-san has a different opinion, I would rather just stick
with using sdhci-cadence.c.

Yamada-san agreed with the recommendation.  This is a summary of the
changes for the updated patchset based upon all the review comments.

- Add Elba support into sdhci-cadence.c
- Removes new files sdhci-cadence-elba.c and sdhci-cadence.h.
- Remove Kconfig option MMC_SDHCI_CADENCE_ELBA
- Use the existing SDHCI_CDNS_* register defines
- Prefixed the remaining Elba SoC specific functions with elba_
- Use C Comment syntax
- The Elba specific phy init code is removed and the existing
sdhci_cdns_phy_init() is used for these Elba properties set in the DT

cdns,phy-input-delay-sd-highspeed = <0x4>;
cdns,phy-input-delay-legacy = <0x4>;
cdns,phy-input-delay-sd-uhs-sdr50 = <0x6>;
cdns,phy-input-delay-sd-uhs-ddr50 = <0x16>;

quoted

+config MMC_SDHCI_CADENCE_ELBA
+       tristate "SDHCI support for the Pensando/Cadence SD/SDIO/eMMC controller"
+       depends on ARCH_PENSANDO_ELBA_SOC
+       depends on MMC_SDHCI
+       depends on OF
+       depends on MMC_SDHCI_CADENCE
+       depends on MMC_SDHCI_PLTFM
+       select MMC_SDHCI_IO_ACCESSORS

According to the comment above - then you should probably just extend
the conditions for when building MMC_SDHCI_CADENCE, rather than having
to add a new Kconfig for "*_ELBA".

Yes, config MMC_SDHCI_CADENCE_ELBA is removed.  Elba support is
enabled with CONFIG_MMC_SDHCI_CADENCE.

quoted

+// delay regs address

Please don't use "//" when adding comments, but instead "/* ... */".
This applies to several more places of the patch.

Only C syntax is now used for comments.

quoted

+static void sd4_set_dlyvr(struct sdhci_host *host,
+                         unsigned char addr, unsigned char data)

Please, try to think of a better function name that's more
descriptive. Moreover, please use a common prefix for functions that
is used on elba.

The Elba specific functions such as sd4_set_dlyvr() are deleted and
the existing sdhci_cdns_phy_init() with DT properties are used for PHY
init.  The remaining Elba specific functions have the prefix elba_

quoted

+{
+       unsigned long dlyrv_reg;
+
+       dlyrv_reg = ((unsigned long)data << 8);
+       dlyrv_reg |= addr;
+
+       // set data and address
+       writel(dlyrv_reg, host->ioaddr + SDIO_REG_HRS4);
+       dlyrv_reg |= (1uL << 24uL);
+       // send write request
+       writel(dlyrv_reg, host->ioaddr + SDIO_REG_HRS4);
+       dlyrv_reg &= ~(1uL << 24);
+       // clear write request
+       writel(dlyrv_reg, host->ioaddr + SDIO_REG_HRS4);
+}
+
+static void phy_config(struct sdhci_host *host)

Ditto.

Yes, C syntax only.  Refactored to use existing sdhci_cdns_phy_init()
and removed phy_config().

quoted

@@ -453,8 +444,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

No need to hide this minor piece of code behind ifdefs.

No longer hidden by an ifdef, the config option
CONFIG_MMC_SDHCI_CADENCE_ELBA is gone.

quoted

--- /dev/null
+++ b/drivers/mmc/host/sdhci-cadence.h

This new header file can go away too, if you fold in all the code in
the common c-file.

Exactly, the added file sdhci-cadence.h is gone.

Regards,
Brad

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel