linux-kernel - Re: [PATCH] PCI: j721e: Extend j721e_pcie_ctrl

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]

Message-ID: <2a373360-9bf2-411c-a9aa-0a7451b9ba38@ti.com>
Date: Tue, 30 Jan 2024 09:00:01 -0600
From: Andrew Davis <afd@...com>
To: Siddharth Vadapalli <s-vadapalli@...com>
CC: <bhelgaas@...gle.com>, <lpieralisi@...nel.org>, <kw@...ux.com>,
        <robh@...nel.org>, <vigneshr@...com>, <linux-pci@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
        <srk@...com>
Subject: Re: [PATCH] PCI: j721e: Extend j721e_pcie_ctrl_init() for non syscon
 nodes

On 1/29/24 10:50 PM, Siddharth Vadapalli wrote:
> Hello Andrew,
> 
> On 29/01/24 20:49, Andrew Davis wrote:
>> On 1/29/24 4:49 AM, Siddharth Vadapalli wrote:
> 
> ...
> 
>>>        int ret;
>>>    -    syscon = syscon_regmap_lookup_by_phandle(node, "ti,syscon-pcie-ctrl");
>>> +    scm_conf = of_parse_phandle(node, "ti,syscon-pcie-ctrl", 0);
>>> +    if (!scm_conf) {
>>> +        dev_err(dev, "unable to get System Controller node\n");
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    syscon = device_node_to_regmap(scm_conf);
>>
>> Turning the entire "simple-bus" region into a regmap using this
>> function is just as broken as having it as a "syscon". The core
>> problem we are solving by getting rid of the blanket syscon nodes
>> is that it causes multiple mappings of the same register. This
>> can cause issues with regmap caching, read–modify–write, etc..
>>
>> What you want to do is add a subnode to the simple-bus, have that
>> encapsulate just the registers used for PCIe, and have the PCIe
>> node point to that. Then this patch isn't needed.
>>
>> For an example, see how it's done for DSS[0].
> 
> Thank you for reviewing the patch. I will implement it similar to what's done
> for DSS as you pointed out. However, what about the existing SoCs which make use
> of the "ti,syscon-pcie-ctrl" property? Do you suggest that I add another
> device-tree property for pointing to the PCIE_CTRL register within the CTRL_MMR
> region, or do you suggest that I reuse the existing "ti,syscon-pcie-ctrl"
> property differently in the SoCs like J784S4 where the scm_conf node is a
> "simple-bus"?
> 
> The "ti,syscon-pcie-ctrl" property as defined in the device-tree bindings has
> two elements with the first being the phandle to the scm_conf node and the
> second being the offset of the PCIE_CTRL register. The newer implementation you
> are suggesting will either require a new property which accepts only one element
> namely the phandle to the node within scm_conf corresponding to the PCIE_CTRL
> register. Will adding a new property be acceptable?
> 

Why would you need a new property? If there is no offset to the PCIE_CTRL register
in the new syscon space then just set the offset = 0x0, easy.

Andrew