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

[Timur Tabi]

Florian Fainelli wrote:

There is a helper function to obtain the platform device associated with a 
device_node: of_find_device_by_node.


Thank you, this is exactly what I needed.

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Florian Fainelli]

On August 17, 2016 9:19:23 PM PDT, Timur Tabi  wrote:
>Florian Fainelli wrote:
>> The larger issue is that the emac_sgmii node in the form you posted
>> is going to be backed by a platform device in Linux while you want a
>> PHY device with a reg property that describes a MDIO address
>> (#address-cells = 1, #size-cells = 0).
>
>But how do I get the platform device for the emac_sgmii node?  If I 
>create an of_device_id ID for it, like this:
>
>static const struct of_device_id emac_dt_match[] = {
>   {
>   .compatible = "qcom,fsm9900-emac",
>   },
>   {}
>};
>
>static const struct of_device_id emac_sgmii_dt_match[] = {
>   {
>   .compatible = "qcom,fsm9900-emac-sgmii",
>   },
>   {}
>};
>
>Then the probe function will be called for qcom,fsm9900-emac-sgmii 
>separately from qcom,fsm9900-emac, which just confuses things. So I 
>can't create emac_sgmii_dt_match.
>
>I know this is standard DT stuff, and I used to do a lot of work on DT 
>so maybe I should know this already.  But it seems to me that I need to
>
>manually create the platform_device for qcom,fsm9900-emac-sgmii.

There is a helper function to obtain the platform device associated with a 
device_node: of_find_device_by_node. AFAICT probe ordering is based on DT 
ordering, but if there was a dependency order to solve you could use 
EPROBE_DEFER until the emac driver finds its dependencies.

>
>> IIRC the amd xgbe driver mainline had a similar design but still
>> implemented a PHY device anyway although it may not have been using
>> Device Tree. It should still be possible to implement a PHY driver
>> that you manually register and bind to its device_node pointer such
>> that of_phy_find_device and friends still work. You would do this
>> from the emac_sgmii platform device driver and parent devices in a
>> way that satisfy the PHY device driver lifecycle as well.
>>
>> Hope this helps.
>
>It doesn't, sorry.  The emac_sgmii is really just another register
>block 
>for the driver to program.  Creating another PHY driver for it doesn't 
>really make sense.  It's not on an MDIO bus.

First of a PHY does not have to be on a MDIO bus, it just happens to be the 
most common thing, and second, this is a PHY of some kind as far as the 
Ethernet MAC is concerned and this is the abstraction you want at the MAC 
driver level so you can plug arbitrary external PHYs or internal PHYs 
irrespective of their backing transport/bus layer.

As long as your DT representation is sane and accurate, however this ends up 
being implemented matters less. Whether the PHY device model suits you here, 
you need to tell the emac0 node that it is bound to the emac_sgmii node somehow 
since there is programming of this block required for operating and for that a 
phandle property of some kind is required, phy-handle is a standard one which 
would help with the driver acceptance IMHO.


-- 
Florian

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

[Timur Tabi]

Florian Fainelli wrote:

The larger issue is that the emac_sgmii node in the form you posted
is going to be backed by a platform device in Linux while you want a
PHY device with a reg property that describes a MDIO address
(#address-cells = 1, #size-cells = 0).


But how do I get the platform device for the emac_sgmii node?  If I 
create an of_device_id ID for it, like this:


static const struct of_device_id emac_dt_match[] = {
{
.compatible = "qcom,fsm9900-emac",
},
{}
};

static const struct of_device_id emac_sgmii_dt_match[] = {
{
.compatible = "qcom,fsm9900-emac-sgmii",
},
{}
};

Then the probe function will be called for qcom,fsm9900-emac-sgmii 
separately from qcom,fsm9900-emac, which just confuses things. So I 
can't create emac_sgmii_dt_match.


I know this is standard DT stuff, and I used to do a lot of work on DT 
so maybe I should know this already.  But it seems to me that I need to 
manually create the platform_device for qcom,fsm9900-emac-sgmii.



IIRC the amd xgbe driver mainline had a similar design but still
implemented a PHY device anyway although it may not have been using
Device Tree. It should still be possible to implement a PHY driver
that you manually register and bind to its device_node pointer such
that of_phy_find_device and friends still work. You would do this
from the emac_sgmii platform device driver and parent devices in a
way that satisfy the PHY device driver lifecycle as well.

Hope this helps.


It doesn't, sorry.  The emac_sgmii is really just another register block 
for the driver to program.  Creating another PHY driver for it doesn't 
really make sense.  It's not on an MDIO bus.


--
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] [v7] net: emac: emac gigabit ethernet controller driver

[Florian Fainelli]

On August 17, 2016 8:27:19 PM PDT, Timur Tabi  wrote:
>Florian Fainelli wrote:
>
>>>  emac_sgmii: ethernet-phy@410400 {
>>>  compatible = "qcom,qdf2432-emac-phy";
>>>  reg = <0x0 0x00410400 0x0 0x100>;
>>>  interrupts = <0 0x104 0>;
>>>  };
>>>
>>
>> Is this register range relative to the emac0 node here, or is this
>> really a separate node, within the same adress space as your emac0
>node?
>
>It's a separate node within the same address space.  We can't guarantee
>
>that it's adjacent to any other EMAC register -- it could be anywhere
>in 
>memory. 0x00410400 is the real physical address of those registers.

Ok

>
>> Answer largely depends on whether your device is really located
>outside
>> of the emac, if it located outside, then a platform device matching
>the
>> compatible string would get you what you want. If the emac_sgmii
>block
>> is a sub-block within the EMAC, then a few things need fixing:
>>
>> - your emac_sgmii node should be a sub-node of the emac node, not a
>sibling
>> - the emac0 node should have a "ranges" property that indicates how
>to
>> translate the sub-nodes' "reg" property based on the base register
>> address of the emac0 block
>> - you would have to call of_platform_populate from the EMAC driver to
>> ensure that the emac_sgmii child node and therefore platform device
>gets
>> created
>
>Even if the emac_sgmii block were a sub-block, wouldn't it conflict
>with 
>the ethernet-phy@4 node?  The #address-cells and #size-cells properties
>
>cannot be valid for both the emac_sgmii and ethernet-phy@4 nodes.

These two properties apply to subnodes within the emac0 and emac_sgmii nodes, 
this is standard DT stuff here.

The larger issue is that the emac_sgmii node in the form you posted is going to 
be backed by a platform device in Linux while you want a PHY device with a reg 
property that describes a MDIO address (#address-cells = 1, #size-cells = 0). 

IIRC the amd xgbe driver mainline had a similar design but still implemented a 
PHY device anyway although it may not have been using Device Tree. It should 
still be possible to implement a PHY driver that you manually register and bind 
to its device_node pointer such that of_phy_find_device and friends still work. 
You would do this from the emac_sgmii platform device driver and parent devices 
in a way that satisfy the PHY device driver lifecycle as well.

Hope this helps.

NB: another way to do this is to give the SGMII PHY device a MDIO address, 
provided that it has one, and you pass the memory mapped register range as a 
syscon property pointing to the emac_sgmii register range. This is not really 
clean through.


-- 
Florian

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

[Timur Tabi]

Florian Fainelli wrote:


 emac_sgmii: ethernet-phy@410400 {
 compatible = "qcom,qdf2432-emac-phy";
 reg = <0x0 0x00410400 0x0 0x100>;
 interrupts = <0 0x104 0>;
 };



Is this register range relative to the emac0 node here, or is this
really a separate node, within the same adress space as your emac0 node?


It's a separate node within the same address space.  We can't guarantee 
that it's adjacent to any other EMAC register -- it could be anywhere in 
memory. 0x00410400 is the real physical address of those registers.



Answer largely depends on whether your device is really located outside
of the emac, if it located outside, then a platform device matching the
compatible string would get you what you want. If the emac_sgmii block
is a sub-block within the EMAC, then a few things need fixing:

- your emac_sgmii node should be a sub-node of the emac node, not a sibling
- the emac0 node should have a "ranges" property that indicates how to
translate the sub-nodes' "reg" property based on the base register
address of the emac0 block
- you would have to call of_platform_populate from the EMAC driver to
ensure that the emac_sgmii child node and therefore platform device gets
created


Even if the emac_sgmii block were a sub-block, wouldn't it conflict with 
the ethernet-phy@4 node?  The #address-cells and #size-cells properties 
cannot be valid for both the emac_sgmii and ethernet-phy@4 nodes.


--
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] [v7] net: emac: emac gigabit ethernet controller driver

[Florian Fainelli]

On 08/17/2016 03:32 PM, Timur Tabi wrote:
> Timur Tabi wrote:
>>>
>>> Nothing prevents you from detailing in ACPI or DT sub-components of a
>>> larger HW block, if there is such a desire, but just make it in a way
>>> that it looks like what would be done for e.g: separate discrete parts,
>>> only the parenting of nodes would change.
>>
>> So instead of just having a single property, instead create a child node
>> just for the SGMII phy, with its own compatible string and various other
>> properties (like interrupt and base register)?  That's not a bad idea.
> 
> I need help getting this to work.
> 
> emac0: ethernet@3880 {
> compatible = "qcom,qdf2432-emac";
> reg-names = "base", "csr";
> reg = <0x0 0x3880 0x0 0x1 >,
> <0x0 0x38816000 0x0 0x1000>,
> <0x0 0x3881C000 0x0 0x4000>;
> interrupts = <0 0x100 0 0 0x104 0>;
> interrupt-names = "core0", "sgmii";
> 
> sgmii-handle = <_sgmii>;
> phy-handle = <>;
> 
> #address-cells = <1>;
> #size-cells = <0>;
> phy0: ethernet-phy@4 {
> reg = <4>;
> };
> };
> 
> emac_sgmii: ethernet-phy@410400 {
> compatible = "qcom,qdf2432-emac-phy";
> reg = <0x0 0x00410400 0x0 0x100>;
> interrupts = <0 0x104 0>;
> };
> 

Is this register range relative to the emac0 node here, or is this
really a separate node, within the same adress space as your emac0 node?

> When my driver probes, the platform_device object points to the emac0
> node, as always.
> 
> How do I parse the emac_sgmii node?  How do I get functions like
> platform_get_resource() to work?  How do I create a new platform_device
> object that points to the emac_sgmii node?

Answer largely depends on whether your device is really located outside
of the emac, if it located outside, then a platform device matching the
compatible string would get you what you want. If the emac_sgmii block
is a sub-block within the EMAC, then a few things need fixing:

- your emac_sgmii node should be a sub-node of the emac node, not a sibling
- the emac0 node should have a "ranges" property that indicates how to
translate the sub-nodes' "reg" property based on the base register
address of the emac0 block
- you would have to call of_platform_populate from the EMAC driver to
ensure that the emac_sgmii child node and therefore platform device gets
created


-- 
Florian

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

[Timur Tabi]

Timur Tabi wrote:


Nothing prevents you from detailing in ACPI or DT sub-components of a
larger HW block, if there is such a desire, but just make it in a way
that it looks like what would be done for e.g: separate discrete parts,
only the parenting of nodes would change.


So instead of just having a single property, instead create a child node
just for the SGMII phy, with its own compatible string and various other
properties (like interrupt and base register)?  That's not a bad idea.


I need help getting this to work.

emac0: ethernet@3880 {
compatible = "qcom,qdf2432-emac";
reg-names = "base", "csr";
reg = <0x0 0x3880 0x0 0x1 >,
<0x0 0x38816000 0x0 0x1000>,
<0x0 0x3881C000 0x0 0x4000>;
interrupts = <0 0x100 0 0 0x104 0>;
interrupt-names = "core0", "sgmii";

sgmii-handle = <_sgmii>;
phy-handle = <>;

#address-cells = <1>;
#size-cells = <0>;
phy0: ethernet-phy@4 {
reg = <4>;
};
};

emac_sgmii: ethernet-phy@410400 {
compatible = "qcom,qdf2432-emac-phy";
reg = <0x0 0x00410400 0x0 0x100>;
interrupts = <0 0x104 0>;
};

When my driver probes, the platform_device object points to the emac0 
node, as always.


How do I parse the emac_sgmii node?  How do I get functions like 
platform_get_resource() to work?  How do I create a new platform_device 
object that points to the emac_sgmii node?


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Timur Tabi]

