Re: [PATCH] ARM: dts: BCM5301X: Fix pin controller node

[Florian Fainelli <f.fainelli@gmail.com>]

On 8/19/20 2:14 PM, Ray Jui wrote:
> 
> 
> On 8/19/2020 1:49 PM, Florian Fainelli wrote:
>> On 8/19/20 1:48 PM, Christian Lamparter wrote:
>>> On 2020-08-19 06:23, Florian Fainelli wrote:
>>>> The pin controller resources start at 0xc0 from the CRU base which is at
>>>> 0x100 from th DMU base, for a final address of 0x1800_c1c0, whereas we
>>>> are currently off by 0x100. The resource size of the CRU is also
>>>> incorrect and should end at 0x248 bytes from 0x100 which is the start
>>>> address. Finally, the compatibility strings defined for the
>>>> pin-controller node should reflect the SoC being used.
>>>>
>>>> Fixes: 9994241ac97c ("ARM: dts: BCM5301X: Describe Northstar pins mux
>>>> controller")
>>>> Reported-by: Christian Lamparter <chunkeey@gmail.com>
>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>> ---
>>>> Christian, can you test this as a preliminary patch for your Cisco
>>>> Meraki MR32 series? Thanks!
>>>
>>> Hm, it looks like this is more complicated than this. We should have
>>> looked at pinctrl-ns.c's ns_pinctrl_probe() [0] before calling it.
>>>
>>> |    ns_pinctrl->regmap = syscon_node_to_regmap(of_get_parent(np));
>>> |    if (IS_ERR(ns_pinctrl->regmap)) {
>>> |        int err = PTR_ERR(ns_pinctrl->regmap);
>>> |
>>> |        dev_err(dev, "Failed to map pinctrl regs: %d\n", err);
>>> |
>>> |        return err;
>>> |    }
>>> |
>>> |    if (of_property_read_u32(np, "offset", &ns_pinctrl->offset)) {
>>> |        dev_err(dev, "Failed to get register offset\n");
>>> |        return -ENOENT;
>>> |    }
>>>
>>> So, the ns_pinctrl_probe() takes the address of the parent node (cru)
>>> and then looks for a "offset" property to add to this (which is missing
>>> in the bcm5301x.dtsi [1]).
>>>
>>> Thing is, for this to work, the parent-node should be a "simple-mfd" (so
>>> a regmap is created for the reg), right? This would also mean that the
>>> "reg" property in the pin-controller node is just cosmetic.
>>>
>>> I guess the reason why this sort-of-works for me is because I'm using
>>> this MR32 with OpenWrt (Rafał Miłecki is probably using it too ;) ).
>>>
>>> (Note: We should not forget to update the binding-documentation as well!)
>>>
>>> BTW: I'll reply my findings for the i2c issue with the MR32 in the other
>>> mail.
>>
>> Rafal, has this driver ever worked to begin with? None of this should be
>> necessary, we should just be using a simple platform device resource here.
>>
> 
> Florian, what if CDRU is a shared resource whose registers are accessed
> and shared by multiple blocks (and therefore device drivers) within the
> chip? Then accessing this shared CDRU resource through syscon makes sure
> there's no race condition, isn't it?

In this particular case there is no register overlap, and the driver has
been written with a binding that does not match what we have in tree,
that would need fixing one way or the other.

In fact, the entire Device Tree tree should be re-organized such that
all relevant child nodes are in the CDRU (like PLL controls).
--
Florian