Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

Florian Fainelli wrote:

+/* Transmit the packet */
>+static int emac_start_xmit(struct sk_buff *skb, struct net_device *netdev)
>+{
>+   struct emac_adapter *adpt = netdev_priv(netdev);
>+
>+   return emac_mac_tx_buf_send(adpt, >tx_q, skb);



I would inline emac_mac_tx_buf_send()'s body here to make it much easier
to read and audit...


I'm close to submitting a v5 of this patchset.  The change to phylib has 
resulted in significant other changes.  I'm sure it's still not quite 
right, so I ask your patience in reviewing it.


However, I'm not sure inlining emac_mac_tx_buf_send() into 
emac_start_xmit() is good idea.  That would result in moving several 
functions from emac-mac.c into emac.c.  I'm concerned about maintaining 
the functional split between the two files.


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

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

Timur Tabi wrote:

I think the problem is that the current driver seems to be too eager to
start/stop the MAC.

Please take a look at emac_work_thread_link_check() at
https://lkml.org/lkml/2016/4/13/670.  Every time the PHY link goes up,
it does this:


Never mind, I figured out the problem.  I still have a lot of work ahead 
of me, but at least I'm not stuck any more.


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

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

Florian Fainelli wrote:

The Ethernet MAC should be started in ndo_open() and stopped in
ndo_close(), in between, there are link state changes, but you are not
supposed to stop or start your Ethernet MAC and its DMA for instance
during link change, if that is a HW requirement, your HW is pretty funky.


I think the problem is that the current driver seems to be too eager to 
start/stop the MAC.


Please take a look at emac_work_thread_link_check() at 
https://lkml.org/lkml/2016/4/13/670.  Every time the PHY link goes up, 
it does this:


if (phy->link_up) {
if (netif_carrier_ok(netdev))
goto link_task_done;

pm_runtime_get_sync(netdev->dev.parent);
netif_info(adpt, timer, adpt->netdev, "NIC Link is Up %s\n",
   speed);

emac_mac_start(adpt);
netif_carrier_on(netdev);
netif_wake_queue(netdev);


The call to emac_mac_start seems wrong to me here.

--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Florian Fainelli]

On 05/10/2016 04:18 PM, Timur Tabi wrote:
> Florian Fainelli wrote:
>> Are you utilizing the PHYLIB APIs properly? You need at least a
>> phy_start() to start the PHY state machine, and an adjust_link callback
>> to be provided to phy_connect() (or of_phy_connect()) to manage link
>> state changes. And that's the very basic minimum here, there could be
>> additional APIs that you may end up using.
>>
>> There are tons of example in tree of drivers doing this, bcmgenet,
>> bcmsysport, tg3 etc.
> 
> Thank you.  I think I finally got phylib working, more or less.
> 
> Unfortunately, it seems I have some kind of race condition.  The driver
> has a lot that's wrong with it, and I'm trying to fix it all.  One crazy
> the driver does is it create a workqueue to handle a lot of the tasks
> that would normally be handled in the interrupt handler itself.

That sounds like a typicall top half/bottom half split, fair enough.

> 
> With phylib support, I know my driver can call phy_mac_interrupt() when
> it gets a link status change interrupt.  I then have an .adjust_link
> callback which starts or stops the mac accordingly.

The Ethernet MAC should be started in ndo_open() and stopped in
ndo_close(), in between, there are link state changes, but you are not
supposed to stop or start your Ethernet MAC and its DMA for instance
during link change, if that is a HW requirement, your HW is pretty funky.

> 
> My problem is that I'm not really sure what adjust_link is supposed to
> be doing.

Well, it's pretty simple, it is about re-configuring your Ethernet MAC
based on what the PHY link state mandates: duplex, pause, speed changes,
EEE etc is what this callback is supposed to take care of, at the
Ethernet MAC level.

>  In addition, it seems that I need to keep the workqueue
> running, otherwise the interface will not function.  I bring the
> interface up, and the driver reports success, but pings do not work.
> 
> I'm getting really frustrated.  The sample code isn't really helping a
> whole lot, because I lack a fundamental understanding of what needs to
> be done.  None of the documentation I've read is helpful, and I don't
> know how to debug it.

Seriously, no documentation is helpful? The PHY library seems pretty
well documented to me, but I suppose I have a bias, oh, and patches are
welcome of course.

> 
> Can you give me some advice on how to debug this?

Take a look at drivers/net/ethernet/broadcom/genet/bcmgenet.c and see
how it deals with managing link state changes for instance. The code is
pretty straight forward: link interrupt (and other causes) trigger a
workqueue schedule, which then processes link state changes and calls
phy_mac_interrupt(), which in turn makes the PHY library adjust the
interface carrier state.
-- 
Florian

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

Florian Fainelli wrote:

Are you utilizing the PHYLIB APIs properly? You need at least a
phy_start() to start the PHY state machine, and an adjust_link callback
to be provided to phy_connect() (or of_phy_connect()) to manage link
state changes. And that's the very basic minimum here, there could be
additional APIs that you may end up using.

There are tons of example in tree of drivers doing this, bcmgenet,
bcmsysport, tg3 etc.


Thank you.  I think I finally got phylib working, more or less.

Unfortunately, it seems I have some kind of race condition.  The driver 
has a lot that's wrong with it, and I'm trying to fix it all.  One crazy 
the driver does is it create a workqueue to handle a lot of the tasks 
that would normally be handled in the interrupt handler itself.


With phylib support, I know my driver can call phy_mac_interrupt() when 
it gets a link status change interrupt.  I then have an .adjust_link 
callback which starts or stops the mac accordingly.


My problem is that I'm not really sure what adjust_link is supposed to 
be doing.  In addition, it seems that I need to keep the workqueue 
running, otherwise the interface will not function.  I bring the 
interface up, and the driver reports success, but pings do not work.


I'm getting really frustrated.  The sample code isn't really helping a 
whole lot, because I lack a fundamental understanding of what needs to 
be done.  None of the documentation I've read is helpful, and I don't 
know how to debug it.


Can you give me some advice on how to debug this?

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

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Andrew Lunn]

> Does the compatible property of the phy node (for the external phy)
> need to list the actual external phy?  That is, should it look like
> this:
> 
>   phy0: ethernet-phy@0 {
>   compatible = "qcom,fsm9900-emac-phy";
>   reg = <0>;
>   }
> 
> or this:
> 
>   phy0: ethernet-phy@0 {
>   compatible = "athr,whatever-phy";
>   reg = <0>;
>   }
> 

Documentation/devicetree/bindings/net/phy.txt says:

Optional Properties:

- compatible: Compatible list, may contain
  "ethernet-phy-ieee802.3-c22" or "ethernet-phy-ieee802.3-c45" for
  PHYs that implement IEEE802.3 clause 22 or IEEE802.3 clause 45
  specifications. If neither of these are specified, the default is to
  assume clause 22.

  If the phy's identifier is known then the list may contain an entry
  of the form: "ethernet-phy-id." where
  - The value of the 16 bit Phy Identifier 1 register as
4 hex digits. This is the chip vendor OUI bits 3:18
  - The value of the 16 bit Phy Identifier 2 register as