Florian Fainelli wrote:


If it has self identification, and the MAC is capable of knowing what
kind of PHY it is internally bundled with, why does it matter to
represent that specific piece of information in Device Tree or ACPI?


That's the thing - the MAC is not capable of knowing.  From a hardware 
perspective, the MAC and PHY are physically distinct IP blocks, and so 
the MAC hardware has no idea what it's really connected to.  In theory, 
we don't even need an internal PHY, but if there is an internal PHY, we 
have to program it correctly.


This is why we need a DT and ACPI property (or whatever) to tell the 
driver what kind of PHY it's attached to.



The problem is that the internal PHY has mechanism for
self-identification.  There is no ACPI or device tree node for it. There
is no register you can query to see if it's there or what version it
is.  The MAC part of the EMAC has no knowledge of the PHY part.  The
driver just has to know it's there.


Nothing prevents you from detailing in ACPI or DT sub-components of a
larger HW block, if there is such a desire, but just make it in a way
that it looks like what would be done for e.g: separate discrete parts,
only the parenting of nodes would change.


So instead of just having a single property, instead create a child node 
just for the SGMII phy, with its own compatible string and various other 
properties (like interrupt and base register)?  That's not a bad idea.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Florian Fainelli]

On 08/16/2016 02:37 PM, Timur Tabi wrote:
> Al Stone wrote:
>> Does the ACPI portion of the driver*have*  to know about the PHY?  In
>> general,
>> the ACPI assumption on ARM [**] is that those have all been set up
>> before we
>> get to the kernel.  So, does it need to be visible to the ACPI part of
>> the
>> driver at all?
> 
> Yes, the driver supports both "v1" and "v2" of the PHY, and it has code
> like this:
> 
> if (adpt->phy.version == 2)
> emac_sgmii_init_v2(adpt);
> else
> emac_sgmii_init_v1(adpt);
> 
> The question, how should adpt->phy.version be initialized on device tree
> and ACPI platforms?
> 
> The reason why this is confusing is because there are conflicting
> perspectives of this "internal PHY".  It both is and is not part of the
> EMAC.  It is a separate block in the chip, and can be replaced with
> other SGMII blocks, and in the future there will be a "v3" and maybe a
> "v4" or who knows what.  In fact, maybe using version numbers is
> inappropriate and we'll need to use vendor names or something.

If it has self identification, and the MAC is capable of knowing what
kind of PHY it is internally bundled with, why does it matter to
represent that specific piece of information in Device Tree or ACPI?

> 
> The problem is that the internal PHY has mechanism for
> self-identification.  There is no ACPI or device tree node for it. There
> is no register you can query to see if it's there or what version it
> is.  The MAC part of the EMAC has no knowledge of the PHY part.  The
> driver just has to know it's there.

Nothing prevents you from detailing in ACPI or DT sub-components of a
larger HW block, if there is such a desire, but just make it in a way
that it looks like what would be done for e.g: separate discrete parts,
only the parenting of nodes would change.

