On 2024/11/30 3:50, Bjorn Helgaas wrote:
On Fri, Nov 29, 2024 at 05:51:39PM +0800, Chen Wang wrote:
On 2024/11/13 5:20, Bjorn Helgaas wrote:
On Mon, Nov 11, 2024 at 01:59:56PM +0800, Chen Wang wrote:
From: Chen Wang <unicorn_wang@xxxxxxxxxxx>
Add support for PCIe controller in SG2042 SoC. The controller
uses the Cadence PCIe core programmed by pcie-cadence*.c. The
PCIe controller will work in host mode only.
+++ b/drivers/pci/controller/cadence/Kconfig
@@ -67,4 +67,15 @@ config PCI_J721E_EP
Say Y here if you want to support the TI J721E PCIe platform
controller in endpoint mode. TI J721E PCIe controller uses Cadence PCIe
core.
+
+config PCIE_SG2042
+ bool "Sophgo SG2042 PCIe controller (host mode)"
+ depends on ARCH_SOPHGO || COMPILE_TEST
+ depends on OF
+ select PCIE_CADENCE_HOST
+ help
+ Say Y here if you want to support the Sophgo SG2042 PCIe platform
+ controller in host mode. Sophgo SG2042 PCIe controller uses Cadence
+ PCIe core.
Reorder to keep these menu items in alphabetical order by vendor.
Sorry, I don't understand your question. I think the menu items in this
Kconfig file are already sorted alphabetically.
Here's what menuconfig looks like after this patch:
[*] Cadence platform PCIe controller (host mode)
[*] Cadence platform PCIe controller (endpoint mode)
[*] TI J721E PCIe controller (host mode)
[*] TI J721E PCIe controller (endpoint mode)
[ ] Sophgo SG2042 PCIe controller (host mode) (NEW)
"Sophgo" sorts *before* "TI".
I see what you mean. I thought we just had to make sure the item IDs in
the Kconfig file were in alphabetical order.
I will make changes in the next version based on your suggestions.
+ if (pcie->link_id == 1) {
+ regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_LOW,
+ lower_32_bits(pcie->msi_phys));
+ regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_HIGH,
+ upper_32_bits(pcie->msi_phys));
+
+ regmap_read(pcie->syscon, REG_LINK1_MSI_ADDR_SIZE, &value);
+ value = (value & REG_LINK1_MSI_ADDR_SIZE_MASK) | MAX_MSI_IRQS;
+ regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_SIZE, value);
+ } else {
+ regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_LOW,
+ lower_32_bits(pcie->msi_phys));
+ regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_HIGH,
+ upper_32_bits(pcie->msi_phys));
+
+ regmap_read(pcie->syscon, REG_LINK0_MSI_ADDR_SIZE, &value);
+ value = (value & REG_LINK0_MSI_ADDR_SIZE_MASK) | (MAX_MSI_IRQS << 16);
+ regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_SIZE, value);
+ }
Lot of pcie->link_id checking going on here. Consider saving these
offsets in the struct sg2042_pcie so you don't need to test
everywhere.
Actually, there are not many places in the code to check link_id. If to add
storage information in struct sg2042_pcie, at least four u32 are needed.
And this logic will only be called one time in the probe. So I think it is
better to keep the current method. What do you think?
1) I don't think "link_id" is a very good name since it appears to
refer to one of two PCIe Root Ports. mvebu uses "marvell,pcie-port"
which looks like a similar usage, although unnecessarily
Marvell-specific. And arch/arm/boot/dts/marvell/armada-370-db.dts has
separate stanzas for two Root Ports, without needing a property to
distinguish them, which would be even better. I'm not sure why
arch/arm/boot/dts/marvell/armada-370.dtsi needs "marvell,pcie-port"
even though it also has separate stanzas.
2) I think checking pcie->link_id is likely to be harder to maintain
in the future, e.g., if a new chip adds more Root Ports, you'll have
to touch each use.
Bjorn
Thanks for your input. I looked at the Marvell solution you recommended.
It seems that it is a relatively complex IP design to support mvebu,
which can flexibly support the export of 1 to 4 variable root ports.
The Cadence IP used by SG2042 is much simpler, and it is fixed to export
up to two root ports. So I plan to implement it in a simple way.
Changing "link_id" to "pcie-port" will look better. The writing of link
actually refers to the terminology in the Cadence manual, which is
actually the concept of port.
By the way, there is no demand for scalability at present, because it is
said that Sophgo currently only uses Cadence IP for the SG2042 SoC, and
subsequent products should switch to solutions from other suppliers.
Any question please let me know.
Regards,
Chen