4 hex digits. This is the chip vendor OUI bits 19:24,
followed by 10 bits of a vendor specific ID.

  The compatible list should not contain other values than those
  listed here.

  Andrew

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Florian Fainelli]

On 22/04/16 12:45, Timur Tabi wrote:
> Timur Tabi wrote:
>>>
>>
>> So I've done some more research, and I believe that the internal phy is
>> not a candidate for phylib, but the external phy (which is a real phy)
>> might be.  There's no MDIO bus to the internal phy.
>>
>> Does this mean that I will need to enable a PHY driver, and that driver
>> will control the external phy?  If so, then does that mean that I would
>> delete all to code in my driver that calls emac_phy_read() and
>> emac_phy_write()?  For example, I wouldn't need emac_phy_link_check()
>> any more?
> 
> So I think I have it partially working, but I'm not sure if I'm doing
> things correctly, and I'd like some help.
> 
> The external phy is an Atheros 8031, so I load the at803x driver.  I
> added this code to my driver:
> 
> mii_bus = devm_mdiobus_alloc(>dev);
> mii_bus->phy_mask = ~(1 << adpt->hw.phy_addr);
> mii_bus->read = emac_mdio_read;
> mii_bus->write = emac_mdio_write;
> mii_bus->reset = emac_mdio_reset;
> mii_bus->parent = >dev;
> mii_bus->priv = hw;
> 
> mdiobus_register(mii_bus);
> 
> When I call mdiobus_register, I can see that the at803x_probe() probe
> function is called, so a connection is made.
> 
> The problem is that after that point, it appears that the at803x driver
> is never called again.  I tried bring the interface up and down, and
> connecting and disconnecting an Ethernet cable, but that didn't trigger
> anything.  I would expect the PHY driver to do more than just probe.

Are you utilizing the PHYLIB APIs properly? You need at least a
phy_start() to start the PHY state machine, and an adjust_link callback
to be provided to phy_connect() (or of_phy_connect()) to manage link
state changes. And that's the very basic minimum here, there could be
additional APIs that you may end up using.

There are tons of example in tree of drivers doing this, bcmgenet,
bcmsysport, tg3 etc.
-- 
Florian

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

Timur Tabi wrote:




So I've done some more research, and I believe that the internal phy is
not a candidate for phylib, but the external phy (which is a real phy)
might be.  There's no MDIO bus to the internal phy.

Does this mean that I will need to enable a PHY driver, and that driver
will control the external phy?  If so, then does that mean that I would
delete all to code in my driver that calls emac_phy_read() and
emac_phy_write()?  For example, I wouldn't need emac_phy_link_check()
any more?


So I think I have it partially working, but I'm not sure if I'm doing 
things correctly, and I'd like some help.


The external phy is an Atheros 8031, so I load the at803x driver.  I 
added this code to my driver:


mii_bus = devm_mdiobus_alloc(>dev);
mii_bus->phy_mask = ~(1 << adpt->hw.phy_addr);
mii_bus->read = emac_mdio_read;
mii_bus->write = emac_mdio_write;
mii_bus->reset = emac_mdio_reset;
mii_bus->parent = >dev;
mii_bus->priv = hw;

mdiobus_register(mii_bus);

When I call mdiobus_register, I can see that the at803x_probe() probe 
function is called, so a connection is made.


The problem is that after that point, it appears that the at803x driver 
is never called again.  I tried bring the interface up and down, and 
connecting and disconnecting an Ethernet cable, but that didn't trigger 
anything.  I would expect the PHY driver to do more than just probe.


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

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

Florian Fainelli wrote:


Well, PHYLIB does prefer using MDIO accesses to "speak" to PHYs,
built-in or external, but there is always the option of investing into
some custom development with the subsystem to make it play nicely with
your HW.


So I've done some more research, and I believe that the internal phy is 
not a candidate for phylib, but the external phy (which is a real phy) 
might be.  There's no MDIO bus to the internal phy.


Does this mean that I will need to enable a PHY driver, and that driver 
will control the external phy?  If so, then does that mean that I would 
delete all to code in my driver that calls emac_phy_read() and 
emac_phy_write()?  For example, I wouldn't need emac_phy_link_check() 
any more?



The MDIO bus on these chips is not accessible as a separate entity.  It
is melded (for lack of a better word) into the EMAC itself.  That's why
there is a "qcom,no-external-phy" property.  You could, in theory, wire
the internal phy of one SOC directly to the internal phy of another SOC,
and use that as in interconnect between SOCs.  I don't know of any such
use-cases however.


The fact the MDIO bus is built-into the MAC is really not a problem
here, there are tons of drivers that deal with that just fine, yet, the
DT binding needs to reflect that properly by having a sub-node of the
Ethernet MAC which is a MDIO bus controller node. If external or
internal PHYs are accessible through that MDIO bus, they also need to
appear as child-nodes of that MDIO bus controller node.


Does the compatible property of the phy node (for the external phy) need 
to list the actual external phy?  That is, should it look like this:


phy0: ethernet-phy@0 {
compatible = "qcom,fsm9900-emac-phy";
reg = <0>;
}

or this:

phy0: ethernet-phy@0 {
compatible = "athr,whatever-phy";
reg = <0>;
}




Can we just say that, an absence of PHY specified in the Device Tree (no
phy-handle property and PHY not a child node of the MDIO bus), means
that there is no external PHY?


Yes, that works.



[snip]


Do you need to maintain these flags when most, if not all of them
already exist in dev->flags or dev->features?


So you're saying that, for example, in emac_set_features() I should
remove this:

 if (netdev->features & NETIF_F_HW_VLAN_CTAG_RX)
 set_bit(EMAC_STATUS_VLANSTRIP_EN, >status);
 else
 clear_bit(EMAC_STATUS_VLANSTRIP_EN, >status);

and then in emac_mac_mode_config(), I should do this instead:

