> This patch enhances the debug information reported
> for the mmc card by parsing the extended CSD registers
> obviously according to all the current specifications.
Does this belong kernel or in userspace? I'm not sure, and
I'm hoping that any of the old-timers here chime in on it.
Anyway I supply you with a few comments on you patch
below...
BTW, you are parsing EXT_CSD here, but then one really
should expand CSD, SCR, CID as well. One of those
contains a numerical customer id which leads me to believe
that it would require some kind of list of number->name to
make it accessible to users, something akin to the lspci
database. I did write an initial draft of such a userspace tool
at my old employers over at ST-Ericsson and tried to open
source it just before I resigned, but I don't know whether it
has made it through the legal barrier yet. I'll let you know
if I see it.
> I have no HW to test eMMC 4.5 at this moment. In any case,
> the patch supports JEDEC Standard No. 84-B45.
> No issues on JESD84-A441 and older specs raised on my side.
>
> Output from /sys/kernel/debug/mmc0/mmc0:0001/ext_csd
> looks like this:
>
> Extended CSD rev 1.5 (MMC 4.41)
> s_cmd_set: 0x01
> hpi_features: 0x00
> blops_support: 0x00
> ini_timeout_ap: 0x0a
> pwr_cl_ddr_52_360 0x00
> [snip]
>
> Reported-by: Youssef Triki <youssef.triki@xxxxxx>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@xxxxxx>
> ---
> drivers/mmc/core/debugfs.c | 266 ++++++++++++++++++++++++++++++++++++++------
> 1 files changed, 232 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
> index 0b9a4aa..3771624 100644
> --- a/drivers/mmc/core/debugfs.c
> +++ b/drivers/mmc/core/debugfs.c
> @@ -223,19 +223,13 @@ static int mmc_dbg_card_status_get(void *data, u64 *val)
> DEFINE_SIMPLE_ATTRIBUTE(mmc_dbg_card_status_fops, mmc_dbg_card_status_get,
> NULL, "%08llx\n");
>
> -#define EXT_CSD_STR_LEN 1025
> -
> -static int mmc_ext_csd_open(struct inode *inode, struct file *filp)
> +static int mmc_ext_csd_read(struct seq_file *s, void *data)
> {
> - struct mmc_card *card = inode->i_private;
> - char *buf;
> - ssize_t n = 0;
> + struct mmc_card *card = s->private;
> u8 *ext_csd;
> - int err, i;
> -
> - buf = kmalloc(EXT_CSD_STR_LEN + 1, GFP_KERNEL);
> - if (!buf)
> - return -ENOMEM;
> + u8 ext_csd_rev;
> + int err;
> + const char *str;
>
> ext_csd = kmalloc(512, GFP_KERNEL);
> if (!ext_csd) {
> @@ -249,41 +243,245 @@ static int mmc_ext_csd_open(struct inode *inode, struct file *filp)
> if (err)
> goto out_free;
>
> - for (i = 511; i >= 0; i--)
> - n += sprintf(buf + n, "%02x", ext_csd[i]);
> - n += sprintf(buf + n, "\n");
> - BUG_ON(n != EXT_CSD_STR_LEN);
> + ext_csd_rev = ext_csd[192];
>
> - filp->private_data = buf;
> - kfree(ext_csd);
> - return 0;
> + if (ext_csd_rev > 6)
> + goto out_free;
Isn't the goto in default case of the switch
below enough?
> +
> + switch (ext_csd_rev) {
> + case 6:
> + str = "4.5";
> + break;
> + case 5:
> + str = "4.41";
> + break;
> + case 3:
> + str = "4.3";
> + break;
> + case 2:
> + str = "4.2";
> + break;
> + case 1:
> + str = "4.1";
> + break;
case 0:
str = "4.0";
break;
> + default:
> + goto out_free;
> + }
> + seq_printf(s, "Extended CSD rev 1.%d (MMC %s)\n", ext_csd_rev, str);
> +
> + if (ext_csd_rev < 3)
> + goto out_free; /* No ext_csd */
> +
> + /* Parse the Extended CSD registers.
> + * Reserved bit should be read as "0" in case of spec older
> + * than A441.
> + */
> + seq_printf(s, "s_cmd_set: 0x%02x\n", ext_csd[504]);
> + seq_printf(s, "hpi_features: 0x%02x\n", ext_csd[503]);
> + seq_printf(s, "blops_support: 0x%02x\n", ext_csd[502]);
bkops
> +
> + if (ext_csd_rev >= 6) { /* eMMC 4.5 */
> + unsigned int cache_size = ext_csd[249] << 0 |
should this be declared u32?
> + ext_csd[250] << 8 |
> + ext_csd[251] << 16 |
> + ext_csd[252] << 24;
> + seq_printf(s, "max_packed_reads: 0x%02x\n", ext_csd[501]);
> + seq_printf(s, "max_packed_writes: 0x%02x\n", ext_csd[500]);
> + seq_printf(s, "data_tag_support: 0x%02x\n", ext_csd[499]);
> + seq_printf(s, "tag_unit_size: 0x%02x\n", ext_csd[498]);
> + seq_printf(s, "tag_res_size: 0x%02x\n", ext_csd[497]);
> + seq_printf(s, "context_capability: 0x%02x\n", ext_csd[496]);
capabilities
> + seq_printf(s, "large_unit_size_m1: 0x%02x\n", ext_csd[495]);
> + seq_printf(s, "ext_support: 0x%02x\n", ext_csd[494]);
> + if (cache_size)
> + seq_printf(s, "cache_size[]: %d KiB\n", cache_size);
why the brackets?
> + else
> + seq_printf(s, "No cache existing\n");
I think "cache_size: 0 KiB\n" be more appropriate. The fact that 0 should
be interpreted as "no cache exists" should be done by the one/sw reading
this file. Also just printing the cache_size as given is simpler. Also I think
you could treat cache_size the same as sec_count below and not declare
a local variable.
> +
> + seq_printf(s, "generic_cmd6_time: 0x%02x\n", ext_csd[248]);
> + seq_printf(s, "power_off_long_time: 0x%02x\n", ext_csd[247]);
> + }
> +
> + /* A441: Reserved [501:247]
> + A43: reserved [246:229] */
> + if (ext_csd_rev >= 5) {
> + seq_printf(s, "ini_timeout_ap: 0x%02x\n", ext_csd[241]);
> + /* A441: reserved [240] */
> + seq_printf(s, "pwr_cl_ddr_52_360 0x%02x\n", ext_csd[239]);
You lost the colon after the property name starting from this line.
I sugges you keep it. (I had to read the spec to know that these
were actually called properities, who knew?)
> + seq_printf(s, "pwr_cl_ddr_52_195 0x%02x\n", ext_csd[238]);
> +
> + /* A441: reserved [237-236] */
> +
> + if (ext_csd_rev >= 6) {
> + seq_printf(s, "pwr_cl_200_360 0x%02x\n", ext_csd[237]);
> + seq_printf(s, "pwr_cl_200_195 0x%02x\n", ext_csd[236]);
> + }
> +
> + seq_printf(s, "min_perf_ddr_w_8_52 0x%02x\n", ext_csd[235]);
> + seq_printf(s, "min_perf_ddr_r_8_52 0x%02x\n", ext_csd[234]);
> + /* A441: reserved [233] */
> + seq_printf(s, "trim_mult 0x%02x\n", ext_csd[232]);
> + seq_printf(s, "sec_feature_support 0x%02x\n", ext_csd[231]);
> + }
> + if (ext_csd_rev == 5) { /* Obsolete in 4.5 */
> + seq_printf(s, "sec_erase_mult 0x%02x\n", ext_csd[230]);
> + seq_printf(s, "sec_trim_mult 0x%02x\n", ext_csd[229]);
> + }
> + seq_printf(s, "boot_info 0x%02x\n", ext_csd[228]);
> + /* A441/A43: reserved [227] */
> + seq_printf(s, "boot_size_multi 0x%02x\n", ext_csd[226]);
> + seq_printf(s, "acc_size 0x%02x\n", ext_csd[225]);
> + seq_printf(s, "hc_erase_grp_size 0x%02x\n", ext_csd[224]);
> + seq_printf(s, "erase_timeout_mult 0x%02x\n", ext_csd[223]);
> + seq_printf(s, "rel_wr_sec_c 0x%02x\n", ext_csd[222]);
> + seq_printf(s, "hc_wp_grp_size 0x%02x\n", ext_csd[221]);
> + seq_printf(s, "s_c_vcc 0x%02x\n", ext_csd[220]);
> + seq_printf(s, "s_c_vccq 0x%02x\n", ext_csd[219]);
> + /* A441/A43: reserved [218] */
> + seq_printf(s, "s_a_timeout 0x%02x\n", ext_csd[217]);
> + /* A441/A43: reserved [216] */
> + seq_printf(s, "sec_count 0x%02x\n", (ext_csd[215] << 24) |
0x%08x or %d would be better I believe.
> + (ext_csd[214] << 16) | (ext_csd[213] << 8) |
> + ext_csd[212]);
> + /* A441/A43: reserved [211] */
> + seq_printf(s, "min_perf_w_8_52 0x%02x\n", ext_csd[210]);
> + seq_printf(s, "min_perf_r_8_52 0x%02x\n", ext_csd[209]);
> + seq_printf(s, "min_perf_w_8_26_4_52 0x%02x\n", ext_csd[208]);
> + seq_printf(s, "min_perf_r_8_26_4_52 0x%02x\n", ext_csd[207]);
> + seq_printf(s, "min_perf_w_4_26 0x%02x\n", ext_csd[206]);
> + seq_printf(s, "min_perf_r_4_26 0x%02x\n", ext_csd[205]);
> + /* A441/A43: reserved [204] */
> + seq_printf(s, "pwr_cl_26_360 0x%02x\n", ext_csd[203]);
> + seq_printf(s, "pwr_cl_52_360 0x%02x\n", ext_csd[202]);
> + seq_printf(s, "pwr_cl_26_195 0x%02x\n", ext_csd[201]);
> + seq_printf(s, "pwr_cl_52_195 0x%02x\n", ext_csd[200]);
> +
> + /* A43: reserved [199:198] */
> + if (ext_csd_rev >= 5) {
> + seq_printf(s, "partition_switch_time 0x%02x\n", ext_csd[199]);
> + seq_printf(s, "out_of_interrupt_time 0x%02x\n", ext_csd[198]);
> + }
> +
> + /* A441/A43: reserved [197] [195] [193] [190] [188]
> + * [186] [184] [182] [180] [176] */
> +
> + if (ext_csd_rev >= 6)
> + seq_printf(s, "driver_strength 0x%02x\n", ext_csd[197]);
> +
> + seq_printf(s, "card_type 0x%02x\n", ext_csd[196]);
> + seq_printf(s, "csd_structure 0x%02x\n", ext_csd[194]);
> + seq_printf(s, "ext_csd_rev 0x%02x\n", ext_csd[192]);
> + seq_printf(s, "cmd_set 0x%02x\n", ext_csd[191]);
> + seq_printf(s, "cmd_set_rev 0x%02x\n", ext_csd[189]);
> + seq_printf(s, "power_class 0x%02x\n", ext_csd[187]);
> + seq_printf(s, "hs_timing 0x%02x\n", ext_csd[185]);
> + seq_printf(s, "bus_width 0x%02x\n", ext_csd[183]);
hm.. spec 4.5 defines this as not readable. I wonder it it makes
sense to print it? I guess that any card would transfer 0 or some
other constant value in this property...
> + seq_printf(s, "erased_mem_cont 0x%02x\n", ext_csd[181]);
> + seq_printf(s, "partition_config 0x%02x\n", ext_csd[179]);
> + seq_printf(s, "boot_config_prot 0x%02x\n", ext_csd[178]);
> + seq_printf(s, "boot_bus_width 0x%02x\n", ext_csd[177]);
boot_bus_conditions according to spec 4.5...
> + seq_printf(s, "erase_group_def 0x%02x\n", ext_csd[175]);
> +
> + /* A43: reserved [174:0] / A441: reserved [174] */
maybe, but spec 4.5 seems to define (though I admit that the line
in the table is curiously grayed out, maybe it's a typo in the spec
since 174 used to be reserved in 4.41...):
seq_printf(s, "boot_wp_status 0x%02x\n", ext_csd[174]);
> + if (ext_csd_rev >= 5) {
> + seq_printf(s, "boot_wp 0x%02x\n", ext_csd[173]);
> + /* A441: reserved [172] */
> + seq_printf(s, "user_wp 0x%02x\n", ext_csd[171]);
> + /* A441: reserved [170] */
> + seq_printf(s, "fw_config 0x%02x\n", ext_csd[169]);
> + seq_printf(s, "rpmb_size_mult 0x%02x\n", ext_csd[168]);
> + seq_printf(s, "wr_rel_set 0x%02x\n", ext_csd[167]);
> + seq_printf(s, "wr_rel_param 0x%02x\n", ext_csd[166]);
> + /* A441: reserved [165] */
spec 4.5 seems to define sanitize_start here, but as for bus_width
above, it is not really readable. I don't know whether you should
include it or not. it may just be confusing. but either way the comment
should be updated to reflect that 4.5 does indeed have a property here.
seq_printf(s, "sanitize_start: 0x%02x\n", ext_csd[165]);
> + seq_printf(s, "bkops_start 0x%02x\n", ext_csd[164]);
this is only writeable, same as for bus_width...
> + seq_printf(s, "bkops_en 0x%02x\n", ext_csd[163]);
> + seq_printf(s, "rst_n_function 0x%02x\n", ext_csd[162]);
> + seq_printf(s, "hpi_mgmt 0x%02x\n", ext_csd[161]);
> + seq_printf(s, "partitioning_support 0x%02x\n", ext_csd[160]);
> + seq_printf(s, "max_enh_size_mult[2] 0x%02x\n", ext_csd[159]);
> + seq_printf(s, "max_enh_size_mult[1] 0x%02x\n", ext_csd[158]);
> + seq_printf(s, "max_enh_size_mult[0] 0x%02x\n", ext_csd[157]);
seq_printf(s, "max_enh_size_mult: 0x%06x\n", (ext_csd[159] << 16) |
(ext_csd[158] << 8) | ext_csd[157]);
this seems more to fit with your handling of sec_count above.
> + seq_printf(s, "partitions_attribute 0x%02x\n", ext_csd[156]);
> + seq_printf(s, "partition_setting_completed 0x%02x\n",
> + ext_csd[155]);
> + seq_printf(s, "gp_size_mult_4[2] 0x%02x\n", ext_csd[154]);
> + seq_printf(s, "gp_size_mult_4[1] 0x%02x\n", ext_csd[153]);
> + seq_printf(s, "gp_size_mult_4[0] 0x%02x\n", ext_csd[152]);
> + seq_printf(s, "gp_size_mult_3[2] 0x%02x\n", ext_csd[151]);
> + seq_printf(s, "gp_size_mult_3[1] 0x%02x\n", ext_csd[150]);
> + seq_printf(s, "gp_size_mult_3[0] 0x%02x\n", ext_csd[149]);
> + seq_printf(s, "gp_size_mult_2[2] 0x%02x\n", ext_csd[148]);
> + seq_printf(s, "gp_size_mult_2[1] 0x%02x\n", ext_csd[147]);
> + seq_printf(s, "gp_size_mult_2[0] 0x%02x\n", ext_csd[146]);
> + seq_printf(s, "gp_size_mult_1[2] 0x%02x\n", ext_csd[145]);
> + seq_printf(s, "gp_size_mult_1[1] 0x%02x\n", ext_csd[144]);
> + seq_printf(s, "gp_size_mult_1[0] 0x%02x\n", ext_csd[143]);
> + seq_printf(s, "enh_size_mult[2] 0x%02x\n", ext_csd[142]);
> + seq_printf(s, "enh_size_mult[1] 0x%02x\n", ext_csd[141]);
> + seq_printf(s, "enh_size_mult[0] 0x%02x\n", ext_csd[140]);
> + seq_printf(s, "enh_start_addr[3] 0x%02x\n", ext_csd[139]);
> + seq_printf(s, "enh_start_addr[2] 0x%02x\n", ext_csd[138]);
> + seq_printf(s, "enh_start_addr[1] 0x%02x\n", ext_csd[137]);
> + seq_printf(s, "enh_start_addr[0] 0x%02x\n", ext_csd[136]);
seq_printf(s, "gp_size_mult_4: 0x%06x\n", (ext_csd[154] << 16) |
(ext_csd[153] << 8) | ext_csd[152]);
seq_printf(s, "gp_size_mult_3: 0x%06x\n", (ext_csd[151] << 16) |
(ext_csd[150] << 8) | ext_csd[149]);
seq_printf(s, "gp_size_mult_2: 0x%06x\n", (ext_csd[148] << 16) |
(ext_csd[147] << 8) | ext_csd[146]);
seq_printf(s, "gp_size_mult_1: 0x%06x\n", (ext_csd[145] << 16) |
(ext_csd[144] << 8) | ext_csd[143]);
seq_printf(s, "enh_size_mult: 0x%06x\n", (ext_csd[142] << 16) |
(ext_csd[141] << 8) | ext_csd[140]);
seq_printf(s, "enh_start_addr: 0x%06x\n", (ext_csd[139] << 16) |
(ext_csd[138] << 8) | ext_csd[137]);
same comment as for max_enh_size_mult...
> + /* A441: reserved [135] */
> + seq_printf(s, "sec_bad_blk_mgmnt 0x%02x\n", ext_csd[134]);
> + /* A441: reserved [133:0] */
> + }
> + /* B45 */
> + if (ext_csd_rev >= 6) {
> + int j;
> + seq_printf(s, "tcase_support 0x%02x\n", ext_csd[132]);
not readable according to spec 4.5...
also spec 4.5 defines which you may want to include:
seq_printf(s, "periodic_wakeup: 0x%02x\n", ext_csd[131]);
> + seq_printf(s, "program_cid_csd_ddr_support 0x%02x\n",
> + ext_csd[130]);
> +
> + seq_printf(s, "vendor_specific_field:\n");
> + for (j = 127; j >= 64; j--)
> + seq_printf(s, "\t[%d] 0x%02x\n", j, ext_csd[j]);
printing an array here make sense (since it _is_ really a byte array),
but what about replacing the last three lines with:
for (j = 127; j >= 64; j--)
seq_printf(s, "vendor_specific_field[%d]: 0x%02x\n", j, ext_csd[j]);
that way all lines in the sysfs-file follow the same format without indentation.
> +
> + seq_printf(s, "native_sector_size 0x%02x\n", ext_csd[63]);
> + seq_printf(s, "use_native_sector 0x%02x\n", ext_csd[62]);
> + seq_printf(s, "data_sector_size 0x%02x\n", ext_csd[61]);
> + seq_printf(s, "ini_timeout_emu 0x%02x\n", ext_csd[60]);
> + seq_printf(s, "class_6_ctrl 0x%02x\n", ext_csd[59]);
> + seq_printf(s, "dyncap_needed 0x%02x\n", ext_csd[58]);
> + seq_printf(s, "exception_events_ctrl[1] 0x%02x\n", ext_csd[57]);
> + seq_printf(s, "exception_events_ctrl[0] 0x%02x\n", ext_csd[56]);
> + seq_printf(s, "exception_events_status[1] 0x%02x\n",
> + ext_csd[55]);
> + seq_printf(s, "exception_events_status[0] 0x%02x\n",
> + ext_csd[54]);
> + seq_printf(s, "ext_partition_attribute[1] 0x%02x\n",
> + ext_csd[53]);
> + seq_printf(s, "ext_partition_attribute[0] 0x%02x\n",
> + ext_csd[52]);
seq_printf(s, "exception_events_ctrl: 0x%04x\n", (ext_csd[57] << 8) |
ext_csd[56]);
seq_printf(s, "exception_events_status: 0x%04x\n", (ext_csd[55] << 8)
| ext_csd[54]);
seq_printf(s, "ext_partitions_attribute: 0x%04x\n", (ext_csd[53] << 8)
| ext_csd[52]);
notice the s in partitions...
> +
> + seq_printf(s, "context_conf:\n");
> + for (j = 51; j >= 37; j--)
> + seq_printf(s, "\t[%d] 0x%02x\n", j, ext_csd[j]);
same as for vendor_specific_field above, I would suggest doing
for (j = 51; j >= 37; j--)
seq_printf(s, "context_conf[%d]: 0x%02x\n", j, ext_csd[j]);
> +
> + seq_printf(s, "packed_command_status 0x%02x\n", ext_csd[36]);
> + seq_printf(s, "packed_failure_index 0x%02x\n", ext_csd[35]);
> + seq_printf(s, "power_off_notification 0x%02x\n", ext_csd[34]);
> + seq_printf(s, "cache_ctrl 0x%02x\n", ext_csd[33]);
> + seq_printf(s, "flush_cache 0x%02x\n", ext_csd[32]);
this property is not readable.
> + /*Reserved [31:0] */
> + }
>
> out_free:
> - kfree(buf);
> kfree(ext_csd);
> return err;
> }
>
> -static ssize_t mmc_ext_csd_read(struct file *filp, char __user *ubuf,
> - size_t cnt, loff_t *ppos)
> +static int mmc_ext_csd_open(struct inode *inode, struct file *file)
> {
> - char *buf = filp->private_data;
> -
> - return simple_read_from_buffer(ubuf, cnt, ppos,
> - buf, EXT_CSD_STR_LEN);
> -}
> -
> -static int mmc_ext_csd_release(struct inode *inode, struct file *file)
> -{
> - kfree(file->private_data);
> - return 0;
> + return single_open(file, mmc_ext_csd_read, inode->i_private);
> }
>
> static const struct file_operations mmc_dbg_ext_csd_fops = {
> .open = mmc_ext_csd_open,
> - .read = mmc_ext_csd_read,
> - .release = mmc_ext_csd_release,
> - .llseek = default_llseek,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release,
> };
>
> void mmc_add_card_debugfs(struct mmc_card *card)
> --
> 1.7.4.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
/ Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
