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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 19 Aug 2020 14:29:01 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Ray Jui <ray.jui@...adcom.com>,
        Christian Lamparter <chunkeey@...il.com>,
        linux-arm-kernel@...ts.infradead.org
Cc:     Hauke Mehrtens <hauke@...ke-m.de>,
        Rafał Miłecki <zajec5@...il.com>,
        "maintainer:BROADCOM BCM5301X ARM ARCHITECTURE" 
        <bcm-kernel-feedback-list@...adcom.com>,
        Rob Herring <robh+dt@...nel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ARM: dts: BCM5301X: Fix pin controller node

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@...il.com>
>>>> Signed-off-by: Florian Fainelli <f.fainelli@...il.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