void emac_mac_mode_config(struct emac_adapter *adpt)
{
 struct net_device *netdev = adpt->netdev;

 if (netdev->features & NETIF_F_HW_VLAN_CTAG_RX)
 mac |= VLAN_STRIP;
 else
 mac &= ~VLAN_STRIP;


If so, then what do I do in emac_rx_mode_set()?  Should I delete this
entire block:

 /* Check for Promiscuous and All Multicast modes */
 if (netdev->flags & IFF_PROMISC) {
 set_bit(EMAC_STATUS_PROMISC_EN, >status);
 } else if (netdev->flags & IFF_ALLMULTI) {
 set_bit(EMAC_STATUS_MULTIALL_EN, >status);
 clear_bit(EMAC_STATUS_PROMISC_EN, >status);
 } else {
 clear_bit(EMAC_STATUS_MULTIALL_EN, >status);
 clear_bit(EMAC_STATUS_PROMISC_EN, >status);
 }

It does look like Gilad is just mirroring the flags/features variable
into adpt->status.  What I can't figure out is why.  It seems completely
redundant, but I have a nagging feeling that there is a good reason.


Yes, I think your set_features and set_rx_mode functions would be
greatly simplified, if each of them did take care of programming the HW
immediately based on function arguments/flags. Unless absolutely
required (e.g: suspend/resume, outside of the scope of the function
etc..) having bookeeping variables is always something that can be out
of sync, so better avoid them as much as possible.


Ok, I'll try to clean this up.


So I should move the netif_start_queue() to the end of this function?
Sorry if that's a stupid question, but I know little about the MAC side
of network drivers.


That's fine, yes moving netif_start_queue() at the far end of the
function is a good change.


Ok.


+/* Bring down the interface/HW */
+void emac_mac_down(struct emac_adapter *adpt, bool reset)
+{
+struct net_device *netdev = adpt->netdev;
+struct emac_phy *phy = >phy;
+unsigned long flags;
+
+set_bit(EMAC_STATUS_DOWN, >status);


Do you need to maintain that? Would not netif_running() tell you what
you want if you reflect the carrier state properly?


I think that emac_work_thread_link_check() handles this.  It's a timer
thread that polls the link state and calls netif_carrier_off() if the
link is down.  Is that sufficient?



Probably, then again, with PHYLIB you have the option of either
switching the PHY to interrupt mode (thsus saving the polling_), or it
polls the PHY for link statuses every HZ.


I'll have to check and see if interrupt mode is even an 

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

Bjorn Andersson wrote:

Your driver is a platform driver and it's being probed from DT. As such
all this is already taken care of for you, by the core.

The listing is for your reference to know why the dma-ranges property
would affect your device.


Ah, sorry. I misunderstood what you meant by "you will pass".

Coincidentally, it looks like Lorenzo Pieralisi has posted patches the 
ACPI equivalent just yesterday: https://lkml.org/lkml/2016/4/14/694


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

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Bjorn Andersson]

On Fri 15 Apr 10:00 PDT 2016, Timur Tabi wrote:

> Bjorn Andersson wrote:
> >For platform devices being populated via from DT you will pass:
> >of_platform_bus_create()
> >   of_platform_device_create_pdata()
> > of_dma_configure()
> >
> >Which calls of_dma_get_range() to acquire this information from the
> >dma-ranges property and set up the dma ops and properties.
> 
> This seems excessive.  I have to create a platform bus just to configure the
> DMA mask?  Most drivers just call dma_set_mask and give it a number, and
> that's not device-tree specific.  I also need to come up with a way to get
> this to work on ACPI.
> 
> I just seems like a lot of work only because I need to determine at runtime
> what my DMA mask is.  I also don't see any drivers that call
> of_dma_configure().
> 

Your driver is a platform driver and it's being probed from DT. As such
all this is already taken care of for you, by the core.

The listing is for your reference to know why the dma-ranges property
would affect your device.

Regards,
Bjorn

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

Rob Herring wrote:

You may only care about the size, but the binding has to handle the
more complex case. Here's an example

<0x0 0x2 0x0 0x1 0x0>

dma address 0 (cell 0) maps to cpu (parent) address 0x2_ (cell
1-2) and the range/size is 4G (cell 3-4).

If you have the same base address, then use the same address. The core
will calculate the mask based on the size. IIRC, we also handle ~0 as
a special case to support 4G for #size-cell=1.


So the first thing I noticed is that Gilad had this:

reg =   <0xfeb2 0x1>,
<0xfeb36000 0x1000>,
<0xfeb3c000 0x4000>,
<0xfeb38000 0x400>;
#address-cells = <0>;

Shouldn't address-cells have been 1 instead?

Ok, let me see if I get this right:

32-bit:

soc {
#address-cells = <1>;
#size-cells = <1>;

emac0: qcom,emac@feb2 {
compatible = "qcom,fsm9900-emac";
#address-cells = <1>;
#size-cells = <1>;
reg-names = "base", "csr", "ptp", "sgmii";
reg =   <0xfeb2 0x1>,
<0xfeb36000 0x1000>,
<0xfeb3c000 0x4000>,
<0xfeb38000 0x400>;
dma-ranges = <0 0 0x>;
interrupt-parent = <>;

64-bit

soc {
#address-cells = <2>;
#size-cells = <2>;

emac0: qcom,emac@feb2 {
compatible = "qcom,fsm9900-emac";
#address-cells = <2>;
#size-cells = <2>;
reg-names = "base", "csr", "ptp", "sgmii";
reg =   <0 0xfeb2 0 0x1>,
<0 0xfeb36000 0 0x1000>,
<0 0xfeb3c000 0 0x4000>,
<0 0xfeb38000 0 0x400>;
dma-ranges = <0 0 0 0 0x 0x>;

This seems inelegant, though.

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

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

Bjorn Andersson wrote:

For platform devices being populated via from DT you will pass:
of_platform_bus_create()
   of_platform_device_create_pdata()
 of_dma_configure()

Which calls of_dma_get_range() to acquire this information from the
dma-ranges property and set up the dma ops and properties.


This seems excessive.  I have to create a platform bus just to configure 
the DMA mask?  Most drivers just call dma_set_mask and give it a number, 
and that's not device-tree specific.  I also need to come up with a way 
to get this to work on ACPI.


I just seems like a lot of work only because I need to determine at 
runtime what my DMA mask is.  I also don't see any drivers that call 
of_dma_configure().


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

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Bjorn Andersson]

On Thu 14 Apr 16:34 PDT 2016, Timur Tabi wrote:

[..]
> So I think the solution is to create a device tree (and ACPI) property that
> holds the mask.
> 
>   dma-mask = <0 0x>;
> 
> or
> 
>   dma-mask = <0x 0x>;
> 
> The driver will then do this:
> 
>   u64 dma_mask;
>   device_property_read_u64(>dev, "dma-mask", _mask);
>   dma_coerce_mask_and_coherent(>dev, dma_mask);
> 
> What I'm not sure yet is whether I should call
> dma_coerce_mask_and_coherent() or dma_set_coherent_mask().
> 

For platform devices being populated via from DT you will pass:
of_platform_bus_create()
  of_platform_device_create_pdata()
of_dma_configure()

Which calls of_dma_get_range() to acquire this information from the
dma-ranges property and set up the dma ops and properties.

Regards,
Bjorn

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Rob Herring]

On Fri, Apr 15, 2016 at 10:44 AM, Timur Tabi  wrote:
> Rob Herring wrote:
>>>
>>> >
>>> > dma-mask = <0 0x>;
>>> >
>>> >or
>>> >
>>> > dma-mask = <0x 0x>;
>>
>> No. See dma-ranges.
>
>
> How exactly should I use dma-ranges?  I can't find any other drivers that
> queries that property and uses the result to call dma_set_mask.  I thought
> the dma-ranges property is intended to specify address translation.  I don't
> need to translate any address, I just need to know a single number.

You may only care about the size, but the binding has to handle the
more complex case. Here's an example

<0x0 0x2 0x0 0x1 0x0>

dma address 0 (cell 0) maps to cpu (parent) address 0x2_ (cell
1-2) and the range/size is 4G (cell 3-4).

If you have the same base address, then use the same address. The core
will calculate the mask based on the size. IIRC, we also handle ~0 as
a special case to support 4G for #size-cell=1.

Rob

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

Rob Herring wrote:

>
> dma-mask = <0 0x>;
>
>or
>
> dma-mask = <0x 0x>;

No. See dma-ranges.


How exactly should I use dma-ranges?  I can't find any other drivers 
that queries that property and uses the result to call dma_set_mask.  I 
thought the dma-ranges property is intended to specify address 
translation.  I don't need to translate any address, I just need to know 
a single number.


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

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Rob Herring]

On Thu, Apr 14, 2016 at 6:34 PM, Timur Tabi  wrote:
> Vikram Sethi wrote:

 >> retval = dma_coerce_mask_and_coherent(>dev,
 >> DMA_BIT_MASK(64));
 >> if (retval) {
 >> dev_err(>dev, "failed to set DMA mask err %d\n",
 >> retval);
 >> goto err_res;
 >> }
>>
>> How can you set the mask to 64 bits when the EMAC IP on FSM9900 and
>> QDF2432 can only do 32 bit DMA?
>> The mask in that API is a bit mask describing which bits of an address
>> your device supports.
>
>
> Vikram, Shanker, and I discussed this offline, and came to a consensus.
>
> The FSM9900 is a 32-bit platform, so the kernel will never create a DMA
> address above 4GB. Even if the driver sets the mask to 64 bits, it will
> technically work.  However, the mask should be set to 32 because all address
> buses are 32 bits.
>
> The QDF2432 is different.  Although it's an ARM64 platform, we have the
> unfortunate situation that only 32 bits of that address is wired to the rest
> of the chip.  So even though the Emac can handle 64-bit bus addresses, if it
> actually attempts to DMA above 4GB, the address will get truncated and
> corrupt memory.  The mask needs to be set to 32.
>
> There may or may not be other ARM64 chips from us that won't have this
> problem in the future, so these hypothetical chips would have a mask of 64.
>
> So I think the solution is to create a device tree (and ACPI) property that
> holds the mask.
>
> dma-mask = <0 0x>;
>
> or
>
> dma-mask = <0x 0x>;

No. See dma-ranges.

Rob

>
> The driver will then do this:
>
> u64 dma_mask;
> device_property_read_u64(>dev, "dma-mask", _mask);
> dma_coerce_mask_and_coherent(>dev, dma_mask);
>
> What I'm not sure yet is whether I should call
> dma_coerce_mask_and_coherent() or dma_set_coherent_mask().
>
> --
> Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum, a Linux Foundation collaborative project.

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

Vikram Sethi wrote:

>> retval = dma_coerce_mask_and_coherent(>dev, DMA_BIT_MASK(64));
>> if (retval) {
>> dev_err(>dev, "failed to set DMA mask err %d\n", retval);
>> goto err_res;
>> }

How can you set the mask to 64 bits when the EMAC IP on FSM9900 and QDF2432 can 
only do 32 bit DMA?
The mask in that API is a bit mask describing which bits of an address your 
device supports.


Vikram, Shanker, and I discussed this offline, and came to a consensus.

The FSM9900 is a 32-bit platform, so the kernel will never create a DMA 
address above 4GB. Even if the driver sets the mask to 64 bits, it will 
technically work.  However, the mask should be set to 32 because all 
address buses are 32 bits.


The QDF2432 is different.  Although it's an ARM64 platform, we have the 
unfortunate situation that only 32 bits of that address is wired to the 
rest of the chip.  So even though the Emac can handle 64-bit bus 
addresses, if it actually attempts to DMA above 4GB, the address will 
get truncated and corrupt memory.  The mask needs to be set to 32.


There may or may not be other ARM64 chips from us that won't have this 
problem in the future, so these hypothetical chips would have a mask of 64.


So I think the solution is to create a device tree (and ACPI) property 
that holds the mask.


dma-mask = <0 0x>;

or

dma-mask = <0x 0x>;

The driver will then do this:

u64 dma_mask;
device_property_read_u64(>dev, "dma-mask", _mask);
dma_coerce_mask_and_coherent(>dev, dma_mask);

What I'm not sure yet is whether I should call 
dma_coerce_mask_and_coherent() or dma_set_coherent_mask().


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

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Vikram Sethi]

A couple of clarifications on the SGMII internal PHY and the DMA capability of 
the EMAC inline.

On 04/14/2016 04:19 PM, Florian Fainelli wrote:
> On 14/04/16 13:19, Timur Tabi wrote:
>> Florian Fainelli wrote:
>>> On 13/04/16 10:59, Timur Tabi wrote:
 From: Gilad Avidov 

 Add supports for ethernet controller HW on Qualcomm Technologies,
 Inc. SoC.
 This driver supports the following features:
 1) Checksum offload.
 2) Runtime power management support.
 3) Interrupt coalescing support.
 4) SGMII phy.
 5) SGMII direct connection without external phy.
