Re: Boundary between pinctrl and peripheral settings

["David.Wu" <david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>]

Hi Heiko,

在 2017/5/23 0:29, Heiko Stuebner 写道:
> Hi David,
> 
> [I've dropped Linus Walleij, so we don't spam him with our discussions :-) .
>   I've kept the list though, so people can read up on it if necessary ]

> 
> Am Montag, 22. Mai 2017, 21:58:00 CEST schrieb David.Wu:
>> 在 2017/5/19 17:13, Heiko Stuebner 写道:
>>>> If a pin-routing contains a few pins as gmac, need all the pins (pin,
>>>> pinfunc) to be configured correctly from DT, then the corresponding GRF
>>>> setting is configured.
>>>> If configured grf-setting per pin, assuming that one of the pin's
>>>> pinfunc is wrong, it may cause pin-routing not to be effective or wrong.
>>>
>>> I still think it should be enough to watch for one pin of a specifc group
>>> to be set to its special pinmux function. If the pinmux setting is wrong
>>> for that pin the ip block won't work correctly anyway (independent of
>>> its inner-soc routing).
>>>
>>> Our pinctrl groups also are defined on a global per-soc level
>>> (rk3399.dtsi etc) and thus if they are really wrong, a lot of people will
>>> complain anyway ;-) .
>>>
>>> So for example just take
>>> uart2dbga_sin, uart2dbgb_sin, uart2dbgc_sin on rk3399
>>> as pins to watch, some thing like
>>>
>>> struct rk3399_pinrouting[] = {
>>> 	{
>>> 		/* uart2dbga */
>>> 		.bank = 4,
>>> 		.pin = 8,
>>> 		.func = 2,
>>> 		.route_offset = 0x0e21c,
>>> 		.route_val = (3 << (10+16)),
>>> 	}, {
>>> 		/* uart2dbgb */
>>> 		.bank = 4,
>>> 		.pin = 16,
>>> 		.func = 2,
>>> 		.route_offset = 0x0e21c,
>>> 		.route_val = (3 << (10+16) | 1 << 10),
>>> 	},
>>> ...
>>> };
>>>
>>> That way you can just hook it into the rockchip_set_mux function similar
>>> to the recalc stuff, without to much trouble.
>>>
>> The gmac_m1_sel is different, it need to configure both bit[10] and
>> bit[2] with value:1, used after optimization. The gmac_m1 after
>> optimization need to configure all pins iomux of gmac_m1 and tx pins
>> iomux of gmac_m0. The tx pins iomux of gmac_m0 is also required,so it is
>> conflict of gmac_m1 after optimization. Besides, we don't need to use
>> m1, because m1 's signal is bad, don't think about it, but
>> gmac_m1 after optimization is obligatory.
> 
> I'm not sure I understand the paragraph above. Especially what is the
> "optimization" thing.
> 
> But if I understand you correctly, that still leaves us with only 2 settings.
> gmac-m0 and gmac-m1-optimized (or whatever we want to call it).
> 
> For gmac-m0 just hook it onto mac_rxd0, which is only used on m0;
> just use any of the gmac-m1 pins for that gmac-m1-optimized state;
> and as yo wrote just ignore the gmac-m1 state, as it is bad anyway.

Yes, Just use the gmac-m0 and gmac-m1-optimized two states.
Another aspect is I think per pin needs to traverse, and array element 
too much, want to improve route match rate.

How do you feel about the two points below:
1. add IOMUX_ROUTE type,that only per 8 pins need to traverse.
static struct rockchip_pin_bank rk3228_pin_banks[] = {
	PIN_BANK_IOMUX_FLAGS(0, 32, "gpio0", IOMUX_ROUTE, IOMUX_ROUTE, 0, 
IOMUX_ROUTE),
	PIN_BANK_IOMUX_FLAGS(1, 32, "gpio1", IOMUX_ROUTE, IOMUX_ROUTE, 
IOMUX_ROUTE, 0),
	PIN_BANK_IOMUX_FLAGS(2, 32, "gpio2", IOMUX_ROUTE, 0, 0, 0),
	PIN_BANK_IOMUX_FLAGS(3, 32, "gpio3", IOMUX_ROUTE, IOMUX_ROUTE, 
IOMUX_ROUTE, IOMUX_ROUTE),
};

2. add route mark for per bank, that only if current BIT(pin) & 
route_mask is ture, need to traverse.
static struct rockchip_pin_bank rk3228_pin_banks[] = {
	PIN_BANK_ROUTE_MASK(0, 32, "gpio0", 0x5c002002),
	......
	......
	......
};