> 
> This is why I'm advocating a separate property (DT and ACPI) to identify
> the internal PHY
> 
> The truth is that I don't really care HOW it's done, and as long as we
> come to a consensus soon.  I want my driver to be merged into 4.9.
> 


-- 
Florian

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

[Timur Tabi]

Timur Tabi wrote:


The question, how should adpt->phy.version be initialized on device tree
and ACPI platforms?

The reason why this is confusing is because there are conflicting
perspectives of this "internal PHY".  It both is and is not part of the
EMAC.  It is a separate block in the chip, and can be replaced with
other SGMII blocks, and in the future there will be a "v3" and maybe a
"v4" or who knows what.  In fact, maybe using version numbers is
inappropriate and we'll need to use vendor names or something.

The problem is that the internal PHY has mechanism for
self-identification.  There is no ACPI or device tree node for it. There
is no register you can query to see if it's there or what version it
is.  The MAC part of the EMAC has no knowledge of the PHY part.  The
driver just has to know it's there.

This is why I'm advocating a separate property (DT and ACPI) to identify
the internal PHY.


So we had some internal discussion on this.  Different compatible 
properties for DT are okay, and for ACPI we'll either create a 
"phy-version" property (DT-like properties are called DSDs in ACPI) or 
use an HRV (a specific property for the hardware version).


But this speaks to the greater problem of mapping DT compatible strings 
to ACPI.  Like I said, the ACPI equivalent of a compatible string is the 
HID.  But unlike DT, we cannot create new HIDs whenever we feel like it. 
 The process for generating and assigning HIDs is much more involved, 
and we do have a very limited namespace.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Timur Tabi]

Al Stone wrote:

Does the ACPI portion of the driver*have*  to know about the PHY?  In general,
the ACPI assumption on ARM [**] is that those have all been set up before we
get to the kernel.  So, does it need to be visible to the ACPI part of the
driver at all?


Yes, the driver supports both "v1" and "v2" of the PHY, and it has code 
like this:


if (adpt->phy.version == 2)
emac_sgmii_init_v2(adpt);
else
emac_sgmii_init_v1(adpt);

The question, how should adpt->phy.version be initialized on device tree 
and ACPI platforms?


The reason why this is confusing is because there are conflicting 
perspectives of this "internal PHY".  It both is and is not part of the 
EMAC.  It is a separate block in the chip, and can be replaced with 
other SGMII blocks, and in the future there will be a "v3" and maybe a 
"v4" or who knows what.  In fact, maybe using version numbers is 
inappropriate and we'll need to use vendor names or something.


The problem is that the internal PHY has mechanism for 
self-identification.  There is no ACPI or device tree node for it. 
There is no register you can query to see if it's there or what version 
it is.  The MAC part of the EMAC has no knowledge of the PHY part.  The 
driver just has to know it's there.


This is why I'm advocating a separate property (DT and ACPI) to identify 
the internal PHY.


The truth is that I don't really care HOW it's done, and as long as we 
come to a consensus soon.  I want my driver to be merged into 4.9.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Al Stone]

On 08/16/2016 07:39 AM, Timur Tabi wrote:
> Rob Herring wrote:
> 
>>> In ACPI, the equivalent to a compatible string is the HID, which is QCOM8070
>>> for the EMAC.  The problem is that it's very difficult, if not impossible,
>>> to create new HIDs for different versions of the same device.
>>
>> Different versions are different devices IMO.
> 
> Not that I disagree, but this appears to be an inherent problem with ACPI.  
> The
> namespace for ACPI HIDs is very limited.  We only really have control over the
> last two digits.

My apologies if the stuff below is things you already know...I never know how
much people already know about ACPI, and sometimes just don't know when to shut
up :).

An _HID is a three or four character company ID followed by four digits that
the company decides on.  The only thing the ASWG (ACPI Spec Working Group)
controls are those first characters; you can do whatever you'd like with the
four digits that follow.

Not knowing enough about the products in mind here, or future plans for them,
I'd recommend taking a look at section 6.1 of the spec.  There's an ACPI object
called _HRV (Hardware Revision) that may do what you need; there is also the
_CID (Compatible ID), or if this is a PCI device, perhaps the _CLS (Class Code)
can be used.  I guess what I'm really trying to say is that the "name space"
mentioned here [*] is not that limited and has quite a bit of flexibility.


[*] "ACPI namespace" has a fairly specific meaning that is different.

>>> The other problem is that the "internal PHY" of the EMAC is technically a
>>> separate device, and it's interchangeable.  Future versions of our chips
>>> will use different internal PHYs, but the EMAC will stay the same.
>>
>> How do you know? EMAC could just as easily change. It's Gigabit today,
>> 10G tomorrow.
> 
> My point is that the EMAC part won't change for the foreseeable future, but I
> know the internal PHY component will change.  The new "version" of the 
> EMAC/PHY
> combo on a future chip will have the same ACPI HID.  So I need some other way 
> to
> differentiate the two.  I can't query the hardware, because the EMAC half will
> be identical.
> 
>> But if it is separate, then maybe you should model it as a separate
>> device using the phy binding.
> 
> It's only separate in hardware.  The driver controls both parts as a unified 
> whole.
> 
>>> So I would like a solution that works on DT and ACPI.  I suppose I could use
>>> compatible strings on DT, and a "phy-version" DSD (property) on ACPI.  If
>>> that's acceptable to everyone, then I can do that.  It seems clunky to me.
>>
>> On one hand, why should I care about ACPI for defining DT bindings?
>> OTOH, having a phy-version property alone would not be a big deal, but
>> you still need distinct compatible strings regardless.
> 
> So you're saying that it's okay to have separate compatible strings AND a
> phy-version property?  That would solve the problem.
> 

Does the ACPI portion of the driver *have* to know about the PHY?  In general,
the ACPI assumption on ARM [**] is that those have all been set up before we
get to the kernel.  So, does it need to be visible to the ACPI part of the
driver at all?


[**] See Documentation/arm64/arm-acpi.txt.

-- 
ciao,
al
---
Al Stone
Software Engineer
Linaro Enterprise Group
al.st...@linaro.org
---

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

[Timur Tabi]

Rob Herring wrote:


In ACPI, the equivalent to a compatible string is the HID, which is QCOM8070
for the EMAC.  The problem is that it's very difficult, if not impossible,
to create new HIDs for different versions of the same device.


Different versions are different devices IMO.


Not that I disagree, but this appears to be an inherent problem with 
ACPI.  The namespace for ACPI HIDs is very limited.  We only really have 
control over the last two digits.



The other problem is that the "internal PHY" of the EMAC is technically a
separate device, and it's interchangeable.  Future versions of our chips
will use different internal PHYs, but the EMAC will stay the same.


How do you know? EMAC could just as easily change. It's Gigabit today,
10G tomorrow.


My point is that the EMAC part won't change for the foreseeable future, 
but I know the internal PHY component will change.  The new "version" of 
the EMAC/PHY combo on a future chip will have the same ACPI HID.  So I 
need some other way to differentiate the two.  I can't query the 
hardware, because the EMAC half will be identical.



But if it is separate, then maybe you should model it as a separate
device using the phy binding.


It's only separate in hardware.  The driver controls both parts as a 
unified whole.



So I would like a solution that works on DT and ACPI.  I suppose I could use
compatible strings on DT, and a "phy-version" DSD (property) on ACPI.  If
that's acceptable to everyone, then I can do that.  It seems clunky to me.


On one hand, why should I care about ACPI for defining DT bindings?
OTOH, having a phy-version property alone would not be a big deal, but
you still need distinct compatible strings regardless.


