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