diff mbox series

[RFC,02/11] dt-bindings: phy: add cpsw port interface mode selection phy bindings

Message ID 20181008234949.15416-3-grygorii.strashko@ti.com
State Changes Requested, archived
Headers show
Series net: ethernet: ti: cpsw: replace cpsw-phy-sel with phy driver | expand

Commit Message

Grygorii Strashko Oct. 8, 2018, 11:49 p.m. UTC 
Add CPSW Port's Interface Mode Selection PHY (phy-gmii-sel) DT Bindings

Cc: Kishon Vijay Abraham I <kishon@ti.com>
Cc: Tony Lindgren <tony@atomide.com>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 .../devicetree/bindings/phy/ti-phy-gmii-sel.txt    | 68 ++++++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/ti-phy-gmii-sel.txt

Comments

Tony Lindgren Oct. 9, 2018, 2:40 p.m. UTC | #1 
* Grygorii Strashko <grygorii.strashko@ti.com> [181008 23:54]:
> +Examples:
> +	phy_gmii_sel: phy-gmii-sel {
> +		compatible = "ti,am3352-phy-gmii-sel";
> +		syscon-scm = <&scm_conf>;
> +		#phy-cells = <2>;
> +	};

Now that this driver can live in it's proper place in the
dts, you may want to consider just using standard reg
property for it instead of the syscon-scm. And also get
rid of the syscon reads and writes.

Regards,

Tony
Grygorii Strashko Oct. 9, 2018, 8:10 p.m. UTC | #2 
On 10/09/2018 09:40 AM, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko@ti.com> [181008 23:54]:
>> +Examples:
>> +	phy_gmii_sel: phy-gmii-sel {
>> +		compatible = "ti,am3352-phy-gmii-sel";
>> +		syscon-scm = <&scm_conf>;
>> +		#phy-cells = <2>;
>> +	};
> 
> Now that this driver can live in it's proper place in the

right

> dts, you may want to consider just using standard reg
> property for it instead of the syscon-scm. And also get
> rid of the syscon reads and writes.

Could you help clarify how to get syscon in this case?
syscon_node_to_regmap(dev->parent->of_node)?

Also, there are could be more then one gmii_sel registers in SCM in the future,
so I hidden offsets in of_match data. 
As result, "reg" not needed at all now.
Tony Lindgren Oct. 9, 2018, 8:30 p.m. UTC | #3 
* Grygorii Strashko <grygorii.strashko@ti.com> [181009 20:10]:
> 
> 
> On 10/09/2018 09:40 AM, Tony Lindgren wrote:
> > * Grygorii Strashko <grygorii.strashko@ti.com> [181008 23:54]:
> >> +Examples:
> >> +	phy_gmii_sel: phy-gmii-sel {
> >> +		compatible = "ti,am3352-phy-gmii-sel";
> >> +		syscon-scm = <&scm_conf>;
> >> +		#phy-cells = <2>;
> >> +	};
> > 
> > Now that this driver can live in it's proper place in the
> 
> right
> 
> > dts, you may want to consider just using standard reg
> > property for it instead of the syscon-scm. And also get
> > rid of the syscon reads and writes.
> 
> Could you help clarify how to get syscon in this case?
> syscon_node_to_regmap(dev->parent->of_node)?

Hmm I don't think you need syscon at all now. You can just
ioremap the register(s) and use readl/writel and that's it.
Or use regmap without syscon if you prefer that.

The ioremap in this case should be hitting cached ranges
anyways, so no extra overhead there.

> Also, there are could be more then one gmii_sel registers in SCM in the future,
> so I hidden offsets in of_match data. 
> As result, "reg" not needed at all now.

But then you have to patch driver for various SoCs
instead of just configuring the standard reg property
in the dts file :)

Regards,

