On Mon, Jun 4, 2018 at 3:25 PM, NeilBrown <n...@brown.name> wrote: > On Mon, Jun 04 2018, Rosen Penev wrote: > >> This currently fixes the remaining dtb warnings: >> >> Node /pcie@1e140000/pcie0 has a reg or ranges property, but no unit name >> Node /pcie@1e140000/pcie1 has a reg or ranges property, but no unit name >> Node /pcie@1e140000/pcie2 has a reg or ranges property, but no unit name >> Node /pcie@1e140000/pcie0 node name is not "pci" or "pcie" >> Node /pcie@1e140000/pcie0 missing ranges for PCI bridge (or not a bridge) >> Node /pcie@1e140000/pcie0 missing bus-range for PCI bridge >> Node /pcie@1e140000/pcie1 node name is not "pci" or "pcie" >> Node /pcie@1e140000/pcie1 missing ranges for PCI bridge (or not a bridge) >> Node /pcie@1e140000/pcie1 missing bus-range for PCI bridge >> Node /pcie@1e140000/pcie2 node name is not "pci" or "pcie" >> Node /pcie@1e140000/pcie2 missing ranges for PCI bridge (or not a bridge) >> Node /pcie@1e140000/pcie2 missing bus-range for PCI bridge >> Warning (unit_address_format): Failed prerequisite 'pci_bridge' >> Warning (pci_device_reg): Failed prerequisite 'pci_bridge' >> Warning (pci_device_bus_num): Failed prerequisite 'pci_bridge' >> >> In addition, it sets each pcie port to disabled in order to allow future >> boards to be added that only have 1 or 2 pcie lanes wired up. gbpc1.dts >> was changed to accomidate. > > I don't think you are using the word "lanes" correctly. > A lane is a pair of wires for carrying data. a x1 PCIe slot has > 1 lane and send all data serially. A x8 PCIe slot has 8 lanes > and sends 8 bits at a time. According to the datasheet, mt7621 has 3 pcie 1.1 interfaces running at x1 speeds. Seems to be correct. Although... > The enable/disable here seem to apply to ports (hosts? end-points?). > The mt7621 has a 3port pcie bridge (switch?) and 3 individual pci > end-points which can connect to 3 different devices. > >> >> Signed-off-by: Rosen Penev <ros...@gmail.com> >> --- >> drivers/staging/mt7621-dts/gbpc1.dts | 15 +++++++++++++++ >> drivers/staging/mt7621-dts/mt7621.dtsi | 24 ++++++++++++------------ >> 2 files changed, 27 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/staging/mt7621-dts/gbpc1.dts >> b/drivers/staging/mt7621-dts/gbpc1.dts >> index 6b13d85d9d34..cd7f90e4ec6d 100644 >> --- a/drivers/staging/mt7621-dts/gbpc1.dts >> +++ b/drivers/staging/mt7621-dts/gbpc1.dts >> @@ -113,7 +113,22 @@ >> }; >> >> &pcie { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pcie_pins>; >> status = "okay"; >> + >> + pcie@0,0 { >> + status = "okay"; >> + }; >> + >> + pcie@1,0 { >> + status = "okay"; >> + }; >> + >> + pcie@2,0 { >> + status = "okay"; >> + }; >> + >> }; >> >> ðernet { >> diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi >> b/drivers/staging/mt7621-dts/mt7621.dtsi >> index 5a94fcb96402..16583647797f 100644 >> --- a/drivers/staging/mt7621-dts/mt7621.dtsi >> +++ b/drivers/staging/mt7621-dts/mt7621.dtsi >> @@ -452,31 +452,31 @@ >> clocks = <&clkctrl 24 &clkctrl 25 &clkctrl 26>; >> clock-names = "pcie0", "pcie1", "pcie2"; >> >> - pcie0 { >> + pcie@0,0 { >> reg = <0x0000 0 0 0 0>; >> - >> #address-cells = <3>; >> #size-cells = <2>; >> - >> - device_type = "pci"; >> + ranges; >> + num-lanes = <1>; >> + status = "disabled"; > > Some of this looks good, however... > > 1/ The 'status = "disabled"' doesn't actually work. I removed the > 'status = "okay"' for pcie@2,0 in gbpc1, and all 3 PCIe devices were > still active. Maybe we just need to add code somewhere. > > 2/ Why do you remove 'device_type = "pci";'?? Without a device type, > no-one knows what sort of device it is - though that doesn't seem to > stop it from working I did it to match mt7623.dtsi. I think it continues to work because of the pcie name being recognized. > > 3/ the "num-lanes" might make sense, but no code actually processes > it. "num-lanes" is only examined by: > > drivers/pci/dwc/pcie-designware.c: ret = of_property_read_u32(np, > "num-lanes", &lanes); > drivers/pci/host/pcie-mediatek.c: err = of_property_read_u32(node, > "num-lanes", &port->lane); > drivers/pci/host/pcie-rockchip.c: err = of_property_read_u32(node, > "num-lanes", &rockchip->lanes); > > so I cannot see why it belongs here. Will remove. The SoC does not support anything higher than x1 anyway. > So: > I like the name change (pcie0 -> pcie@0,0) > I like the status="disabled", except that it doesn't work. > I think the addition of "ranges;" is correct. Is it really? On most dts files that I've seen there are values for it. I'm not really sure if it's needed though. > I don't understand the rest. One thing I also do not understand is that after this change, the vIRQs in /proc/interrupts get incremented with ttyS0 and above as was the case prior to staging: mt7621-pci: improve interrupt mapping
Anyway, I will be respining this. > > Thanks, > NeilBrown > >> }; >> >> - pcie1 { >> + pcie@1,0 { >> reg = <0x0800 0 0 0 0>; >> - >> #address-cells = <3>; >> #size-cells = <2>; >> - >> - device_type = "pci"; >> + ranges; >> + num-lanes = <1>; >> + status = "disabled"; >> }; >> >> - pcie2 { >> + pcie@2,0 { >> reg = <0x1000 0 0 0 0>; >> - >> #address-cells = <3>; >> #size-cells = <2>; >> - >> - device_type = "pci"; >> + ranges; >> + num-lanes = <1>; >> + status = "disabled"; >> }; >> }; >> }; >> -- >> 2.17.1 _______________________________________________ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel