[U-Boot] [PATCH 1/2] mmc: add bcm2835 driver

This adds a simple driver for the BCM2835's SD controller.
Workarounds are implemented for: * Register writes can't be too close to each-other in time, or they will be lost. * Register accesses must all be 32-bit, so implement custom accessors.
This code was extracted from: git://github.com/gonzoua/u-boot-pi.git master which was created by Oleksandr Tymoshenko.
Portions of the code there were obviously based on the Linux kernel at: git://github.com/raspberrypi/linux.git rpi-3.2.27
No s-o-b tags were present in either location.
swarren changed the following for upstream: * Removed hack udelay()s in bcm2835_sdhci_raw_writel(); setting SDHCI_QUIRK_WAIT_SEND_CMD appears to solve the issues. * Remove register logging from read*/write* functions. * Sort out confusion with min/max_freq values passed to add_sdhci(). * Simplified and commented twoticks_delay calculation. * checkpatch fixes.
Cc: Oleksandr Tymoshenko gonzo@bluezbox.com Signed-off-by: Stephen Warren swarren@wwwdotorg.org --- This series is based on the previous bcm2835 patches I sent, to add the mbox and video drivers. Patch 1 should be independant, but patch 2 depends on those other patches.
arch/arm/include/asm/arch-bcm2835/sdhci.h | 24 ++++ drivers/mmc/Makefile | 1 + drivers/mmc/bcm2835_sdhci.c | 172 +++++++++++++++++++++++++++++ 3 files changed, 197 insertions(+) create mode 100644 arch/arm/include/asm/arch-bcm2835/sdhci.h create mode 100644 drivers/mmc/bcm2835_sdhci.c
diff --git a/arch/arm/include/asm/arch-bcm2835/sdhci.h b/arch/arm/include/asm/arch-bcm2835/sdhci.h new file mode 100644 index 0000000..a4f867b --- /dev/null +++ b/arch/arm/include/asm/arch-bcm2835/sdhci.h @@ -0,0 +1,24 @@ +/* + * (C) Copyright 2012 Stephen Warren + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef _BCM2835_SDHCI_H_ +#define _BCM2835_SDHCI_H_ + +#define BCM2835_SDHCI_BASE 0x20300000 + +int bcm2835_sdhci_init(u32 regbase, u32 emmc_freq); + +#endif diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile index a1dd730..6be36f2 100644 --- a/drivers/mmc/Makefile +++ b/drivers/mmc/Makefile @@ -43,6 +43,7 @@ COBJS-$(CONFIG_MXS_MMC) += mxsmmc.o COBJS-$(CONFIG_OMAP_HSMMC) += omap_hsmmc.o COBJS-$(CONFIG_PXA_MMC_GENERIC) += pxa_mmc_gen.o COBJS-$(CONFIG_SDHCI) += sdhci.o +COBJS-$(CONFIG_BCM2835_SDHCI) += bcm2835_sdhci.o COBJS-$(CONFIG_S5P_SDHCI) += s5p_sdhci.o COBJS-$(CONFIG_SH_MMCIF) += sh_mmcif.o COBJS-$(CONFIG_TEGRA_MMC) += tegra_mmc.o diff --git a/drivers/mmc/bcm2835_sdhci.c b/drivers/mmc/bcm2835_sdhci.c new file mode 100644 index 0000000..1d623c0 --- /dev/null +++ b/drivers/mmc/bcm2835_sdhci.c @@ -0,0 +1,172 @@ +/* + * This code was extracted from: + * git://github.com/gonzoua/u-boot-pi.git master + * and hence presumably (C) 2012 Oleksandr Tymoshenko + * + * Tweaks for U-Boot upstreaming + * (C) 2012 Stephen Warren + * + * Portions (e.g. read/write macros, concepts for back-to-back register write + * timing workarounds) obviously extracted from the Linux kernel at: + * https://github.com/raspberrypi/linux.git rpi-3.2.27 + * + * The Linux kernel code has the following (c) and license, which is hence + * propagated to Oleksandr's tree and here: + * + * Support for SDHCI device on 2835 + * Based on sdhci-bcm2708.c (c) 2010 Broadcom + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + */ + +/* Supports: + * SDHCI platform device - Arasan SD controller in BCM2708 + * + * Inspired by sdhci-pci.c, by Pierre Ossman + */ + +#include <common.h> +#include <malloc.h> +#include <sdhci.h> + +/* 400KHz is max freq for card ID etc. Use that as min */ +#define MIN_FREQ 400000 + +static uint twoticks_delay; + +static inline void bcm2835_sdhci_raw_writel(struct sdhci_host *host, u32 val, + int reg) +{ + static ulong last_write; + + /* + * The Arasan has a bugette whereby it may lose the content of + * successive writes to registers that are within two SD-card clock + * cycles of each other (a clock domain crossing problem). + * It seems, however, that the data register does not have this problem. + * (Which is just as well - otherwise we'd have to nobble the DMA engine + * too) + */ + while (get_timer(last_write) < twoticks_delay) + ; + + writel(val, host->ioaddr + reg); + last_write = get_timer(0); +} + +static inline u32 bcm2835_sdhci_raw_readl(struct sdhci_host *host, int reg) +{ + return readl(host->ioaddr + reg); +} + +static void bcm2835_sdhci_writel(struct sdhci_host *host, u32 val, int reg) +{ + bcm2835_sdhci_raw_writel(host, val, reg); +} + +static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val, int reg) +{ + static u32 shadow; + + u32 p = reg == SDHCI_COMMAND ? shadow : + bcm2835_sdhci_raw_readl(host, reg & ~3); + u32 s = reg << 3 & 0x18; + u32 l = val << s; + u32 m = 0xffff << s; + + if (reg == SDHCI_TRANSFER_MODE) + shadow = (p & ~m) | l; + else + bcm2835_sdhci_raw_writel(host, (p & ~m) | l, reg & ~3); +} + +static void bcm2835_sdhci_writeb(struct sdhci_host *host, u8 val, int reg) +{ + u32 p = bcm2835_sdhci_raw_readl(host, reg & ~3); + u32 s = reg << 3 & 0x18; + u32 l = val << s; + u32 m = 0xff << s; + + bcm2835_sdhci_raw_writel(host, (p & ~m) | l, reg & ~3); +} + +static u32 bcm2835_sdhci_readl(struct sdhci_host *host, int reg) +{ + u32 val = bcm2835_sdhci_raw_readl(host, reg); + + return val; +} + +static u16 bcm2835_sdhci_readw(struct sdhci_host *host, int reg) +{ + u32 val = bcm2835_sdhci_raw_readl(host, (reg & ~3)); + val = val >> (reg << 3 & 0x18) & 0xffff; + + return (u16)val; +} + +static u8 bcm2835_sdhci_readb(struct sdhci_host *host, int reg) +{ + u32 val = bcm2835_sdhci_raw_readl(host, (reg & ~3)); + val = val >> (reg << 3 & 0x18) & 0xff; + + return (u8)val; +} + +static const struct sdhci_ops bcm2835_ops = { + .write_l = bcm2835_sdhci_writel, + .write_w = bcm2835_sdhci_writew, + .write_b = bcm2835_sdhci_writeb, + .read_l = bcm2835_sdhci_readl, + .read_w = bcm2835_sdhci_readw, + .read_b = bcm2835_sdhci_readb, +}; + +int bcm2835_sdhci_init(u32 regbase, u32 emmc_freq) +{ + struct sdhci_host *host = NULL; + + /* + * See the comments in bcm2835_sdhci_raw_writel(). + * + * This should probably be dynamically calculated based on the actual + * frequency. However, this is the longest we'll have to wait, and + * doesn't seem to slow access down too much, so the added complexity + * doesn't seem worth it for now. + * + * 1/MIN_FREQ is (max) time per tick of eMMC clock. + * 2/MIN_FREQ is time for two ticks. + * Multiply by 1000000 to get uS per two ticks. + * +1 for hack rounding. + */ + twoticks_delay = ((2 * 1000000) / MIN_FREQ) + 1; + + host = malloc(sizeof(struct sdhci_host)); + if (!host) { + printf("sdhci_host malloc fail!\n"); + return 1; + } + + host->name = "bcm2835_sdhci"; + host->ioaddr = (void *)regbase; + host->quirks = SDHCI_QUIRK_BROKEN_VOLTAGE | SDHCI_QUIRK_BROKEN_R1B | + SDHCI_QUIRK_WAIT_SEND_CMD; + host->voltages = MMC_VDD_32_33 | MMC_VDD_33_34 | MMC_VDD_165_195; + host->ops = &bcm2835_ops; + + host->version = sdhci_readw(host, SDHCI_HOST_VERSION); + add_sdhci(host, emmc_freq, MIN_FREQ); + + return 0; +}

