On Fri, Nov 1, 2013 at 1:21 AM, Seungwon Jeon <tgih.jun@xxxxxxxxxxx> wrote:
> On Fri, November 01, 2013, zhangfei gao wrote:
>> Dear Seungwon
>>
>> Thanks for giving suggestion.
>>
>> On Thu, Oct 31, 2013 at 11:24 PM, Seungwon Jeon <tgih.jun@xxxxxxxxxxx> wrote:
>> > Hi Zhangfei,
>>
>> >> +static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>> >> +{
>> >> + struct dw_mci_k3_priv_data *priv = host->priv;
>> >> +
>> >> + if (priv->type == DW_MCI_TYPE_HI4511)
>> > There are several checking controller type.
>> > But now DW_MCI_TYPE_HI4511 is just introduced alone.
>> > It seems like unnecessary.
>>
>> Yes, currently only K3V2 is added, and k3v3 code will be added soon.
>> Do you think type is more suitable to add later?
> Yes, it would be better.
OK, got it.
>
>>
>> >> +static int dw_mci_k3_parse_dt(struct dw_mci *host)
>> >> +{
>> >> + struct dw_mci_k3_priv_data *priv = host->priv;
>> >> + struct device_node *np = host->dev->of_node;
>> >> + u32 data[3];
>> >> + int ret;
>> >> +
>> >> + if (priv->type == DW_MCI_TYPE_HI4511) {
>> > Didn't you see Kernel panic here?
>> > host->priv is not allocated yet, it's a invalid pointer dereference.
>> > dw_mci_k3_parse_dt() is called prior to dw_mci_k3_priv_init().
>> > You can check dw_mci_probe() sequence.
>>
>> It is no problem here
>> dw_mci_pltfm_register -> drv_data->init(host) -> dw_mci_probe -> dw_mci_parse_dt
> I guess your base is not latest.
> Can you check cjb's tree?
Yes, thanks for the info.
It is changed in 3.12-rc2
While what we use is 3.12-rc1, will rebase.
>
>>
>> >
>> >> + ret = of_property_read_u32_array(np,
>> >> + "clken-reg", data, 2);
>> >> + if (!ret) {
>> >> + priv->clken_reg = data[0];
>> >> + priv->clken_bit = data[1];
>> >> + }
>> >> +
>> >> + ret = of_property_read_u32_array(np,
>> >> + "drv-sel-reg", data, 3);
>> >> + if (!ret) {
>> >> + priv->drv_reg = data[0];
>> >> + priv->drv_off = data[1];
>> >> + priv->drv_bits = data[2];
>> >> + }
>> >> +
>> >> + ret = of_property_read_u32_array(np,
>> >> + "sam-sel-reg", data, 3);
>> >> + if (!ret) {
>> >> + priv->sam_reg = data[0];
>> >> + priv->sam_off = data[1];
>> >> + priv->sam_bits = data[2];
>> >> + }
>> >> +
>> >> + ret = of_property_read_u32_array(np,
>> >> + "div-reg", data, 3);
>> >> + if (!ret) {
>> >> + priv->div_reg = data[0];
>> >> + priv->div_off = data[1];
>> >> + priv->div_bits = data[2];
>> >> + }
>> > Should these register information be got from dt?
>> > It could be define in source code instead.
>>
>> There are many register from pctrl node instead of mmc controller.
>> If set in code, we may read id and then switch id, set register, start
>> bit, bits number,
>> which is no rules for different controller, using defiine is not helpful.
>> for example:
>> switch (idx) {
>> case 0:
>> clken_reg = 0x1F8;
>> clken_bit = 0;
>> drv_sel_reg = 0x1F8;
>> drv_sel_bit = 4;
>> sam_sel_reg = 0x1F8;
>> sam_sel_bit = 8;
>> div_reg = 0x1F8;
>> div_bit = 1;
>> break;
>> case 1:
>> clken_reg = 0x1F8;
>> clken_bit = 12;
>> drv_sel_reg = 0x1F8;
>> drv_sel_bit = 16;
>> sam_sel_reg = 0x1F8;
>> sam_sel_bit = 20;
>> div_reg = 0x1F8;
>> div_bit = 13;
>> break;
>> case 2:
>> clken_reg = 0x1F8;
>> clken_bit = 24;
>> drv_sel_reg = 0x1F8;
>> drv_sel_bit = 28;
>> sam_sel_reg = 0x1FC;
>> sam_sel_bit = 0;
>> div_reg = 0x1F8;
>> div_bit = 25;
>> break;
>>
>> So define register offset, start bit and bits number in dts make it simplier.
>> Is this acceptable?
> It's up to you. I think there will be better ways.
> For example, some table for variable register can be used for each controller type.
> Additionally, if you intend to use dt, error handling is needed.
> dw_mci_k3_parse_dt always returns success although of_property_read_u32_array is failed.
The reason is they are optional property.
SD would fail if not setting the property, while emmc does not matter.
So set for optional, dw_mci_k3_set_timing will check whether they exists.
Thanks
--
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
