| Message ID | 20160824212046.2416-1-swarren@wwwdotorg.org |
|---|---|
| State | Deferred |
| Headers | show |
On Wed, Aug 24, 2016 at 03:20:46PM -0600, Stephen Warren wrote: > From: Stephen Warren <swarren@nvidia.com> > > 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. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > v2: > * Add an explicit compatible value for the Axis SoC's version of the EQOS > IP; this allows the driver to handle any SoC-specific integration quirks > that are required, rather than only knowing about the IP block in > isolation. This is good general DT practice. The existing value is still > documented to support existing DTs. > * Reworked the list of clocks the binding requires: > - Combined "tx" and "phy_ref_clk"; for GMII/RGMII configurations, these > are the same thing. > - Added extra description to the "rx" and "tx" clocks, to make it clear > exactly which HW clock they represent. > - Made the new "tx" and "slave_bus" names more prominent than the > original "phy_ref_clk" and "apb_pclk". The new names are more generic > and should work for any enhanced version of the binding (e.g. to > support additional PHY types). New compatible values will hopefully > choose to require the new names. > * Added a couple extra clocks to the list that may need to be supported in > future binding revisions. > * Fixed a typo; "clocks" -> "resets". > --- > .../bindings/net/snps,dwc-qos-ethernet.txt | 75 ++++++++++++++++++++-- > 1 file changed, 71 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt b/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt > index 51f8d2eba8d8..1d028259824a 100644 > --- a/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt > +++ b/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt > @@ -1,21 +1,87 @@ > * Synopsys DWC Ethernet QoS IP version 4.10 driver (GMAC) > > +This binding supports the Synopsys Designware Ethernet QoS (Quality Of Service) > +IP block. The IP supports multiple options for bus type, clocking and reset > +structure, and feature list. Consequently, a number of properties and list > +entries in properties are marked as optional, or only required in specific HW > +configurations. > > Required properties: > -- compatible: Should be "snps,dwc-qos-ethernet-4.10" > +- compatible: One of: > + - "axis,artpec6-eqos", "snps,dwc-qos-ethernet-4.10" > + Represents the IP core when integrated into the Axis ARTPEC-6 SoC. > + - "nvidia,tegra186-eqos", "snps,dwc-qos-ethernet-4.10" > + Represents the IP core when integrated into the NVIDIA Tegra186 SoC. > + - "snps,dwc-qos-ethernet-4.10" > + This combination is deprecated. It should be treated as equivalent to > + "axis,artpec6-eqos", "snps,dwc-qos-ethernet-4.10". It is supported to be > + compatible with earlier revisions of this binding. > - reg: Address and length of the register set for the device > -- 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. > + - "tx" > + (Alternate name "phy_ref_clk"; only one alternate must appear.) Obviously, the prior clocks were just made up for what someone needed at the time and did not read the spec. I think it would be better to just separate the old names and state they are deprecated and which compatibles they are for. > + 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. > + - "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. > + - "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? > + - "ptp_ref" > + The PTP reference clock. The HW signal name is clk_ptp_ref_i. > + > + 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. > - interrupt-parent: Should be the phandle for the interrupt controller > that services interrupts for this device > - interrupts: Should contain the core's combined interrupt signal > - phy-mode: See ethernet.txt file in the same directory > +- resets: Phandle and reset specifiers for each entry in reset-names, in the > + same order. See ../reset/reset.txt. > +- reset-names: May contain any/all of the following depending on the IP > + configuration, in any order: > + - "eqos". The reset to the entire module. The HW signal name is hreset_n > + (AHB) or aresetn_i (AXI). > + > + The following compatible values require the following set of resets: > + (the reset properties may be omitted if empty) > + - "nvidia,tegra186-eqos", "snps,dwc-qos-ethernet-4.10": > + - "eqos". > + - "axis,artpec6-eqos", "snps,dwc-qos-ethernet-4.10": > + - None. > > Optional properties: > - dma-coherent: Present if dma operations are coherent > - mac-address: See ethernet.txt in the same directory > - local-mac-address: See ethernet.txt in the same directory > +- phy-reset-gpios: Phandle and specifier for any GPIO used to reset the PHY. > + See ../gpio/gpio.txt. > - snps,en-lpi: If present it enables use of the AXI low-power interface > - snps,write-requests: Number of write requests that the AXI port can issue. > It depends on the SoC configuration. > @@ -52,6 +118,7 @@ ethernet2@40010000 { > reg = <0x40010000 0x4000>; > phy-handle = <&phy2>; > phy-mode = "gmii"; > + phy-reset-gpios = <&gpioctlr 43 GPIO_ACTIVE_LOW>; > > snps,en-tx-lpi-clockgating; > snps,en-lpi; > -- > 2.9.3 > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/30/2016 01:01 PM, Rob Herring wrote: > On Wed, Aug 24, 2016 at 03:20:46PM -0600, Stephen Warren wrote: >> From: Stephen Warren <swarren@nvidia.com> >> >> 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. >> diff --git a/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt b/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt >> Required properties: >> -- 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. ... >> + 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. >> + - "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. > >> + - "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. >> + 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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/30/2016 10:50 PM, Stephen Warren wrote: > On 08/30/2016 01:01 PM, Rob Herring wrote: >> On Wed, Aug 24, 2016 at 03:20:46PM -0600, Stephen Warren wrote: >>> From: Stephen Warren <swarren@nvidia.com> >>> >>> 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. > >>> diff --git >>> a/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt >>> b/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt > >>> Required properties: > >>> -- 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. > > ... >>> + 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 ? >>> + - "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. >> >>> + - "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. >>> + 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 linux-tegra" in the body of a message to majordomo@vger.kernel.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: >> On 08/30/2016 01:01 PM, Rob Herring wrote: >>> On Wed, Aug 24, 2016 at 03:20:46PM -0600, Stephen Warren wrote: >>>> 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. >>>> diff --git >>>> a/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt >>>> b/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt >>>> +- 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. >>>> + 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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/31/2016 11:48 PM, Stephen Warren wrote: > On 08/31/2016 03:15 AM, Lars Persson wrote: >> On 08/30/2016 10:50 PM, Stephen Warren wrote: >>> On 08/30/2016 01:01 PM, Rob Herring wrote: >>>> On Wed, Aug 24, 2016 at 03:20:46PM -0600, Stephen Warren wrote: >>>>> 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. > >>>>> diff --git >>>>> a/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt >>>>> b/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt > >>>>> +- 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. > 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. >>>>> + 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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/01/2016 01:50 AM, Lars Persson wrote: > > > On 08/31/2016 11:48 PM, Stephen Warren wrote: >> On 08/31/2016 03:15 AM, Lars Persson wrote: >>> On 08/30/2016 10:50 PM, Stephen Warren wrote: >>>> On 08/30/2016 01:01 PM, Rob Herring wrote: >>>>> On Wed, Aug 24, 2016 at 03:20:46PM -0600, Stephen Warren wrote: >>>>>> 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. >> >>>>>> diff --git >>>>>> a/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt >>>>>> b/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt >> >>>>>> +- 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. >> > > 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. Ah, I've found the source of confusion. We don't have an SoC-level clock that feeds both the MAC and the PHY RX logic, but simply a clock gate that sits between the RX clock output of the PHY and the RX clock input of the MAC (clk_rx_i). This gate is what's currently represented by the "rx" clock name the binding mentions. tl;dr is that yes I agree to update the description of this rx clock along the lines you ask for. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt b/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt index 51f8d2eba8d8..1d028259824a 100644 --- a/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt +++ b/Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt @@ -1,21 +1,87 @@ * Synopsys DWC Ethernet QoS IP version 4.10 driver (GMAC) +This binding supports the Synopsys Designware Ethernet QoS (Quality Of Service) +IP block. The IP supports multiple options for bus type, clocking and reset +structure, and feature list. Consequently, a number of properties and list +entries in properties are marked as optional, or only required in specific HW +configurations. Required properties: -- compatible: Should be "snps,dwc-qos-ethernet-4.10" +- compatible: One of: + - "axis,artpec6-eqos", "snps,dwc-qos-ethernet-4.10" + Represents the IP core when integrated into the Axis ARTPEC-6 SoC. + - "nvidia,tegra186-eqos", "snps,dwc-qos-ethernet-4.10" + Represents the IP core when integrated into the NVIDIA Tegra186 SoC. + - "snps,dwc-qos-ethernet-4.10" + This combination is deprecated. It should be treated as equivalent to + "axis,artpec6-eqos", "snps,dwc-qos-ethernet-4.10". It is supported to be + compatible with earlier revisions of this binding. - reg: Address and length of the register set for the device -- 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: + - "tx" + (Alternate name "phy_ref_clk"; only one alternate must appear.) + 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. + - "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). + - "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). + - "ptp_ref" + The PTP reference clock. The HW signal name is clk_ptp_ref_i. + + 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" - interrupt-parent: Should be the phandle for the interrupt controller that services interrupts for this device - interrupts: Should contain the core's combined interrupt signal - phy-mode: See ethernet.txt file in the same directory +- resets: Phandle and reset specifiers for each entry in reset-names, in the + same order. See ../reset/reset.txt. +- reset-names: May contain any/all of the following depending on the IP + configuration, in any order: + - "eqos". The reset to the entire module. The HW signal name is hreset_n + (AHB) or aresetn_i (AXI). + + The following compatible values require the following set of resets: + (the reset properties may be omitted if empty) + - "nvidia,tegra186-eqos", "snps,dwc-qos-ethernet-4.10": + - "eqos". + - "axis,artpec6-eqos", "snps,dwc-qos-ethernet-4.10": + - None. Optional properties: - dma-coherent: Present if dma operations are coherent - mac-address: See ethernet.txt in the same directory - local-mac-address: See ethernet.txt in the same directory +- phy-reset-gpios: Phandle and specifier for any GPIO used to reset the PHY. + See ../gpio/gpio.txt. - snps,en-lpi: If present it enables use of the AXI low-power interface - snps,write-requests: Number of write requests that the AXI port can issue. It depends on the SoC configuration. @@ -52,6 +118,7 @@ ethernet2@40010000 { reg = <0x40010000 0x4000>; phy-handle = <&phy2>; phy-mode = "gmii"; + phy-reset-gpios = <&gpioctlr 43 GPIO_ACTIVE_LOW>; snps,en-tx-lpi-clockgating; snps,en-lpi;