On Mon, Jun 4, 2018 at 3:25 PM, NeilBrown <neil@xxxxxxxxxx> 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 <rosenp@xxxxxxxxx>
>> ---
>> 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
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