So you're saying that it's okay to have separate compatible strings AND 
a phy-version property?  That would solve the problem.


--
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] [v7] net: emac: emac gigabit ethernet controller driver

[Rob Herring]

On Mon, Aug 15, 2016 at 3:06 PM, Timur Tabi  wrote:
> + Rafael and Al, for ACPI help.
>
> Rob Herring wrote:
>>>
>>> >+Optional properties:
>>> >+- phy-version : the version of the integrated emac phy, either 1 or 2.
>
>
>> Sounds like 2 different h/w. The compatible property should distinguish
>> this.
>
>
> So I implemented this in v8 of my driver, but it is causing problems for
> ACPI.
>
> In ACPI, the equivalent to a compatible string is the HID, which is QCOM8070
> for the EMAC.  The problem is that it's very difficult, if not impossible,
> to create new HIDs for different versions of the same device.

Different versions are different devices IMO.

> The driver will primarily (if not exclusively) be used on ACPI platforms,
> not DT.  In Qualcomm, DT is for mobile chips, and ACPI is for server.  To my
> knowledge, there are no plans to use this driver on an actual DT-enabled
> platform, but I want to support DT anyway.
>
> The other problem is that the "internal PHY" of the EMAC is technically a
> separate device, and it's interchangeable.  Future versions of our chips
> will use different internal PHYs, but the EMAC will stay the same.

How do you know? EMAC could just as easily change. It's Gigabit today,
10G tomorrow.

But if it is separate, then maybe you should model it as a separate
device using the phy binding.

> So I would like a solution that works on DT and ACPI.  I suppose I could use
> compatible strings on DT, and a "phy-version" DSD (property) on ACPI.  If
> that's acceptable to everyone, then I can do that.  It seems clunky to me.

On one hand, why should I care about ACPI for defining DT bindings?
OTOH, having a phy-version property alone would not be a big deal, but
you still need distinct compatible strings regardless.

Rob

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

[Timur Tabi]

+ Rafael and Al, for ACPI help.

Rob Herring wrote:

>+Optional properties:
>+- phy-version : the version of the integrated emac phy, either 1 or 2.



Sounds like 2 different h/w. The compatible property should distinguish
this.


So I implemented this in v8 of my driver, but it is causing problems for 
ACPI.


In ACPI, the equivalent to a compatible string is the HID, which is 
QCOM8070 for the EMAC.  The problem is that it's very difficult, if not 
impossible, to create new HIDs for different versions of the same device.


The driver will primarily (if not exclusively) be used on ACPI 
platforms, not DT.  In Qualcomm, DT is for mobile chips, and ACPI is for 
server.  To my knowledge, there are no plans to use this driver on an 
actual DT-enabled platform, but I want to support DT anyway.


The other problem is that the "internal PHY" of the EMAC is technically 
a separate device, and it's interchangeable.  Future versions of our 
chips will use different internal PHYs, but the EMAC will stay the same.


So I would like a solution that works on DT and ACPI.  I suppose I could 
use compatible strings on DT, and a "phy-version" DSD (property) on 
ACPI.  If that's acceptable to everyone, then I can do that.  It seems 
clunky to me.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Timur Tabi]

Timur Tabi wrote:




The hang occurs with this loop in napi_disable():

 while (test_and_set_bit(NAPI_STATE_SCHED, >state))
 msleep(1);

This loop never exits.  Can you give me a clue as to what could be the
reason?


I figured it out.  I cannot call napi_disable() twice in a row.  It's
not smart enough to know that NAPI is already disabled.


I realized that I was going about this the wrong way.  Rather than try 
to protect the setting of adpt->rxbuf_size, I instead just calculate 
that value when RX path is configured, which is much cleaner.  This 
ensures that adpt->rxbuf_size is changed only when the interface is down.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Timur Tabi]

Timur Tabi wrote:


The hang occurs with this loop in napi_disable():

 while (test_and_set_bit(NAPI_STATE_SCHED, >state))
 msleep(1);

This loop never exits.  Can you give me a clue as to what could be the
reason?


I figured it out.  I cannot call napi_disable() twice in a row.  It's 
not smart enough to know that NAPI is already disabled.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Timur Tabi]

Florian Fainelli wrote:

 netif_stop_queue(netdev);
> napi_disable(>rx_q.napi);
>
>I cannot call just napi_disable() in emac_change_mtu(), because when I
>then call emac_mac_down(), the first thing it does is call
>netif_stop_queue(), and that's when I timeout/hang.

Whatever emac_mac_down() does you can unroll it in the change_mtu
callback anyway, so if this a problematic sequence you can work around it.


I'm having difficulty coming up with an actual sequence that works. This 
odd interaction between netif_stop_queue() and napi_disable() is giving 
me inconsistent results.


The hang occurs with this loop in napi_disable():

while (test_and_set_bit(NAPI_STATE_SCHED, >state))
msleep(1);

This loop never exits.  Can you give me a clue as to what could be the 
reason?



>Unfortunately, I cannot even do this:
>
> netif_stop_queue(netdev);
> napi_disable(>rx_q.napi);
> netif_stop_queue(netdev);
> napi_disable(>rx_q.napi);
>
>Even though I've already called netif_stop_queue(), calling it again
>causes the timeout/hang.



Buf if this is really what you copy/pasted here, why do this twice anyway?


Sorry, what I meant was that I was hoping that *both* emac_change_mtu() 
and emac_mac_down() could do


netif_stop_queue(netdev);
napi_disable(>rx_q.napi);

Which would have the effect of what I wrote above.

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Florian Fainelli]

On 08/10/2016 09:38 AM, Timur Tabi wrote:
> Florian Fainelli wrote:
>>> >Is there an easy way for me to stop the RX path before I set
>>> rxbuf_size?
>>> >  Some netif_xxx function I can call?
>> napi_disable() should take care of that.
> 
> It appears that if I call netif_stop_queue() *afer* calling
> napi_disable(), I get a hang and/or TX timeout.  Since emac_mac_down()
> does this:
> 
> netif_stop_queue(netdev);
> napi_disable(>rx_q.napi);
> 
> I cannot call just napi_disable() in emac_change_mtu(), because when I
> then call emac_mac_down(), the first thing it does is call
> netif_stop_queue(), and that's when I timeout/hang.

Whatever emac_mac_down() does you can unroll it in the change_mtu
callback anyway, so if this a problematic sequence you can work around it.

> 
> Unfortunately, I cannot even do this:
> 
> netif_stop_queue(netdev);
> napi_disable(>rx_q.napi);
> netif_stop_queue(netdev);
> napi_disable(>rx_q.napi);
> 
> Even though I've already called netif_stop_queue(), calling it again
> causes the timeout/hang.

Buf if this is really what you copy/pasted here, why do this twice anyway?

> 
> Is this expected?  I never understood why I needed to call
> netif_stop_queue() before napi_disable().  I do see some drivers do not
> call netif_stop_queue().  I even saw a driver that calls them in reverse
> order, so I don't understand why that sequence breaks for me but not him.
> 

Not clear how the two relate with each other here, must be specific to
your driver implementation somehow.
-- 
Florian

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

[Timur Tabi]

Florian Fainelli wrote:

>Is there an easy way for me to stop the RX path before I set rxbuf_size?
>  Some netif_xxx function I can call?

napi_disable() should take care of that.


It appears that if I call netif_stop_queue() *afer* calling 
napi_disable(), I get a hang and/or TX timeout.  Since emac_mac_down() 
does this:


netif_stop_queue(netdev);
napi_disable(>rx_q.napi);

I cannot call just napi_disable() in emac_change_mtu(), because when I 
then call emac_mac_down(), the first thing it does is call 
netif_stop_queue(), and that's when I timeout/hang.