Tony
Grygorii Strashko Oct. 9, 2018, 10:04 p.m. UTC | #4 
On 10/09/2018 03:30 PM, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko@ti.com> [181009 20:10]:
>>
>>
>> On 10/09/2018 09:40 AM, Tony Lindgren wrote:
>>> * Grygorii Strashko <grygorii.strashko@ti.com> [181008 23:54]:
>>>> +Examples:
>>>> +	phy_gmii_sel: phy-gmii-sel {
>>>> +		compatible = "ti,am3352-phy-gmii-sel";
>>>> +		syscon-scm = <&scm_conf>;
>>>> +		#phy-cells = <2>;
>>>> +	};
>>>
>>> Now that this driver can live in it's proper place in the
>>
>> right
>>
>>> dts, you may want to consider just using standard reg
>>> property for it instead of the syscon-scm. And also get
>>> rid of the syscon reads and writes.
>>
>> Could you help clarify how to get syscon in this case?
>> syscon_node_to_regmap(dev->parent->of_node)?
> 
> Hmm I don't think you need syscon at all now. You can just
> ioremap the register(s) and use readl/writel and that's it.
> Or use regmap without syscon if you prefer that.

It will overlap with already remapped SCM syscon and i'd like to avoid this.
+ it seems common practice to use syscon for devices/drivers which are
child to SCM node - makes overall system more consistent.

> 
> The ioremap in this case should be hitting cached ranges
> anyways, so no extra overhead there.
> 
>> Also, there are could be more then one gmii_sel registers in SCM in the future,
>> so I hidden offsets in of_match data.
>> As result, "reg" not needed at all now.
> 
> But then you have to patch driver for various SoCs
> instead of just configuring the standard reg property
> in the dts file :)

Problem is that they are not guarantee to be standard between SoC's families 
(number of regs and fields placement), as result it might require to change
driver any way for various SoCs to handle properly new fields placement.

I prefer to fix driver then fight with DT ;) as it's static for SoC family
and need to be changed only once when new SoC family introduced.
Tony Lindgren Oct. 9, 2018, 10:07 p.m. UTC | #5 
* Grygorii Strashko <grygorii.strashko@ti.com> [181009 22:04]:
> 
> 
> On 10/09/2018 03:30 PM, Tony Lindgren wrote:
> > * Grygorii Strashko <grygorii.strashko@ti.com> [181009 20:10]:
> >>
> >>
> >> On 10/09/2018 09:40 AM, Tony Lindgren wrote:
> >>> * Grygorii Strashko <grygorii.strashko@ti.com> [181008 23:54]:
> >>>> +Examples:
> >>>> +	phy_gmii_sel: phy-gmii-sel {
> >>>> +		compatible = "ti,am3352-phy-gmii-sel";
> >>>> +		syscon-scm = <&scm_conf>;
> >>>> +		#phy-cells = <2>;
> >>>> +	};
> >>>
> >>> Now that this driver can live in it's proper place in the
> >>
> >> right
> >>
> >>> dts, you may want to consider just using standard reg
> >>> property for it instead of the syscon-scm. And also get
> >>> rid of the syscon reads and writes.
> >>
> >> Could you help clarify how to get syscon in this case?
> >> syscon_node_to_regmap(dev->parent->of_node)?
> > 
> > Hmm I don't think you need syscon at all now. You can just
> > ioremap the register(s) and use readl/writel and that's it.
> > Or use regmap without syscon if you prefer that.
> 
> It will overlap with already remapped SCM syscon and i'd like to avoid this.
> + it seems common practice to use syscon for devices/drivers which are
> child to SCM node - makes overall system more consistent.

Well it was just set up with syscon in deperation earlier with
drivers just blindly mapping registers outside of their
range..

> > The ioremap in this case should be hitting cached ranges
> > anyways, so no extra overhead there.
> > 
> >> Also, there are could be more then one gmii_sel registers in SCM in the future,
> >> so I hidden offsets in of_match data.
> >> As result, "reg" not needed at all now.
> > 
> > But then you have to patch driver for various SoCs
> > instead of just configuring the standard reg property
> > in the dts file :)
> 
> Problem is that they are not guarantee to be standard between SoC's families 
> (number of regs and fields placement), as result it might require to change
> driver any way for various SoCs to handle properly new fields placement.
> 
> I prefer to fix driver then fight with DT ;) as it's static for SoC family
> and need to be changed only once when new SoC family introduced.

Fine with me, that can be changed later too no problem.

Regards,

Tony
Rob Herring Oct. 17, 2018, 3:39 p.m. UTC | #6 
On Tue, Oct 09, 2018 at 03:10:34PM -0500, Grygorii Strashko wrote:
> 
> 
> On 10/09/2018 09:40 AM, Tony Lindgren wrote:
> > * Grygorii Strashko <grygorii.strashko@ti.com> [181008 23:54]:
> >> +Examples:
> >> +	phy_gmii_sel: phy-gmii-sel {
> >> +		compatible = "ti,am3352-phy-gmii-sel";
> >> +		syscon-scm = <&scm_conf>;
> >> +		#phy-cells = <2>;
> >> +	};
> > 
> > Now that this driver can live in it's proper place in the
> 
> right
> 
> > dts, you may want to consider just using standard reg
> > property for it instead of the syscon-scm. And also get
> > rid of the syscon reads and writes.
> 
> Could you help clarify how to get syscon in this case?
> syscon_node_to_regmap(dev->parent->of_node)?
> 
> Also, there are could be more then one gmii_sel registers in SCM in the future,
> so I hidden offsets in of_match data. 
> As result, "reg" not needed at all now.

If there's a defined register range which doesn't overlap with other 
things (other than the parent), then use reg whether you currently need 
it or not.

Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/phy/ti-phy-gmii-sel.txt b/Documentation/devicetree/bindings/phy/ti-phy-gmii-sel.txt
new file mode 100644
index 0000000..fd81421
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/ti-phy-gmii-sel.txt
@@ -0,0 +1,68 @@ 
+CPSW Port's Interface Mode Selection PHY Tree Bindings
+-----------------------------------------------
+
+TI am335x/am437x/dra7(am5)/dm814x CPSW3G Ethernet Subsystem supports
+two 10/100/1000 Ethernet ports with selectable G/MII, RMII, and RGMII interfaces.
+The interface mode is selected by configuring the MII mode selection register(s)
+(GMII_SEL) in the System Control Module chapter (SCM). GMII_SEL register(s) and
+bit fields placement in SCM are different between SoCs while fields meaning
+is the same.
+                                               +--------------+
+        +-------------------------------+      |SCM           |
+        |                     CPSW      |      |  +---------+ |
+        |        +--------------------------------+gmii_sel | |
+        |        |                      |      |  +---------+ |
+        |   +----v---+     +--------+   |      +--------------+
+        |   |Port 1..<--+-->GMII/MII<------->
+        |   |        |  |  |        |   |
+        |   +--------+  |  +--------+   |
+        |               |               |
+        |               |  +--------+   |
+        |               |  | RMII   <------->
+        |               +-->        |   |
+        |               |  +--------+   |
+        |               |               |
+        |               |  +--------+   |
+        |               |  | RGMII  <------->
+        |               +-->        |   |
+        |                  +--------+   |
+        +-------------------------------+
+
+CPSW Port's Interface Mode Selection PHY describes MII interface mode between
+CPSW Port and Ethernet PHY which depends on Eth PHY and board configuration.
+
+CPSW Port's Interface Mode Selection PHY device should defined as child device
+of SCM node (scm_conf) and can be attached to each CPSW port node using standard
+PHY bindings (See phy/phy-bindings.txt).
+
+Required properties:
+- compatible		: Should be "ti,am3352-phy-gmii-sel" for am335x platform
+			  "ti,dra7xx-phy-gmii-sel" for dra7xx/am57xx platform
+			  "ti,am43xx-phy-gmii-sel" for am43xx platform
+			  "ti,dm814-phy-gmii-sel" for dm814x platform
+- #phy-cells		: must be 2.
+			  cell 1 - CPSW port number (starting from 1)
+			  cell 2 - RMII refclk mode
+- syscon-scm		: phandle on SCM node (mfd/syscon.txt)
+
+Examples:
+	phy_gmii_sel: phy-gmii-sel {
+		compatible = "ti,am3352-phy-gmii-sel";
+		syscon-scm = <&scm_conf>;
+		#phy-cells = <2>;
+	};
+
+	mac: ethernet@4a100000 {
+		compatible = "ti,am335x-cpsw","ti,cpsw";
+		...
+
+		cpsw_emac0: slave@4a100200 {
+			...
+			phys = <&phy_gmii_sel 1 1>;
+		};
+
+		cpsw_emac1: slave@4a100300 {
+			...
+			phys = <&phy_gmii_sel 2 1>;
+		};
+	};