On 08/30/2016 10:50 PM, Stephen Warren wrote:
On 08/30/2016 01:01 PM, Rob Herring wrote:
quoted
On Wed, Aug 24, 2016 at 03:20:46PM -0600, Stephen Warren wrote:
quoted
From: Stephen Warren <redacted>
The Synopsys DWC EQoS is a configurable IP block which supports multiple
options for bus type, clocking and reset structure, and feature list.
Extend the DT binding to define a "compatible value" for the
configuration
contained in NVIDIA's Tegra186 SoC, and define some new properties and
list property entries required by that configuration.
quoted
quoted
diff --git
a/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt
b/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt
quoted
quoted
Required properties:
quoted
quoted
-- clocks: Phandles to the reference clock and the bus clock
-- clock-names: Should be "phy_ref_clk" for the reference clock and
"apb_pclk"
- for the bus clock.
+- clocks: Phandle and clock specifiers for each entry in
clock-names, in the
+ same order. See ../clock/clock-bindings.txt.
+- clock-names: May contain any/all of the following depending on the IP
+ configuration, in any order:
No, they should be in a defined order.
If the binding only defines "clocks", then yes the order must be specified.
If the binding defines clock-names too, then the order is arbitrary
since all clocks must be looked up via clock-names. That's the entire
point of having a clock-names property.
...
quoted
quoted
+ The EQOS transmit path clock. The HW signal name is clk_tx_i.
+ In some configurations (e.g. GMII/RGMII), this clock also drives
the PHY TX
+ path. In other configurations, other clocks (such as tx_125,
rmii) may
+ drive the PHY TX path.
+ - "rx"
+ The EQOS receive path clock. The HW signal name is clk_rx_i.
+ In some configurations (e.g. GMII/RGMII), this clock also drives
the PHY RX
+ path. In other configurations, other clocks (such as rx_125,
pmarx_0,
+ pmarx_1, rmii) may drive the PHY RX path.
It is not correct that clk_rx_i drives the PHY rx path for GMII/RGMII.
The PHY is the source of the rx clock for these modes.
Will the driver need to make any clock ops on the "rx" clock ?
quoted
quoted
+ - "slave_bus"
+ (Alternate name "apb_pclk"; only one alternate must appear.)
+ The CPU/slave-bus (CSR) interface clock. Despite the name, this
applies to
+ any bus type; APB, AHB, AXI, etc. The HW signal name is hclk_i
(AHB) or
+ clk_csr_i (other buses).
Sounds like 2 clocks here.
quoted
+ - "master_bus"
+ The master bus interface clock. Only required in configurations
that use a
+ separate clock for the master and slave bus interfaces. The HW
signal name
+ is hclk_i (AHB) or aclk_i (AXI).
Sounds like 2 clocks. I'm guessing these are mutually exclusive based on
whether you configure the IP for AHB or AXI?
Yes, my understanding is that the two clocks are mutually exclusive in
both cases.
It seems simpler to have an entry in clocks/clock-names for each logical
purpose, so that the driver can always retrieve a "slave bus clock" and
a "master bus clock". That way, there's never any conditional code in
the driver; it just gets two fixed clock names and enables them, no
matter what the HW configuration.
If the binding specifies 3 clocks, hclk_i, clk_csr_i, and aclk_i, then
the driver needs to know which subset of clocks to retrieve based on
compatible value or HW configuration. That seems like unnecessary
complexity. I suppose the driver could just attempt to retrieve all 3
clocks, and ignore any missing clocks, but that would allow malformed
DTs not to be noticed since the driver wouldn't validate the set of
clocks present, and could lead to the driver touching the HW without all
required clocks active, which at least in Tegra can lead to a HW hang.
I agree, keep the logical names.
quoted
quoted
+ Note: Support for additional IP configurations may require adding the
+ following clocks to this list in the future: clk_rx_125_i,
clk_tx_125_i,
+ clk_pmarx_0_i, clk_pmarx1_i, clk_rmii_i, clk_revmii_rx_i,
clk_revmii_tx_i.
+
+ The following compatible values require the following set of clocks:
+ - "nvidia,tegra186-eqos", "snps,dwc-qos-ethernet-4.10":
+ - "slave_bus"
+ - "master_bus"
+ - "rx"
+ - "tx"
+ - "ptp_ref"
+ - "axis,artpec6-eqos", "snps,dwc-qos-ethernet-4.10":
+ - "phy_ref_clk"
+ - "apb_clk"
It would be good if this was marked deprecated and the full set of
clocks could be described and supported. Not sure if you can figure that
out. Is it really only 2 clocks, or these have multiple connections to
the same source.
Lars, can you answer here?
I deliberately didn't attempt to change the binding definition for the
existing use-case, since I'm not familiar with that SoC, and don't
relish changing DTs for a platform I can't test.
For the artpec-6 the clocks are like this:
apb_clk: It is both the master and slave bus clock.
phy_ref_clk: It corresponds to tx clock in the proposed new binding.
There is a also a ptp reference clock that will map to the new ptp_ref
clock binding.
So the full set of clocks in a new artpec-6 binding is:
slave_bus
master_bus
tx
ptp_ref
BR,
Lars
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/31/2016 03:15 AM, Lars Persson wrote:
On 08/30/2016 10:50 PM, Stephen Warren wrote:
quoted
On 08/30/2016 01:01 PM, Rob Herring wrote:
quoted
On Wed, Aug 24, 2016 at 03:20:46PM -0600, Stephen Warren wrote:
quoted
The Synopsys DWC EQoS is a configurable IP block which supports multiple
options for bus type, clocking and reset structure, and feature list.
Extend the DT binding to define a "compatible value" for the configuration
contained in NVIDIA's Tegra186 SoC, and define some new properties and
list property entries required by that configuration.
quoted
quoted
quoted
diff --git
a/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt
b/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt
quoted
quoted
quoted
+- clock-names: May contain any/all of the following depending on the IP
+ configuration, in any order:
+ The EQOS transmit path clock. The HW signal name is clk_tx_i.
+ In some configurations (e.g. GMII/RGMII), this clock also drives
the PHY TX
+ path. In other configurations, other clocks (such as tx_125,
rmii) may
+ drive the PHY TX path.
+ - "rx"
+ The EQOS receive path clock. The HW signal name is clk_rx_i.
+ In some configurations (e.g. GMII/RGMII), this clock also drives
the PHY RX
+ path. In other configurations, other clocks (such as rx_125,
pmarx_0,
+ pmarx_1, rmii) may drive the PHY RX path.
It is not correct that clk_rx_i drives the PHY rx path for GMII/RGMII.
The PHY is the source of the rx clock for these modes.
I think both of our statements are true.
There's a clock input to the EQOS module (clk_rx_i) that does drive the
RX path in the EQOS module.
That clock also drives the PHY's RX path.
Those statements make no comment regarding the /source/ of that clock;
either of the following might be true:
1) The PHY could generate the clock internally somehow, feed its own
internal logic with that clock, and send the clock out to feed the EQOS
RX path too.
or,
2) SoC integration could drive the same clock into both the EQOS and
PHY modules, so that both sets of logic are fed from the same external
clock.
Perhaps the phrase "PHY RX path" is confusing; I was talking about the
EQOS modules' RX path from the PHY more than the PHY itself, although
given what I said above I believe either interpretation is valid and
correct.
Will the driver need to make any clock ops on the "rx" clock ?
Yes. The EQOS driver needs to ensure that the clock is running before
attempting to receive data from the PHY, otherwise the EQOS's own RX
logic won't be clocked.
Whether the phandle for this clock points at a SoC-level provider (it
will in Tegra) or a clock provider in the PHY (it might in other SoCs),
shouldn't matter as far as the DT binding goes, although it might affect
device probe ordering in some implementations.
quoted
quoted
quoted
+ Note: Support for additional IP configurations may require adding the
+ following clocks to this list in the future: clk_rx_125_i, clk_tx_125_i,
+ clk_pmarx_0_i, clk_pmarx1_i, clk_rmii_i, clk_revmii_rx_i, clk_revmii_tx_i.
+
+ The following compatible values require the following set of clocks:
+ - "nvidia,tegra186-eqos", "snps,dwc-qos-ethernet-4.10":
+ - "slave_bus"
+ - "master_bus"
+ - "rx"
+ - "tx"
+ - "ptp_ref"
+ - "axis,artpec6-eqos", "snps,dwc-qos-ethernet-4.10":
+ - "phy_ref_clk"
+ - "apb_clk"
It would be good if this was marked deprecated and the full set of
clocks could be described and supported. Not sure if you can figure that
out. Is it really only 2 clocks, or these have multiple connections to
the same source.
Lars, can you answer here?
I deliberately didn't attempt to change the binding definition for the
existing use-case, since I'm not familiar with that SoC, and don't
relish changing DTs for a platform I can't test.
For the artpec-6 the clocks are like this:
apb_clk: It is both the master and slave bus clock.
phy_ref_clk: It corresponds to tx clock in the proposed new binding.
There is a also a ptp reference clock that will map to the new ptp_ref
clock binding.
So the full set of clocks in a new artpec-6 binding is:
slave_bus
master_bus
tx
ptp_ref
Given the discussion above, I think we should represent the rx clock too.
On 08/31/2016 11:48 PM, Stephen Warren wrote:
On 08/31/2016 03:15 AM, Lars Persson wrote:
quoted
On 08/30/2016 10:50 PM, Stephen Warren wrote:
quoted
On 08/30/2016 01:01 PM, Rob Herring wrote:
quoted
On Wed, Aug 24, 2016 at 03:20:46PM -0600, Stephen Warren wrote:
quoted
The Synopsys DWC EQoS is a configurable IP block which supports
multiple
options for bus type, clocking and reset structure, and feature list.
Extend the DT binding to define a "compatible value" for the
configuration
contained in NVIDIA's Tegra186 SoC, and define some new properties and
list property entries required by that configuration.
quoted
quoted
quoted
quoted
diff --git
a/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt
b/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt
quoted
quoted
quoted
quoted
+- clock-names: May contain any/all of the following depending on
the IP
+ configuration, in any order:
+ The EQOS transmit path clock. The HW signal name is clk_tx_i.
+ In some configurations (e.g. GMII/RGMII), this clock also drives
the PHY TX
+ path. In other configurations, other clocks (such as tx_125,
rmii) may
+ drive the PHY TX path.
+ - "rx"
+ The EQOS receive path clock. The HW signal name is clk_rx_i.
+ In some configurations (e.g. GMII/RGMII), this clock also drives
the PHY RX
+ path. In other configurations, other clocks (such as rx_125,
pmarx_0,
+ pmarx_1, rmii) may drive the PHY RX path.
It is not correct that clk_rx_i drives the PHY rx path for GMII/RGMII.
The PHY is the source of the rx clock for these modes.
I think both of our statements are true.
There's a clock input to the EQOS module (clk_rx_i) that does drive the
RX path in the EQOS module.
That clock also drives the PHY's RX path.
Those statements make no comment regarding the /source/ of that clock;
either of the following might be true:
1) The PHY could generate the clock internally somehow, feed its own
internal logic with that clock, and send the clock out to feed the EQOS
RX path too.
or,
2) SoC integration could drive the same clock into both the EQOS and
PHY modules, so that both sets of logic are fed from the same external
clock.
Perhaps the phrase "PHY RX path" is confusing; I was talking about the
EQOS modules' RX path from the PHY more than the PHY itself, although
given what I said above I believe either interpretation is valid and
correct.
quoted
Will the driver need to make any clock ops on the "rx" clock ?
Yes. The EQOS driver needs to ensure that the clock is running before
attempting to receive data from the PHY, otherwise the EQOS's own RX
logic won't be clocked.
Whether the phandle for this clock points at a SoC-level provider (it
will in Tegra) or a clock provider in the PHY (it might in other SoCs),
shouldn't matter as far as the DT binding goes, although it might affect
device probe ordering in some implementations.
I understand your point. The lines between PHY, MAC and SoC gets blurred
with high integration.
The thing is that when we talk about standard PHY interfaces the clock
is a well defined part of the interface and it should not be mixed up
with SoC-specific implementations that deviate from this. Please update
the description of the rx clock to only cover cases when the clock is
not implicitly sourced from the PHY.
When the PHY is the source of the clock then it is managed by the phy
library and the phy driver so it does not need to be also handled by the
common clock framework.
quoted
quoted
quoted
quoted
+ Note: Support for additional IP configurations may require
adding the
+ following clocks to this list in the future: clk_rx_125_i,
clk_tx_125_i,
+ clk_pmarx_0_i, clk_pmarx1_i, clk_rmii_i, clk_revmii_rx_i,
clk_revmii_tx_i.
+
+ The following compatible values require the following set of
clocks:
+ - "nvidia,tegra186-eqos", "snps,dwc-qos-ethernet-4.10":
+ - "slave_bus"
+ - "master_bus"
+ - "rx"
+ - "tx"
+ - "ptp_ref"
+ - "axis,artpec6-eqos", "snps,dwc-qos-ethernet-4.10":
+ - "phy_ref_clk"
+ - "apb_clk"
It would be good if this was marked deprecated and the full set of
clocks could be described and supported. Not sure if you can figure
that
out. Is it really only 2 clocks, or these have multiple connections to
the same source.
Lars, can you answer here?
I deliberately didn't attempt to change the binding definition for the
existing use-case, since I'm not familiar with that SoC, and don't
relish changing DTs for a platform I can't test.
For the artpec-6 the clocks are like this:
apb_clk: It is both the master and slave bus clock.
phy_ref_clk: It corresponds to tx clock in the proposed new binding.
There is a also a ptp reference clock that will map to the new ptp_ref
clock binding.
So the full set of clocks in a new artpec-6 binding is:
slave_bus
master_bus
tx
ptp_ref
Given the discussion above, I think we should represent the rx clock too.
It does not make sense to add rx clock for artpec-6.