[PATCH 2/2] mmc: dw_mmc: add dw_mmc-k3 for k3 platform

[zhangfei gao]

On Fri, Nov 1, 2013 at 1:21 AM, Seungwon Jeon <tgih.jun at samsung.com> 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 at samsung.com> 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