Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1373698 > unrolled thread

Re: [PATCH V3] net: emac: emac gigabit ethernet controller driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V3] net: emac: emac gigabit ethernet controller driver Timur Tabi <timur@codeaurora.org> - 2016-04-07 21:30 +0200
    Re: [PATCH V3] net: emac: emac gigabit ethernet controller driver Andrew Lunn <andrew@lunn.ch> - 2016-04-07 22:20 +0200
      Re: [PATCH V3] net: emac: emac gigabit ethernet controller driver Timur Tabi <timur@codeaurora.org> - 2016-04-07 23:50 +0200
        Re: [PATCH V3] net: emac: emac gigabit ethernet controller driver Andrew Lunn <andrew@lunn.ch> - 2016-04-08 03:00 +0200
          Re: [PATCH V3] net: emac: emac gigabit ethernet controller driver Timur Tabi <timur@codeaurora.org> - 2016-04-08 21:10 +0200
            Re: [PATCH V3] net: emac: emac gigabit ethernet controller driver Vikram Sethi <vikrams@codeaurora.org> - 2016-04-08 23:20 +0200
              Re: [PATCH V3] net: emac: emac gigabit ethernet controller driver Timur Tabi <timur@codeaurora.org> - 2016-04-08 23:50 +0200
            Re: [PATCH V3] net: emac: emac gigabit ethernet controller driver Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-04-09 00:50 +0200
              Re: [PATCH V3] net: emac: emac gigabit ethernet controller driver Timur Tabi <timur@codeaurora.org> - 2016-04-09 01:10 +0200
                Re: [PATCH V3] net: emac: emac gigabit ethernet controller driver Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-04-09 01:30 +0200

#1373698 — Re: [PATCH V3] net: emac: emac gigabit ethernet controller driver

Rob Herring wrote:

>>>> >>>+- reg : Offset and length of the register regions for the device
>>>> >>>+- reg-names : Register region names referenced in 'reg' above.
>>>> >>>+       Required register resource entries are:
>>>> >>>+       "base"   : EMAC controller base register block.
>>>> >>>+       "csr"    : EMAC wrapper register block.
>>>> >>>+       Optional register resource entries are:
>>>> >>>+       "ptp"    : EMAC PTP (1588) register block.
>>>> >>>+                  Required if 'qcom,emac-tstamp-en' is present.
>>>> >>>+       "sgmii"  : EMAC SGMII PHY register block.
>>>> >>>+- interrupts : Interrupt numbers used by this controller
>>>> >>>+- interrupt-names : Interrupt resource names referenced in 'interrupts'
>>>> >>>above.
>>>> >>>+       Required interrupt resource entries are:
>>>> >>>+       "emac_core0"   : EMAC core0 interrupt.
>>>> >>>+       "sgmii_irq"   : EMAC SGMII interrupt.
>>>> >>>+- qcom,emac-gpio-mdc  : GPIO pin number of the MDC line of MDIO bus.
>>>> >>>+- qcom,emac-gpio-mdio : GPIO pin number of the MDIO line of MDIO bus.
>>> >>
>>> >>
>>> >>Use the standard binding for GPIO controlled MDIO bus.
>> >
>> >
>> >I'm not familiar with that one.  Are you talking about
>> >bindings/net/mdio-gpio.txt?

> Yes.
>
>
>>>> >>>+- phy-addr            : Specifies phy address on MDIO bus.
>>>> >>>+                       Required if the optional property
>>>> >>>"qcom,no-external-phy"
>>>> >>>+                       is not specified.
>>> >>
>>> >>
>>> >>Don't you think you will need to know the specific phy device or other
>>> >>properties of the phy?
>> >
>> >
>> >That, I can't answer.  Aren't all MDIO devices basically the same?  It's
>> >been a while since I've worked on them.

> No. There was some discussion just this week about needing to require
> phy devices to have compatible strings.

I'm back to working on this driver, and I need some more help with how 
to handle the phy.  mdio-gpio.txt doesn't really tell me much. I'm 
actually working on an ACPI system and not DT.  I don't want to strip 
out the DT code, but I can't really test it.  I want to keep changes to 
Gilad's patch to a minimum.

Part of the problem with the Emac (and Gilad tried to explain this) is 
that it has an internal phy.  Technically, you can connect this internal 
phy directly to another internal phy on another SOC, and use this as an 
SOC interconnect.  However, I don't know of anyone actually doing that.

Instead, most systems have the internal phy connect to an external phy. 
  This connection is how the Emac receives packets from the external phy.

So I don't understand how I'm supposed to use the binding in 
mdio-gpio.txt.  For one thing, there is no such binding on ACPI systems. 
  On my ACPI system, firmware has set up the GPIOs.  The driver never 