Enable the SD controller driver for the Raspberry Pi. Enable a number of useful MMC, partition, and filesystem-related commands. Set up the environment to provide standard locations for loading a kernel, DTB, etc. Provide a boot command that loads and executes boot.scr.uimg from the SD card; this is written considering future extensibilty to USB storage.
Signed-off-by: Stephen Warren swarren@wwwdotorg.org --- arch/arm/include/asm/arch-bcm2835/mbox.h | 26 ++++++++++++ board/raspberrypi/rpi_b/rpi_b.c | 26 ++++++++++++ include/configs/rpi_b.h | 68 ++++++++++++++++++++++++++++-- 3 files changed, 117 insertions(+), 3 deletions(-)
diff --git a/arch/arm/include/asm/arch-bcm2835/mbox.h b/arch/arm/include/asm/arch-bcm2835/mbox.h index 6e040c5..5a9bd87 100644 --- a/arch/arm/include/asm/arch-bcm2835/mbox.h +++ b/arch/arm/include/asm/arch-bcm2835/mbox.h @@ -143,6 +143,32 @@ struct bcm2835_mbox_tag_get_arm_mem { } body; };
+#define BCM2835_MBOX_TAG_GET_CLOCK_RATE 0x00030002 + +#define BCM2835_MBOX_CLOCK_ID_EMMC 1 +#define BCM2835_MBOX_CLOCK_ID_UART 2 +#define BCM2835_MBOX_CLOCK_ID_ARM 3 +#define BCM2835_MBOX_CLOCK_ID_CORE 4 +#define BCM2835_MBOX_CLOCK_ID_V3D 5 +#define BCM2835_MBOX_CLOCK_ID_H264 6 +#define BCM2835_MBOX_CLOCK_ID_ISP 7 +#define BCM2835_MBOX_CLOCK_ID_SDRAM 8 +#define BCM2835_MBOX_CLOCK_ID_PIXEL 9 +#define BCM2835_MBOX_CLOCK_ID_PWM 10 + +struct bcm2835_mbox_tag_get_clock_rate { + struct bcm2835_mbox_tag_hdr tag_hdr; + union { + struct { + u32 clock_id; + } req; + struct { + u32 clock_id; + u32 rate_hz; + } resp; + } body; +}; + #define BCM2835_MBOX_TAG_ALLOCATE_BUFFER 0x00040001
struct bcm2835_mbox_tag_allocate_buffer { diff --git a/board/raspberrypi/rpi_b/rpi_b.c b/board/raspberrypi/rpi_b/rpi_b.c index 3c654a1..6b3e095 100644 --- a/board/raspberrypi/rpi_b/rpi_b.c +++ b/board/raspberrypi/rpi_b/rpi_b.c @@ -16,6 +16,7 @@
#include <common.h> #include <asm/arch/mbox.h> +#include <asm/arch/sdhci.h> #include <asm/global_data.h>
DECLARE_GLOBAL_DATA_PTR; @@ -26,6 +27,12 @@ struct msg_get_arm_mem { u32 end_tag; };
+struct msg_get_clock_rate { + struct bcm2835_mbox_hdr hdr; + struct bcm2835_mbox_tag_get_clock_rate get_clock_rate; + u32 end_tag; +}; + int dram_init(void) { ALLOC_ALIGN_BUFFER(struct msg_get_arm_mem, msg, 1, 16); @@ -51,3 +58,22 @@ int board_init(void)
return 0; } + +int board_mmc_init(void) +{ + ALLOC_ALIGN_BUFFER(struct msg_get_clock_rate, msg_clk, 1, 16); + int ret; + + BCM2835_MBOX_INIT_HDR(msg_clk); + BCM2835_MBOX_INIT_TAG(&msg_clk->get_clock_rate, GET_CLOCK_RATE); + msg_clk->get_clock_rate.body.req.clock_id = BCM2835_MBOX_CLOCK_ID_EMMC; + + ret = bcm2835_mbox_call_prop(BCM2835_MBOX_PROP_CHAN, &msg_clk->hdr); + if (ret) { + printf("bcm2835: Could not query eMMC clock rate\n"); + return -1; + } + + return bcm2835_sdhci_init(BCM2835_SDHCI_BASE, + msg_clk->get_clock_rate.body.resp.rate_hz); +} diff --git a/include/configs/rpi_b.h b/include/configs/rpi_b.h index a709feb..4d834e1 100644 --- a/include/configs/rpi_b.h +++ b/include/configs/rpi_b.h @@ -50,6 +50,7 @@ #define CONFIG_SYS_MALLOC_LEN SZ_4M #define CONFIG_SYS_MEMTEST_START 0x00100000 #define CONFIG_SYS_MEMTEST_END 0x00200000 +#define CONFIG_LOADADDR 0x00200000
/* Flash */ #define CONFIG_SYS_NO_FLASH @@ -69,6 +70,13 @@ #define CONFIG_VIDEO_BCM2835 #define CONFIG_SYS_WHITE_ON_BLACK
+/* SD/MMC configuration */ +#define CONFIG_GENERIC_MMC +#define CONFIG_MMC +#define CONFIG_SDHCI +#define CONFIG_MMC_SDHCI_IO_ACCESSORS +#define CONFIG_BCM2835_SDHCI + /* Console UART */ #define CONFIG_PL011_SERIAL #define CONFIG_PL011_CLOCK 3000000 @@ -84,12 +92,59 @@ /* Environment */ #define CONFIG_ENV_SIZE SZ_16K #define CONFIG_ENV_IS_NOWHERE +#define CONFIG_ENV_VARS_UBOOT_CONFIG #define CONFIG_SYS_LOAD_ADDR 0x1000000 #define CONFIG_CONSOLE_MUX #define CONFIG_SYS_CONSOLE_IS_IN_ENV -#define CONFIG_EXTRA_ENV_SETTINGS "stdin=serial\0" \ - "stderr=serial,lcd\0" \ - "stdout=serial,lcd\0" +/* + * Memory layout for where various images get loaded by boot scripts: + * + * scriptaddr can be pretty much anywhere that doesn't conflict with something + * else. Put it low in memory to avoid conflicts. + * + * kernel_addr_r must be within the first 128M of RAM in order for the + * kernel's CONFIG_AUTO_ZRELADDR option to work. Since the kernel will + * decompress itself to 0x8000 after the start of RAM, kernel_addr_r + * should not overlap that area, or the kernel will have to copy itself + * somewhere else before decompression. Similarly, the address of any other + * data passed to the kernel shouldn't overlap the start of RAM. Pushing + * this up to 16M allows for a sizable kernel to be decompressed below the + * compressed load address. + * + * fdt_addr_r simply shouldn't overlap anything else. Choosing 32M allows for + * the compressed kernel to be up to 16M too. + * + * ramdisk_addr_r simply shouldn't overlap anything else. Choosing 33M allows + * for the FDT/DTB to be up to 1M, which is hopefully plenty. + */ +#define CONFIG_EXTRA_ENV_SETTINGS \ + "stdin=serial\0" \ + "stderr=serial,lcd\0" \ + "stdout=serial,lcd\0" \ + "scriptaddr=0x00000000\0" \ + "kernel_addr_r=0x01000000\0" \ + "fdt_addr_r=0x02000000\0" \ + "ramdisk_addr_r=0x02100000\0" \ + "boot_targets=mmc0\0" \ + \ + "script_boot=" \ + "if fatload ${devtype} ${devnum}:1 " \ + "${scriptaddr} boot.scr.uimg; then " \ + "source ${scriptaddr}; " \ + "fi;\0" \ + \ + "mmc_boot=" \ + "setenv devtype mmc; " \ + "if mmc dev ${devnum}; then " \ + "run script_boot; " \ + "fi\0" \ + \ + "bootcmd_mmc0=setenv devnum 0; run mmc_boot\0" \ + +#define CONFIG_BOOTCOMMAND \ + "for target in ${boot_targets}; do run bootcmd_${target}; done" + +#define CONFIG_BOOTDELAY 2
/* Shell */ #define CONFIG_SYS_HUSH_PARSER @@ -104,6 +159,13 @@ #include <config_cmd_default.h> #define CONFIG_CMD_BOOTZ #define CONFIG_CMD_GPIO +#define CONFIG_CMD_MMC +#define CONFIG_DOS_PARTITION +#define CONFIG_PARTITION_UUIDS +#define CONFIG_CMD_PART +#define CONFIG_CMD_FS_GENERIC +#define CONFIG_CMD_FAT +#define CONFIG_CMD_EXT /* Some things don't make sense on this HW or yet */ #undef CONFIG_CMD_FPGA #undef CONFIG_CMD_NET

Some nitpicks.
On 10/24/2012 10:20 AM, Stephen Warren wrote:
This adds a simple driver for the BCM2835's SD controller.
Workarounds are implemented for:
- Register writes can't be too close to each-other in time, or they will be lost.
- Register accesses must all be 32-bit, so implement custom accessors.
This code was extracted from: git://github.com/gonzoua/u-boot-pi.git master which was created by Oleksandr Tymoshenko.
Portions of the code there were obviously based on the Linux kernel at: git://github.com/raspberrypi/linux.git rpi-3.2.27
No s-o-b tags were present in either location.
swarren changed the following for upstream:
- Removed hack udelay()s in bcm2835_sdhci_raw_writel(); setting SDHCI_QUIRK_WAIT_SEND_CMD appears to solve the issues.
- Remove register logging from read*/write* functions.
- Sort out confusion with min/max_freq values passed to add_sdhci().
- Simplified and commented twoticks_delay calculation.
- checkpatch fixes.
Cc: Oleksandr Tymoshenkogonzo@bluezbox.com Signed-off-by: Stephen Warrenswarren@wwwdotorg.org
This series is based on the previous bcm2835 patches I sent, to add the mbox and video drivers. Patch 1 should be independant, but patch 2 depends on those other patches.
arch/arm/include/asm/arch-bcm2835/sdhci.h | 24 ++++ drivers/mmc/Makefile | 1 + drivers/mmc/bcm2835_sdhci.c | 172 +++++++++++++++++++++++++++++ 3 files changed, 197 insertions(+) create mode 100644 arch/arm/include/asm/arch-bcm2835/sdhci.h create mode 100644 drivers/mmc/bcm2835_sdhci.c
diff --git a/arch/arm/include/asm/arch-bcm2835/sdhci.h b/arch/arm/include/asm/arch-bcm2835/sdhci.h new file mode 100644 index 0000000..a4f867b --- /dev/null +++ b/arch/arm/include/asm/arch-bcm2835/sdhci.h @@ -0,0 +1,24 @@ +/*
- (C) Copyright 2012 Stephen Warren
- See file CREDITS for list of people who contributed to this
- project.
- This program is free software; you can redistribute it and/or
- modify it under the terms of the GNU General Public License
- version 2 as published by the Free Software Foundation.
- This program is distributed in the hope that it will be useful, but
- WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- */
+#ifndef _BCM2835_SDHCI_H_ +#define _BCM2835_SDHCI_H_
+#define BCM2835_SDHCI_BASE 0x20300000
+int bcm2835_sdhci_init(u32 regbase, u32 emmc_freq);
+#endif diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile index a1dd730..6be36f2 100644 --- a/drivers/mmc/Makefile +++ b/drivers/mmc/Makefile @@ -43,6 +43,7 @@ COBJS-$(CONFIG_MXS_MMC) += mxsmmc.o COBJS-$(CONFIG_OMAP_HSMMC) += omap_hsmmc.o COBJS-$(CONFIG_PXA_MMC_GENERIC) += pxa_mmc_gen.o COBJS-$(CONFIG_SDHCI) += sdhci.o +COBJS-$(CONFIG_BCM2835_SDHCI) += bcm2835_sdhci.o COBJS-$(CONFIG_S5P_SDHCI) += s5p_sdhci.o COBJS-$(CONFIG_SH_MMCIF) += sh_mmcif.o COBJS-$(CONFIG_TEGRA_MMC) += tegra_mmc.o diff --git a/drivers/mmc/bcm2835_sdhci.c b/drivers/mmc/bcm2835_sdhci.c new file mode 100644 index 0000000..1d623c0 --- /dev/null +++ b/drivers/mmc/bcm2835_sdhci.c @@ -0,0 +1,172 @@ +/*
- This code was extracted from:
- git://github.com/gonzoua/u-boot-pi.git master
- and hence presumably (C) 2012 Oleksandr Tymoshenko
- Tweaks for U-Boot upstreaming
- (C) 2012 Stephen Warren
- Portions (e.g. read/write macros, concepts for back-to-back register write
- timing workarounds) obviously extracted from the Linux kernel at:
- https://github.com/raspberrypi/linux.git rpi-3.2.27
- The Linux kernel code has the following (c) and license, which is hence
- propagated to Oleksandr's tree and here:
- Support for SDHCI device on 2835
- Based on sdhci-bcm2708.c (c) 2010 Broadcom
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License version 2 as
- published by the Free Software Foundation.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
- */
+/* Supports:
- SDHCI platform device - Arasan SD controller in BCM2708
- Inspired by sdhci-pci.c, by Pierre Ossman
- */
+#include<common.h> +#include<malloc.h> +#include<sdhci.h>
+/* 400KHz is max freq for card ID etc. Use that as min */ +#define MIN_FREQ 400000
+static uint twoticks_delay;
+static inline void bcm2835_sdhci_raw_writel(struct sdhci_host *host, u32 val,
int reg)
+{
- static ulong last_write;
- /*
* The Arasan has a bugette whereby it may lose the content of
* successive writes to registers that are within two SD-card clock
* cycles of each other (a clock domain crossing problem).
* It seems, however, that the data register does not have this problem.
* (Which is just as well - otherwise we'd have to nobble the DMA engine
* too)
*/
- while (get_timer(last_write)< twoticks_delay)
;
- writel(val, host->ioaddr + reg);
- last_write = get_timer(0);
+}
+static inline u32 bcm2835_sdhci_raw_readl(struct sdhci_host *host, int reg) +{
- return readl(host->ioaddr + reg);
+}
+static void bcm2835_sdhci_writel(struct sdhci_host *host, u32 val, int reg) +{
- bcm2835_sdhci_raw_writel(host, val, reg);
+}
+static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val, int reg) +{
- static u32 shadow;
- u32 p = reg == SDHCI_COMMAND ? shadow :
bcm2835_sdhci_raw_readl(host, reg& ~3);
- u32 s = reg<< 3& 0x18;
- u32 l = val<< s;
- u32 m = 0xffff<< s;
- if (reg == SDHCI_TRANSFER_MODE)
shadow = (p& ~m) | l;
- else
bcm2835_sdhci_raw_writel(host, (p& ~m) | l, reg& ~3);
+}
+static void bcm2835_sdhci_writeb(struct sdhci_host *host, u8 val, int reg) +{
- u32 p = bcm2835_sdhci_raw_readl(host, reg& ~3);
- u32 s = reg<< 3& 0x18;
- u32 l = val<< s;
- u32 m = 0xff<< s;
- bcm2835_sdhci_raw_writel(host, (p& ~m) | l, reg& ~3);
+}
+static u32 bcm2835_sdhci_readl(struct sdhci_host *host, int reg) +{
- u32 val = bcm2835_sdhci_raw_readl(host, reg);
- return val;
+}
+static u16 bcm2835_sdhci_readw(struct sdhci_host *host, int reg) +{
- u32 val = bcm2835_sdhci_raw_readl(host, (reg& ~3));
- val = val>> (reg<< 3& 0x18)& 0xffff;
- return (u16)val;
+}
+static u8 bcm2835_sdhci_readb(struct sdhci_host *host, int reg) +{
- u32 val = bcm2835_sdhci_raw_readl(host, (reg& ~3));
- val = val>> (reg<< 3& 0x18)& 0xff;
- return (u8)val;
+}
Can the above used magics be made as macros?
~Vikram

