lkml.org 
[lkml]   [2021]   [Aug]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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

    \
     
     \ /
      Last update: 2021-08-23 03:30    [W:2.852 / U:0.584 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site