actually makes any gpio_xxx calls.  At this point, I'm tempted to just 
remove all the GPIO stuff from Gilad's patch, if I can't help enough 
help to figure out how to modify the driver the way you think I should.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

[toc] | [next] | [standalone]

#1373716

> I'm back to working on this driver, and I need some more help with
> how to handle the phy.  mdio-gpio.txt doesn't really tell me much.
> I'm actually working on an ACPI system and not DT.

I can help you with DT, but not ACPI.

The MDIO bus can be a separate Linux device. Since you have GPIO lines
for the MDIO bus, it makes sense for this to be a mdio-gpio device. So
in DT, you would have:

mdio0: mdio {
        compatible = "virtual,mdio-gpio";
        #address-cells = <1>;
        #size-cells = <0>;
        gpios = <&qcomgpio 123 0
                 &qcomgpio 124 0>;

	phy0: ethernet-phy@8 {
        	reg = <9>;
	};
};

Here i've assumed the PHY is using address 8 on the bus. Change as
needed.

In your MAC DT node, you then have phy-handle pointing to this phy:

  emac0: qcom,emac@feb20000 {
  	cell-index = <0>;
	compatible = "qcom,emac";
	reg-names = "base", "csr", "ptp", "sgmii";
	reg =   <0xfeb20000 0x10000>,
	    	<0xfeb36000 0x1000>,
		<0xfeb3c000 0x4000>,
		<0xfeb38000 0x400>;
	#address-cells = <0>;
	interrupt-parent = <&emac0>;
	#interrupt-cells = <1>;
	interrupts = <0 1>;
	interrupt-map-mask = <0xffffffff>;
	interrupt-map = <0 &intc 0 76 0
	 	         1 &intc 0 80 0>;
       	interrupt-names = "emac_core0", "sgmii_irq";
  	qcom,emac-tstamp-en;
	qcom,emac-ptp-frac-ns-adj = <125000000 1>;

	phy-handle = <&phy0>
}