On 10/26/2012 05:33 AM, Vikram Narayanan wrote:
Some nitpicks.
On 10/24/2012 10:20 AM, Stephen Warren wrote:
This adds a simple driver for the BCM2835's SD controller.
Workarounds are implemented for:
- Register writes can't be too close to each-other in time, or they will be lost.
- Register accesses must all be 32-bit, so implement custom accessors.
...
+static u8 bcm2835_sdhci_readb(struct sdhci_host *host, int reg) +{
- u32 val = bcm2835_sdhci_raw_readl(host, (reg& ~3));
- val = val>> (reg<< 3& 0x18)& 0xff;
- return (u8)val;
+}
Can the above used magics be made as macros?
This code was taken directly from the downstream Linux kernel, so I changed it as little as possible, to make comparisons easier. Still, if people want I can certainly make it easier to understand the expression a bit.
I don't think the issue is the magic numbers so much as understanding what the expression does; the magic are obvious then. It's simply extracting byte n from from a u32. Would the following be more obvious:
byte_num = reg & 3; byte_shift = bytenum * 8; byte = (val >> byte_shift) & 0xff;
... and similar for the other functions?

On 10/28/2012 8:58 AM, Stephen Warren wrote:
On 10/26/2012 05:33 AM, Vikram Narayanan wrote:
Some nitpicks.
On 10/24/2012 10:20 AM, Stephen Warren wrote:
<snip>
+static u8 bcm2835_sdhci_readb(struct sdhci_host *host, int reg) +{
- u32 val = bcm2835_sdhci_raw_readl(host, (reg& ~3));
- val = val>> (reg<< 3& 0x18)& 0xff;
- return (u8)val;
+}
Can the above used magics be made as macros?
This code was taken directly from the downstream Linux kernel, so I changed it as little as possible, to make comparisons easier. Still, if people want I can certainly make it easier to understand the expression a bit.
Seems reasonable.
I don't think the issue is the magic numbers so much as understanding what the expression does; the magic are obvious then. It's simply extracting byte n from from a u32. Would the following be more obvious:
byte_num = reg& 3; byte_shift = bytenum * 8; byte = (val>> byte_shift)& 0xff;
... and similar for the other functions?
This looks better to me than the former. Thanks.
~Vikram

