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
