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?
...
--
Regards,
Siddharth.