Hi Stephen,
On Sun, 28 Oct 2012 22:36:25 +0530, Vikram Narayanan vikram186@gmail.com wrote:
On 10/28/2012 8:58 AM, Stephen Warren wrote:
On 10/26/2012 05:33 AM, Vikram Narayanan wrote:
Some nitpicks.
On 10/24/2012 10:20 AM, Stephen Warren wrote:
<snip> >>> +static u8 bcm2835_sdhci_readb(struct sdhci_host *host, int reg) >>> +{ >>> + u32 val = bcm2835_sdhci_raw_readl(host, (reg& ~3)); >>> + val = val>> (reg<< 3& 0x18)& 0xff; >>> + >>> + return (u8)val; >>> +} >> >> Can the above used magics be made as macros? > > This code was taken directly from the downstream Linux kernel, so I > changed it as little as possible, to make comparisons easier. Still, if > people want I can certainly make it easier to understand the expression > a bit.
Seems reasonable.
I don't think the issue is the magic numbers so much as understanding what the expression does; the magic are obvious then. It's simply extracting byte n from from a u32. Would the following be more obvious:
byte_num = reg& 3; byte_shift = bytenum * 8; byte = (val>> byte_shift)& 0xff;
... and similar for the other functions?
This looks better to me than the former. Thanks.
Stephen,
Does this mean there'll be a new version of this patch?
Amicalement,