Unfortunately, I cannot even do this:

netif_stop_queue(netdev);
napi_disable(>rx_q.napi);
netif_stop_queue(netdev);
napi_disable(>rx_q.napi);

Even though I've already called netif_stop_queue(), calling it again 
causes the timeout/hang.


Is this expected?  I never understood why I needed to call 
netif_stop_queue() before napi_disable().  I do see some drivers do not 
call netif_stop_queue().  I even saw a driver that calls them in reverse 
order, so I don't understand why that sequence breaks for me but not him.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Florian Fainelli]

On 08/09/2016 06:09 PM, Timur Tabi wrote:
> Florian Fainelli wrote:
> 
>> nr_frags can't be bigger than MAX_SKB_FRAGS, hence these checks all
>> other drivers do against 1 + MAX_SKB_FRAGS.
> 
> Doh, I just realized something.  emac_mac_tx_buf_send() just needs to
> make sure that there's enough room for ONE skb.  For some reason I
> thought it had to make sure there's enough room for multiple SKBs.
> 
> Now it makes a lot more sense.  Thank you.
> 
> So it looks like a given SKB can occupy 3 + nr_frags descriptors.  So I
> need to change that line to:
> 
> if (emac_tpd_num_free_descs(tx_q) < (MAX_SKB_FRAGS + 3))
> netif_stop_queue(adpt->netdev);
> 
> Question, some  drivers do <= instead of just <, like this:
> 
> if (ring->free_bds <= (MAX_SKB_FRAGS + 1))
> netif_tx_stop_queue(txq);
> 
> Is it necessary to stop the queue if there exactly enough descriptors to
> hold an SKB?  Shouldn't the above be this instead:
> 
> if (ring->free_bds < (MAX_SKB_FRAGS + 1))
> netif_tx_stop_queue(txq);

Humm, it kind of depends, but I would err on the side of strictly lesser
than as a better behavior where you may still allow a full fragmented
SKB to make it through on the next xmit call (I know the code you quote
is exactly not doing that).

> 
> 
>>> However, I'm confused about one thing.  Almost every other driver just
>>> sets "netdev->mtu = new_mtu" and does nothing else.  I can't find any
>>> other driver that actually stops the RX path, reprograms the hardware,
>>> and then restarts the RX path.  I know this is a stupid question, but
>>> why is my driver doing that?
>>
>> Most drivers allocate RX buffer sizes that are usually bigger than the
>> MTU, but would probably silently fail or expose transient behavior once
>> the MTU changes to greater than the size pre-defined.
> 
> So it looks like the real problem is a race condition between
> 
> adpt->rxbuf_size = new_mtu > EMAC_DEF_RX_BUF_SIZE ?
> ALIGN(max_frame, 8) : EMAC_DEF_RX_BUF_SIZE;
> 
> and
> 
> if (netif_running(netdev))
> return emac_reinit_locked(adpt);
> 
> 
> That is, if the interface is running, I set rxbuf_size.  If suddenly I
> receive some packets, then the driver will use the wrong buffer size.

Correct, and possibly other HW settings that you have may have to
program to tell the MAC what the maximum packet length should be.

> 
> Is there an easy way for me to stop the RX path before I set rxbuf_size?
>  Some netif_xxx function I can call?

napi_disable() should take care of that.
-- 
Florian

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

[Timur Tabi]

Florian Fainelli wrote:


nr_frags can't be bigger than MAX_SKB_FRAGS, hence these checks all
other drivers do against 1 + MAX_SKB_FRAGS.


Doh, I just realized something.  emac_mac_tx_buf_send() just needs to 
make sure that there's enough room for ONE skb.  For some reason I 
thought it had to make sure there's enough room for multiple SKBs.


Now it makes a lot more sense.  Thank you.

So it looks like a given SKB can occupy 3 + nr_frags descriptors.  So I 
need to change that line to:


if (emac_tpd_num_free_descs(tx_q) < (MAX_SKB_FRAGS + 3))
netif_stop_queue(adpt->netdev);

Question, some  drivers do <= instead of just <, like this:

if (ring->free_bds <= (MAX_SKB_FRAGS + 1))
netif_tx_stop_queue(txq);

Is it necessary to stop the queue if there exactly enough descriptors to 
hold an SKB?  Shouldn't the above be this instead:


if (ring->free_bds < (MAX_SKB_FRAGS + 1))
netif_tx_stop_queue(txq);



However, I'm confused about one thing.  Almost every other driver just
sets "netdev->mtu = new_mtu" and does nothing else.  I can't find any
other driver that actually stops the RX path, reprograms the hardware,
and then restarts the RX path.  I know this is a stupid question, but
why is my driver doing that?


Most drivers allocate RX buffer sizes that are usually bigger than the
MTU, but would probably silently fail or expose transient behavior once
the MTU changes to greater than the size pre-defined.


So it looks like the real problem is a race condition between

adpt->rxbuf_size = new_mtu > EMAC_DEF_RX_BUF_SIZE ?
ALIGN(max_frame, 8) : EMAC_DEF_RX_BUF_SIZE;

and

if (netif_running(netdev))
return emac_reinit_locked(adpt);


That is, if the interface is running, I set rxbuf_size.  If suddenly I 
receive some packets, then the driver will use the wrong buffer size.


Is there an easy way for me to stop the RX path before I set rxbuf_size? 
 Some netif_xxx function I can call?



--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Timur Tabi]

Timur Tabi wrote:

I used to work on PowerPC, so I respect making things work for both
endians.  However, even I think that this is overkill for my driver.
First, there's no way this driver will ever be used on a big-endian
system.  Second, I'm pretty sure there are lots of places that would
need cpu_to_le32() in order to make this driver big-endian compatible.
It would be a huge mess.

#define TPD_BUFFER_ADDR_H_SET(tpd, val)BITS_SET((tpd)->word[3], 18,
30, val)

This macros sets specific bits based on the definition of the register.
  I could change it to this:

#define TPD_BUFFER_ADDR_H_SET(tpd, val) BITS_SET((tpd)->word[3], 18, 30,
cpu_to_le32(val))

But I honestly don't see what good that will do.  There are still
thousands of other places that assume little-endian.


Ok, so I took another look at this, and even though I still think that 
it's useless, it seems to be much less difficult to implement than I 
initially thought.


I think all I need to do is to modify the BITS_GET() and BITS_SET() 
macros in emac-mac.h, as well as any of the RRD_xxx and TPD_xxx macros 
that do not use BITS_GET() or BITS_SET().  This is a minor change which 
I will implement in v8 of the patch.


Every other hardware access uses readl/writel, which is already endian-safe.

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Florian Fainelli]

On 08/09/2016 11:25 AM, Timur Tabi wrote:
> I need some help figuring that out.  Like I said, I didn't write this
> driver initially, so there are parts that I don't really understand.  I
> copied the above code from other drivers, but after studying your
> question, I think I understand what you're asking.  I just don't know
> how to fix it.
> 
> First of all, why do other drivers test MAX_SKB_FRAGS + 1?  Why the +1?

The 1 is for the non-fragment part of the SKB, like its head.

> 
> The driver originally used function emac_tx_has_enough_descs() to
> determine if there is enough room for the new packet.  Then I changed
> the code as you suggested, and now it guesses how many descriptors need
> to be free to support the next packet.

That seems fine and expected.

> 
> If I'm reading emac_tx_fill_tpd() correctly, there could be as many as
> (2 + skb_shinfo(skb)->nr_frags) descriptors for a given packet.  I don't
> know how big nr_frags could get, so I don't know how to calculate the
> number of descriptors I really need.  I'm assuming I need to do
> something like this:

