Messages in this thread | | | From | Brad Larson <> | Date | Sun, 22 Aug 2021 18:28:50 -0700 | Subject | 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 <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
| |