static const struct rockchip_mux_route_data  rk3228_mux_route_data[] = {
	{
		/* pwm0-0 */
		.bank = 0,
		.pin = 26,
		.func = 1,
		.route_offset = 0x50,
		.route_val = BIT(16),
	}, {
		/* pwm0-1 */
		.bank = 3,
		.pin = 21,
		.func = 1,
		.route_offset = 0x50,
		.route_val = BIT(16) | BIT(0),
	},{
		/* pwm1-0 */
		.bank = 0,
		.pin = 27,
		.func = 1,
		.route_offset = 0x50,
		.route_val = BIT(16 + 1),
	}, {
		/* pwm1-1 */
		.bank = 0,
		.pin = 30,
		.func = 2,
		.route_offset = 0x50,
		.route_val = BIT(16 + 1) | BIT(1),
	},{
		/* pwm2-0 */
		.bank = 0,
		.pin = 28,
		.func = 1,
		.route_offset = 0x50,
		.route_val = BIT(16 + 2),
	}, {
		/* pwm2-1 */
		.bank = 1,
		.pin = 12,
		.func = 2,
		.route_offset = 0x50,
		.route_val = BIT(16 + 2) | BIT(2),
	},{
		/* pwm3-0 */
		.bank = 3,
		.pin = 26,
		.func = 1,
		.route_offset = 0x50,
		.route_val = BIT(16 + 3),
	}, {
		/* pwm3-1 */
		.bank = 1,
		.pin = 13,
		.func = 2,
		.route_offset = 0x50,
		.route_val = BIT(16 + 3) | BIT(3),
	},{
		/* sdio-0_d0 */
		.bank = 1,
		.pin = 1,
		.func = 1,
		.route_offset = 0x50,
		.route_val = BIT(16 + 4),
	}, {
		/* sdio-1_d0 */
		.bank = 3,
		.pin = 2,
		.func = 1,
		.route_offset = 0x50,
		.route_val = BIT(16 + 4) | BIT(4),
	},{
		/* spi-0_rx */
		.bank = 0,
		.pin = 13,
		.func = 2,
		.route_offset = 0x50,
		.route_val = BIT(16 + 5),
	}, {
		/* spi-1_rx */
		.bank = 0,
		.pin = 1,
		.func = 2,
		.route_offset = 0x50,
		.route_val = BIT(16 + 5) | BIT(5),
	},{
		/* emmc-0_cmd */
		.bank = 1,
		.pin = 22,
		.func = 2,
		.route_offset = 0x50,
		.route_val = BIT(16 + 7),
	}, {
		/* emmc-1_cmd */
		.bank = 2,
		.pin = 4,
		.func = 2,
		.route_offset = 0x50,
		.route_val = BIT(16 + 7) | BIT(7),
	},{
		/* uart2-0_rx */
		.bank = 1,
		.pin = 19,
		.func = 2,
		.route_offset = 0x50,
		.route_val = BIT(16 + 8),
	}, {
		/* uart2-1_rx */
		.bank = 1,
		.pin = 9,
		.func = 2,
		.route_offset = 0x50,
		.route_val = BIT(16 + 8) | BIT(8),
	},{
		/* uart1-0_rx */
		.bank = 1,
		.pin = 9,
		.func = 1,
		.route_offset = 0x50,
		.route_val = BIT(16 + 11),
	}, {
		/* uart1-1_rx */
		.bank = 3,
		.pin = 13,
		.func = 1,
		.route_offset = 0x50,
		.route_val = BIT(16 + 11) | BIT(11),
	},
};

