On Wed 28 Jul 10:43 PDT 2021, chevron wrote:
> Improve the signal integrity for long SD bus trace by using 845+BH201 SD host redriver chip
>
This is a large patch with a very short commit message, which likely is
the reason why you've not gotten any reviews.
What is this hardware? The commit message gives me a feeling that the
BH201 is an external redriver that has been paired with the SDM845
sometimes?
Please describe properly how the hardware setup looks like.
I did skim through the patch trying to figure out what you're doing and
gave you some feedback below, but this is far from a complete review.
And many of the things I point out repeats throughout the patch.
I think the first step is to figure out what this is and if your design
is the right one...
> Signed-off-by: chevron.li <chevron.li@xxxxxxxxxxxxxx>
Is your name really chevron.li? Perhaps Chevron Li?
> ---
> change in V1:
I'm not able to find v0, can you point me to it, because I would like to
understand why these changes was made:
> 1. copy Qualcomm driver base from sdhci-msm.c to sdhci-bayhub.c
Why do we need to duplicate all this code.
> 2. implement the BH201 chip initialization function at sdhci-bayhub.c
Submitting this as a large "copy sdhci-msm and implement BH201" makes it
impossible to figure out what the BH201 specific pieces (that needs the
review) are.
> 3. implement the BH201 chip power control function at sdhci-bayhub.c
> 4. implement the BH201 chip tuning function at sdhci-bayhub.c
> 5. re-implement mmc_rescan to match BH201 mode switch flow at sdhci-bayhub.c
If this is an external redriver, can this be paired with any other SDHCI
controller?
> ---
> ---
[..]
> diff --git a/drivers/mmc/host/sdhci-bayhub.c b/drivers/mmc/host/sdhci-bayhub.c
> new file mode 100644
> index 000000000000..75029470bd22
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-bayhub.c
> @@ -0,0 +1,6563 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Bayhub Technologies, Inc. BH201 SDHCI bridge IC for
> + * VENDOR SDHCI platform driver source file
> + *
> + * Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only 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.
The SPDX header covers this, so don't repeat the license text here.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/delay.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pm_opp.h>
> +#include <linux/slab.h>
> +#include <linux/iopoll.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/interconnect.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/card.h>
> +#include <linux/mmc/sd.h>
> +#include <linux/io.h>
> +#include <linux/of_gpio.h>
> +
> +#include "sdhci-pltfm.h"
> +#include "cqhci.h"
> +#include "../core/core.h"
> +#include "../core/sd_ops.h"
> +#include "../core/sdio_ops.h"
> +#include "../core/mmc_ops.h"
> +#include "../core/sd.h"
> +#include "../core/bus.h"
> +#include "../core/host.h"
> +#include "../core/card.h"
> +#include "../core/pwrseq.h"
> +
> +#define TRUE 1
> +#define FALSE 0
Use "true" and "false"...
> +
> +#define SD_FNC_AM_SDR50 0x2
> +#define SD_FNC_AM_SDR104 0x3
> +#define BIT_PASS_MASK (0x7ff)
> +#define SDR104_MANUAL_INJECT 0x3ff
> +#define SDR50_MANUAL_INJECT 0x77f
> +
[..]
> +#define TUNING_PHASE_SIZE 11
> +#define GGC_CFG_DATA {0x07000000, 0x07364022, 0x01015412, 0x01062400,\
> + 0x10400076, 0x00025432, 0x01046076, 0x62011000,\
> + 0x30503106, 0x64141711, 0x10057513, 0x00336200,\
> + 0x00020006, 0x40000400, 0x12200310, 0x3A314177}
> +
> +struct ggc_bus_mode_cfg_t {
Does "_t" say this is a struct? No need for that as "struct ggc..."
already says "it's a struct".
> + u32 tx_selb_tb[TUNING_PHASE_SIZE];
> + u32 all_selb_tb[TUNING_PHASE_SIZE];
> + u32 tx_selb_failed_history;
> + int bus_mode;
> + int default_sela;
> + int default_selb;
> + u32 default_delaycode;
> + u32 dll_voltage_unlock_cnt[4];
> + u32 max_delaycode;
> + u32 min_delaycode;
> + u32 delaycode_narrowdown_index;
> + u32 fail_phase;
> +};
> +
> +enum tuning_stat_et {
Does "_et" make this an enum?
> + NO_TUNING = 0,
> + OUTPUT_PASS_TYPE = 1,
> + SET_PHASE_FAIL_TYPE = 2,
> + TUNING_FAIL_TYPE = 3,
> + READ_STATUS_FAIL_TYPE = 4,
> + TUNING_CMD7_TIMEOUT = 5,
> + RETUNING_CASE = 6,
> +};
> +
> +
> +struct t_gg_reg_strt {
What is a "t gg reg strt"?
> + u32 ofs;
> + u32 mask;
> + u32 value;
> +};
> +
> +struct rl_bit_lct {
> + u8 bits;
> + u8 rl_bits;
> +};
> +
> +struct chk_type_t {
This only time this is used, it's there to pass 3 bools into a function.
Just use function arguments.
> + u8 right_valid:1;
> + u8 first_valid:1;
> + u8 record_valid:1;
> + u8 reserved:5;
> +};
> +
> +static const char *const op_dbg_str[] = {
> + "no tuning",
> + "pass",
> + "set_phase_fail",
> + "tuning fail",
> + "read status fail",
> + "tuning CMD7 timeout",
> + "retuning case"
> +};
> +
> +struct ggc_platform_t {
What is this "ggc" and what "platform" does this represent?
> + struct ggc_bus_mode_cfg_t sdr50;
> + struct ggc_bus_mode_cfg_t sdr104;
> + struct ggc_bus_mode_cfg_t *cur_bus_mode;
> +
> + struct t_gg_reg_strt pha_stas_rx_low32;
> + struct t_gg_reg_strt pha_stas_rx_high32;
> + struct t_gg_reg_strt pha_stas_tx_low32;
> + struct t_gg_reg_strt pha_stas_tx_high32;
> + struct t_gg_reg_strt dll_sela_after_mask;
> + struct t_gg_reg_strt dll_selb_after_mask;
> +
> + struct t_gg_reg_strt dll_delay_100m_backup;
> + struct t_gg_reg_strt dll_delay_200m_backup;
> +
> + struct t_gg_reg_strt dll_sela_100m_cfg;
> + struct t_gg_reg_strt dll_sela_200m_cfg;
> + struct t_gg_reg_strt dll_selb_100m_cfg;
> + struct t_gg_reg_strt dll_selb_200m_cfg;
> + struct t_gg_reg_strt dll_selb_100m_cfg_en;
> + struct t_gg_reg_strt dll_selb_200m_cfg_en;
> + struct t_gg_reg_strt internl_tuning_en_100m;
> + struct t_gg_reg_strt internl_tuning_en_200m;
> + struct t_gg_reg_strt cmd19_cnt_cfg;
> +
> + struct t_gg_reg_strt inject_failure_for_tuning_enable_cfg;
> + struct t_gg_reg_strt inject_failure_for_200m_tuning_cfg;
> + struct t_gg_reg_strt inject_failure_for_100m_tuning_cfg;
> +
> + int def_sela_100m;
> + int def_sela_200m;
> + int def_selb_100m;
> + int def_selb_200m;
> +
> + u32 _gg_reg_cur[16];
> + u8 _cur_read_buf[512];
Why does these have a '_' prefix?
> +
> + bool dll_unlock_reinit_flg;
> + u8 driver_strength_reinit_flg;
> + bool tuning_cmd7_timeout_reinit_flg;
> + u32 tuning_cmd7_timeout_reinit_cnt;
> + u32 ggc_cur_sela;
> + bool selx_tuning_done_flag;
> + u32 ggc_cmd_tx_selb_failed_range;
> + int ggc_sw_selb_tuning_first_selb;
> + enum tuning_stat_et ggc_sela_tuning_result[11];
> + int dll_voltage_scan_map[4];
> + int cur_dll_voltage_idx;
> + int sdr50_notuning_sela_inject_flag;
> + int sdr50_notuning_crc_error_flag;
> + u32 sdr50_notuning_sela_rx_inject;
> + u32 bh201_sdr50_sela_sw_inject;
> + u32 bh201_sdr50_selb_hw_inject;
> + u32 bh201_sdr104_selb_hw_inject;
> + u32 bh201_drive_strength;
> + bool tuning_in_progress;
> + int bh201_used;
> + int pwr_gpio; /* External power enable pin for Redriver IC */
> + int det_gpio;
> +};
[..]
> +static const struct sdhci_msm_bayhub_offset sdhci_msm_bayhub_v5_offset = {
> + .core_mci_data_cnt = 0x35c,
> + .core_mci_status = 0x324,
> + .core_mci_fifo_cnt = 0x308,
> + .core_mci_version = 0x318,
> + .core_generics = 0x320,
> + .core_testbus_config = 0x32c,
> + .core_testbus_sel2_bit = 3,
> + .core_testbus_ena = (1 << 31),
> + .core_testbus_sel2 = (1 << 3),
> + .core_pwrctl_status = 0x240,
> + .core_pwrctl_mask = 0x244,
> + .core_pwrctl_clear = 0x248,
> + .core_pwrctl_ctl = 0x24c,
> + .core_sdcc_debug_reg = 0x358,
> + .core_dll_config = 0x200,
> + .core_dll_status = 0x208,
> + .core_vendor_spec = 0x20c,
> + .core_vendor_spec_adma_err_addr0 = 0x214,
> + .core_vendor_spec_adma_err_addr1 = 0x218,
> + .core_vendor_spec_func2 = 0x210,
> + .core_vendor_spec_capabilities0 = 0x21c,
> + .core_ddr_200_cfg = 0x224,
> + .core_vendor_spec3 = 0x250,
> + .core_dll_config_2 = 0x254,
> + .core_dll_config_3 = 0x258,
> + .core_ddr_config = 0x25c,
> + .core_dll_usr_ctl = 0x388,
> +};
> +
> +static const struct sdhci_msm_bayhub_offset sdhci_msm_bayhub_mci_offset = {
This is unused and as such you don't need the sdhci_msm_bayhub_offset
either.
> + .core_hc_mode = 0x78,
> + .core_mci_data_cnt = 0x30,
> + .core_mci_status = 0x34,
> + .core_mci_fifo_cnt = 0x44,
> + .core_mci_version = 0x050,
> + .core_generics = 0x70,
> + .core_testbus_config = 0x0cc,
> + .core_testbus_sel2_bit = 4,
> + .core_testbus_ena = (1 << 3),
> + .core_testbus_sel2 = (1 << 4),
> + .core_pwrctl_status = 0xdc,
> + .core_pwrctl_mask = 0xe0,
> + .core_pwrctl_clear = 0xe4,
> + .core_pwrctl_ctl = 0xe8,
> + .core_sdcc_debug_reg = 0x124,
> + .core_dll_config = 0x100,
> + .core_dll_status = 0x108,
> + .core_vendor_spec = 0x10c,
> + .core_vendor_spec_adma_err_addr0 = 0x114,
> + .core_vendor_spec_adma_err_addr1 = 0x118,
> + .core_vendor_spec_func2 = 0x110,
> + .core_vendor_spec_capabilities0 = 0x11c,
> + .core_ddr_200_cfg = 0x184,
> + .core_vendor_spec3 = 0x1b0,
> + .core_dll_config_2 = 0x1b4,
> + .core_ddr_config_old = 0x1b8,
> + .core_ddr_config = 0x1bc,
> +};
> +
[..]
> +static const unsigned int freqs[] = { 400000, 300000, 200000, 100000 };
> +
> +static void cfg_bit_2_bt(int max_bit, int tar, int *byt, int *bit)
> +{
> + struct rl_bit_lct cfg_bit_map[6] = {
> + {0, 6}, {1, 5}, {2, 4},
> + {3, 2}, {4, 1}, {5, 0},
> + };
So you have an array of tuples, where the first entry in the tuple is
the index in the array? And then you only use rl_bits anyways...
How about:
u8 bit_map[6] = { 6, 5, 4, 2, 1, 0 }
> +
> + *byt = (max_bit - tar) / 6;
> + *bit = cfg_bit_map[(max_bit - tar) % 6].rl_bits;
> +}
> +
> +static u32 cfg_read_bits_ofs_mask(u8 *cfg, struct t_gg_reg_strt *bts)
> +{
> + u32 rv = 0;
> + u32 msk = bts->mask;
> + int byt = 0, bit = 0;
> + int i = 0;
> +
> + do {
> + cfg_bit_2_bt(MAX_CFG_BIT_VAL, bts->ofs + i, &byt, &bit);
So "bt" is short for "byte" and "bts" is "bytes"?
> + if (cfg[byt] & (1 << bit))
> + rv |= 1 << i;
> +
> + i++;
> + msk >>= 1;
> + if (msk == 0)
> + break;
There's no case where mask is 0 (and I don't see why it would be), so
while (msk) {
...;
msk >>= 1;
}
Seems cleaner
But all of this would benefit from better naming and probably some
comments describing the actual format/structure of these bits and bytes.
> + } while (1);
> + return rv;
> +}
> +
> +static void get_default_setting(struct sdhci_host *host, u8 *data)
> +{
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_msm_bayhub_host *vendor_host = sdhci_pltfm_priv(pltfm_host);
> +
> + vendor_host->ggc.def_sela_100m =
> + cfg_read_bits_ofs_mask(data, &vendor_host->ggc.dll_sela_100m_cfg);
> + vendor_host->ggc.def_sela_200m =
> + cfg_read_bits_ofs_mask(data, &vendor_host->ggc.dll_sela_200m_cfg);
> + vendor_host->ggc.def_selb_100m =
> + cfg_read_bits_ofs_mask(data, &vendor_host->ggc.dll_sela_100m_cfg);
> + vendor_host->ggc.def_selb_200m =
> + cfg_read_bits_ofs_mask(data, &vendor_host->ggc.dll_sela_200m_cfg);
> +}
> +
> +static void cfg_write_bits_ofs_mask(u8 *cfg,
> + struct t_gg_reg_strt *bts, u32 w_value)
> +{
> + u32 wv = w_value & bts->mask;
> + u32 msk = bts->mask;
> + int byt = 0, bit = 0;
> + int i = 0;
> +
> + do {
> + cfg_bit_2_bt(MAX_CFG_BIT_VAL, bts->ofs + i, &byt, &bit);
> + if (wv & 1)
> + cfg[byt] |= (u8) (1 << bit);
> + else
> + cfg[byt] &= (u8) (~(1 << bit));
> +
> + i++;
> + wv >>= 1;
> + msk >>= 1;
> + if (msk == 0)
> + break;
> + } while (1);
> +}
> +
> +static void ram_bit_2_bt(int tar, int *byt, int *bit)
So the "ram" variant is just a regular bitmap with 8 bits per byte?
include/linux/bitmap.h gives your an interface for dealing with those.
> +{
> + *byt = tar / 8;
> + *bit = tar % 8;
> +}
> +
> +static void set_gg_reg_cur_val(struct ggc_platform_t *ggc,
> + u8 *data, u8 len)
> +{
> + memcpy(&ggc->_gg_reg_cur[0], data, len);
> +}
> +
> +static void get_gg_reg_cur_val(struct ggc_platform_t *ggc,
> + u8 *data, u8 len)
> +{
> + memcpy(data, &ggc->_gg_reg_cur[0], len);
> +}
> +
> +static void get_gg_reg_def(struct sdhci_host *host, u8 *data)
> +{
> + u32 gg_sw_def[16] = GGC_CFG_DATA;
> +
> + memcpy(data, (u8 *)&(gg_sw_def[0]), sizeof(gg_sw_def));
(u8 *)&(gg_sw_def[0])... The address of the first element, type casted
to a (u8*), then passed to a void *?
Isn't that the same thing as just typing "gg_sw_def"?
But why is GGC_CFG_DATA a define and not just a static const u32 array,
in which case you can replace this function with just a simple memcpy in
ggc_chip_init().
> +}
> +
> +static u32 read_ram_bits_ofs_mask(u8 *cfg, struct t_gg_reg_strt *bts)
> +{
> + u32 rv = 0;
> + u32 msk = bts->mask;
> + int byt = 0, bit = 0;
> + int i = 0;
> +
> + do {
> + ram_bit_2_bt(bts->ofs + i, &byt, &bit);
> + if (cfg[byt] & (1 << bit))
> + rv |= 1 << i;
> +
> + i++;
> + msk >>= 1;
> + if (msk == 0)
> + break;
> +
> + } while (1);
> + return rv;
> +}
> +
> +static void ggc_dll_voltage_init(struct sdhci_host *host)
> +{
> + int i = 0;
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_msm_bayhub_host *vendor_host =
> + sdhci_pltfm_priv(pltfm_host);
> +
> + for (i = 0; i < 4; i++) {
> + vendor_host->ggc.dll_voltage_scan_map[i] = 0;
> + vendor_host->ggc.sdr50.dll_voltage_unlock_cnt[i] = 0;
> + vendor_host->ggc.sdr104.dll_voltage_unlock_cnt[i] = 0;
> + }
> +}
> +
> +static void ggc_chip_init(struct sdhci_host *host)
> +{
> + u8 data[512] = { 0 };
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_msm_bayhub_host *vendor_host =
> + sdhci_pltfm_priv(pltfm_host);
> +
> + get_gg_reg_def(host, data);
> + get_default_setting(host, data);
> + set_gg_reg_cur_val(&vendor_host->ggc, data, 64);
> +}
> +
> +static int driver_send_command7(struct sdhci_host *host)
> +{
> + int ret = 0;
> + int err;
> + struct mmc_host *mmc = host->mmc;
> + struct mmc_command cmd = {0};
> +
> + cmd.opcode = MMC_SELECT_CARD;
> + cmd.arg = 0;
> + cmd.flags = MMC_RSP_NONE | MMC_CMD_AC;
> + err = mmc_wait_for_cmd(mmc, &cmd, 3);
> + if (err)
> + ret = 0;
> + else
> + ret = 1;
> +
> + return ret;
return mmc_wait_for_cmd() seems more reasonable, but you don't check the
return value in any off the call sites...
> +}
> +
> +static void driver_send_command24(struct sdhci_host *host,
> + u32 *cfg_data, int data_len)
> +{
> + struct mmc_host *mmc = host->mmc;
> + struct mmc_request mrq = {0};
> + struct mmc_command cmd = { 0 };
> + struct mmc_data data = { 0 };
> + struct scatterlist sg;
> + u8 *data1 = kzalloc(PAGE_SIZE, GFP_KERNEL);
This allocation needs a NULL check.
> +
> + memcpy(data1, (u8 *)&(cfg_data[0]), data_len);
> + sg_init_one(&sg, data1, 512);
> +
> + cmd.opcode = MMC_WRITE_BLOCK;
> + cmd.arg = 0;
> + cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
> + data.blksz = 512;
> + data.blocks = 1;
> + data.flags = MMC_DATA_WRITE;
> + data.timeout_ns = 1000 * 1000 * 1000;
> + data.sg = &sg;
> + data.sg_len = 1;
> + mrq.cmd = &cmd;
> + mrq.data = &data;
> + mrq.stop = NULL;
> +
> + mmc_wait_for_req(mmc, &mrq);
> + kfree(data1);
> +}
> +
> +static void bht_update_cfg(struct mmc_host *mmc_host,
> + struct mmc_card *card, u32 *cfg_data, int data_len)
> +{
> + int ret = 0;
This is unused.
> + struct sdhci_host *host;
> +
> + host = mmc_priv(mmc_host);
> + mmc_set_bus_width(mmc_host, MMC_BUS_WIDTH_4);
> + if (host->ops->reset)
> + host->ops->reset(host, SDHCI_RESET_CMD|SDHCI_RESET_DATA);
> +
> + driver_send_command7(host);
> + driver_send_command24(host, cfg_data, data_len);
> + driver_send_command7(host);
> +
> + mmc_set_bus_width(mmc_host, MMC_BUS_WIDTH_1);
> +}
> +
> +
[..]
> +static void bht_load_hw_inject(struct mmc_host *mmc_host,
> + struct mmc_card *card, u32 *cfg_data, int data_len,
> + u32 sel200, u32 sel100)
> +{
> + struct sdhci_host *host = mmc_priv(mmc_host);
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_msm_bayhub_host *vendor_host =
> + sdhci_pltfm_priv(pltfm_host);
> + u32 gg_hw_inj[16] = GGC_CFG_DATA;
> +
> + gg_hw_inj[1] = 0x7364032;
> +
> + if (vendor_host->ggc.bh201_sdr104_selb_hw_inject)
> + gg_hw_inj[11] = vendor_host->ggc.bh201_sdr104_selb_hw_inject;
> + else
> + gg_hw_inj[11] = 0x57336200;
Make this the default value of bh201_sdr50_selb_hw_inject and you don't
need to make these conditionals throughout the code.
> +
> + if (vendor_host->ggc.bh201_sdr50_selb_hw_inject)
> + gg_hw_inj[12] = vendor_host->ggc.bh201_sdr50_selb_hw_inject;
> + else
> + gg_hw_inj[12] = 0x00725777;
> +
> + if (vendor_host->ggc.bh201_drive_strength)
> + gg_hw_inj[15] = vendor_host->ggc.bh201_drive_strength;
> +
> + bht_update_cfg(mmc_host, card, gg_hw_inj, data_len);
> +}
> +
[..]
> +static void sdhci_msm_bayhub_writeb(struct sdhci_host *host, u8 val, int reg)
> +{
> + u32 req_type = 0;
Don't initialize variables when the next operation on that variable is
an assignment.
> +
> + req_type = __sdhci_msm_bayhub_check_write(host, val, reg);
> +
> + writeb_relaxed(val, host->ioaddr + reg);
> +
> + if (req_type)
> + sdhci_msm_bayhub_check_power_status(host, req_type);
> +}
[..]
> +static int sdhci_msm_bayhub_probe(struct platform_device *pdev)
> +{
[..]
> + if (of_find_property(node, "use-bayhub-bh201", NULL)) {
Why would someone use the bayhub compatible, but not have the bh201?
Isn't this a leftover from a previous version where this was a patch
ontop of sdhci-msm?
> + sdhci_bh201_parse(msm_bayhub_host->mmc);
> + INIT_DELAYED_WORK(&host->mmc->detect, mmc_rescan_bayhub);
> + } else
> + msm_bayhub_host->ggc.bh201_used = 0;
> +
Regards,
Bjorn
