Re: [PATCH V1 1/1]mmc:sdhci-bayhub:fix Qualcomm sd host 845 SD card compatibility issue

Re: [PATCH V1 1/1]mmc:sdhci-bayhub:fix Qualcomm sd host 845 SD card compatibility issue

[Bjorn Andersson]

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@bayhubtech.com>

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

回复: [PATCH V1 1/1]mmc:sdhci-bayhub:fix Qualcomm sd host 845 SD card compatibility issue

[Chevron Li (WH)]

Hi, Bjorn,

Thanks for your generous advice.
I have answered your questions embedded.
If there are no problem, I will come out the V2 as your advice.

Thank you for your patient again!

BR, 
Chevron 

> -----邮件原件-----
> 发件人: Bjorn Andersson <bjorn.andersson@linaro.org>
> 发送时间: Saturday, August 21, 2021 2:21
> 收件人: Chevron Li (WH) <chevron.li@bayhubtech.com>
> 抄送: agross@kernel.org; linux-arm-msm@vger.kernel.org; linux-
> kernel@vger.kernel.org; Shaper Liu (WH) <shaper.liu@bayhubtech.com>;
> Xiaoguang Yu (WH) <xiaoguang.yu@bayhubtech.com>; Shirley Her (SC)
> <shirley.her@bayhubtech.com>
> 主题: Re: [PATCH V1 1/1]mmc:sdhci-bayhub:fix Qualcomm sd host 845 SD
> card compatibility issue
> 
> 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.
> 
Yes, I'm a freshman for the patch submission. Thanks for your patient review!

> 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?
> 
The hardware is GGC(chip name), and this project name is BH201. 
GGC is a SD bus signal re-timing IC that has been paired with the SDM845 sometimes.

> Please describe properly how the hardware setup looks like.
> 
Assign resource for GGC chip during Qualcomm host probe:
1. GGC structure initialization
2. GGC GPIO resource apply
3. Implement GGC chip special initialization function with "host->mmc_host_ops.init_card" callback.
4. Reload host->mmc->detect with GGC chip special initiation flow.
    GGC chip special initiation flow match with SD spec, but not match with current SD initialization flow of Linux.
    It's not necessary if we can patch the code at sd.c.
5. Reload the host->mmc_host_ops.execute_tuning with GGC chip special tuning flow.

The key points are initialized GGC chip during SD initialization and use GGC special tuning flow to re-timing SD bus signal.

> 
> 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...
> 
Yes, absolutely agree, thanks for your patient again.
The function of the patch is already verified on Chrome OS, and Google request us to submit the patch to Linux.

> > Signed-off-by: chevron.li <chevron.li@bayhubtech.com>
> 
> Is your name really chevron.li? Perhaps Chevron Li?
> 
Yes, thanks for your advice.

> > ---
> > 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:
> 
Sorry to confuse you, this patch is the first version, you can think it as V0.

> > 1. copy Qualcomm driver base from sdhci-msm.c to sdhci-bayhub.c
> 
> Why do we need to duplicate all this code.
> 
1. GGC is a SD bus signal re-timing IC, has no vendor ID,  and need to work with sd host IC together.
2. Qualcomm engineer doesn’t allow us to add code into their source code file.
3. We will remove the unused code of Qualcomm next version.
4. We also copy some codes from core folder to implement GGC chip initialization. It matches with SD host spec, but don’t match with current sd initialazation flow.
Key points as below:
  4.1 Bus width switch should at front of transfer mode switch during mode switch stage
  4.2 ACMD42 should be added between Bus width switch and transfer mode switch
  4.3 If above two points can be patched to sd.c directly, we can decrease much redundant 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.
> 
If we need to submit BH201 chip initialization function with an independent patch?
If yes, I will do it at next version.

> > 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?
> 
1. GGC is a SD bus signal re-timing IC, can work with any other standard SDHCI controller to improve SD signal SI and Timing, specially used at high-speed SD mode(ex. SDR104) and long wires of SD signals. 
2. We cooperated with Intel/Qualcomm/MTK/SPRD sd host already.

> > ---
> > ---
> [..]
> > 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.
> 
Yes, thanks for your advice.

> > + */
> > +
> > +#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"...
> 
Yes, thanks for your advice.

> > +
> > +#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".
> 
Yes, thanks for your advice.

> > +	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?
Yes, will modify at next version.

> 
> > +	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"?
> 
It means GGC register structure, use the special structure to configure GGC registers.

> > +	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.
> 
Yes, thanks for your advice.

> > +	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?
> 
GGC is the chip IC name for BH201 project, platform means the IC related maybe can use host to replace it.

> > +	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?
> 
It's no need, will modify at next version.

> > +
> > +	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.
> 
Yes, will modify at next version.

> > +	.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 }
> 
Yes, thanks for your advice.

> > +
> > +	*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"?
Yes, will use full name at next version.

> 
> > +		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.
> 
Yes, thanks for your advice.

> > +	} 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.
> 
Yes, thanks for your advice. I will check it.

> > +{
> > +	*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().
> 
Yes, thanks for your advice. I will check it.

> > +}
> > +
> > +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...
> 
Yes, thanks for your advice. I will check it.

> > +}
> > +
> > +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.
> 
Yes, thanks for your advice.

> > +
> > +	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.
> 
Yes, will remove it next version.

> > +	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.
> 
Yes, thanks for your advice.

> > +
> > +	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.
> 
Yes, thanks for your advice.

> > +
> > +	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?
> 
Yes, you are right, will modify it at next version.

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

回复: [PATCH V1 1/1]mmc:sdhci-bayhub:fix Qualcomm sd host 845 SD card compatibility issue

[Chevron Li (WH)]

Hi, Bjorn,