nr_frags can't be bigger than MAX_SKB_FRAGS, hence these checks all
other drivers do against 1 + MAX_SKB_FRAGS.


> 
> However, I'm confused about one thing.  Almost every other driver just
> sets "netdev->mtu = new_mtu" and does nothing else.  I can't find any
> other driver that actually stops the RX path, reprograms the hardware,
> and then restarts the RX path.  I know this is a stupid question, but
> why is my driver doing that?

Most drivers allocate RX buffer sizes that are usually bigger than the
MTU, but would probably silently fail or expose transient behavior once
the MTU changes to greater than the size pre-defined.

> 
> Can I get away with just calling netdev_update_features()?

MTU change is a pretty disruptive change for the HW I typically work
with since we need to choose a RX buffer size that is aligned to the
DRAM controller burst size, reprogram the MAC to accept packets up to
that size, and potentially change the RX ring allocation strategy and
typical buffer size. None of these requirements are unusual or unique,
they ost likely apply to most MACs out there, my guess is that MTU
change is barely tested.
-- 
Florian

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

[Rob Herring]

On Tue, Aug 9, 2016 at 1:25 PM, Timur Tabi  wrote:
> Lino Sanfilippo wrote:
>
>>> +/* Fill up transmit descriptors */
>>> +static void emac_tx_fill_tpd(struct emac_adapter *adpt,
>>> +struct emac_tx_queue *tx_q, struct sk_buff
>>> *skb,
>>> +struct emac_tpd *tpd)
>>> +{
>>> +   u16 nr_frags = skb_shinfo(skb)->nr_frags;
>>> +   unsigned int len = skb_headlen(skb);
>>> +   struct emac_buffer *tpbuf = NULL;
>>> +   unsigned int mapped_len = 0;
>>> +   unsigned int i;
>>> +   int ret;
>>> +
>>> +   /* if Large Segment Offload is (in TCP Segmentation Offload
>>> struct) */
>>> +   if (TPD_LSO(tpd)) {
>>> +   mapped_len = skb_transport_offset(skb) + tcp_hdrlen(skb);
>>> +
>>> +   tpbuf = GET_TPD_BUFFER(tx_q, tx_q->tpd.produce_idx);
>>> +   tpbuf->length = mapped_len;
>>> +   tpbuf->dma_addr =
>>> dma_map_single(adpt->netdev->dev.parent,
>>> +skb->data, mapped_len,
>>> +DMA_TO_DEVICE);
>>> +   ret = dma_mapping_error(adpt->netdev->dev.parent,
>>> +   tpbuf->dma_addr);
>>> +   if (ret) {
>>> +   dev_kfree_skb(skb);
>>> +   return;
>>> +   }
>>> +
>>> +   TPD_BUFFER_ADDR_L_SET(tpd,
>>> lower_32_bits(tpbuf->dma_addr));
>>> +   TPD_BUFFER_ADDR_H_SET(tpd,
>>> upper_32_bits(tpbuf->dma_addr));
>>
>>
>> You should also take big endian systems into account. This means that if
>> the multi-byte values
>> in the descriptors require little-endian you have to convert from host
>> byte order to le and
>> vice versa. You can use cpu_to_le32() and friends for this.
>
>
> I used to work on PowerPC, so I respect making things work for both endians.
> However, even I think that this is overkill for my driver. First, there's no
> way this driver will ever be used on a big-endian system.  Second, I'm
> pretty sure there are lots of places that would need cpu_to_le32() in order
> to make this driver big-endian compatible. It would be a huge mess.

I thought that too about Calxeda systems and then someone went off and
made them run BE. I was surprised it worked, but I guess when the h/w
doesn't try to do swizzling of i/o things just work.

Rob

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

[Timur Tabi]

Lino Sanfilippo wrote:


+/* Fill up transmit descriptors */
+static void emac_tx_fill_tpd(struct emac_adapter *adpt,
+struct emac_tx_queue *tx_q, struct sk_buff *skb,
+struct emac_tpd *tpd)
+{
+   u16 nr_frags = skb_shinfo(skb)->nr_frags;
+   unsigned int len = skb_headlen(skb);
+   struct emac_buffer *tpbuf = NULL;
+   unsigned int mapped_len = 0;
+   unsigned int i;
+   int ret;
+
+   /* if Large Segment Offload is (in TCP Segmentation Offload struct) */
+   if (TPD_LSO(tpd)) {
+   mapped_len = skb_transport_offset(skb) + tcp_hdrlen(skb);
+
+   tpbuf = GET_TPD_BUFFER(tx_q, tx_q->tpd.produce_idx);
+   tpbuf->length = mapped_len;
+   tpbuf->dma_addr = dma_map_single(adpt->netdev->dev.parent,
+skb->data, mapped_len,
+DMA_TO_DEVICE);
+   ret = dma_mapping_error(adpt->netdev->dev.parent,
+   tpbuf->dma_addr);
+   if (ret) {
+   dev_kfree_skb(skb);
+   return;
+   }
+
+   TPD_BUFFER_ADDR_L_SET(tpd, lower_32_bits(tpbuf->dma_addr));
+   TPD_BUFFER_ADDR_H_SET(tpd, upper_32_bits(tpbuf->dma_addr));


You should also take big endian systems into account. This means that if the 
multi-byte values
in the descriptors require little-endian you have to convert from host byte 
order to le and
vice versa. You can use cpu_to_le32() and friends for this.


I used to work on PowerPC, so I respect making things work for both 
endians.  However, even I think that this is overkill for my driver. 
First, there's no way this driver will ever be used on a big-endian 
system.  Second, I'm pretty sure there are lots of places that would 
need cpu_to_le32() in order to make this driver big-endian compatible. 
It would be a huge mess.


#define TPD_BUFFER_ADDR_H_SET(tpd, val)	BITS_SET((tpd)->word[3], 18, 30, 
val)


This macros sets specific bits based on the definition of the register. 
 I could change it to this:


#define TPD_BUFFER_ADDR_H_SET(tpd, val) BITS_SET((tpd)->word[3], 18, 30, 
cpu_to_le32(val))


But I honestly don't see what good that will do.  There are still 
thousands of other places that assume little-endian.



+   TPD_BUF_LEN_SET(tpd, tpbuf->length);
+   emac_tx_tpd_create(adpt, tx_q, tpd);
+   }
+
+   if (mapped_len < len) {
+   tpbuf = GET_TPD_BUFFER(tx_q, tx_q->tpd.produce_idx);
+   tpbuf->length = len - mapped_len;
+   tpbuf->dma_addr = dma_map_single(adpt->netdev->dev.parent,
+skb->data + mapped_len,
+tpbuf->length, DMA_TO_DEVICE);
+   ret = dma_mapping_error(adpt->netdev->dev.parent,
+   tpbuf->dma_addr);
+   if (ret) {
+   dev_kfree_skb(skb);
+   return;
+   }
+
+   TPD_BUFFER_ADDR_L_SET(tpd, lower_32_bits(tpbuf->dma_addr));
+   TPD_BUFFER_ADDR_H_SET(tpd, upper_32_bits(tpbuf->dma_addr));
+   TPD_BUF_LEN_SET(tpd, tpbuf->length);
+   emac_tx_tpd_create(adpt, tx_q, tpd);
+   }
+
+   for (i = 0; i < nr_frags; i++) {
+   struct skb_frag_struct *frag;
+
+   frag = _shinfo(skb)->frags[i];
+
+   tpbuf = GET_TPD_BUFFER(tx_q, tx_q->tpd.produce_idx);
+   tpbuf->length = frag->size;
+   tpbuf->dma_addr = dma_map_page(adpt->netdev->dev.parent,
+  frag->page.p, frag->page_offset,
+  tpbuf->length, DMA_TO_DEVICE);
+   ret = dma_mapping_error(adpt->netdev->dev.parent,
+   tpbuf->dma_addr);
+   if (ret) {
+   dev_kfree_skb(skb);
+   return;
+   }


In case of error you need to undo all mappings that you have done so far.


Ok.




+
+   TPD_BUFFER_ADDR_L_SET(tpd, lower_32_bits(tpbuf->dma_addr));
+   TPD_BUFFER_ADDR_H_SET(tpd, upper_32_bits(tpbuf->dma_addr));
+   TPD_BUF_LEN_SET(tpd, tpbuf->length);
+   emac_tx_tpd_create(adpt, tx_q, tpd);
+   }
+
+   /* The last tpd */
+   emac_tx_tpd_mark_last(adpt, tx_q);


Use a wmb() here to make sure that all writes to the descriptors in dma memory
are completed before you update the producer register (see memory-barriers.txt
for the reason why this is needed)


Ok.


+/* Transmit the packet using specified transmit queue */
+int emac_mac_tx_buf_send(struct emac_adapter *adpt, struct emac_tx_queue *tx_q,
+struct sk_buff *skb)
+{
+   struct emac_tpd tpd;
+   

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

[Timur Tabi]

Rob Herring wrote:

Sounds like 2 different h/w. The compatible property should distinguish
this.


So I changed it to this:

- compatible : Should be "qcom,fsm9900-emac" or "qcom,qdf2432-emac",
depending on the version of the internal PHY.
"qcom,fsm9900-emac" is for v1, and "qcom,qdf2432-emac"
is for v2.

(and changed the code to match).

However, now this doesn't work look right any more:

The external phy child node:
- compatible : Should be "qcom,fsm9900-emac-phy".
- reg : The phy address

I was never crazy about this, because the external PHY is whatever 
device it happens to be.


The phy driver uses of_mdiobus_register, which calls 
of_mdiobus_child_is_phy().  Based on my reading of the code, I think I 
should just drop the compatible property altogether.  Because the way it 
is now causes of_mdiobus_child_is_phy() to return false when it parses 
that node.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Lino Sanfilippo]

Hi Timur,

On 03.08.2016 22:12, Timur Tabi wrote:


> +/* Fill up transmit descriptors */
> +static void emac_tx_fill_tpd(struct emac_adapter *adpt,
> +  struct emac_tx_queue *tx_q, struct sk_buff *skb,
> +  struct emac_tpd *tpd)
> +{
> + u16 nr_frags = skb_shinfo(skb)->nr_frags;
> + unsigned int len = skb_headlen(skb);
> + struct emac_buffer *tpbuf = NULL;
> + unsigned int mapped_len = 0;
> + unsigned int i;
> + int ret;
> +
> + /* if Large Segment Offload is (in TCP Segmentation Offload struct) */
> + if (TPD_LSO(tpd)) {
> + mapped_len = skb_transport_offset(skb) + tcp_hdrlen(skb);
> +
> + tpbuf = GET_TPD_BUFFER(tx_q, tx_q->tpd.produce_idx);
> + tpbuf->length = mapped_len;
> + tpbuf->dma_addr = dma_map_single(adpt->netdev->dev.parent,
> +  skb->data, mapped_len,
> +  DMA_TO_DEVICE);
> + ret = dma_mapping_error(adpt->netdev->dev.parent,
> + tpbuf->dma_addr);
> + if (ret) {
> + dev_kfree_skb(skb);
> + return;
> + }
> +
> + TPD_BUFFER_ADDR_L_SET(tpd, lower_32_bits(tpbuf->dma_addr));
> + TPD_BUFFER_ADDR_H_SET(tpd, upper_32_bits(tpbuf->dma_addr));

You should also take big endian systems into account. This means that if the 
multi-byte values
in the descriptors require little-endian you have to convert from host byte 
order to le and
vice versa. You can use cpu_to_le32() and friends for this. 


> + TPD_BUF_LEN_SET(tpd, tpbuf->length);
> + emac_tx_tpd_create(adpt, tx_q, tpd);
> + }
> +
> + if (mapped_len < len) {
> + tpbuf = GET_TPD_BUFFER(tx_q, tx_q->tpd.produce_idx);
> + tpbuf->length = len - mapped_len;
> + tpbuf->dma_addr = dma_map_single(adpt->netdev->dev.parent,
> +  skb->data + mapped_len,
> +  tpbuf->length, DMA_TO_DEVICE);
> + ret = dma_mapping_error(adpt->netdev->dev.parent,
> + tpbuf->dma_addr);
> + if (ret) {
> + dev_kfree_skb(skb);
> + return;
> + }
> +
> + TPD_BUFFER_ADDR_L_SET(tpd, lower_32_bits(tpbuf->dma_addr));
> + TPD_BUFFER_ADDR_H_SET(tpd, upper_32_bits(tpbuf->dma_addr));
> + TPD_BUF_LEN_SET(tpd, tpbuf->length);
> + emac_tx_tpd_create(adpt, tx_q, tpd);
> + }
> +
> + for (i = 0; i < nr_frags; i++) {
> + struct skb_frag_struct *frag;
> +
> + frag = _shinfo(skb)->frags[i];
> +
> + tpbuf = GET_TPD_BUFFER(tx_q, tx_q->tpd.produce_idx);
> + tpbuf->length = frag->size;
> + tpbuf->dma_addr = dma_map_page(adpt->netdev->dev.parent,
> +frag->page.p, frag->page_offset,
> +tpbuf->length, DMA_TO_DEVICE);
> + ret = dma_mapping_error(adpt->netdev->dev.parent,
> + tpbuf->dma_addr);
> + if (ret) {
> + dev_kfree_skb(skb);
> + return;
> + }

In case of error you need to undo all mappings that you have done so far.

> +
> + TPD_BUFFER_ADDR_L_SET(tpd, lower_32_bits(tpbuf->dma_addr));
> + TPD_BUFFER_ADDR_H_SET(tpd, upper_32_bits(tpbuf->dma_addr));
> + TPD_BUF_LEN_SET(tpd, tpbuf->length);
> + emac_tx_tpd_create(adpt, tx_q, tpd);
> + }
> +
> + /* The last tpd */
> + emac_tx_tpd_mark_last(adpt, tx_q);

Use a wmb() here to make sure that all writes to the descriptors in dma memory
are completed before you update the producer register (see memory-barriers.txt
for the reason why this is needed)

> + /* The last buffer info contain the skb address,
> +  * so it will be freed after unmap
> +  */
> + tpbuf->skb = skb;
> +}
> +
> +/* Transmit the packet using specified transmit queue */
> +int emac_mac_tx_buf_send(struct emac_adapter *adpt, struct emac_tx_queue 
> *tx_q,
> +  struct sk_buff *skb)
> +{
> + struct emac_tpd tpd;
> + u32 prod_idx;
> +
> + memset(, 0, sizeof(tpd));
> +
> + if (emac_tso_csum(adpt, tx_q, skb, ) != 0) {
> + dev_kfree_skb_any(skb);
> + return NETDEV_TX_OK;
> + }
> +
> + if (skb_vlan_tag_present(skb)) {
> + u16 tag;
> +
> + EMAC_VLAN_TO_TAG(skb_vlan_tag_get(skb), tag);
> + TPD_CVLAN_TAG_SET(, tag);
> + TPD_INSTC_SET(, 1);
> + }
> +
> + if (skb_network_offset(skb) != ETH_HLEN)
> + TPD_TYP_SET(, 1);
> +
> + emac_tx_fill_tpd(adpt, tx_q, skb, );
> +
> + 

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

[Timur Tabi]

Rob Herring wrote:

>+- phy-version : the version of the integrated emac phy, either 1 or 2.

Sounds like 2 different h/w. The compatible property should distinguish
this.


Well, the MAC parts of the EMAC is the same.  I could do 
qcom,fsm9900-emac for v1 and qcom,qdf2432-emac for v2.  I can't really 
think of a reason why that wouldn't work.


For ACPI, we can assume that QCOM8070 is a v2 PHY.  If Qualcomm creates 
another server chip with a v1 PHY, it would need a new ACPI HID.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Rob Herring]

On Wed, Aug 03, 2016 at 03:12:23PM -0500, Timur Tabi wrote:
> Add supports for ethernet controller HW on Qualcomm Technologies, Inc. SoC.
> This driver supports the following features:
> 1) Checksum offload.
> 2) Interrupt coalescing support.
> 3) SGMII phy.
> 4) phylib interface for external phy
> 
> Based on original work by
>   Niranjana Vishwanathapura 
>   Gilad Avidov 
> 
> Signed-off-by: Timur Tabi 
> ---
> 
> v7:
>  - add "phy-version" to DT docs
>  - remove emac_status_bits, use mutexes instead of polling a status bit
>  - test return code from dma_map_single() calls
>  - use __netdev_alloc_skb_ip_align instead of dev_alloc_skb
>  - add a missing free_irq on an error path
>  - return an error code from emac_reinit_locked
>  - support setting the MTU even when the interface is down
>  - use a spinlock not a mutex in emac_get_stats64
>  - don't redundantly set the ethernet device name
>  - remove all hardware timestamp code, since it's incomplete
>  - remove redundant 'reset' param to emac_mac_down()
>  - handle queue throttling properly
> 
> v6:
>  - Properly ordered local variables
>  - use built-in GEN_MASK instead of BITS_MASK
>  - remove redundant call to emac_rx_mode_set from emac_mac_up
>  - removed emac_rfd structure, use dma_addr_t directly instead
>  - removed emac_mac_speed enun, replaced with macros
>  - removed superfluous phy_stop from emac_mac_down(), which prevented 
> reloading module
>  - add missing netif_napi_del
>  - set the DMA mask
> 
> v5:
>  - changed author to Timur, added MAINTAINERS entry
>  - use phylib, replacing internal phy code
>  - added support for EMAC internal SGMII v2
>  - fix ~DIS_INT warning
>  - update DT bindings, including removing unused properties
>  - removed interrupt handler for internal sgmii
>  - removed link status check handler/state (replaced with phylib)
>  - removed periodic timer handler (replaced with phylib)
>  - removed power management code (will be rewritten later)
>  - external phy is now required, not optional
>  - removed redundant EMAC_STATUS_DOWN status flag
>  - removed redundant link status and speed variables
>  - removed redundant status bits (vlan strip, promiscuous, loopback, etc)
>  - removed useless watchdog status
>  - removed command-line parameters
>  - cleaned up probe messages
>  - removed redundant params from emac_sgmii_link_init()
>  - always call netdev_completed_queue() (per review comment)
>  - fix emac_napi_rtx() (per review comment)
>  - removed max_ints loop in interrupt handler
>  - removed redundant mutex around phy read/write calls
>  - added lock for reading emac status (per review comment)
>  - generate random MAC address if it can't be read from firmware
>  - replace EMAC_DMA_ADDR_HI/LO with upper/lower_32_bits
>  - don't test return value from platform_get_resource (per review comment)
>  - use net_warn_ratelimited (per review comment)
>  - don't set the dma masks (will be set by DT or IORT code)
>  - remove unused emac_tx_tpd_ts_save()
>  - removed redundant local MTU variable
> 
> 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  |   66 +
>  MAINTAINERS|6 +
>  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  | 1511 
> 
>  drivers/net/ethernet/qualcomm/emac/emac-mac.h  |  251 
>  drivers/net/ethernet/qualcomm/emac/emac-phy.c  |  211 +++
>  drivers/net/ethernet/qualcomm/emac/emac-phy.h  |   32 +
>  drivers/net/ethernet/qualcomm/emac/emac-sgmii.c|  700 +
>  drivers/net/ethernet/qualcomm/emac/emac-sgmii.h|   24 +
>  

