Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Received-SPF: Pass (protection.outlook.com: domain of amd.com designates
 165.204.84.17 as permitted sender) receiver=protection.outlook.com;
 client-ip=165.204.84.17; helo=SATLEXMB03.amd.com; pr=C
Message-ID: <0cd5fe3b-8039-e534-03ac-aa6fd8f9c7c0@amd.com>
Date:   Tue, 5 Dec 2023 10:53:47 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH v2 0/2] Attach DT nodes to existing PCI devices
Content-Language: en-US
To:     Rob Herring <robh@kernel.org>,
        Herve Codina <herve.codina@bootlin.com>
CC:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Max Zhen <max.zhen@amd.com>,
        Sonal Santan <sonal.santan@amd.com>,
        Stefano Stabellini <stefano.stabellini@xilinx.com>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        PCI <linux-pci@vger.kernel.org>,
        "Allan Nielsen" <allan.nielsen@microchip.com>,
        Horatiu Vultur <horatiu.vultur@microchip.com>,
        Steen Hegelund <steen.hegelund@microchip.com>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>
References: <20231130165700.685764-1-herve.codina@bootlin.com>
 <CAL_JsqJvt6FpXK+FgAwE8xN3G5Z23Ktq=SEY-K7VA7nM5XgZRg@mail.gmail.com>
 <20231204134335.3ded3d46@bootlin.com>
 <CAL_JsqLtCS3otZ1sfiPEWwrWB4dyNpu4e0xANWJriCEUYr+4Og@mail.gmail.com>
 <20231204163014.4da383f2@bootlin.com>
 <CAL_JsqJJ64513pyQggU71agTzawNWPpm6ZpWMB6e0zu-tWL8yw@mail.gmail.com>
From:   Lizhi Hou <lizhi.hou@amd.com>
In-Reply-To: <CAL_JsqJJ64513pyQggU71agTzawNWPpm6ZpWMB6e0zu-tWL8yw@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Dec 2023 18:53:50.0778
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 8165f3c9-9886-44d9-3115-08dbf5c3850c
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB03.amd.com]
X-MS-Exchange-CrossTenant-AuthSource: CO1PEPF000044F3.namprd05.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR12MB5067
X-Spam-Status: No, score=-4.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on morse.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (morse.vger.email [0.0.0.0]); Tue, 05 Dec 2023 10:54:07 -0800 (PST)


On 12/4/23 15:03, Rob Herring wrote:
> On Mon, Dec 4, 2023 at 9:30 AM Herve Codina <herve.codina@bootlin.com> wrote:
>> Hi Rob,
>>
>> On Mon, 4 Dec 2023 07:59:09 -0600
>> Rob Herring <robh@kernel.org> wrote:
>>
>> [...]
>>
>>>>> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
>>>>> index 9c2137dae429..46b252bbe500 100644
>>>>> --- a/drivers/pci/bus.c
>>>>> +++ b/drivers/pci/bus.c
>>>>> @@ -342,8 +342,6 @@ void pci_bus_add_device(struct pci_dev *dev)
>>>>>           */
>>>>>          pcibios_bus_add_device(dev);
>>>>>          pci_fixup_device(pci_fixup_final, dev);
>>>>> -       if (pci_is_bridge(dev))
>>>>> -               of_pci_make_dev_node(dev);
>>>>>          pci_create_sysfs_dev_files(dev);
>>>>>          pci_proc_attach_device(dev);
>>>>>          pci_bridge_d3_update(dev);
>>>>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>>>>> index 51e3dd0ea5ab..e15eaf0127fc 100644
>>>>> --- a/drivers/pci/of.c
>>>>> +++ b/drivers/pci/of.c
>>>>> @@ -31,6 +31,8 @@ int pci_set_of_node(struct pci_dev *dev)
>>>>>                  return 0;
>>>>>
>>>>>          node = of_pci_find_child_device(dev->bus->dev.of_node, dev->devfn);
>>>>> +       if (!node && pci_is_bridge(dev))
>>>>> +               of_pci_make_dev_node(dev);
>>>>>          if (!node)
>>>>>                  return 0;
>>>> Maybe it is too early.
>>>> of_pci_make_dev_node() creates a node and fills some properties based on
>>>> some already set values available in the PCI device such as its struct resource
>>>> values.
>>>> We need to have some values set by the PCI infra in order to create our DT node
>>>> with correct values.
>>> Indeed, that's probably the issue I'm having. In that case,
>>> DECLARE_PCI_FIXUP_HEADER should work. That's later, but still before
>>> device_add().
>>>
>>> I think modifying sysfs after device_add() is going to race with
>>> userspace. Userspace is notified of a new device, and then the of_node
>>> link may or may not be there when it reads sysfs. Also, not sure if
>>> we'll need DT modaliases with PCI devices, but they won't work if the
>>> DT node is not set before device_add().
>> Ok, we can try using DECLARE_PCI_FIXUP_HEADER.
>> On your side, is moving from DECLARE_PCI_FIXUP_EARLY to DECLARE_PCI_FIXUP_HEADER
>> fix your QEMU unittest ?
> No...
>
> And testing the bridge part crashes. That's because there's a
> dependency on the bridge->subordinate to write out bus-range and
> interrupt-map. I think the fix there is we should just not write those
> properties. The bus range isn't needed because the kernel does its own
> assignments. For interrupt-map, it is only needed if "interrupts" is
Without 'bus-range', dtc will output a warning while compiling the 
generated node.
> present in the child devices. If not present, then the standard PCI
> swizzling is used. Alternatively, I think the interrupt mapping could
> be simplified to just implement the standard swizzling at each level
> which isn't dependent on any of the devices on the bus. I gave that a
> go where each interrupt-map just points to the parent bridge, but ran
> into an issue that the bridge nodes don't have a phandle. That should
> be fixable, but I'd rather go with the first option. I suppose that

Do you mean it might be something similar with I posted in V12?

https://lore.kernel.org/lkml/97ae2eda-f712-0b83-dc90-0f29edd5db8b@amd.com/


Thanks,

Lizhi

> depends on how the interrupts downstream of the PCI device need to get
> resolved. It could be that the PCI device serves as the interrupt
> controller and can resolve the parent interrupt on its own (which may
> be needed for ACPI host anyways).
>
>> We have to note that between the pci_fixup_device(pci_fixup_header, dev) call
>> and the device_add() call, the call to pci_set_msi_domain() is present.
>> MSIs are not supported currently but in the future ...
> MSI's aren't ever described in PCI nodes. Only the host bridge. So I
> don't think we should have problems there.
>
>> Related to DT modaliases, I don't think they are needed.
>> All drivers related to PCI device should be declared as pci_driver.
>> Correct me if I am wrong but I think that the core PCI will load the correct
>> module without any DT modalias.
> Yes, you are probably right.
>
> Rob