On 11/04/2012 08:32 AM, Albert ARIBAUD wrote:
Hi Stephen,
On Sun, 28 Oct 2012 22:36:25 +0530, Vikram Narayanan vikram186@gmail.com wrote:
On 10/28/2012 8:58 AM, Stephen Warren wrote:
On 10/26/2012 05:33 AM, Vikram Narayanan wrote:
Some nitpicks.
On 10/24/2012 10:20 AM, Stephen Warren wrote:
<snip> >>> +static u8 bcm2835_sdhci_readb(struct sdhci_host *host, int reg) >>> +{ >>> + u32 val = bcm2835_sdhci_raw_readl(host, (reg& ~3)); >>> + val = val>> (reg<< 3& 0x18)& 0xff; >>> + >>> + return (u8)val; >>> +} >> >> Can the above used magics be made as macros? > > This code was taken directly from the downstream Linux kernel, so I > changed it as little as possible, to make comparisons easier. Still, if > people want I can certainly make it easier to understand the expression > a bit.
Seems reasonable.
I don't think the issue is the magic numbers so much as understanding what the expression does; the magic are obvious then. It's simply extracting byte n from from a u32. Would the following be more obvious:
byte_num = reg& 3; byte_shift = bytenum * 8; byte = (val>> byte_shift)& 0xff;
... and similar for the other functions?
This looks better to me than the former. Thanks.
Stephen,
Does this mean there'll be a new version of this patch?
Amicalement,
Hopefully. I'm waiting on Signed-off-by lines from Oleksandr Tymoshenko and Dom Cobley (assuming they can provide them...) before I make this change and repost.