Excuse me.
Have you checked our feedback yet?
If there are other points need to be noticed at V2 besides your advice?

Look forward to your reply.

BR,
Chevron

-----邮件原件-----
发件人: Chevron Li (WH) 
发送时间: Monday, August 30, 2021 20:15
收件人: Bjorn Andersson <bjorn.andersson@linaro.org>
抄送: agross@kernel.org; linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org; Shaper Liu (WH) <shaper.liu@bayhubtech.com>; Xiaoguang Yu (WH) <xiaoguang.yu@bayhubtech.com>; Shirley Her (SC) <shirley.her@bayhubtech.com>
主题: 回复: [PATCH V1 1/1]mmc:sdhci-bayhub:fix Qualcomm sd host 845 SD card compatibility issue

Hi, Bjorn,

Thanks for your generous advice.
I have answered your questions embedded.
If there are no problem, I will come out the V2 as your advice.

Thank you for your patient again!

BR,
Chevron 

> -----邮件原件-----
> 发件人: Bjorn Andersson <bjorn.andersson@linaro.org>
> 发送时间: Saturday, August 21, 2021 2:21
> 收件人: Chevron Li (WH) <chevron.li@bayhubtech.com>
> 抄送: agross@kernel.org; linux-arm-msm@vger.kernel.org; linux- 
> kernel@vger.kernel.org; Shaper Liu (WH) <shaper.liu@bayhubtech.com>; 
> Xiaoguang Yu (WH) <xiaoguang.yu@bayhubtech.com>; Shirley Her (SC) 
> <shirley.her@bayhubtech.com>
> 主题: Re: [PATCH V1 1/1]mmc:sdhci-bayhub:fix Qualcomm sd host 845 SD 
> card compatibility issue
> 
> 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.
> 
Yes, I'm a freshman for the patch submission. Thanks for your patient review!

> 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?
> 
The hardware is GGC(chip name), and this project name is BH201. 
GGC is a SD bus signal re-timing IC that has been paired with the SDM845 sometimes.

> Please describe properly how the hardware setup looks like.
> 
Assign resource for GGC chip during Qualcomm host probe:
1. GGC structure initialization
2. GGC GPIO resource apply
3. Implement GGC chip special initialization function with "host->mmc_host_ops.init_card" callback.
4. Reload host->mmc->detect with GGC chip special initiation flow.
    GGC chip special initiation flow match with SD spec, but not match with current SD initialization flow of Linux.
    It's not necessary if we can patch the code at sd.c.
5. Reload the host->mmc_host_ops.execute_tuning with GGC chip special tuning flow.

The key points are initialized GGC chip during SD initialization and use GGC special tuning flow to re-timing SD bus signal.

> 
> 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...
> 
Yes, absolutely agree, thanks for your patient again.
The function of the patch is already verified on Chrome OS, and Google request us to submit the patch to Linux.

> > Signed-off-by: chevron.li <chevron.li@bayhubtech.com>
> 
> Is your name really chevron.li? Perhaps Chevron Li?
> 
Yes, thanks for your advice.

> > ---
> > 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:
> 
Sorry to confuse you, this patch is the first version, you can think it as V0.

> > 1. copy Qualcomm driver base from sdhci-msm.c to sdhci-bayhub.c
> 
> Why do we need to duplicate all this code.
> 
1. GGC is a SD bus signal re-timing IC, has no vendor ID,  and need to work with sd host IC together.
2. Qualcomm engineer doesn’t allow us to add code into their source code file.
3. We will remove the unused code of Qualcomm next version.
4. We also copy some codes from core folder to implement GGC chip initialization. It matches with SD host spec, but don’t match with current sd initialazation flow.
Key points as below:
  4.1 Bus width switch should at front of transfer mode switch during mode switch stage
  4.2 ACMD42 should be added between Bus width switch and transfer mode switch
  4.3 If above two points can be patched to sd.c directly, we can decrease much redundant 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.
> 
If we need to submit BH201 chip initialization function with an independent patch?
If yes, I will do it at next version.

> > 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?
> 
1. GGC is a SD bus signal re-timing IC, can work with any other standard SDHCI controller to improve SD signal SI and Timing, specially used at high-speed SD mode(ex. SDR104) and long wires of SD signals. 
2. We cooperated with Intel/Qualcomm/MTK/SPRD sd host already.

> > ---
> > ---
> [..]
> > 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.
> 
Yes, thanks for your advice.

> > + */
> > +
> > +#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"...
> 
Yes, thanks for your advice.

> > +
> > +#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".
> 
Yes, thanks for your advice.

> > +	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?
Yes, will modify at next version.

> 
> > +	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"?
> 
It means GGC register structure, use the special structure to configure GGC registers.

> > +	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.
> 
Yes, thanks for your advice.

> > +	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?
> 
GGC is the chip IC name for BH201 project, platform means the IC related maybe can use host to replace it.

> > +	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?
> 
It's no need, will modify at next version.

> > +
> > +	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.
> 
Yes, will modify at next version.

> > +	.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 }
> 
Yes, thanks for your advice.

> > +
> > +	*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"?
Yes, will use full name at next version.

> 
> > +		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.
> 
Yes, thanks for your advice.

> > +	} 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.
> 
Yes, thanks for your advice. I will check it.

> > +{
> > +	*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().
> 
Yes, thanks for your advice. I will check it.

> > +}
> > +
> > +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...
> 
Yes, thanks for your advice. I will check it.

> > +}
> > +
> > +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.
> 
Yes, thanks for your advice.

> > +
> > +	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.
> 
Yes, will remove it next version.

> > +	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.
> 
Yes, thanks for your advice.

> > +
> > +	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.
> 
Yes, thanks for your advice.

> > +
> > +	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?
> 
Yes, you are right, will modify it at next version.

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