Re: [PATCH v4 2/3] misc: tc9564: introduce base PCI driver

[Alex Elder <elder@riscstar.com>]

On 9/18/26 10:42 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] pci: of_pci_make_dev_node quirk fails to update ranges of existing DT node

This series depends on another series that affects the
code that is the subject of the above report.
  
https://lore.kernel.org/sashiko-reviews/ed783639-05ed-4ad9-b881-d1689a8b2418@riscstar.com/

> - [Medium] kconfig: TC9564_PCI selects PCI_DYNAMIC_OF_NODES without inheriting OF_IRQ dependency
I attempted to include this but wound up with a report of a circular
dependency when configuring the kernel.

A bit more below.

> --
> 
> commit d693efad7a099f6326c4b836ade779f0f07366be
> Author: Alex Elder <elder@riscstar.com>
> 
> misc: tc9564: introduce base PCI driver
> 
> The Toshiba TC9564 is small and highly-specialized SoC that implements
> a PCIe switch as well as an Ethernet AVB/TSN bridge. Create a new PCI
> driver under drivers/misc that binds with the embedded PCI endpoint
> functions within the TC9564 SoC. Use the PCI quirks mechanism to update
> endpoint device nodes dynamically.
> 
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 7364931dad3a1..9c5b1e9d672e8 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
> 
> [ ... ]
> 
>> @@ -568,6 +568,21 @@ config MCHP_LAN966X_PCI
>>   	    - lan966x-miim (MDIO_MSCC_MIIM)
>>   	    - lan966x-switch (LAN966X_SWITCH)
>>   
>> +config TC9564_PCI
>> +	tristate "Toshiba TC9564 PCI function support"
>> +	depends on OF
>> +	depends on PCI
>> +	select PCI_DYNAMIC_OF_NODES
> 
> [Severity: Medium]
> Can this cause a Kconfig unmet direct dependency warning?
> 
> PCI_DYNAMIC_OF_NODES explicitly defines 'depends on OF_IRQ' in
> drivers/pci/Kconfig. Selecting it here without inheriting its dependencies
> could cause unmet direct dependency warnings and potential build failures on
> architectures lacking OF_IRQ support.

I attempted to include "depends on OF_IRQ", but that led to
a circular dependency, so I simply left it out.

> 
>> +	select PCI_QUIRKS
>> +	default m
>> +	help
> 
> [ ... ]
> 
>> diff --git a/drivers/misc/tc9564-pci.c b/drivers/misc/tc9564-pci.c
>> index 0000000000000..e4d00dfecfca9 100644
>> --- /dev/null
>> +++ b/drivers/misc/tc9564-pci.c
> 
> [ ... ]
> 
>> +	/* Scan for pci-ep-bus (simple-bus) nodes and probe their sub-devices */
>> +	ret = of_platform_default_populate(np, NULL, dev);
> 
> [ ... ]
> 
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index de9bbccda21fd..54e310c4153df 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -6391,6 +6391,7 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5020, of_pci_make_dev_node);
>>   DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5021, of_pci_make_dev_node);
>>   DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REDHAT, 0x0005, of_pci_make_dev_node);
>>   DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_EFAR, 0x9660, of_pci_make_dev_node);
>> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_TOSHIBA, 0x0220, of_pci_make_dev_node);
> 
> [Severity: High]
> Will this quirk successfully update the ranges property of the existing
> devicetree node?
> 
> Because the tc9564 device has a static device tree node, pci_set_of_node()
> attaches the static node to pdev->dev.of_node before this fixup runs.
> 
> Inside of_pci_make_dev_node(), the early check for an existing node:
> 
>      if (pci_device_to_OF_node(pdev))
>          return;
> 
> evaluates to true, causing it to return immediately. Consequently,
> of_pci_prop_ranges() is never executed.

The series mentioned above changes this exact code so that instead
of returning immediately, it first checks to see if a non-bridge node
has a pci-ep-bus sub-node.  If it does, it generates a new ranges
property for the endpoint and updates the devicetree to use it.

					-Alex

> 
> This means endpoint platform devices populated by of_platform_default_populate()
> in tc9564_function_probe() will fail to map their MMIO addresses correctly,
> translating addresses using the placeholder static 'ranges' property rather
> than dynamically assigned PCI BAR addresses.
>