>>>
>>>
>>> [snip]
>>>
 +- qcom,no-external-phy  : Indicates there is no external PHY
 connected to
 +  EMAC. Include this only if the EMAC is directly
 +  connected to the peer end without EPHY.
>>> How is the internal PHY accessed, is it responding on the MDIO bus at a
>>> particular address?
>> There is a set of memory-mapped registers.  It's not connected via MDIO
>> at all.  It's mapped via the "sgmii" addresses in the device tree (see
>> function emac_sgmii_config).
>>
>>> If so, standard MDIO scanning/probing works, and you
>>> can have your PHY driver flag this device has internal. Worst case, you
>>> can do what BCMGENET does, and have a special "phy-mode" value set to
>>> "internal" when this knowledge needs to exist prior to MDIO bus scanning
>>> (e.g: to power on the PHY).
>> So the internal phy is not a real phy.  It's not capable of driving an
>> RJ45 port (there's no analog part).  It's an SGMII-like device that is
>> hard-wired to the EMAC itself.
There *is* an analog part to the internal SGMII PHY. Please check the SGMII 
specification. The only non-standard part is that it's not on MDIO.

> OK, that explains things a bit, thanks, this is quite a bit of important
> detail actually.
>
>> In theory, the internal PHY is optional.  You could design an SOC that
>> has just the EMAC connected via normal MDIO to an external phy.  I
>> really wish our hardware designers has done that.  But unfortunately,
>> there are no SOCs like that, and so we have to treat the internal phy as
>> an extension of the EMAC.
>>
>> My preference would be to get rid of the "qcom,no-external-phy" property
>> and have an external phy be required, at least until Qualcomm creates an
>> SOC without the internal phy (which may never happen, for all I know).
>>
> Can we just say that, an absence of PHY specified in the Device Tree (no
> phy-handle property and PHY not a child node of the MDIO bus), means
> that there is no external PHY?
>
> [snip]
>
>
[snip]
 +dev_set_drvdata(>dev, netdev);
 +SET_NETDEV_DEV(netdev, >dev);
 +
 +adpt = netdev_priv(netdev);
 +adpt->netdev = netdev;
 +phy = >phy;
 +adpt->msg_enable = netif_msg_init(debug, EMAC_MSG_DEFAULT);
 +
 +dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(32));