In the driver, you need to connect the PHY to the MAC. You do this
using something like:

         if (dev->of_node) {
                 phy_np = of_parse_phandle(dev->of_node, "phy-handle", 0);
                 if (!phy_np) {
                         netdev_dbg(ndev, "No phy-handle found in DT\n");
                         return -ENODEV;
                 }
 
                 phy_dev = of_phy_connect(ndev, phy_np, &xxxx_enet_adjust_link,
                                          0, pdata->phy_mode);
                 if (!phy_dev) {
                         netdev_err(ndev, "Could not connect to PHY\n");
                         return -ENODEV;
                 }

Do you have an ACPI table describing this hardware? What does it look
like?

	Andrew

[toc] | [prev] | [next] | [standalone]

#1373801

Andrew Lunn wrote:
>> I'm back to working on this driver, and I need some more help with
>> how to handle the phy.  mdio-gpio.txt doesn't really tell me much.
>> I'm actually working on an ACPI system and not DT.
>
> I can help you with DT, but not ACPI.
>
> The MDIO bus can be a separate Linux device. Since you have GPIO lines
> for the MDIO bus, it makes sense for this to be a mdio-gpio device. So
> in DT, you would have:
>
> mdio0: mdio {
>          compatible = "virtual,mdio-gpio";
>          #address-cells = <1>;
>          #size-cells = <0>;
>          gpios = <&qcomgpio 123 0
>                   &qcomgpio 124 0>;
>
> 	phy0: ethernet-phy@8 {
>          	reg = <9>;
> 	};
> };
>
> Here i've assumed the PHY is using address 8 on the bus. Change as
> needed.
>
> In your MAC DT node, you then have phy-handle pointing to this phy:
>
>    emac0: qcom,emac@feb20000 {
>    	cell-index = <0>;
> 	compatible = "qcom,emac";
> 	reg-names = "base", "csr", "ptp", "sgmii";
> 	reg =   <0xfeb20000 0x10000>,
> 	    	<0xfeb36000 0x1000>,
> 		<0xfeb3c000 0x4000>,
> 		<0xfeb38000 0x400>;
> 	#address-cells = <0>;
> 	interrupt-parent = <&emac0>;
> 	#interrupt-cells = <1>;
> 	interrupts = <0 1>;
> 	interrupt-map-mask = <0xffffffff>;
> 	interrupt-map = <0 &intc 0 76 0
> 	 	         1 &intc 0 80 0>;
>         	interrupt-names = "emac_core0", "sgmii_irq";
>    	qcom,emac-tstamp-en;
> 	qcom,emac-ptp-frac-ns-adj = <125000000 1>;
>
> 	phy-handle = <&phy0>
> }
>
> In the driver, you need to connect the PHY to the MAC. You do this
> using something like:
>
>           if (dev->of_node) {
>                   phy_np = of_parse_phandle(dev->of_node, "phy-handle", 0);
>                   if (!phy_np) {
>                           netdev_dbg(ndev, "No phy-handle found in DT\n");
>                           return -ENODEV;
>                   }
>
>                   phy_dev = of_phy_connect(ndev, phy_np, &xxxx_enet_adjust_link,
>                                            0, pdata->phy_mode);
>                   if (!phy_dev) {
>                           netdev_err(ndev, "Could not connect to PHY\n");
>                           return -ENODEV;
>                   }

Thank you very much.  I'll study this in detail.

> Do you have an ACPI table describing this hardware? What does it look
> like?

So a little background.  There are several versions of this driver 
floating in Qualcomm, and this is the first serious attempt to upstream 
it.  I'm trying to reconcile Gilad's driver with the one we use 
internally for our ACPI-enabled ARM server platform (the QDF2432).

My goal is to get Gilad's driver accepted upstream with minimal changes 
on my part.  I will then follow up with several patches that enable ACPI 
and our SOC, as well as adding missing parts like ethtool and 1588 support.

On my platform, firmware (UEFI) configures all of the GPIOs.  I need to 
get confirmation, but it appears that we don't actually make any GPIO 
calls at all.  I see code that looks like this:

	for (i = 0; (!adpt->no_mdio_gpio) && i < EMAC_NUM_GPIO; i++) {
		gpio_info = &adpt->gpio_info[i];
		retval = of_get_named_gpio(node, gpio_info->name, 0);
		if (retval < 0)
			return retval;

And on our ACPI system, adpt->no_mdio_gpio is always true:

	/* Assume GPIOs required for MDC/MDIO are enabled in firmware */
	adpt->no_mdio_gpio = true;

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

[toc] | [prev] | [next] | [standalone]

#1373915

On Thu, Apr 07, 2016 at 04:43:47PM -0500, Timur Tabi wrote:
> On my platform, firmware (UEFI) configures all of the GPIOs.  I need
> to get confirmation, but it appears that we don't actually make any
> GPIO calls at all.  I see code that looks like this:
> 
> 	for (i = 0; (!adpt->no_mdio_gpio) && i < EMAC_NUM_GPIO; i++) {
> 		gpio_info = &adpt->gpio_info[i];
> 		retval = of_get_named_gpio(node, gpio_info->name, 0);
> 		if (retval < 0)
> 			return retval;
> 
> And on our ACPI system, adpt->no_mdio_gpio is always true:
> 
> 	/* Assume GPIOs required for MDC/MDIO are enabled in firmware */
> 	adpt->no_mdio_gpio = true;

There are two different things here. One is configuring the pin to be
a GPIO. The second is using the GPIO as a GPIO. In this case,
bit-banging the MDIO bus.

The firmware could be doing the configuration, setting the pin as a
GPIO. However, the firmware cannot be doing the MDIO bit-banging to
make an MDIO bus available. Linux has to do that.

Or it could be we have all completely misunderstood the hardware, and
we are not doing bit-banging GPIO MDIO. There is a real MDIO
controller there, we don't use these pins as GPIOs, etc....

	   Andrew

[toc] | [prev] | [next] | [standalone]

#1374428

Andrew Lunn wrote:

> There are two different things here. One is configuring the pin to be
> a GPIO. The second is using the GPIO as a GPIO. In this case,
> bit-banging the MDIO bus.
>
> The firmware could be doing the configuration, setting the pin as a
> GPIO. However, the firmware cannot be doing the MDIO bit-banging to
> make an MDIO bus available. Linux has to do that.
>
> Or it could be we have all completely misunderstood the hardware, and
> we are not doing bit-banging GPIO MDIO. There is a real MDIO
> controller there, we don't use these pins as GPIOs, etc....

Actually, I think there is a misunderstanding.

On the FSM9900 SOC (which uses device-tree), the two pins that connect 
to the external PHY are gpio pins.  However, the driver needs to 
reprogram the pinmux so that those pins are wired to the Emac 
controller.  That's what the the gpio code in this driver is doing: it's 
just configuring the pins so that they connect directly between the Emac 
and the external PHY.  After that, they are no longer GPIO pins, and you 
cannot use the "GPIO controlled MDIO bus".  There is no MDIO controller 
on the SOC.  The external PHY is controlled directly from the Emac and 
also from the internal PHY.  It is screwy, I know, but that's what Gilad 
was trying to explain.

On the QDF2432 (which uses ACPI), those two wires are now dedicated. 
There are not muxed GPIOs any more -- they are hard wired between Emac 
and the external PHY.

In both cases, you need to use Emac registers to communicate with the 
external PHY.  Stuff like link detect and link speed are configured by 
programming the Emac and/or the internal phy.

And the internal phy isn't really an internal phy.  It's an SGMII-like 
device that's connected to the Emac and handles various phy-related 
tasks.  It has its own register block, but you still have to program it 
in concert with the Emac.  You can't really treat it separately.

So I'm beginning to believe that Gilad's driver is actually correct 
as-is.  There are a few minor bug fixes, but in general it's correct.  I 
would like to post a V4 soon that has those minor fixes.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

[toc] | [prev] | [next] | [standalone]

#1374471

On 04/08/2016 02:06 PM, Timur Tabi wrote:
> Andrew Lunn wrote:
>
>> There are two different things here. One is configuring the pin to be
>> a GPIO. The second is using the GPIO as a GPIO. In this case,
>> bit-banging the MDIO bus.
>>
>> The firmware could be doing the configuration, setting the pin as a
>> GPIO. However, the firmware cannot be doing the MDIO bit-banging to
>> make an MDIO bus available. Linux has to do that.
>>
>> Or it could be we have all completely misunderstood the hardware, and
>> we are not doing bit-banging GPIO MDIO. There is a real MDIO
>> controller there, we don't use these pins as GPIOs, etc....
>
> Actually, I think there is a misunderstanding.
>
> On the FSM9900 SOC (which uses device-tree), the two pins that connect to the external PHY are gpio pins.  However, the driver needs to reprogram the pinmux so that those pins are wired to the Emac controller.  That's what the the gpio code in this driver is doing: it's just configuring the pins so that they connect directly between the Emac and the external PHY.  After that, they are no longer GPIO pins, and you cannot use the "GPIO controlled MDIO bus".  There is no MDIO controller on the SOC.  The external PHY is controlled directly from the Emac and also from the internal PHY.  It is screwy, I know, but that's what Gilad was trying to explain.
It is incorrect to say there's no MDIO controller on the SoC. The EMAC Core on the SoC itself has a MDIO controller which talks to the external PHY. The internal SGMII is not on MDIO however.
Please see the EMAC specification.
>
> On the QDF2432 (which uses ACPI), those two wires are now dedicated. There are not muxed GPIOs any more -- they are hard wired between Emac and the external PHY.
>
> In both cases, you need to use Emac registers to communicate with the external PHY.  Stuff like link detect and link speed are configured by programming the Emac and/or the internal phy.
You need to use EMAC *MDIO* registers to communicate with external PHY.
>
> And the internal phy isn't really an internal phy.  It's an SGMII-like device that's connected to the Emac and handles various phy-related tasks.  It has its own register block, but you still have to program it in concert with the Emac.  You can't really treat it separately.
>
> So I'm beginning to believe that Gilad's driver is actually correct as-is.  There are a few minor bug fixes, but in general it's correct.  I would like to post a V4 soon that has those minor fixes.
>


-- 
Vikram Sethi
Qualcomm Technologies Inc, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1374483

Vikram Sethi wrote:

>> On the FSM9900 SOC (which uses device-tree), the two pins that connect to the external PHY are gpio pins.  However, the driver needs to reprogram the pinmux so that those pins are wired to the Emac controller.  That's what the the gpio code in this driver is doing: it's just configuring the pins so that they connect directly between the Emac and the external PHY.  After that, they are no longer GPIO pins, and you cannot use the "GPIO controlled MDIO bus".  There is no MDIO controller on the SOC.  The external PHY is controlled directly from the Emac and also from the internal PHY.  It is screwy, I know, but that's what Gilad was trying to explain.
> It is incorrect to say there's no MDIO controller on the SoC. The EMAC Core on the SoC itself has a MDIO controller which talks to the external PHY. The internal SGMII is not on MDIO however.
> Please see the EMAC specification.

Sorry, I should have said that there is no *independent* MDIO controller 
(one that has its own driver).  As you said, you can only talk to the 
external PHY through the Emac.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

[toc] | [prev] | [next] | [standalone]

#1374506

On Fri, Apr 8, 2016 at 12:06 PM, Timur Tabi <timur@codeaurora.org> wrote:
> Andrew Lunn wrote:
>
>> There are two different things here. One is configuring the pin to be
>> a GPIO. The second is using the GPIO as a GPIO. In this case,
>> bit-banging the MDIO bus.
>>
>> The firmware could be doing the configuration, setting the pin as a
>> GPIO. However, the firmware cannot be doing the MDIO bit-banging to
>> make an MDIO bus available. Linux has to do that.
>>
>> Or it could be we have all completely misunderstood the hardware, and
>> we are not doing bit-banging GPIO MDIO. There is a real MDIO
>> controller there, we don't use these pins as GPIOs, etc....
>
>
> Actually, I think there is a misunderstanding.
>
> On the FSM9900 SOC (which uses device-tree), the two pins that connect to
> the external PHY are gpio pins.  However, the driver needs to reprogram the
> pinmux so that those pins are wired to the Emac controller.  That's what the
> the gpio code in this driver is doing: it's just configuring the pins so
> that they connect directly between the Emac and the external PHY.  After
> that, they are no longer GPIO pins, and you cannot use the "GPIO controlled
> MDIO bus".  There is no MDIO controller on the SOC.  The external PHY is
> controlled directly from the Emac and also from the internal PHY.  It is
> screwy, I know, but that's what Gilad was trying to explain.
>

It sounds like you're trying to say that the pins used can be are
muxed as GPIO or MDIO, in the TLMM.

In the downstream kernel this is often seen with the drivers calling
gpio_request() to "reserve" said pins, but all you should do is
described the desired configuration and muxing in the pinctrl node,
reference that from your driver and simply ignore the fact that those
pins could have been used as GPIO pins.

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1374519

Bjorn Andersson wrote:

> It sounds like you're trying to say that the pins used can be are
> muxed as GPIO or MDIO, in the TLMM.

I'm not 100% sure, but I think that's correct.  If you don't want to 
have normal networking, you could connect those external pins to some 
GPIO device (like an LED or whatever), and then configure the pin muxing 
for GPIO purposes.  But if that's true, it's only true on the FSM9900. 
On the QDF2432, those lines are not connected to the TLMM.  They are 
instead hard-wired to the Emac.

> In the downstream kernel this is often seen with the drivers calling
> gpio_request() to "reserve" said pins, but all you should do is
> described the desired configuration and muxing in the pinctrl node,
> reference that from your driver and simply ignore the fact that those
> pins could have been used as GPIO pins.

That makes sense, but I think the driver already does that.

https://patchwork.ozlabs.org/patch/561667/

Function emac_probe_resources() has a call to of_get_named_gpio().  And 
then emac_mac_up() calls gpio_request().  As far as I can tell, that's it.

I'm guessing that the of_get_named_gpio() call needs to be changed 
somehow, but I'm not sure how.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

[toc] | [prev] | [next] | [standalone]

#1374526

On Fri 08 Apr 16:01 PDT 2016, Timur Tabi wrote:

> Bjorn Andersson wrote:
> 
> >It sounds like you're trying to say that the pins used can be are
> >muxed as GPIO or MDIO, in the TLMM.
> 
> I'm not 100% sure, but I think that's correct.  If you don't want to have
> normal networking, you could connect those external pins to some GPIO device
> (like an LED or whatever), and then configure the pin muxing for GPIO
> purposes.  But if that's true, it's only true on the FSM9900. On the
> QDF2432, those lines are not connected to the TLMM.  They are instead
> hard-wired to the Emac.
> 

Then through proper use of the pinctrl framework you should configure
the FSM9900 to mux these pins appropriately and the two solutions are
equivalent.

> >In the downstream kernel this is often seen with the drivers calling
> >gpio_request() to "reserve" said pins, but all you should do is
> >described the desired configuration and muxing in the pinctrl node,
> >reference that from your driver and simply ignore the fact that those
> >pins could have been used as GPIO pins.
> 
> That makes sense, but I think the driver already does that.
> 
> https://patchwork.ozlabs.org/patch/561667/
> 
> Function emac_probe_resources() has a call to of_get_named_gpio().  And then
> emac_mac_up() calls gpio_request().  As far as I can tell, that's it.
> 
> I'm guessing that the of_get_named_gpio() call needs to be changed somehow,
> but I'm not sure how.
> 

Thanks for the link.

In short those call to the gpio framework should just be removed. They
should only be there if you're using the gpiolib to control the state of
those pins, and you're not as far as I can see.


The general outline of what you should have in your dts instead is:

soc {
	tlmm {
		compatible = "qcom,pinctrl-xyz";

		mdio_pins_a: mdio {
			state {
				pins = "gpio0", "gpio1";
				function = "mdio";
			};
		};
	};

	emac {
		compatible = "qcom,somthing-emac";

		pinctrl-names = "default";
		pinctrl-0 = <&mdio_pins_a>;
	};
};

Regards,
Bjorn

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web