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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-08-23 1:31 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-29 1:59 [PATCH v2 00/13] Support Pensando Elba SoC Brad Larson
2021-03-29 1:59 ` [PATCH v2 01/13] gpio: Add Elba SoC gpio driver for spi cs control Brad Larson
2021-03-29 10:41 ` Andy Shevchenko
2021-08-23 1:22 ` Brad Larson
2021-03-29 13:46 ` Linus Walleij
2021-03-31 18:10 ` Serge Semin
2021-08-23 1:24 ` Brad Larson
2021-03-29 1:59 ` [PATCH v2 02/13] spi: cadence-quadspi: Add QSPI support for Pensando Elba SoC Brad Larson
2021-03-30 11:13 ` Pratyush Yadav
2021-03-29 1:59 ` [PATCH v2 03/13] spi: dw: Add support for Pensando Elba SoC SPI Brad Larson
2021-03-29 10:43 ` Andy Shevchenko
2021-08-23 1:25 ` Brad Larson
2021-03-29 15:58 ` Mark Brown
2021-03-30 2:28 ` Brad Larson
2021-03-31 18:00 ` Serge Semin
2021-08-23 1:26 ` Brad Larson
2021-03-29 1:59 ` [PATCH v2 04/13] spidev: Add Pensando CPLD compatible Brad Larson
2021-03-29 10:44 ` Andy Shevchenko
2021-03-30 3:27 ` Brad Larson
2021-03-29 1:59 ` [PATCH v2 05/13] mmc: sdhci-cadence: Add Pensando Elba SoC support Brad Larson
2021-03-30 10:30 ` Ulf Hansson
2021-04-09 8:24 ` Masahiro Yamada
2021-08-23 1:31 ` Brad Larson
2021-08-23 1:28 ` Brad Larson [this message]
2021-03-29 1:59 ` [PATCH v2 06/13] arm64: Add config for Pensando SoC platforms Brad Larson
2021-03-29 1:59 ` [PATCH v2 07/13] arm64: dts: Add Pensando Elba SoC support Brad Larson
2021-03-30 21:55 ` Rob Herring
2021-08-23 1:36 ` Brad Larson
2021-03-31 17:51 ` Serge Semin
2021-08-23 1:55 ` Brad Larson
2021-03-29 1:59 ` [PATCH v2 08/13] dt-bindings: Add pensando vendor prefix Brad Larson
2021-03-30 21:56 ` Rob Herring
2021-03-29 1:59 ` [PATCH v2 09/13] dt-bindings: mmc: Add Pensando Elba SoC binding Brad Larson
2021-03-30 21:56 ` Rob Herring
2021-03-29 1:59 ` [PATCH v2 10/13] dt-bindings: spi: cadence-qspi: Add support for Pensando Elba SoC Brad Larson
2021-03-29 16:00 ` Mark Brown
2021-03-30 2:12 ` Brad Larson
2021-03-30 11:12 ` Pratyush Yadav
2021-08-23 1:57 ` Brad Larson
2021-03-29 1:59 ` [PATCH v2 11/13] dt-bindings: gpio: Add Pensando Elba SoC support Brad Larson
2021-03-29 1:59 ` [PATCH v2 12/13] MAINTAINERS: Add entry for PENSANDO Brad Larson
2021-03-29 1:59 ` [PATCH v2 13/13] gpio: Use linux/gpio/driver.h Brad Larson
2021-03-29 6:48 ` Greg KH
2021-03-30 2:20 ` Brad Larson
2021-03-29 13:44 ` Linus Walleij
2021-03-30 2:21 ` Brad Larson
2021-03-31 16:17 ` [PATCH v2 00/13] Support Pensando Elba SoC Serge Semin
2021-08-23 1:18 ` Brad Larson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAK9rFnwBGV=J+duhoFkX4Npr+fbf-EyP0UnhzMVGfJr77f=KFw@mail.gmail.com' \
--to=brad@pensando.io \
--cc=adrian.hunter@intel.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mmc@vger.kernel.org \
--cc=ulf.hansson@linaro.org \
--cc=yamada.masahiro@socionext.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).