| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 15 Oct 2015 11:17:58 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Chaotian Jing <chaotian.jing@...iatek.com>
Cc: Rob Herring <robh+dt@...nel.org>, Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
Matthias Brugger <matthias.bgg@...il.com>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will.deacon@....com>,
Hans de Goede <hdegoede@...hat.com>,
Lars-Peter Clausen <lars@...afoo.de>,
Javier Martinez Canillas <javier.martinez@...labora.co.uk>,
Sascha Hauer <s.hauer@...gutronix.de>,
Howard Chen <ibanezchen@...il.com>,
Daniel Kurtz <djkurtz@...omium.org>,
Adrian Hunter <adrian.hunter@...el.com>,
Kristina Martsenko <kristina.martsenko@...il.com>,
Sergei Shtylyov <sergei.shtylyov@...entembedded.com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
linux-mediatek@...ts.infradead.org,
linux-mmc <linux-mmc@...r.kernel.org>,
srv_heupstream <srv_heupstream@...iatek.com>
Subject: Re: [PATCH 3/4] mmc: mediatek: Add tune support
[...]
>> >
>> > struct clk *src_clk; /* msdc source clock */
>> > + struct clk *src_clk_parent; /* src_clk's parent */
>> > + struct clk *hs400_src; /* 400Mhz source clock */
>>
>> Hmm, so you need to control the upper level clocks. Can you elaborate
>> on why this is needed?
>>
> hs400 is DDR200, in our host design, if the mode is DDR(HS400), if want
> to get 200Mhz mmc bus clock frequency, the minimum source clock is
> double of the mmc bus clock, So, we need it for HS400 mode with 200Mhz
> bus clock.
Thanks for clarifying.
[...]
> flags = readl(host->base + MSDC_INTEN);
>> > sdr_clr_bits(host->base + MSDC_INTEN, flags);
>> > - if (ddr) { /* may need to modify later */
>> > - mode = 0x2; /* ddr mode and use divisor */
>> > + sdr_clr_bits(host->base + MSDC_CFG, MSDC_CFG_HS400_CK_MODE);
>> > + if (timing == MMC_TIMING_UHS_DDR50 ||
>> > + timing == MMC_TIMING_MMC_DDR52 ||
>>
>> So, no support for HS200?
>>
> HS200 is the same with other SDR modes, so it will be handled at else..
Okay, nice!
So, your the driver currently supports HS200, but without need for tuning!?
[...]
>> > +static struct msdc_delay_phase get_best_delay(u32 delay)
>> > +{
>> > + int start = 0, len = 0;
>> > + int start_final = 0, len_final = 0;
>> > + u8 final_phase = 0xff;
>> > + struct msdc_delay_phase delay_phase;
>> > +
>> > + if (delay == 0) {
>> > + pr_err("phase error: [map:%x]\n", delay);
>>
>> Please use dev_err|warn|dbg|info instead.
>>
> As you know, this function is just only parse the argument "u32 delay",
> it do not bind with any device.
You may just add a msdc_host * as a parameter to the function, that
would solve this.
[...]
>> > +static int msdc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error)
>>
>> I think you can remove this function and use mmc_send_tuning() instead.
> Hmm, I also noticed that there was a mmc_send_tuning, but, I need to get
> the cmd_error when tune cmd response, in this case, do not care the data
> error.
Well, if you need to extend the mmc_send_tuning() API to suite your
needs, let's do that instead of duplicating code.
>>
>> > +{
>> > + struct mmc_request mrq = {NULL};
>> > + struct mmc_command cmd = {0};
>> > + struct mmc_data data = {0};
>> > + struct scatterlist sg;
>> > + struct mmc_ios *ios = &host->ios;
>> > + int size, err = 0;
>> > + u8 *data_buf;
>> > +
>> > + if (ios->bus_width == MMC_BUS_WIDTH_8)
>> > + size = 128;
>> > + else if (ios->bus_width == MMC_BUS_WIDTH_4)
>> > + size = 64;
>> > + else
>> > + return -EINVAL;
>> > +
>> > + data_buf = kzalloc(size, GFP_KERNEL);
>> > + if (!data_buf)
>> > + return -ENOMEM;
>> > +
>> > + mrq.cmd = &cmd;
>> > + mrq.data = &data;
>> > +
>> > + cmd.opcode = opcode;
>> > + cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
>> > +
>> > + data.blksz = size;
>> > + data.blocks = 1;
>> > + data.flags = MMC_DATA_READ;
>> > +
>> > + /*
>> > + * According to the tuning specs, Tuning process
>> > + * is normally shorter 40 executions of CMD19,
>> > + * and timeout value should be shorter than 150 ms
>> > + */
>> > + data.timeout_ns = 150 * NSEC_PER_MSEC;
>> > +
>> > + data.sg = &sg;
>> > + data.sg_len = 1;
>> > + sg_init_one(&sg, data_buf, size);
>> > +
>> > + mmc_wait_for_req(host, &mrq);
>> > +
>> > + if (cmd_error)
>> > + *cmd_error = cmd.error;
>> > +
>> > + if (cmd.error) {
>> > + err = cmd.error;
>> > + goto out;
>> > + }
>> > +
>> > + if (data.error) {
>> > + err = data.error;
>> > + goto out;
>> > + }
>> > +
>> > +out:
>> > + kfree(data_buf);
>> > + return err;
>> > +}
>> > +
[...]
>> > + host->src_clk_parent = clk_get_parent(host->src_clk);
>>
>> Don't you need to check the return value here?
>>
> will check it.
>> > + host->hs400_src = devm_clk_get(&pdev->dev, "400mhz");
>>
>> That's a really weird conid for a clock. If it's not too late to
>> change, please do that!
>>
>> > + if (IS_ERR(host->hs400_src)) {
>> > + dev_dbg(&pdev->dev, "Cannot find 400mhz at dts!\n");
>> > + } else if (clk_set_parent(host->src_clk_parent, host->hs400_src) < 0) {
>> > + dev_err(host->dev, "Failed to set 400mhz source clock!\n");
>> > + ret = -EINVAL;
>>
>> I think it seems more apropriate to use the return value from
>> clk_set_parent() instead of inventing your own return value.
>>
> OK.
>> > + goto host_free;
>> > + }
>>
>> It seems like you don't need to store the src_clk_parent and the
>> hs400_src in the host struct, as you are only using it during
>> ->probe().
> OK,will remove the member src_clk.
According to your earlier clarification about the clock source and
clock rate. I think a more proper solution would be to use the
clk_set_min_rate() or clk_set_rate_range() API, instead of dealing
with re-parenting of the clock as above.
FYI, a clock provider may implement the ->determine_rate() ops to deal
with re-parenting to find the requested clock rate.
[...]
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists