| Message ID | 201308310429.34856.sergei.shtylyov@cogentembedded.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, Aug 31, 2013 at 01:29:33AM +0100, Sergei Shtylyov wrote: > Add support of the device tree probing for the Renesas SH-Mobile SoCs. > > This work is loosely based on an original patch by Nobuhiro Iwamatsu > <nobuhiro.iwamatsu.yj@renesas.com>. > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > This patch is against Dave's 'net-next.git' repo. > > Documentation/devicetree/bindings/net/sh_eth.txt | 40 +++++++++++++ > drivers/net/ethernet/renesas/sh_eth.c | 66 ++++++++++++++++++++++- > 2 files changed, 105 insertions(+), 1 deletion(-) > > Index: net-next/Documentation/devicetree/bindings/net/sh_eth.txt > =================================================================== > --- /dev/null > +++ net-next/Documentation/devicetree/bindings/net/sh_eth.txt > @@ -0,0 +1,40 @@ > +* Renesas Electronics SH EtherMAC > + > +This file provides information on what the device node for the SH EtherMAC > +interface contains. > + > +Required properties: > +- compatible: "renesas,gether-r8a7740" if the device is a part of R8A7740 SoC. > + "renesas,ether-r8a7779" if the device is a part of R8A7778/9 SoCs. > + "renesas,ether-r8a7790" if the device is a part of R8A7790/1 SoCs. What are the functional differences between the blocks in these devices that mean they have different compatible strings? > +- reg: offset and length of the register set for the device; if the device has > + TSU registers, you need to specify two register sets here. This doesn't explicitly state ordering, and doesn't describe what the first register set is (control registers?). If possible, it would be nice to refer to the set of registers by the name given in documentation; is there any available? I think we should have something like the below to ensure it's explicit. In general we need more consistency in the the way bindings describe reg properties. - reg: offset and length of: [1] the control registers of the device (required) [2] the TSU registers for the device (optional) > +- interrupt-parent: the phandle for the interrupt controller that services > + interrupts for this device. Why is that required? > +- interrupts: interrupt mapping for the interrupt source. Interrupts are defined in terms of interrupt-specifiers. How about: - interrupts: an interrupt-specifier for the sole interrupt generated by the device. > +- phy-mode: string, operation mode of the PHY interface (a string that > + of_get_phy_mode() can understand). That looks suspicious. Bindings should *not* refer to Linux internals. Instead, we should document the phy-handle and phy-mode properties and how they are meant to be used in a generic binding document (I couldn't see a generic document doing this so far...). > +- phy-handle: phandle of the PHY device. > + > +Optional properties: > +- local-mac-address: 6 bytes, MAC address. > +- renesas,no-ether-link: specify when a board does not provide a proper LINK > + signal. > +- renesas,ether-link-active-low: specify when the LINK signal is active-low. What types are these? I know local-mac-address is a byte-string by ePAPR, presumably the last two are empty (boolean)? > + > +Example (Armadillo800EVA board): > + > + ethernet@e9a00000 { > + compatible = "renesas,gether-r8a7740"; > + reg = <0xe9a00000 0x800>, <0xe9a01800 0x800>; > + interrupt-parent = <&gic>; > + interrupts = <0 142 0x4>; > + phy-mode = "mii"; > + phy-handle = <&phy0>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + phy0: ethernet-phy@0 { > + reg = <0>; > + }; The binding didn't state anything about sub-nodes. Is it a general property of phy bindings that they may be embedded within a consumer's node? > + }; > Index: net-next/drivers/net/ethernet/renesas/sh_eth.c > =================================================================== > --- net-next.orig/drivers/net/ethernet/renesas/sh_eth.c > +++ net-next/drivers/net/ethernet/renesas/sh_eth.c > @@ -32,6 +32,9 @@ > #include <linux/platform_device.h> > #include <linux/mdio-bitbang.h> > #include <linux/netdevice.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/of_net.h> > #include <linux/phy.h> > #include <linux/cache.h> > #include <linux/io.h> > @@ -2600,6 +2603,52 @@ static const struct net_device_ops sh_et > .ndo_change_mtu = eth_change_mtu, > }; > > +#ifdef CONFIG_OF > +static struct sh_eth_plat_data *sh_eth_parse_dt(struct device *dev) > +{ > + struct device_node *np = dev->of_node; > + struct sh_eth_plat_data *pdata; > + struct device_node *phy; > + const char *mac_addr; > + > + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); > + if (!pdata) > + return NULL; > + > + pdata->phy_interface = of_get_phy_mode(np); > + > + phy = of_parse_phandle(np, "phy-handle", 0); > + if (!phy || of_property_read_u32(phy, "reg", &pdata->phy)) { NAK. You didn't describe the format of the phy node, yet you are reading values from it from a logically separate driver. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Aug 30, 2013, at 7:29 PM, Sergei Shtylyov wrote: > Add support of the device tree probing for the Renesas SH-Mobile SoCs. > > This work is loosely based on an original patch by Nobuhiro Iwamatsu > <nobuhiro.iwamatsu.yj@renesas.com>. > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > This patch is against Dave's 'net-next.git' repo. > > Documentation/devicetree/bindings/net/sh_eth.txt | 40 +++++++++++++ > drivers/net/ethernet/renesas/sh_eth.c | 66 ++++++++++++++++++++++- > 2 files changed, 105 insertions(+), 1 deletion(-) > > Index: net-next/Documentation/devicetree/bindings/net/sh_eth.txt > =================================================================== > --- /dev/null > +++ net-next/Documentation/devicetree/bindings/net/sh_eth.txt > @@ -0,0 +1,40 @@ > +* Renesas Electronics SH EtherMAC > + > +This file provides information on what the device node for the SH EtherMAC > +interface contains. > + > +Required properties: > +- compatible: "renesas,gether-r8a7740" if the device is a part of R8A7740 SoC. > + "renesas,ether-r8a7779" if the device is a part of R8A7778/9 SoCs. > + "renesas,ether-r8a7790" if the device is a part of R8A7790/1 SoCs. Curious, 7779 (vs 7778) but 7790 (vs 7791). > +- reg: offset and length of the register set for the device; if the device has > + TSU registers, you need to specify two register sets here. > +- interrupt-parent: the phandle for the interrupt controller that services > + interrupts for this device. > +- interrupts: interrupt mapping for the interrupt source. > +- phy-mode: string, operation mode of the PHY interface (a string that > + of_get_phy_mode() can understand). > +- phy-handle: phandle of the PHY device. > + Should add something about requiring a phy subnode > +Optional properties: > +- local-mac-address: 6 bytes, MAC address. What's the assumption if local-mac-address is not specified? > +- renesas,no-ether-link: specify when a board does not provide a proper LINK > + signal. How is this different from the fixed link concept we have? > +- renesas,ether-link-active-low: specify when the LINK signal is active-low. > + > +Example (Armadillo800EVA board): > + > + ethernet@e9a00000 { > + compatible = "renesas,gether-r8a7740"; > + reg = <0xe9a00000 0x800>, <0xe9a01800 0x800>; > + interrupt-parent = <&gic>; > + interrupts = <0 142 0x4>; > + phy-mode = "mii"; > + phy-handle = <&phy0>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + phy0: ethernet-phy@0 { > + reg = <0>; > + }; > + }; - k
Hello. On 09/04/2013 02:08 AM, Kumar Gala wrote: >> Add support of the device tree probing for the Renesas SH-Mobile SoCs. >> This work is loosely based on an original patch by Nobuhiro Iwamatsu >> <nobuhiro.iwamatsu.yj@renesas.com>. >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> --- >> This patch is against Dave's 'net-next.git' repo. >> Documentation/devicetree/bindings/net/sh_eth.txt | 40 +++++++++++++ >> drivers/net/ethernet/renesas/sh_eth.c | 66 ++++++++++++++++++++++- >> 2 files changed, 105 insertions(+), 1 deletion(-) >> Index: net-next/Documentation/devicetree/bindings/net/sh_eth.txt >> =================================================================== >> --- /dev/null >> +++ net-next/Documentation/devicetree/bindings/net/sh_eth.txt >> @@ -0,0 +1,40 @@ >> +* Renesas Electronics SH EtherMAC >> + >> +This file provides information on what the device node for the SH EtherMAC >> +interface contains. >> + >> +Required properties: >> +- compatible: "renesas,gether-r8a7740" if the device is a part of R8A7740 SoC. >> + "renesas,ether-r8a7779" if the device is a part of R8A7778/9 SoCs. >> + "renesas,ether-r8a7790" if the device is a part of R8A7790/1 SoCs. > Curious, 7779 (vs 7778) but 7790 (vs 7791). Support for R8A7779 appeared in Linux before support for R8A7778 (probably the chips appeared on the market in the same order). The same can be said about R8A7790 -- it appeared first; R8A7791 isn't yet supported by Linux (it should be in 3.13). R8A7779/90 are high end chips, R8A7778/91 are middle class chips. Anyway, by request of Magnus Damm, I'll have to use the individual "compatible" properties. >> +- reg: offset and length of the register set for the device; if the device has >> + TSU registers, you need to specify two register sets here. >> +- interrupt-parent: the phandle for the interrupt controller that services >> + interrupts for this device. >> +- interrupts: interrupt mapping for the interrupt source. >> +- phy-mode: string, operation mode of the PHY interface (a string that >> + of_get_phy_mode() can understand). >> +- phy-handle: phandle of the PHY device. >> + > Should add something about requiring a phy subnode OK. >> +Optional properties: >> +- local-mac-address: 6 bytes, MAC address. > What's the assumption if local-mac-address is not specified? The driver will try to read the chip's MAC address registers hoping to find an address left there programmed by U-Boot. If no valid address is found (usual case on R8A7778/9 development boards which have an extra network chip), the random one will be used. >> +- renesas,no-ether-link: specify when a board does not provide a proper LINK >> + signal. > How is this different from the fixed link concept we have? Here the chip-specific LINK signal is meant which is supposed to go from a PHY to the SoC. Sometimes it's wired from the PHY's link/active LED output or just absent on the board, so we aren't able to use it for the link detection. In this case, phylib's notification is used. >> +- renesas,ether-link-active-low: specify when the LINK signal is active-low. >> + >> +Example (Armadillo800EVA board): >> + >> + ethernet@e9a00000 { >> + compatible = "renesas,gether-r8a7740"; >> + reg = <0xe9a00000 0x800>, <0xe9a01800 0x800>; >> + interrupt-parent = <&gic>; >> + interrupts = <0 142 0x4>; >> + phy-mode = "mii"; >> + phy-handle = <&phy0>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + phy0: ethernet-phy@0 { >> + reg = <0>; >> + }; >> + }; > - k WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello.
On 09/02/2013 12:52 PM, Mark Rutland wrote:
Don't know why I haven't noticed your mail before...
>> Add support of the device tree probing for the Renesas SH-Mobile SoCs.
>> This work is loosely based on an original patch by Nobuhiro Iwamatsu
>> <nobuhiro.iwamatsu.yj@renesas.com>.
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>> ---
>> This patch is against Dave's 'net-next.git' repo.
>> Documentation/devicetree/bindings/net/sh_eth.txt | 40 +++++++++++++
>> drivers/net/ethernet/renesas/sh_eth.c | 66 ++++++++++++++++++++++-
>> 2 files changed, 105 insertions(+), 1 deletion(-)
>> Index: net-next/Documentation/devicetree/bindings/net/sh_eth.txt
>> ===================================================================
>> --- /dev/null
>> +++ net-next/Documentation/devicetree/bindings/net/sh_eth.txt
>> @@ -0,0 +1,40 @@
>> +* Renesas Electronics SH EtherMAC
>> +
>> +This file provides information on what the device node for the SH EtherMAC
>> +interface contains.
>> +
>> +Required properties:
>> +- compatible: "renesas,gether-r8a7740" if the device is a part of R8A7740 SoC.
>> + "renesas,ether-r8a7779" if the device is a part of R8A7778/9 SoCs.
>> + "renesas,ether-r8a7790" if the device is a part of R8A7790/1 SoCs.
> What are the functional differences between the blocks in these devices
> that mean they have different compatible strings?
RA87740 has Gigabit Ether (GEther) with TSU block (I don't know what TSU
stands for) and the register layout completely different from the other SoCs.
R8A777x and R8A7790 are 100 Mbit/s Ether without TSU block differing in some
minor but vital register details between them; they use so called "R-Car"
register layout contrasted to the "Gigabit" layout of R8A7740.
>> +- reg: offset and length of the register set for the device; if the device has
>> + TSU registers, you need to specify two register sets here.
> This doesn't explicitly state ordering, and doesn't describe what the
> first register set is (control registers?). If possible, it would be
> nice to refer to the set of registers by the name given in
> documentation; is there any available?
No, there's not common name (and I only have R8A777x and R8A7790 docs).
There are two subranges for those SoCs: HDMAC and feLic registers at offsets
0x200 and 0x300 from the "reg" prop start.
> I think we should have something like the below to ensure it's explicit.
> In general we need more consistency in the the way bindings describe reg
> properties.
> - reg: offset and length of:
> [1] the control registers of the device (required)
> [2] the TSU registers for the device (optional)
OK.
>> +- interrupt-parent: the phandle for the interrupt controller that services
>> + interrupts for this device.
> Why is that required?
I'm not sure, maybe it's not.
>> +- interrupts: interrupt mapping for the interrupt source.
> Interrupts are defined in terms of interrupt-specifiers. How about:
> - interrupts: an interrupt-specifier for the sole interrupt
> generated by the device.
OK.
>> +- phy-mode: string, operation mode of the PHY interface (a string that
>> + of_get_phy_mode() can understand).
> That looks suspicious. Bindings should *not* refer to Linux internals.
> Instead, we should document the phy-handle and phy-mode properties and
There's also "phy" variant in use now in addition to the older
"phy-handle". Sigh...
> how they are meant to be used in a generic binding document (I couldn't
> see a generic document doing this so far...).
So you want me to create one?
>> +- phy-handle: phandle of the PHY device.
>> +
>> +Optional properties:
>> +- local-mac-address: 6 bytes, MAC address.
>> +- renesas,no-ether-link: specify when a board does not provide a proper LINK
>> + signal.
>> +- renesas,ether-link-active-low: specify when the LINK signal is active-low.
> What types are these? I know local-mac-address is a byte-string by
> ePAPR, presumably the last two are empty (boolean)?
Exactly.
>> +
>> +Example (Armadillo800EVA board):
>> +
>> + ethernet@e9a00000 {
>> + compatible = "renesas,gether-r8a7740";
>> + reg = <0xe9a00000 0x800>, <0xe9a01800 0x800>;
>> + interrupt-parent = <&gic>;
>> + interrupts = <0 142 0x4>;
>> + phy-mode = "mii";
>> + phy-handle = <&phy0>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + phy0: ethernet-phy@0 {
>> + reg = <0>;
>> + };
> The binding didn't state anything about sub-nodes. Is it a general
> property of phy bindings that they may be embedded within a consumer's
> node?
Considering Ethernet PHY subnodes, yes. They hang off MDIO bus controlled
by the Ethernet MAC registers.
>> + };
>> Index: net-next/drivers/net/ethernet/renesas/sh_eth.c
>> ===================================================================
>> --- net-next.orig/drivers/net/ethernet/renesas/sh_eth.c
>> +++ net-next/drivers/net/ethernet/renesas/sh_eth.c
[...]
>> @@ -2600,6 +2603,52 @@ static const struct net_device_ops sh_et
>> .ndo_change_mtu = eth_change_mtu,
>> };
>>
>> +#ifdef CONFIG_OF
>> +static struct sh_eth_plat_data *sh_eth_parse_dt(struct device *dev)
>> +{
>> + struct device_node *np = dev->of_node;
>> + struct sh_eth_plat_data *pdata;
>> + struct device_node *phy;
>> + const char *mac_addr;
>> +
>> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> + if (!pdata)
>> + return NULL;
>> +
>> + pdata->phy_interface = of_get_phy_mode(np);
>> +
>> + phy = of_parse_phandle(np, "phy-handle", 0);
>> + if (!phy || of_property_read_u32(phy, "reg", &pdata->phy)) {
> NAK. You didn't describe the format of the phy node, yet you are reading
> values from it from a logically separate driver.
See Documentation/devicetree/bindings/net/phy.txt. Although I really
didn't follow it closely (and it looks pretty obsolete, requiring
"device_type" and "linux,phandle" props).
> Thanks,
> Mark.
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: net-next/Documentation/devicetree/bindings/net/sh_eth.txt =================================================================== --- /dev/null +++ net-next/Documentation/devicetree/bindings/net/sh_eth.txt @@ -0,0 +1,40 @@ +* Renesas Electronics SH EtherMAC + +This file provides information on what the device node for the SH EtherMAC +interface contains. + +Required properties: +- compatible: "renesas,gether-r8a7740" if the device is a part of R8A7740 SoC. + "renesas,ether-r8a7779" if the device is a part of R8A7778/9 SoCs. + "renesas,ether-r8a7790" if the device is a part of R8A7790/1 SoCs. +- reg: offset and length of the register set for the device; if the device has + TSU registers, you need to specify two register sets here. +- interrupt-parent: the phandle for the interrupt controller that services + interrupts for this device. +- interrupts: interrupt mapping for the interrupt source. +- phy-mode: string, operation mode of the PHY interface (a string that + of_get_phy_mode() can understand). +- phy-handle: phandle of the PHY device. + +Optional properties: +- local-mac-address: 6 bytes, MAC address. +- renesas,no-ether-link: specify when a board does not provide a proper LINK + signal. +- renesas,ether-link-active-low: specify when the LINK signal is active-low. + +Example (Armadillo800EVA board): + + ethernet@e9a00000 { + compatible = "renesas,gether-r8a7740"; + reg = <0xe9a00000 0x800>, <0xe9a01800 0x800>; + interrupt-parent = <&gic>; + interrupts = <0 142 0x4>; + phy-mode = "mii"; + phy-handle = <&phy0>; + #address-cells = <1>; + #size-cells = <0>; + + phy0: ethernet-phy@0 { + reg = <0>; + }; + }; Index: net-next/drivers/net/ethernet/renesas/sh_eth.c =================================================================== --- net-next.orig/drivers/net/ethernet/renesas/sh_eth.c +++ net-next/drivers/net/ethernet/renesas/sh_eth.c @@ -32,6 +32,9 @@ #include <linux/platform_device.h> #include <linux/mdio-bitbang.h> #include <linux/netdevice.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_net.h> #include <linux/phy.h> #include <linux/cache.h> #include <linux/io.h> @@ -2600,6 +2603,52 @@ static const struct net_device_ops sh_et .ndo_change_mtu = eth_change_mtu, }; +#ifdef CONFIG_OF +static struct sh_eth_plat_data *sh_eth_parse_dt(struct device *dev) +{ + struct device_node *np = dev->of_node; + struct sh_eth_plat_data *pdata; + struct device_node *phy; + const char *mac_addr; + + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); + if (!pdata) + return NULL; + + pdata->phy_interface = of_get_phy_mode(np); + + phy = of_parse_phandle(np, "phy-handle", 0); + if (!phy || of_property_read_u32(phy, "reg", &pdata->phy)) { + devm_kfree(dev, pdata); + return NULL; + } + + mac_addr = of_get_mac_address(np); + if (mac_addr) + memcpy(pdata->mac_addr, mac_addr, ETH_ALEN); + + pdata->no_ether_link = + of_property_read_bool(np, "renesas,no-ether-link"); + pdata->ether_link_active_low = + of_property_read_bool(np, "renesas,ether-link-active-low"); + + return pdata; +} + +static const struct of_device_id sh_eth_match_table[] = { + { .compatible = "renesas,gether-r8a7740", .data = &r8a7740_data }, + { .compatible = "renesas,ether-r8a7779", .data = &r8a777x_data }, + { .compatible = "renesas,ether-r8a7790", .data = &r8a7790_data }, + { } +}; +MODULE_DEVICE_TABLE(of, sh_eth_match_table); +#else +static inline struct sh_eth_plat_data *sh_eth_parse_dt(struct device *dev) +{ + return NULL; +} +#endif + static int sh_eth_drv_probe(struct platform_device *pdev) { int ret, devno = 0; @@ -2653,6 +2702,8 @@ static int sh_eth_drv_probe(struct platf pm_runtime_enable(&pdev->dev); pm_runtime_resume(&pdev->dev); + if (pdev->dev.of_node) + pd = sh_eth_parse_dt(&pdev->dev); if (!pd) { dev_err(&pdev->dev, "no platform data\n"); ret = -EINVAL; @@ -2668,7 +2719,19 @@ static int sh_eth_drv_probe(struct platf mdp->ether_link_active_low = pd->ether_link_active_low; /* set cpu data */ - mdp->cd = (struct sh_eth_cpu_data *)id->driver_data; + if (id) { + mdp->cd = (struct sh_eth_cpu_data *)id->driver_data; + } else { + const struct of_device_id *match; + + match = of_match_device(of_match_ptr(sh_eth_match_table), + &pdev->dev); + if (!match) { + ret = -EINVAL; + goto out_release; + } + mdp->cd = (struct sh_eth_cpu_data *)match->data; + } mdp->reg_offset = sh_eth_get_register_offset(mdp->cd->register_type); sh_eth_set_default_cpu_data(mdp->cd); @@ -2809,6 +2872,7 @@ static struct platform_driver sh_eth_dri .driver = { .name = CARDNAME, .pm = SH_ETH_PM_OPS, + .of_match_table = of_match_ptr(sh_eth_match_table), }, };
Add support of the device tree probing for the Renesas SH-Mobile SoCs. This work is loosely based on an original patch by Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>. Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- This patch is against Dave's 'net-next.git' repo. Documentation/devicetree/bindings/net/sh_eth.txt | 40 +++++++++++++ drivers/net/ethernet/renesas/sh_eth.c | 66 ++++++++++++++++++++++- 2 files changed, 105 insertions(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html