[PATCH] [v7] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

Add supports for ethernet controller HW on Qualcomm Technologies, Inc. SoC.
This driver supports the following features:
1) Checksum offload.
2) Interrupt coalescing support.
3) SGMII phy.
4) phylib interface for external phy

Based on original work by
Niranjana Vishwanathapura 
Gilad Avidov 

Signed-off-by: Timur Tabi 
---

v7:
 - add "phy-version" to DT docs
 - remove emac_status_bits, use mutexes instead of polling a status bit
 - test return code from dma_map_single() calls
 - use __netdev_alloc_skb_ip_align instead of dev_alloc_skb
 - add a missing free_irq on an error path
 - return an error code from emac_reinit_locked
 - support setting the MTU even when the interface is down
 - use a spinlock not a mutex in emac_get_stats64
 - don't redundantly set the ethernet device name
 - remove all hardware timestamp code, since it's incomplete
 - remove redundant 'reset' param to emac_mac_down()
 - handle queue throttling properly

v6:
 - Properly ordered local variables
 - use built-in GEN_MASK instead of BITS_MASK
 - remove redundant call to emac_rx_mode_set from emac_mac_up
 - removed emac_rfd structure, use dma_addr_t directly instead
 - removed emac_mac_speed enun, replaced with macros
 - removed superfluous phy_stop from emac_mac_down(), which prevented reloading 