> 
> 
> Heiko
> 
> 
>>>> So i think the smallest cell is a group, which contains the pins for the
>>>> pin-routing.
>>>
>>> Though that creates more ABI between kernel and devicetree (pingroup
>>> names), which will most likely bite us some time in the future. Also I'd
>>> like to keep these dependencies as minimal as possible, as depending on
>>> how much more funny inventions we accumulate, we might want to restructure
>>> (or split) the driver at some later point - without breaking devicetrees.
>>>
>>>
>>> Heiko
>>>
>>>
>>>> 在 2017/5/18 16:48, Heiko Stübner 写道:
>>>>> Hi Kever, David,
>>>>>
>>>>> Am Donnerstag, 18. Mai 2017, 16:21:09 CEST schrieb Kever Yang:
>>>>>> Hi Heiko, Linus,
>>>>>>        Is there any news to handle this kind of IOMUX?
>>>>>
>>>>> I was talking with David Wu about that a short while ago as well :-) .
>>>>>
>>>>>   From what I've heared, in the future socs may be able to select that
>>>>> routing themselfs based on the iomux and having had time to ponder
>>>>> the whole issue for some time now, I do guess some sort of list would be
>>>>> the least intrusive solution.
>>>>>
>>>>> It seems we're talking about only a handful of such routings per soc,
>>>>> so I guess having the iomux setting check a list
>>>>>
>>>>> 	(pin, pinfunc) -> grf-setting for the pin-routing
>>>>>
>>>>> really is the least intrusive thing to do - as iomux settings really
>>>>> aren't changed that often on a running device and adding more
>>>>> stuff on top would just complicate everything.
>>>>>
>>>>>
>>>>> Heiko
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>> - Kever
>>>>>>
>>>>>> On 08/05/2016 07:36 AM, Heiko Stübner wrote:
>>>>>>> Hi Linus,
>>>>>>>
>>>>>>>
>>>>>>> on the rk3399 we found an interesting new feature and would like to get
>>>>>>> some input from the pinctrl expert :-) , as Doug and me currently are of
>>>>>>> differing opinions on where specific control elements belong.
>>>>>>>
>>>>>>>
>>>>>>> In a nutshell on the rk3399 some things like one specific uart can use
>>>>>>> multiple pins to output data, but control of that seems to be split. The
>>>>>>> actual pin config is identical for all pins - each needs to be configured
>>>>>>> to function 2, pulls set etc. Then somewhere between the pin io-cells and
>>>>>>> the uart it seems to have some sort of switch to decide to which pin to
>>>>>>> actually route the data.
>>>>>>>
>>>>>>>
>>>>>>> +-------+    +--------+  /- GPIO4_B1 (pinmux 2)
>>>>>>>
>>>>>>> | uart2 | -- | switch | --- GPIO4_C1 (pinmux 2)
>>>>>>>
>>>>>>> +-------+    +--------+  \- GPIO4_C4 (pinmux 2)
>>>>>>> (switch selects one of the 3 pins to actually output data to)
>>>>>>>
>>>>>>>
>>>>>>> So the question now is, should the pinctrl driver also flip that switch to
>>>>>>> the correct position itself when pin-function 2 of say gpio4_bq gets
>>>>>>> selected or is that routing outside of pinctrl's scope?
>>>>>>>
>>>>>>> -----
>>>>>>>
>>>>>>> I hope to have presented the core issue above somewhat neutrally, below
>>>>>>> are
>>>>>>> my personal worries about doing that in pinctrl :-) .
>>>>>>>
>>>>>>>
>>>>>>> Apart from it feeling "bolted-on" to me, I have two main worries with that
>>>>>>> approach:
>>>>>>> (1) Right now the unused pins are really unused on the same iomux, so when
>>>>>>> flipping the switch it essentially does
>>>>>>>
>>>>>>>     uart-sout    unused
>>>>>>>
>>>>>>>      |(iomux2)    |(iomux2)
>>>>>>>
>>>>>>> +----------+ +----------+
>>>>>>>
>>>>>>> | gpio4_b0 | | gpio4_c0 |
>>>>>>>
>>>>>>> +----------+ +----------+
>>>>>>>
>>>>>>> going to
>>>>>>>
>>>>>>>     unused       uart-sout
>>>>>>>
>>>>>>>      |(iomux2)    |(iomux2)
>>>>>>>
>>>>>>> +----------+ +----------+
>>>>>>>
>>>>>>> | gpio4_b0 | | gpio4_c0 |
>>>>>>>
>>>>>>> +----------+ +----------+
>>>>>>>
>>>>>>> but nothing keeps designers from doing
>>>>>>>
>>>>>>>     uart-sout    special1
>>>>>>>
>>>>>>>      |(iomux2)    |(iomux2)
>>>>>>>
>>>>>>> +----------+ +----------+
>>>>>>>
>>>>>>> | gpio4_b0 | | gpio4_c0 |
>>>>>>>
>>>>>>> +----------+ +----------+
>>>>>>>
>>>>>>> going to
>>>>>>>
>>>>>>>     special2     uart-sout
>>>>>>>
>>>>>>>      |(iomux2)    |(iomux2)
>>>>>>>
>>>>>>> +----------+ +----------+
>>>>>>>
>>>>>>> | gpio4_b0 | | gpio4_c0 |
>>>>>>>
>>>>>>> +----------+ +----------+
>>>>>>>
>>>>>>> somewhere down the road, so relying on following the selected iomux feels
>>>>>>> not future proof.
>>>>>>>
>>>>>>> (2) Looking at [0] we already have a similar case, where we configure the
>>>>>>> pins for rgmii but still tell the gmac controller that it is supposed to
>>>>>>> do
>>>>>>> rgmii instead of rmii.
>>>>>>>
>>>>>>> Here the pinmux is the same for all pins, rmii just uses less pins when
>>>>>>> compared to rgmii, so binding that to the pinmux isn't even possible.
>>>>>>>
>>>>>>> And doing it one way here and another way for the switch feels very
>>>>>>> strange.
>>>>>>>
>>>>>>>
>>>>>>> I hope this overly long mail was not to confusing and hope for some words
>>>>>>> of wisdom ;-)
>>>>>>>
>>>>>>>
>>>>>>> Big thanks
>>>>>>> Heiko
>>>>>>>
>>>>>>>
>>>>>>> [0]
>>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/
>>>>>>> arm/boot/dts/rk3288-miqi.dts#n139
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> Linux-rockchip mailing list
>>>>>>> Linux-rockchip@lists.infradead.org
>>>>>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>>
> 
> 
> 
> 
> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip