linux-kernel - Re: [PATCH v1 5/9] mmc: sdhci: add Black Sesame Technologies BST C1200 controller driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMuHMdVjR0Ki_vLYtvv1HYpZVdLqUtacG=zs4_ooC0yLnK_CvA@mail.gmail.com>
Date: Wed, 28 May 2025 11:06:40 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Albert Yang <yangzh0906@...ndersoft.com>
Cc: Ulf Hansson <ulf.hansson@...aro.org>, Adrian Hunter <adrian.hunter@...el.com>, 
	Ge Gordon <gordon.ge@....ai>, BST Linux Kernel Upstream Group <bst-upstream@...ai.top>, linux-mmc@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	Geert Uytterhoeven <geert+renesas@...der.be>, Victor Shih <victor.shih@...esyslogic.com.tw>, 
	Shan-Chun Hung <shanchun1218@...il.com>, Arnd Bergmann <arnd@...db.de>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, 
	Peter Robinson <pbrobinson@...il.com>, Ben Chuang <ben.chuang@...esyslogic.com.tw>
Subject: Re: [PATCH v1 5/9] mmc: sdhci: add Black Sesame Technologies BST
 C1200 controller driver

Hi Albert,

On Wed, 28 May 2025 at 10:55, Albert Yang <yangzh0906@...ndersoft.com> wrote:
> Add a driver for the DesignWare Mobile Storage Host Controller (DWCMSHC)
> SDHCI controller found in Black Sesame Technologies C1200 SoCs.
>
> The driver provides specialized clock configuration, tuning, voltage
> switching, and power management for the BST DWCMSHC controller. It also
> includes support for eMMC boot and memory-mapped I/O for CRM registers.
>
> Signed-off-by: Ge Gordon <gordon.ge@....ai>
> Signed-off-by: Albert Yang <yangzh0906@...ndersoft.com>

Thanks for your patch!

> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -1112,3 +1112,14 @@ config MMC_LITEX
>           module will be called litex_mmc.
>
>           If unsure, say N.
> +
> +config MMC_SDHCI_BST
> +       tristate "SDHCI OF support for the BST DWC MSHC"

depends on ARCH_BST || COMPILE_TEST

> +       depends on MMC_SDHCI_PLTFM
> +       depends on OF
> +       depends on COMMON_CLK
> +       help
> +         This selects Synopsys DesignWare Cores Mobile Storage Controller
> +         support.
> +         If you have a controller with this interface, say Y or M here.
> +         If unsure, say N.

> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-of-bst-c1200.c

> +#define SDEMMC_CRM_BCLK_DIV_CTRL       0x08
> +#define SDEMMC_CRM_RX_CLK_CTRL         0x14
> +#define SDEMMC_CRM_TIMER_DIV_CTRL      0x0C
> +#define SDEMMC_CRM_VOL_CTRL            0x1C
> +#define DRIVER_NAME                    "sdhci_bst"
> +#define BST_VOL_STABLE_ON              (0x1 << 7)
> +#define SDHCI_DUMP_BST(f, x...)                \
> +       pr_info("%s: " DRIVER_NAME ": " f, mmc_hostname(host->mmc), ##x)
> +#define SD_3_3V                                0
> +#define SD_1_8V                                1
> +#define default_max_freq               200000ul
> +
> +void sdhci_bst_print_vendor(struct sdhci_host *host)
> +{
> +       SDHCI_DUMP_BST("============ SDHCI VENDOR REGISTER DUMP ===========\n");
> +
> +       SDHCI_DUMP_BST("VER_ID:  0x%08x | VER_TPYE:  0x%08x\n",
> +                      sdhci_readl(host, SDHC_MHSC_VER_ID_R),
> +                      sdhci_readl(host, SDHC_MHSC_VER_TPYE_R));
> +       SDHCI_DUMP_BST("MHSC_CTRL:  0x%08x | MBIU_CTRL:  0x%08x\n",
> +                      sdhci_readw(host, SDHC_MHSC_CTRL_R),
> +                      sdhci_readw(host, SDHC_MBIU_CTRL_R));
> +       SDHCI_DUMP_BST("EMMC_CTRL:  0x%08x | BOOT_CTRL: 0x%08x\n",
> +                      sdhci_readl(host, SDHC_EMMC_CTRL_R),
> +                      sdhci_readw(host, SDHC_BOOT_CTRL_R));
> +       SDHCI_DUMP_BST("GP_IN:   0x%08x | GP_OUT: 0x%08x\n",
> +                      sdhci_readl(host, SDHC_GP_IN_R),
> +                      sdhci_readb(host, SDHC_GP_OUT_R));
> +       SDHCI_DUMP_BST("AT_CTRL:     0x%08x | AT_STAT:  0x%08x\n",
> +                      sdhci_readb(host, SDHC_AT_CTRL_R),
> +                      sdhci_readb(host, SDHC_AT_STAT_R));
> +}
> +EXPORT_SYMBOL_GPL(sdhci_bst_print_vendor);

Why do you need this?

> +
> +static u32 bst_read_phys_bst(u32 phys_addr)
> +{
> +       u32 phys_addr_page = phys_addr & 0xFFFFE000;
> +       u32 phys_offset = phys_addr & 0x00001FFF;
> +       u32 map_size = phys_offset + sizeof(u32);
> +       u32 ret = 0xDEADBEEF;
> +       void *mem_mapped = ioremap(phys_addr_page, map_size);
> +
> +       if (mem_mapped) {
> +               ret = (u32)ioread32(((u8 *)mem_mapped) + phys_offset);
> +               iounmap(mem_mapped);
> +       }

Please do not ioremap()/iounmap() for each register read...

> +
> +       return ret;
> +}
> +
> +static void bst_write_phys_bst(u32 phys_addr, u32 value)
> +{
> +       u32 phys_addr_page = phys_addr & 0xFFFFE000;
> +       u32 phys_offset = phys_addr & 0x00001FFF;
> +       u32 map_size = phys_offset + sizeof(u32);
> +       void *mem_mapped = ioremap(phys_addr_page, map_size);
> +
> +       if (mem_mapped) {
> +               iowrite32(value, ((u8 *)mem_mapped) + phys_offset);
> +               iounmap(mem_mapped);
> +       }

or write...

> +}

Aborting review.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds