[U-Boot] [PATCH v3 17/25] rockchip: rk3288: Add pinctrl driver

[Simon Glass]

Hi Masahiro,

On 8 July 2015 at 03:35, Masahiro Yamada <yamada.masahiro at socionext.com> wrote:
> Hi Simon,
>
>
> 2015-07-07 2:32 GMT+09:00 Simon Glass <sjg at chromium.org>:
>> Hi Masahiro,
>>
>> On 6 July 2015 at 11:24, Masahiro Yamada <yamada.masahiro at socionext.com> wrote:
>>>
>>> Hi Simon,
>>>
>>>
>>> 2015-06-24 8:29 GMT+09:00 Simon Glass <sjg at chromium.org>:
>>>
>>> > +
>>> > +static int rk3288_pinctrl_get_periph_id(struct udevice *dev,
>>> > +                                       struct udevice *periph)
>>> > +{
>>> > +       u32 cell[3];
>>> > +       int ret;
>>> > +
>>> > +       ret = fdtdec_get_int_array(gd->fdt_blob, periph->of_offset,
>>> > +                                  "interrupts", cell, ARRAY_SIZE(cell));
>>> > +       if (ret < 0)
>>> > +               return -EINVAL;
>>> > +
>>> > +       switch (cell[1]) {
>>> > +       case 44:
>>> > +               return PERIPH_ID_SPI0;
>>> > +       case 45:
>>> > +               return PERIPH_ID_SPI1;
>>> > +       case 46:
>>> > +               return PERIPH_ID_SPI2;
>>> > +       case 60:
>>> > +               return PERIPH_ID_I2C0;
>>> > +       case 62: /* Note strange order */
>>> > +               return PERIPH_ID_I2C1;
>>> > +       case 61:
>>> > +               return PERIPH_ID_I2C2;
>>> > +       case 63:
>>> > +               return PERIPH_ID_I2C3;
>>> > +       case 64:
>>> > +               return PERIPH_ID_I2C4;
>>> > +       case 65:
>>> > +               return PERIPH_ID_I2C5;
>>> > +       }
>>> > +
>>> > +       return -ENOENT;
>>> > +}
>>>
>>> Weird.
>>>
>>> Do you parse the "interrupts" property
>>> to identify the peripheral?
>>>
>>> This is what I really do not like to do.
>>
>> Yes. It's similar to a clock number. It doesn't really matter what we
>> use. The interrupt number is a fixed part of the binding and will
>> never change.
>
>
> I guess it is abuse of "interrupts" property.
>
> Moreover, it can not cover cases where a single signal can be routed to
> several pins.
>

This is code that is specific to RK3288. The numbering scheme is
completely arbitrary and is local to that one function. If we wanted
to use a different scheme (now or later) it would just involve
changing that one function.

Other SoCs may have other rules.

>
> For example, say we want to set pinmux for the I2C channel 0 (SDA0 and SCL0).
>
> On some vendors' SoCs, it is possible to connect SDA0 to either pin10
> or pin20 for example.
> Likewise, SCL0 can be connected to pin11 or pin21.
> (Yes, it is possible on my SoCs)
>
>
> The interrupt ID can identify it is I2C0, and that's it.
>
> Some boards expect the signals come out of pin10, 11,
> and other boards pin20, 21.
>
> The interrupt ID cannot distinguish those cases; the "pinctrl-*"
> property can do it.

The interrupt ID is just a way of determining the peripheral. The
actual pinmux setting for that peripheral is handled by the flags
parameter.

>
> This is mentioned in Documentation/pinctrl.txt and
> that is why a single function can have multiple pin-groups in Linux's
> pinctrl drivers.
>
> I think this design cannot meet some vendors' demand.
> At least, I am unhappy with it.
>
> Could you hold your pinctrl patches and give me time to discuss a bit
> more, please?
>
> I understand how pinctrl drivers work in Linux, so please give me some time
> to consider if better design might be possible.

It is clear that we are heading towards a more general pinmux scheme,
and I can imagine us implementing the pinctrl for some SoCs.

The current proposal is a step towards that but is considerably
simpler. I am familiar with pinctrl but decided against a full
implementation for U-Boot as I feel it is overkill and would not
attract substantial SoC support.

There is no hurry with applying these patches. Let me know what you figure out.

Regards,
Simon

>
>
>
> --
> Best Regards
> Masahiro Yamada