>>> Really, is not that supposed to run on ARM64 servers?
>> Well, this version of the driver isn't, which is why it supports DT and
>> not ACPI.  I'm planning on adding that support in a later patch.
>> However, I'll add support for 64-bit masks in the next version of this
>> patch.
>>
>> Would this be okay:
>>
>> retval = dma_coerce_mask_and_coherent(>dev, DMA_BIT_MASK(64));
>> if (retval) {
>> dev_err(>dev, "failed to set DMA mask err %d\n", retval);
>> goto err_res;
>> }

How can you set the mask to 64 bits when the EMAC IP on FSM9900 and QDF2432 can 
only do 32 bit DMA?
The mask in that API is a bit mask describing which bits of an address your 
device supports.

>> I've seen code like this in other drivers:
>>
>> ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
>> if (ret) {
>> ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>> if (ret) {
>> dev_err(dev, "failed to set dma mask\n");
>> return ret;
>> }
>> }
>>
>> and I've never understood why it's necessary to fall back to 32-bits if
>> 64 bits fails.  Isn't 64 bits a superset of 32 bits?  The driver is
>> saying that the hardware supports all of DDR.  How could fail, and how
>> could 32-bit succeed if 64-bits fails?
> I believe there could be cases where the HW is capable of addressing
> more physical memory than the CPU itself (usually unlikely, but it
> could), there could be cases where the HW is behind an IOMMMU which only
> has a window into the DDR, and that could prevent a higher DMA_BIT_MASK
> from being successfully configured.


-- 
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

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Florian Fainelli]

On 14/04/16 13:19, Timur Tabi wrote:
> Florian Fainelli wrote:
>> On 13/04/16 10:59, Timur Tabi wrote:
>>> From: Gilad Avidov 
>>>
>>> Add supports for ethernet controller HW on Qualcomm Technologies,
>>> Inc. SoC.
>>> This driver supports the following features:
>>> 1) Checksum offload.
>>> 2) Runtime power management support.
>>> 3) Interrupt coalescing support.
>>> 4) SGMII phy.
>>> 5) SGMII direct connection without external phy.
>>
>> I think you should shoot for more simple for an initial submission:
>>
>> - no offload
>> - no timestamping
>>
>> get that accepted, and then add features one by one, it sure is more
>> work, but it helps with the review, and makes you work off a solid base.
> 
> Unfortunately, I didn't write this driver initially, so I'm not sure how
> to remove these features from it.  Variants of this driver have been
> bouncing around Qualcomm for years, and even the author of this patch
> (Gilad) is no longer around.

Well, good luck :)

> 
> So although I have a lot of experience upstreaming code, I have little
> experience and knowledge with network drivers.  I'm going to need a lot
> of hand-holding.  I hope you will be patient with me.
> 
> Timestamping support seems to be just a few lines of code, so I can
> probably remove that.  I don't know where offloading is in the driver,
> however.  I don't know how offloading in netdev drivers works.

Based on what the driver seems to do right now, it would be located in
the transmit and receive paths, and would have to access
mac/network/transport offsets and deal with checksums, so anything that
deals with checksums, provided that the HW does not require that to
transmit/receive packets, could be eliminated entirely for now and be
added later.

It is not the biggest part that needs to be slightly re-architected
though, the SGMII/PHY/MDIO stuff is more important as it impacts the
Device Tree binding, see below.

> 
>> You will see below, but a pet peeve of mine is authors reimplementing
>> code that exists in PHYLIB.
> 
> I can understand that, but the PHYs on these SOCs are non-standard.  The
> "internal PHY" (for lack of a better name) is part of the EMAC itself,
> and it acts as a middle-man for the external PHY.  There is an MDIO bus,
> but it's hard-wired to the EMAC, and most of the time you don't touch it
> directly.  Instead you let the EMAC and/or the internal PHY send/receive
> commands/data to the external PHY on your behalf.  The internal phy
> talks to the external phy via SGMII only.  Only the EMAC uses the mdio bus.

Humm OK, this PHY proxy, provided that this is really how it works,
seems a bit unusual, but, is not necessarily a roadblock to having a
proper MDIO implementation here which is standard and will allow you to
utilize re-usable drivers and facilities that are already there.

> 
> I will look at PHYLIB, but I can't tell you whether it will work with
> this hardware (Gilad previously claim that it wouldn't work well).

Well, PHYLIB does prefer using MDIO accesses to "speak" to PHYs,
built-in or external, but there is always the option of investing into
some custom development with the subsystem to make it play nicely with
your HW.

> 
>>> diff --git a/Documentation/devicetree/bindings/net/qcom-emac.txt
>>> b/Documentation/devicetree/bindings/net/qcom-emac.txt
>>> new file mode 100644
>>> index 000..df5e7c0
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/qcom-emac.txt
>>> @@ -0,0 +1,65 @@
>>> +Qualcomm EMAC Gigabit Ethernet Controller
>>> +
>>> +Required properties:
>>> +- compatible : Should be "qcom,emac".
>>> +- 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.
>>> +- phy-addr: Specifies phy address on MDIO bus.
>>> +Required if the optional property "qcom,no-external-phy"
>>> +is not specified.
>>
>> This is not the standard way to represent an Ethernet PHY hanging off a
>> MDIO bus see ethernet.txt and phy.txt in D/dt/bindings/net/
> 
> The MDIO bus on these chips is not accessible as a separate entity.  It
> is melded (for lack of a better word) into the EMAC itself.  That's why
> there is a "qcom,no-external-phy" property.  You could, in theory, wire
> the internal phy of one SOC directly to 

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

Florian Fainelli wrote:

On 13/04/16 10:59, Timur Tabi wrote:

From: Gilad Avidov 

Add supports for ethernet controller HW on Qualcomm Technologies, Inc. SoC.
This driver supports the following features:
1) Checksum offload.
2) Runtime power management support.
3) Interrupt coalescing support.
4) SGMII phy.
5) SGMII direct connection without external phy.


I think you should shoot for more simple for an initial submission:

- no offload
- no timestamping

get that accepted, and then add features one by one, it sure is more
work, but it helps with the review, and makes you work off a solid base.


Unfortunately, I didn't write this driver initially, so I'm not sure how 
to remove these features from it.  Variants of this driver have been 
bouncing around Qualcomm for years, and even the author of this patch 
(Gilad) is no longer around.


So although I have a lot of experience upstreaming code, I have little 
experience and knowledge with network drivers.  I'm going to need a lot 
of hand-holding.  I hope you will be patient with me.


Timestamping support seems to be just a few lines of code, so I can 
probably remove that.  I don't know where offloading is in the driver, 
however.  I don't know how offloading in netdev drivers works.



You will see below, but a pet peeve of mine is authors reimplementing
code that exists in PHYLIB.


I can understand that, but the PHYs on these SOCs are non-standard.  The 
"internal PHY" (for lack of a better name) is part of the EMAC itself, 
and it acts as a middle-man for the external PHY.  There is an MDIO bus, 
but it's hard-wired to the EMAC, and most of the time you don't touch it 
directly.  Instead you let the EMAC and/or the internal PHY send/receive 
commands/data to the external PHY on your behalf.  The internal phy 
talks to the external phy via SGMII only.  Only the EMAC uses the mdio bus.


