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

From: Brad Larson <brad@pensando.io>
To: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>,
	linux-mmc <linux-mmc@vger.kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Masahiro Yamada <yamada.masahiro@socionext.com>
Subject: Re: [PATCH v2 05/13] mmc: sdhci-cadence: Add Pensando Elba SoC support
Date: Sun, 22 Aug 2021 18:28:50 -0700	[thread overview]
Message-ID: <CAK9rFnwBGV=J+duhoFkX4Npr+fbf-EyP0UnhzMVGfJr77f=KFw@mail.gmail.com> (raw)
In-Reply-To: <CAPDyKFpg1qJD0r54sBC3hCoFey_+gwAL1n2o-aGwnAzAan5p7w@mail.gmail.com>

Hi Uffe,

On Tue, Mar 30, 2021 at 3:31 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> + Masahiro Yamada (main author of the driver)
>
> On Mon, 29 Mar 2021 at 03:59, Brad Larson <brad@pensando.io> wrote:
> >
> > 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 <brad@pensando.io>
> > ---
> >  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>;

> > +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.

> > +// 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.

> > +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_

> > +{
> > +       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().

> > @@ -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.

> > --- /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