module
 - add missing netif_napi_del
 - set the DMA mask

v5:
 - changed author to Timur, added MAINTAINERS entry
 - use phylib, replacing internal phy code
 - added support for EMAC internal SGMII v2
 - fix ~DIS_INT warning
 - update DT bindings, including removing unused properties
 - removed interrupt handler for internal sgmii
 - removed link status check handler/state (replaced with phylib)
 - removed periodic timer handler (replaced with phylib)
 - removed power management code (will be rewritten later)
 - external phy is now required, not optional
 - removed redundant EMAC_STATUS_DOWN status flag
 - removed redundant link status and speed variables
 - removed redundant status bits (vlan strip, promiscuous, loopback, etc)
 - removed useless watchdog status
 - removed command-line parameters
 - cleaned up probe messages
 - removed redundant params from emac_sgmii_link_init()
 - always call netdev_completed_queue() (per review comment)
 - fix emac_napi_rtx() (per review comment)
 - removed max_ints loop in interrupt handler
 - removed redundant mutex around phy read/write calls
 - added lock for reading emac status (per review comment)
 - generate random MAC address if it can't be read from firmware
 - replace EMAC_DMA_ADDR_HI/LO with upper/lower_32_bits
 - don't test return value from platform_get_resource (per review comment)
 - use net_warn_ratelimited (per review comment)
 - don't set the dma masks (will be set by DT or IORT code)
 - remove unused emac_tx_tpd_ts_save()
 - removed redundant local MTU variable

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  |   66 +
 MAINTAINERS|6 +
 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  | 1511 
 drivers/net/ethernet/qualcomm/emac/emac-mac.h  |  251 
 drivers/net/ethernet/qualcomm/emac/emac-phy.c  |  211 +++
 drivers/net/ethernet/qualcomm/emac/emac-phy.h  |   32 +
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c|  700 +
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.h|   24 +
 drivers/net/ethernet/qualcomm/emac/emac.c  |  786 ++
 drivers/net/ethernet/qualcomm/emac/emac.h  |  356 +
 13 files changed, 3963 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/qcom-emac.txt
 create mode 100644 drivers/net/ethernet/qualcomm/emac/Makefile