I will look at PHYLIB, but I can't tell you whether it will work with 
this hardware (Gilad previously claim that it wouldn't work well).



diff --git a/Documentation/devicetree/bindings/net/qcom-emac.txt 
b/Documentation/devicetree/bindings/net/qcom-emac.txt
new file mode 100644
index 000..df5e7c0
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/qcom-emac.txt
@@ -0,0 +1,65 @@
+Qualcomm EMAC Gigabit Ethernet Controller
+
+Required properties:
+- compatible : Should be "qcom,emac".
+- 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.
+- phy-addr: Specifies phy address on MDIO bus.
+   Required if the optional property "qcom,no-external-phy"
+   is not specified.


This is not the standard way to represent an Ethernet PHY hanging off a
MDIO bus see ethernet.txt and phy.txt in D/dt/bindings/net/


The MDIO bus on these chips is not accessible as a separate entity.  It 
is melded (for lack of a better word) into the EMAC itself.  That's why 
there is a "qcom,no-external-phy" property.  You could, in theory, wire 
the internal phy of one SOC directly to the internal phy of another SOC, 
and use that as in interconnect between SOCs.  I don't know of any such 
use-cases however.



+Optional properties:
+- qcom,emac-tstamp-en   : Enables the PTP (1588) timestamping feature.
+ Include this only if PTP (1588) timestamping
+ feature is needed. If included, "ptp" register
+ base should be specified.


If the "ptp" register range is not specified, then PTP gets disabled, so
is a boolean really required here, considering that this looks like a
policy decision more than anything.


It is, and I forget to remove it, since this is apparently handled via 
ethtool (which the driver does not currently support).



+- mac-address   : The 6-byte MAC address. If present, it is the
+ default MAC address.


This property is pretty much mandatory


Ok.


+- qcom,no-external-phy  : Indicates there is no external PHY connected to
+ EMAC. Include this only if the EMAC is directly
+ connected to the peer end without EPHY.


How is the internal PHY accessed, is it responding on the MDIO bus at a
particular 

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Rob Herring]

On Thu, Apr 14, 2016 at 11:47 AM, Timur Tabi  wrote:
> Rob Herring wrote:
>
>>> @@ -0,0 +1,65 @@
>>> +Qualcomm EMAC Gigabit Ethernet Controller
>>> +
>>> +Required properties:
>>> +- compatible : Should be "qcom,emac".
>>
>>
>> Come on... Can you guess what I'm going to say here.
>
>
> Ooops, I missed that one.
>
>>
>>> +- 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.
>>> +- phy-addr: Specifies phy address on MDIO bus.
>>> +   Required if the optional property
>>> "qcom,no-external-phy"
>>> +   is not specified.
>>
>>
>> As I mentioned in the last version, you should still describe this with
>> a standard MDIO bus binding even if you can't use the generic code.
>
>
> You mean like this?
>
> phy0: ethernet-phy@0 {
> compatible = "qcom,fsm9900-emac-phy";
> reg = <4>;

Yes, but you mean 0 here or 4 for unit address.

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

Rob Herring wrote:


@@ -0,0 +1,65 @@
+Qualcomm EMAC Gigabit Ethernet Controller
+
+Required properties:
+- compatible : Should be "qcom,emac".


Come on... Can you guess what I'm going to say here.


Ooops, I missed that one.




+- 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.
+- phy-addr: Specifies phy address on MDIO bus.
+   Required if the optional property "qcom,no-external-phy"
+   is not specified.


As I mentioned in the last version, you should still describe this with
a standard MDIO bus binding even if you can't use the generic code.


You mean like this?

phy0: ethernet-phy@0 {
compatible = "qcom,fsm9900-emac-phy";
reg = <4>;
};


+Optional properties:
+- qcom,emac-tstamp-en   : Enables the PTP (1588) timestamping feature.
+ Include this only if PTP (1588) timestamping
+ feature is needed. If included, "ptp" register
+ base should be specified.
+- mac-address   : The 6-byte MAC address. If present, it is the
+ default MAC address.
+- qcom,no-external-phy  : Indicates there is no external PHY connected to
+ EMAC. Include this only if the EMAC is directly
+ connected to the peer end without EPHY.
+Example:
+   emac0: qcom,emac@feb2 {


ethernet@


+   compatible = "qcom,fsm9900-emac";


Ah, I see you fixed it here...


and in the code, I just missed it in the top of the file.  I'll fix it 
everywhere in v5.


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

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Rob Herring]

On Wed, Apr 13, 2016 at 12:59:52PM -0500, Timur Tabi wrote:
> From: Gilad Avidov 
> 
> Add supports for ethernet controller HW on Qualcomm Technologies, Inc. SoC.
> This driver supports the following features:
> 1) Checksum offload.
> 2) Runtime power management support.
> 3) Interrupt coalescing support.
> 4) SGMII phy.
> 5) SGMII direct connection without external phy.
> 
> Based on a driver by Niranjana Vishwanathapura
> .
> 
> Signed-off-by: Gilad Avidov 
> Signed-off-by: Timur Tabi 
> ---
> 
> v4:
>  - add missing ipv6 header file
>  - correct compatible string
>  - fix spacing in emac_reg_write arrays
>  - drop unnecessary cell-index property
>  - remove unsupported DT properties from docs
>  - remove GPIO initialization and update docs
> 
> v3:
>  - remove most of the memory barriers by using the non xxx_relaxed() api.
>  - remove RSS and WOL support.
>  - correct comments from physical address to dma address.
>  - rearrange structs to make them packed.
>  - replace polling loops with readl_poll_timeout().
>  - remove unnecessary wrapper functions from phy layer.
>  - add blank line before return statements.
>  - set to null clocks after clk_put().
>  - use module_platform_driver() and dma_set_mask_and_coherent()
>  - replace long hex bitmasks with BIT() macro.
> 
> v2:
>  - replace hw bit fields to macros with bitwise operations.
>  - change all iterators to unsized types (int)
>  - some minor code flow improvements.
>  - change return type to void for functions which return value is never
>used.
>  - replace instance of l_relaxed() io followed by mb() with a
>readl()/writel().
> 
> ---
>  .../devicetree/bindings/net/qcom-emac.txt  |   65 +
>  drivers/net/ethernet/qualcomm/Kconfig  |   11 +
>  drivers/net/ethernet/qualcomm/Makefile |2 +
>  drivers/net/ethernet/qualcomm/emac/Makefile|7 +
>  drivers/net/ethernet/qualcomm/emac/emac-mac.c  | 1782 
> 
>  drivers/net/ethernet/qualcomm/emac/emac-mac.h  |  286 
>  drivers/net/ethernet/qualcomm/emac/emac-phy.c  |  484 ++
>  drivers/net/ethernet/qualcomm/emac/emac-phy.h  |   68 +
>  drivers/net/ethernet/qualcomm/emac/emac-sgmii.c|  683 
>  drivers/net/ethernet/qualcomm/emac/emac-sgmii.h|   30 +
>  drivers/net/ethernet/qualcomm/emac/emac.c  | 1206 +
>  drivers/net/ethernet/qualcomm/emac/emac.h  |  382 +
>  12 files changed, 5006 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/qcom-emac.txt
>  create mode 100644 drivers/net/ethernet/qualcomm/emac/Makefile
>  create mode 100644 drivers/net/ethernet/qualcomm/emac/emac-mac.c
>  create mode 100644 drivers/net/ethernet/qualcomm/emac/emac-mac.h
>  create mode 100644 drivers/net/ethernet/qualcomm/emac/emac-phy.c
>  create mode 100644 drivers/net/ethernet/qualcomm/emac/emac-phy.h
>  create mode 100644 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
>  create mode 100644 drivers/net/ethernet/qualcomm/emac/emac-sgmii.h
>  create mode 100644 drivers/net/ethernet/qualcomm/emac/emac.c
>  create mode 100644 drivers/net/ethernet/qualcomm/emac/emac.h
> 
> diff --git a/Documentation/devicetree/bindings/net/qcom-emac.txt 
> b/Documentation/devicetree/bindings/net/qcom-emac.txt
> new file mode 100644
> index 000..df5e7c0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/qcom-emac.txt
> @@ -0,0 +1,65 @@
> +Qualcomm EMAC Gigabit Ethernet Controller
> +
> +Required properties:
> +- compatible : Should be "qcom,emac".

