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

Hi Masahiro,

On 8 July 2015 at 03:35, Masahiro Yamada yamada.masahiro@socionext.com wrote:

Hi Simon,

2015-07-07 2:32 GMT+09:00 Simon Glass sjg@chromium.org:

...

Hi Masahiro,

On 6 July 2015 at 11:24, Masahiro Yamada yamada.masahiro@socionext.com wrote:

...

Hi Simon,

2015-06-24 8:29 GMT+09:00 Simon Glass sjg@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