On Tue, Oct 23, 2012 at 10:50:47PM -0600, Stephen Warren wrote:
This adds a simple driver for the BCM2835's SD controller.
Workarounds are implemented for:
- Register writes can't be too close to each-other in time, or they will be lost.
- Register accesses must all be 32-bit, so implement custom accessors.
This code was extracted from: git://github.com/gonzoua/u-boot-pi.git master which was created by Oleksandr Tymoshenko.
Portions of the code there were obviously based on the Linux kernel at: git://github.com/raspberrypi/linux.git rpi-3.2.27
No s-o-b tags were present in either location.
Um, that's a bit worrying to me. Wolfgang?

On 10/26/2012 10:32 AM, Tom Rini wrote:
On Tue, Oct 23, 2012 at 10:50:47PM -0600, Stephen Warren wrote:
This adds a simple driver for the BCM2835's SD controller.
Workarounds are implemented for:
- Register writes can't be too close to each-other in time, or they will be lost.
- Register accesses must all be 32-bit, so implement custom accessors.
This code was extracted from: git://github.com/gonzoua/u-boot-pi.git master which was created by Oleksandr Tymoshenko.
Portions of the code there were obviously based on the Linux kernel at: git://github.com/raspberrypi/linux.git rpi-3.2.27
No s-o-b tags were present in either location.
Um, that's a bit worrying to me. Wolfgang?
I didn't have much experience with s-o-b stuff, so excuse my ignorance. Is there official procedure, or is it enough to state that I sign off on everything in git://github.com/gonzoua/u-boot-pi.git master authored by me?

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/26/12 10:48, Oleksandr Tymoshenko wrote:
On 10/26/2012 10:32 AM, Tom Rini wrote:
On Tue, Oct 23, 2012 at 10:50:47PM -0600, Stephen Warren wrote:
This adds a simple driver for the BCM2835's SD controller.
Workarounds are implemented for: * Register writes can't be too close to each-other in time, or they will be lost. * Register accesses must all be 32-bit, so implement custom accessors.
This code was extracted from: git://github.com/gonzoua/u-boot-pi.git master which was created by Oleksandr Tymoshenko.
Portions of the code there were obviously based on the Linux kernel at: git://github.com/raspberrypi/linux.git rpi-3.2.27
No s-o-b tags were present in either location.
Um, that's a bit worrying to me. Wolfgang?
I didn't have much experience with s-o-b stuff, so excuse my ignorance. Is there official procedure, or is it enough to state that I sign off on everything in git://github.com/gonzoua/u-boot-pi.git master authored by me?
Just about. Please read http://elinux.org/Developer_Certificate_Of_Origin and then assuming that is all true, reply to this email with a properly formatted Signed-off-by line (and then hey, you end up in the contributors for the next U-Boot release, once the code is merged). And then you can get in the habit of doing 'git commit -s' for projects where the above applies. There's still a question about the portions taken from the kernel tree. Thanks!
- -- Tom