Come on... Can you guess what I'm going to say here.

> +- 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.
> +- phy-addr: Specifies phy address on MDIO bus.
> + Required if the optional property "qcom,no-external-phy"
> + is not specified.

As I mentioned in the last version, you should still describe this with 
a standard MDIO bus binding even if you can't use the generic code.

> +
> +Optional properties:
> +- qcom,emac-tstamp-en   : Enables the PTP (1588) timestamping feature.
> +   Include this only if PTP (1588) timestamping
> +   

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Rob Herring]

On Wed, Apr 13, 2016 at 02:31:25PM -0500, Timur Tabi wrote:
> kbuild test robot wrote:
> >
> >drivers/net/ethernet/qualcomm/emac/emac-mac.c: In function 'emac_mac_up':
> >drivers/net/ethernet/qualcomm/emac/emac-mac.c:1076:9: warning: large 
> >integer implicitly truncated to unsigned type [-Woverflow]
> >  writel(~DIS_INT, adpt->base + EMAC_INT_STATUS);
> 
> This doesn't happen on arm64, and I don't know how to fix it.  DIS_INT is
> defined as:

Probably depends on the compiler version. BTW, clang seems to throw 
errors for this type of thing.

> 
>   #define DIS_INT  BIT(31)
> 
> It seems silly to add a typecast to DIS_INT.

BIT() should use 1U instead of 1.

Rob

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[kbuild test robot]

Hi Gilad,

[auto build test WARNING on net/master]
[also build test WARNING on v4.6-rc3 next-20160413]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improving the system]

url:
https://github.com/0day-ci/linux/commits/Timur-Tabi/net-emac-emac-gigabit-ethernet-controller-driver/20160414-020345
config: ia64-allyesconfig (attached as .config)
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=ia64 

All warnings (new ones prefixed by >>):

   In file included from arch/ia64/include/asm/smp.h:20:0,
from include/linux/smp.h:59,
from include/linux/topology.h:33,
from include/linux/gfp.h:8,
from include/linux/slab.h:14,
from include/linux/textsearch.h:8,
from include/linux/skbuff.h:30,
from include/linux/tcp.h:21,
from drivers/net/ethernet/qualcomm/emac/emac-mac.c:16:
   drivers/net/ethernet/qualcomm/emac/emac-mac.c: In function 'emac_mac_up':
   arch/ia64/include/asm/io.h:395:30: warning: large integer implicitly 
truncated to unsigned type [-Woverflow]
#define writel(v,a) __writel((v), (a))
 ^
>> drivers/net/ethernet/qualcomm/emac/emac-mac.c:1076:2: note: in expansion of 
>> macro 'writel'
 writel(~DIS_INT, adpt->base + EMAC_INT_STATUS);
 ^

vim +/writel +1076 drivers/net/ethernet/qualcomm/emac/emac-mac.c

  1060  return ret;
  1061  
  1062  ret = request_irq(irq->irq, emac_isr, 0, EMAC_MAC_IRQ_RES, irq);
  1063  if (ret) {
  1064  netdev_err(adpt->netdev,
  1065 "error:%d on request_irq(%d:%s flags:0)\n", 
ret,
  1066 irq->irq, EMAC_MAC_IRQ_RES);
  1067  emac_sgmii_down(adpt);
  1068  return ret;
  1069  }
  1070  
  1071  emac_mac_rx_descs_refill(adpt, >rx_q);
  1072  
  1073  napi_enable(>rx_q.napi);
  1074  
  1075  /* enable mac irq */
> 1076  writel(~DIS_INT, adpt->base + EMAC_INT_STATUS);
  1077  writel(adpt->irq.mask, adpt->base + EMAC_INT_MASK);
  1078  
  1079  netif_start_queue(netdev);
  1080  clear_bit(EMAC_STATUS_DOWN, >status);
  1081  
  1082  /* check link status */
  1083  set_bit(EMAC_STATUS_TASK_LSC_REQ, >status);
  1084  adpt->link_chk_timeout = jiffies + EMAC_TRY_LINK_TIMEOUT;

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Florian Fainelli]

On 13/04/16 10:59, Timur Tabi wrote:
> From: Gilad Avidov 
> 
> Add supports for ethernet controller HW on Qualcomm Technologies, Inc. SoC.
> This driver supports the following features:
> 1) Checksum offload.
> 2) Runtime power management support.
> 3) Interrupt coalescing support.
> 4) SGMII phy.
> 5) SGMII direct connection without external phy.

I think you should shoot for more simple for an initial submission:

- no offload
- no timestamping

get that accepted, and then add features one by one, it sure is more
work, but it helps with the review, and makes you work off a solid base.

You will see below, but a pet peeve of mine is authors reimplementing
code that exists in PHYLIB.

[snip]

> diff --git a/Documentation/devicetree/bindings/net/qcom-emac.txt 
> b/Documentation/devicetree/bindings/net/qcom-emac.txt
> new file mode 100644
> index 000..df5e7c0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/qcom-emac.txt
> @@ -0,0 +1,65 @@
> +Qualcomm EMAC Gigabit Ethernet Controller
> +
> +Required properties:
> +- compatible : Should be "qcom,emac".
> +- 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.
> +- phy-addr: Specifies phy address on MDIO bus.
> + Required if the optional property "qcom,no-external-phy"
> + is not specified.

This is not the standard way to represent an Ethernet PHY hanging off a
MDIO bus see ethernet.txt and phy.txt in D/dt/bindings/net/

> +
> +Optional properties:
> +- qcom,emac-tstamp-en   : Enables the PTP (1588) timestamping feature.
> +   Include this only if PTP (1588) timestamping
> +   feature is needed. If included, "ptp" register
> +   base should be specified.

If the "ptp" register range is not specified, then PTP gets disabled, so
is a boolean really required here, considering that this looks like a
policy decision more than anything.

> +- mac-address   : The 6-byte MAC address. If present, it is the
> +   default MAC address.

This property is pretty much mandatory

