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

[Brad Larson]

Hi Yamada-san,

On Fri, Apr 9, 2021 at 1:25 AM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote:
>
> On Tue, Mar 30, 2021 at 7:31 PM 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.
>
>
> I agree with Ulf.

I've folded Elba SoC support into sdhci-cadence.c.

> BTW, this patch cannot probe
> "pensando,elba-emmc"
> when CONFIG_MMC_SDHCI_CADENCE_ELBA=m.

Right, that issue is gone now.  There is no separate sdhci-cadence-elba.c

> > > +#define SDIO_REG_HRS4          0x10
>
> This is the same as SDHCI_CDNS_HRS04
>
>
>
> > > +#define REG_DELAY_HS           0x00
>
> This is the same as SDHCI_CDNS_PHY_DLY_SD_HS
>
>
> > > +#define REG_DELAY_DEFAULT      0x01
>
>
> This is the same as SDHCI_CDNS_PHY_DLY_SD_DEFAULT
>
>
>
> > > +#define REG_DELAY_UHSI_SDR50   0x04
>
> This is the same as SDHCI_CDNS_PHY_DLY_UHS_SDR50
>
>
>
> > > +#define REG_DELAY_UHSI_DDR50   0x05
>
>
> This is the same as SDHCI_CDNS_PHY_DLY_UHS_DDR50
>

All of those redundant definitions are gone, I'm now using the
existing definitions you're identifying.

> > > +
> > > +static void elba_write_l(struct sdhci_host *host, u32 val, int reg)
> > > +{
> > > +       struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
> > > +       unsigned long flags;
> > > +
> > > +       spin_lock_irqsave(&priv->wrlock, flags);
> > > +       writel(0x78, priv->ctl_addr);
> > > +       writel(val, host->ioaddr + reg);
> > > +       spin_unlock_irqrestore(&priv->wrlock, flags);
> > > +}
[...]
> > > +static void elba_priv_write_l(struct sdhci_cdns_priv *priv,
> > > +               u32 val, void __iomem *reg)
> > > +{
> > > +       unsigned long flags;
> > > +
> > > +       spin_lock_irqsave(&priv->wrlock, flags);
> > > +       writel(0x78, priv->ctl_addr);
> > > +       writel(val, reg);
> > > +       spin_unlock_irqrestore(&priv->wrlock, flags);
> > > +}
>
>
> Maybe, can this avoid code duplication?
>
> static void elba_hrs_write_l(struct sdhci_cdns_priv *priv,
>                               u32 val, int reg)
> {
>         unsigned long flags;
>
>         spin_lock_irqsave(&priv->wrlock, flags);
>         writel(0x78, priv->ctl_addr);
>         writel(val, priv->hrs_addr + reg);
>         spin_unlock_irqrestore(&priv->wrlock, flags);
> }
>
> static void elba_write_l(struct sdhci_host *host, u32 val, int reg)
> {
>         elba_hrs_write_l(sdhci_cdns_priv(host), val, SDHCI_CDNS_SRS_BASE + reg);
> }

Yes, some code can be reduced.   This is the current implementation I
propose to include with the v3 patchset

/*
 * The Pensando Elba SoC explicitly controls byte-lane enables on writes
 * which includes writes to the HRS registers.
 */
static void elba_priv_write_l(struct sdhci_cdns_priv *priv, u32 val,
                              void __iomem *reg)
{
        unsigned long flags;

        spin_lock_irqsave(&priv->wrlock, flags);
        writel(0x78, priv->ctl_addr);
        writel(val, reg);
        spin_unlock_irqrestore(&priv->wrlock, flags);
}

static void elba_write_l(struct sdhci_host *host, u32 val, int reg)
{
        elba_priv_write_l(sdhci_cdns_priv(host), val, host->ioaddr + reg);
}

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

This function is removed and the existing sdhci_cdns_phy_init() is
used with DT properties

> > > +{
> > > +       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);
>
>
> This seems to be equivalent to sdhci_cdns_write_phy_reg().

Yes after looking at the implementation it is, there was nothing
special required for Elba.  I've refactored and the function
sdhci_cdns_write_phy_reg() which is used in sdhci_cdns_phy_init()
dependent on DT properties is now used for Elba phy init.

> > > +static void phy_config(struct sdhci_host *host)
> >
> > Ditto.
> >
> > > +{
> > > +       sd4_set_dlyvr(host, REG_DELAY_DEFAULT, 0x04);
> > > +       sd4_set_dlyvr(host, REG_DELAY_HS, 0x04);
> > > +       sd4_set_dlyvr(host, REG_DELAY_UHSI_SDR50, 0x06);
> > > +       sd4_set_dlyvr(host, REG_DELAY_UHSI_DDR50, 0x16);
>
>
> Hard-code board (or chip) specific parameters to the driver?
>
> This should go to DT properties.
>
> See Documentation/devicetree/bindings/mmc/cdns,sdhci.yaml

Exactly, v3 of the patchset will incorporate this recommendation.  In
the case of Elba SoC these are the DT properties being used

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

> > > +static int elba_drv_init(struct platform_device *pdev)
> > > +{
> > > +       struct sdhci_host *host = platform_get_drvdata(pdev);
> > > +       struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
> > > +       struct resource *iomem;
> > > +       void __iomem *ioaddr;
> > > +
> > > +       host->mmc->caps |= (MMC_CAP_1_8V_DDR | MMC_CAP_8_BIT_DATA);
> > > +       iomem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > > +       if (!iomem)
> > > +               return -ENOMEM;
> > > +       ioaddr = devm_ioremap_resource(&pdev->dev, iomem);
> > > +       if (IS_ERR(ioaddr))
> > > +               return PTR_ERR(ioaddr);
>
>
>
> Use  devm_platform_ioremap_resource(pdev, 1)

Replaced devm_ioremap_resource(&pdev->dev, iomem) with
devm_platform_ioremap_resource(pdev, 1).  The change will be in v3 of
the patchset.

Regards,
Brad