On 10/26/2012 12:05 PM, Tom Rini wrote:
On 10/26/12 10:48, Oleksandr Tymoshenko wrote:
On 10/26/2012 10:32 AM, Tom Rini wrote:
On Tue, Oct 23, 2012 at 10:50:47PM -0600, Stephen Warren wrote:
This adds a simple driver for the BCM2835's SD controller.
Workarounds are implemented for: * Register writes can't be too close to each-other in time, or they will be lost. * Register accesses must all be 32-bit, so implement custom accessors.
This code was extracted from: git://github.com/gonzoua/u-boot-pi.git master which was created by Oleksandr Tymoshenko.
Portions of the code there were obviously based on the Linux kernel at: git://github.com/raspberrypi/linux.git rpi-3.2.27
No s-o-b tags were present in either location.
Um, that's a bit worrying to me. Wolfgang?
I didn't have much experience with s-o-b stuff, so excuse my ignorance. Is there official procedure, or is it enough to state that I sign off on everything in git://github.com/gonzoua/u-boot-pi.git master authored by me?
Just about. Please read http://elinux.org/Developer_Certificate_Of_Origin and then assuming that is all true, reply to this email with a properly formatted Signed-off-by line (and then hey, you end up in the contributors for the next U-Boot release, once the code is merged). And then you can get in the habit of doing 'git commit -s' for projects where the above applies. There's still a question about the portions taken from the kernel tree. Thanks!
Although do remember that U-Boot uses S-o-b differently to how the Linux kernel uses it, based on our recent discussions. For U-Boot, it's apparently only about original authorship and/or changes to the patches, whereas for the kernel, people who handle the patch without changes (i.e. apply it to a git tree) also sign the patch off.
Equally, since this code was derived from Linux kernel source, we should track down who wrote that and get them to sign it off too.

On Fri, Oct 26, 2012 at 10:48:06AM -0700, Oleksandr Tymoshenko wrote:
On 10/26/2012 10:32 AM, Tom Rini wrote:
On Tue, Oct 23, 2012 at 10:50:47PM -0600, Stephen Warren wrote:
This adds a simple driver for the BCM2835's SD controller.
Workarounds are implemented for:
- Register writes can't be too close to each-other in time, or they will be lost.
- Register accesses must all be 32-bit, so implement custom accessors.
This code was extracted from: git://github.com/gonzoua/u-boot-pi.git master which was created by Oleksandr Tymoshenko.
Portions of the code there were obviously based on the Linux kernel at: git://github.com/raspberrypi/linux.git rpi-3.2.27
No s-o-b tags were present in either location.
Um, that's a bit worrying to me. Wolfgang?
I didn't have much experience with s-o-b stuff, so excuse my ignorance.
Please read the file, Documentation/SubmittingPatches, in the Linux kernel source directory, for what it means, and what it is about, before doing anything.
thanks,
greg k-h
participants (6)
-
Albert ARIBAUD
-
Greg KH
-
Oleksandr Tymoshenko
-
Stephen Warren
-
Tom Rini
-
Vikram Narayanan