Received-SPF: pass (google.com: domain of linux-kernel+bounces-45748-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <126d520e-bd00-467b-b5dc-3423cc405410@ti.com>
Date: Wed, 31 Jan 2024 10:23:03 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
CC: <bhelgaas@google.com>, <lpieralisi@kernel.org>, <kw@linux.com>,
        <robh@kernel.org>, <vigneshr@ti.com>, <linux-pci@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>,
        <srk@ti.com>, <s-vadapalli@ti.com>
Subject: Re: [PATCH] PCI: j721e: Extend j721e_pcie_ctrl_init() for non syscon
 nodes
Content-Language: en-US
To: Andrew Davis <afd@ti.com>
References: <20240129104958.1139787-1-s-vadapalli@ti.com>
 <077682de-7789-4f1f-8dcc-aa47f4fb2dff@ti.com>
 <792c972b-052e-4e24-a85f-9415fe02aa01@ti.com>
 <2a373360-9bf2-411c-a9aa-0a7451b9ba38@ti.com>
From: Siddharth Vadapalli <s-vadapalli@ti.com>
In-Reply-To: <2a373360-9bf2-411c-a9aa-0a7451b9ba38@ti.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit


On 30/01/24 20:30, Andrew Davis wrote:
> 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.

Seems like a Hack to me considering that the offset will always be zero for
non-syscon parent nodes (which will be the convention going forward), implying
that the offset might as well be dropped and just the phandle is sufficient. For
now I shall implement it as you suggested. Maybe once the syscon nodes in
existing SoCs are also converted to simple-bus, then the property can be
redefined to just be the phandle to "pcie_ctrl" register.

> 
> Andrew

-- 
Regards,
Siddharth.