> +- qcom,no-external-phy  : Indicates there is no external PHY connected to
> +   EMAC. Include this only if the EMAC is directly
> +   connected to the peer end without EPHY.

How is the internal PHY accessed, is it responding on the MDIO bus at a
particular address? If so, standard MDIO scanning/probing works, and you
can have your PHY driver flag this device has internal. Worst case, you
can do what BCMGENET does, and have a special "phy-mode" value set to
"internal" when this knowledge needs to exist prior to MDIO bus scanning
(e.g: to power on the PHY).

> +Example:
> + emac0: qcom,emac@feb2 {
> + compatible = "qcom,fsm9900-emac";
> + reg-names = "base", "csr", "ptp", "sgmii";
> + reg =   <0xfeb2 0x1>,
> + <0xfeb36000 0x1000>,
> + <0xfeb3c000 0x4000>,
> + <0xfeb38000 0x400>;
> + #address-cells = <0>;
> + interrupt-parent = <>;
> + #interrupt-cells = <1>;
> + interrupts = <0 1>;
> + interrupt-map-mask = <0x>;
> + interrupt-map = <0  0 76 0
> +  1  0 80 0>;
> + interrupt-names = "emac_core0", "sgmii_irq";
> + qcom,emac-tstamp-en;
> + phy-addr = <0>;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <_pins_a>;
> + };
> +
> + tlmm: pinctrl@fd51 {
> + compatible = "qcom,fsm9900-pinctrl";
> +
> + mdio_pins_a: mdio {
> + state {
> + pins = "gpio123", "gpio124";
> + function = "mdio";
> + };
> + };
> + };
> diff --git a/drivers/net/ethernet/qualcomm/Kconfig 
> b/drivers/net/ethernet/qualcomm/Kconfig
> index a76e380..85b599f 100644
> --- a/drivers/net/ethernet/qualcomm/Kconfig
> +++ b/drivers/net/ethernet/qualcomm/Kconfig
> @@ -24,4 +24,15 @@ config QCA7000
> To compile this driver as a module, choose M 

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Bjørn Mork]

Timur Tabi  writes:
> Shanker Donthineni wrote:
 >> drivers/net/ethernet/qualcomm/emac/emac-mac.c: In function 
 >> 'emac_mac_up':
  >>drivers/net/ethernet/qualcomm/emac/emac-mac.c:1076:9: warning: 
  >>large integer implicitly truncated to unsigned type [-Woverflow]
 >>   writel(~DIS_INT, adpt->base + EMAC_INT_STATUS);
>>> >
>>> >This doesn't happen on arm64, and I don't know how to fix it.  DIS_INT is 
>>> >defined as:
>>> >
>>> > #define DIS_INT  BIT(31)
>>> >
>> Try with (1U<<31).
>>
>
> Except that Gilad was previously asked to use the BIT() macros:
>
>   https://lkml.org/lkml/2015/12/15/797

So typecast it.

  writel((u32)~DIS_INT, adpt->base + EMAC_INT_STATUS);


I believe the reason you don't see this on arm64 is that the writel
macro includes the typecast there.  But it doesn't on x86_64


Bjørn

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

Shanker Donthineni wrote:

>> drivers/net/ethernet/qualcomm/emac/emac-mac.c: In function 'emac_mac_up':

 >>drivers/net/ethernet/qualcomm/emac/emac-mac.c:1076:9: warning: large 
integer implicitly truncated to unsigned type [-Woverflow]

>>   writel(~DIS_INT, adpt->base + EMAC_INT_STATUS);

>
>This doesn't happen on arm64, and I don't know how to fix it.  DIS_INT is 
defined as:
>
> #define DIS_INT  BIT(31)
>

Try with (1U<<31).



Except that Gilad was previously asked to use the BIT() macros:

https://lkml.org/lkml/2015/12/15/797

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

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Shanker Donthineni]

On 04/13/2016 02:31 PM, Timur Tabi wrote:
> kbuild test robot wrote:
>>
>> drivers/net/ethernet/qualcomm/emac/emac-mac.c: In function 'emac_mac_up':
 >>drivers/net/ethernet/qualcomm/emac/emac-mac.c:1076:9: warning: large 
 >>integer implicitly truncated to unsigned type [-Woverflow]
>>   writel(~DIS_INT, adpt->base + EMAC_INT_STATUS);
>
> This doesn't happen on arm64, and I don't know how to fix it.  DIS_INT is 
> defined as:
>
> #define DIS_INT  BIT(31)
>
Try with (1U<<31).

> It seems silly to add a typecast to DIS_INT.
>

-- 
Shanker Donthineni
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

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

kbuild test robot wrote:


drivers/net/ethernet/qualcomm/emac/emac-mac.c: In function 'emac_mac_up':

>>drivers/net/ethernet/qualcomm/emac/emac-mac.c:1076:9: warning: large integer 
implicitly truncated to unsigned type [-Woverflow]

  writel(~DIS_INT, adpt->base + EMAC_INT_STATUS);


This doesn't happen on arm64, and I don't know how to fix it.  DIS_INT 
is defined as:


#define DIS_INT  BIT(31)

It seems silly to add a typecast to DIS_INT.

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

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

[kbuild test robot]

Hi Gilad,

[auto build test WARNING on net/master]
[also build test WARNING on v4.6-rc3 next-20160413]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improving the system]

url:
https://github.com/0day-ci/linux/commits/Timur-Tabi/net-emac-emac-gigabit-ethernet-controller-driver/20160414-020345
config: x86_64-allmodconfig (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/net/ethernet/qualcomm/emac/emac-mac.c: In function 'emac_mac_up':
>> drivers/net/ethernet/qualcomm/emac/emac-mac.c:1076:9: warning: large integer 
>> implicitly truncated to unsigned type [-Woverflow]
 writel(~DIS_INT, adpt->base + EMAC_INT_STATUS);
^

vim +1076 drivers/net/ethernet/qualcomm/emac/emac-mac.c

  1060  return ret;
  1061  
  1062  ret = request_irq(irq->irq, emac_isr, 0, EMAC_MAC_IRQ_RES, irq);
  1063  if (ret) {
  1064  netdev_err(adpt->netdev,
  1065 "error:%d on request_irq(%d:%s flags:0)\n", 
ret,
  1066 irq->irq, EMAC_MAC_IRQ_RES);
  1067  emac_sgmii_down(adpt);
  1068  return ret;
  1069  }
  1070  
  1071  emac_mac_rx_descs_refill(adpt, >rx_q);
  1072  
  1073  napi_enable(>rx_q.napi);
  1074  
  1075  /* enable mac irq */
> 1076  writel(~DIS_INT, adpt->base + EMAC_INT_STATUS);
  1077  writel(adpt->irq.mask, adpt->base + EMAC_INT_MASK);
  1078  
  1079  netif_start_queue(netdev);
  1080  clear_bit(EMAC_STATUS_DOWN, >status);
  1081  
  1082  /* check link status */
  1083  set_bit(EMAC_STATUS_TASK_LSC_REQ, >status);
  1084  adpt->link_chk_timeout = jiffies + EMAC_TRY_LINK_TIMEOUT;

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data