On Wed, May 24, 2023 at 04:01:49PM -0700, Justin Chen wrote:
> From: Florian Fainelli <[email protected]>
>
> Add a binding document for the Broadcom ASP 2.0 Ethernet
> controller.
>
> Signed-off-by: Florian Fainelli <[email protected]>
> Signed-off-by: Justin Chen <[email protected]>
> ---
> v5
> - Fix compatible string yaml format to properly capture what we want
>
> v4
> - Adjust compatible string example to reference SoC and HW ver
>
> v3
> - Minor formatting issues
> - Change channel prop to brcm,channel for vendor specific format
> - Removed redundant v2.0 from compat string
> - Fix ranges field
>
> v2
> - Minor formatting issues
>
> .../devicetree/bindings/net/brcm,asp-v2.0.yaml | 149 +++++++++++++++++++++
> 1 file changed, 149 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml b/Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml
> new file mode 100644
> index 000000000000..c4cd24492bfd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml
> @@ -0,0 +1,149 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/brcm,asp-v2.0.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Broadcom ASP 2.0 Ethernet controller
> +
> +maintainers:
> + - Justin Chen <[email protected]>
> + - Florian Fainelli <[email protected]>
> +
> +description: Broadcom Ethernet controller first introduced with 72165
> +
> +properties:
> + '#address-cells':
> + const: 1
> + '#size-cells':
> + const: 1
> +
> + compatible:
> + oneOf:
> + - items:
> + - enum:
> + - brcm,bcm74165-asp
> + - const: brcm,asp-v2.1
> + - items:
> + - enum:
> + - brcm,bcm72165-asp
> + - const: brcm,asp-v2.0
Sorry if I did not notice this before, conventionally compatible goes
first here. IFF there is another version, could you shuffle things
around? Otherwise,
Reviewed-by: Conor Dooley <[email protected]>
Thanks,
Conor.
On Wed, 24 May 2023 16:01:50 -0700 Justin Chen wrote:
> Add support for the Broadcom ASP 2.0 Ethernet controller which is first
> introduced with 72165. This controller features two distinct Ethernet
> ports that can be independently operated.
>
> This patch supports:
>
> - Wake-on-LAN using magic packets
> - basic ethtool operations (link, counters, message level)
> - MAC destination address filtering (promiscuous, ALL_MULTI, etc.)
> +static netdev_tx_t bcmasp_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> + struct bcmasp_intf *intf = netdev_priv(dev);
> + int spb_index, nr_frags, ret, i, j;
> + unsigned int total_bytes, size;
> + struct bcmasp_tx_cb *txcb;
> + dma_addr_t mapping, valid;
> + struct bcmasp_desc *desc;
> + bool csum_hw = false;
> + struct device *kdev;
> + skb_frag_t *frag;
> +
> + kdev = &intf->parent->pdev->dev;
> +
> + spin_lock(&intf->tx_lock);
What is the tx_lock for? netdevs already have a tx lock, unless you
declare the device as lockless.
> +static void bcmasp_tx_timeout(struct net_device *dev, unsigned int txqueue)
> +{
> + struct bcmasp_intf *intf = netdev_priv(dev);
> +
> + netif_dbg(intf, tx_err, dev, "transmit timeout!\n");
> +
> + netif_trans_update(dev);
> + dev->stats.tx_errors++;
> +
> + netif_wake_queue(dev);
If the queue is full xmit will just put it back to sleep.
You want to try to reap completions if anything, no?
> +static struct net_device_stats *bcmasp_get_stats(struct net_device *dev)
> +{
> + return &dev->stats;
> +}
you don't have to do this, core will use device stats if there's no ndo
> + ndev = alloc_etherdev(sizeof(struct bcmasp_intf));
> + if (!dev) {
*blink* condition is typo'ed
> + dev_warn(dev, "%s: unable to alloc ndev\n", ndev_dn->name);
> + goto err;
> + }
--
pw-bot: cr
On 5/25/23 8:54 PM, Jakub Kicinski wrote:
> On Wed, 24 May 2023 16:01:50 -0700 Justin Chen wrote:
>> Add support for the Broadcom ASP 2.0 Ethernet controller which is first
>> introduced with 72165. This controller features two distinct Ethernet
>> ports that can be independently operated.
>>
>> This patch supports:
>>
>> - Wake-on-LAN using magic packets
>> - basic ethtool operations (link, counters, message level)
>> - MAC destination address filtering (promiscuous, ALL_MULTI, etc.)
>
>> +static netdev_tx_t bcmasp_xmit(struct sk_buff *skb, struct net_device *dev)
>> +{
>> + struct bcmasp_intf *intf = netdev_priv(dev);
>> + int spb_index, nr_frags, ret, i, j;
>> + unsigned int total_bytes, size;
>> + struct bcmasp_tx_cb *txcb;
>> + dma_addr_t mapping, valid;
>> + struct bcmasp_desc *desc;
>> + bool csum_hw = false;
>> + struct device *kdev;
>> + skb_frag_t *frag;
>> +
>> + kdev = &intf->parent->pdev->dev;
>> +
>> + spin_lock(&intf->tx_lock);
>
> What is the tx_lock for? netdevs already have a tx lock, unless you
> declare the device as lockless.
>
Will remove.
>> +static void bcmasp_tx_timeout(struct net_device *dev, unsigned int txqueue)
>> +{
>> + struct bcmasp_intf *intf = netdev_priv(dev);
>> +
>> + netif_dbg(intf, tx_err, dev, "transmit timeout!\n");
>> +
>> + netif_trans_update(dev);
>> + dev->stats.tx_errors++;
>> +
>> + netif_wake_queue(dev);
>
> If the queue is full xmit will just put it back to sleep.
> You want to try to reap completions if anything, no?
>
I can remove the wake. As you mentioned it won't do anything here. There
isn't anything to reap if we are in the timeout condition. If it is some
HW stall, we could flush and restart the ring, but if that is the case I
rather figure out why the HW is stalling. I think we can leave it as a
"tell the user we are stalled" and leave it as that.
>> +static struct net_device_stats *bcmasp_get_stats(struct net_device *dev)
>> +{
>> + return &dev->stats;
>> +}
>
> you don't have to do this, core will use device stats if there's no ndo
>
>> + ndev = alloc_etherdev(sizeof(struct bcmasp_intf));
>> + if (!dev) {
>
> *blink* condition is typo'ed
>
Oops. Good catch.
Thanks,
Justin
>> + dev_warn(dev, "%s: unable to alloc ndev\n", ndev_dn->name);
>> + goto err;
>> + }
>
On 25/05/2023 01:01, Justin Chen wrote:
> From: Florian Fainelli <[email protected]>
>
> Add a binding document for the Broadcom ASP 2.0 Ethernet
> controller.
>
> Signed-off-by: Florian Fainelli <[email protected]>
> Signed-off-by: Justin Chen <[email protected]>
> ---
> v5
> - Fix compatible string yaml format to properly capture what we want
>
> v4
> - Adjust compatible string example to reference SoC and HW ver
>
> v3
> - Minor formatting issues
> - Change channel prop to brcm,channel for vendor specific format
> - Removed redundant v2.0 from compat string
> - Fix ranges field
>
> v2
> - Minor formatting issues
>
> .../devicetree/bindings/net/brcm,asp-v2.0.yaml | 149 +++++++++++++++++++++
> 1 file changed, 149 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml b/Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml
> new file mode 100644
> index 000000000000..c4cd24492bfd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml
> @@ -0,0 +1,149 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/brcm,asp-v2.0.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Broadcom ASP 2.0 Ethernet controller
> +
> +maintainers:
> + - Justin Chen <[email protected]>
> + - Florian Fainelli <[email protected]>
> +
> +description: Broadcom Ethernet controller first introduced with 72165
> +
> +properties:
> + '#address-cells':
Judging by more comments, there will be a v6, thus please also use
consistent quotes - either ' or ".
> + const: 1
> + '#size-cells':
> + const: 1
> +
> + compatible:
As Conor pointed out, compatible is always first.
> + oneOf:
> + - items:
> + - enum:
> + - brcm,bcm74165-asp
> + - const: brcm,asp-v2.1
> + - items:
> + - enum:
> + - brcm,bcm72165-asp
> + - const: brcm,asp-v2.0
> +
> + reg:
> + maxItems: 1
> +
> + ranges: true
> +
> + interrupts:
> + minItems: 1
> + items:
> + - description: RX/TX interrupt
> + - description: Port 0 Wake-on-LAN
> + - description: Port 1 Wake-on-LAN
> +
> + clocks:
> + maxItems: 1
> +
> + ethernet-ports:
> + type: object
> + properties:
> + '#address-cells':
> + const: 1
> + '#size-cells':
> + const: 0
> +
> + patternProperties:
> + "^port@[0-9]+$":
> + type: object
> +
> + $ref: ethernet-controller.yaml#
> +
> + properties:
> + reg:
> + maxItems: 1
> + description: Port number
> +
> + brcm,channel:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: ASP channel number
Why do you need it? reg defines it. Your description does not explain
here much, except copying property name. Can we please avoid
descriptions which just copy name?
> +
> + required:
> + - reg
> + - brcm,channel
> +
> + additionalProperties: false
> +
> +patternProperties:
> + "^mdio@[0-9a-f]+$":
Isn't mdio a property of each ethernet port? Existing users
(e.g.bcmgenet, owl-emac, switches) do it that way...
Otherwise how do you define relation-ship? Can one mdio fit multiple ports?
> + type: object
> + $ref: brcm,unimac-mdio.yaml
> +
> + description:
> + ASP internal UniMAC MDIO bus
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
Best regards,
Krzysztof
On 5/31/23 12:18, Krzysztof Kozlowski wrote:
> On 25/05/2023 01:01, Justin Chen wrote:
>> From: Florian Fainelli <[email protected]>
>>
>> Add a binding document for the Broadcom ASP 2.0 Ethernet
>> controller.
>>
>> Signed-off-by: Florian Fainelli <[email protected]>
>> Signed-off-by: Justin Chen <[email protected]>
>> ---
>> v5
>> - Fix compatible string yaml format to properly capture what we want
>>
>> v4
>> - Adjust compatible string example to reference SoC and HW ver
>>
>> v3
>> - Minor formatting issues
>> - Change channel prop to brcm,channel for vendor specific format
>> - Removed redundant v2.0 from compat string
>> - Fix ranges field
>>
>> v2
>> - Minor formatting issues
>>
>> .../devicetree/bindings/net/brcm,asp-v2.0.yaml | 149 +++++++++++++++++++++
>> 1 file changed, 149 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml b/Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml
>> new file mode 100644
>> index 000000000000..c4cd24492bfd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml
>> @@ -0,0 +1,149 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/net/brcm,asp-v2.0.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Broadcom ASP 2.0 Ethernet controller
>> +
>> +maintainers:
>> + - Justin Chen <[email protected]>
>> + - Florian Fainelli <[email protected]>
>> +
>> +description: Broadcom Ethernet controller first introduced with 72165
>> +
>> +properties:
>> + '#address-cells':
>
> Judging by more comments, there will be a v6, thus please also use
> consistent quotes - either ' or ".
>
>> + const: 1
>> + '#size-cells':
>> + const: 1
>> +
>> + compatible:
>
> As Conor pointed out, compatible is always first.
>
>> + oneOf:
>> + - items:
>> + - enum:
>> + - brcm,bcm74165-asp
>> + - const: brcm,asp-v2.1
>> + - items:
>> + - enum:
>> + - brcm,bcm72165-asp
>> + - const: brcm,asp-v2.0
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + ranges: true
>> +
>> + interrupts:
>> + minItems: 1
>> + items:
>> + - description: RX/TX interrupt
>> + - description: Port 0 Wake-on-LAN
>> + - description: Port 1 Wake-on-LAN
>> +
>> + clocks:
>> + maxItems: 1
>> +
>> + ethernet-ports:
>> + type: object
>> + properties:
>> + '#address-cells':
>> + const: 1
>> + '#size-cells':
>> + const: 0
>> +
>> + patternProperties:
>> + "^port@[0-9]+$":
>> + type: object
>> +
>> + $ref: ethernet-controller.yaml#
>> +
>> + properties:
>> + reg:
>> + maxItems: 1
>> + description: Port number
>> +
>> + brcm,channel:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + description: ASP channel number
>
> Why do you need it? reg defines it. Your description does not explain
> here much, except copying property name. Can we please avoid
> descriptions which just copy name?
>
>> +
>> + required:
>> + - reg
>> + - brcm,channel
>> +
>> + additionalProperties: false
>> +
>> +patternProperties:
>> + "^mdio@[0-9a-f]+$":
>
> Isn't mdio a property of each ethernet port? Existing users
> (e.g.bcmgenet, owl-emac, switches) do it that way...
They are sub-nodes of the larger Ethernet controller block, hence the
property here.
>
> Otherwise how do you define relation-ship? Can one mdio fit multiple ports?
The relationship is established between Ethernet ports and children
nodes of the MDIO controller, such as switches or Ethernet PHYs using
'phy-handle' for instance. And yes, a single/common MDIO controller
could be serving multiple Ethernet ports.
--
Florian
On 31/05/2023 21:29, Florian Fainelli wrote:
>>> + required:
>>> + - reg
>>> + - brcm,channel
>>> +
>>> + additionalProperties: false
>>> +
>>> +patternProperties:
>>> + "^mdio@[0-9a-f]+$":
>>
>> Isn't mdio a property of each ethernet port? Existing users
>> (e.g.bcmgenet, owl-emac, switches) do it that way...
>
> They are sub-nodes of the larger Ethernet controller block, hence the
> property here.
This is the Ethernet controller. They are subnodes here, so what do you
mean by that? They are part of some other block?
>
>>
>> Otherwise how do you define relation-ship? Can one mdio fit multiple ports?
>
> The relationship is established between Ethernet ports and children
> nodes of the MDIO controller, such as switches or Ethernet PHYs using
> 'phy-handle' for instance. And yes, a single/common MDIO controller
> could be serving multiple Ethernet ports.
We do not talk about generic case, but your device.
Best regards,
Krzysztof
On 5/31/23 12:31, Krzysztof Kozlowski wrote:
> On 31/05/2023 21:29, Florian Fainelli wrote:
>>>> + required:
>>>> + - reg
>>>> + - brcm,channel
>>>> +
>>>> + additionalProperties: false
>>>> +
>>>> +patternProperties:
>>>> + "^mdio@[0-9a-f]+$":
>>>
>>> Isn't mdio a property of each ethernet port? Existing users
>>> (e.g.bcmgenet, owl-emac, switches) do it that way...
>>
>> They are sub-nodes of the larger Ethernet controller block, hence the
>> property here.
>
> This is the Ethernet controller. They are subnodes here, so what do you
> mean by that? They are part of some other block?
The block is not just an Ethernet controller it has other functions,
which is why we went with a top-level node with a 'ranges' property. One
of those functions are the MDIO bus controllers. The examples makes it
reasonably clear.
>
>>
>>>
>>> Otherwise how do you define relation-ship? Can one mdio fit multiple ports?
>>
>> The relationship is established between Ethernet ports and children
>> nodes of the MDIO controller, such as switches or Ethernet PHYs using
>> 'phy-handle' for instance. And yes, a single/common MDIO controller
>> could be serving multiple Ethernet ports.
>
> We do not talk about generic case, but your device.
The generic case is true here as well. We so happen to have a 1:1
mapping between the MDIO controller, PHY, and Ethernet port, in this
particular example.
--
Florian
On 5/24/23 16:01, Justin Chen wrote:
> Add support for the Broadcom ASP 2.0 Ethernet controller which is first
> introduced with 72165. This controller features two distinct Ethernet
> ports that can be independently operated.
>
> This patch supports:
>
> - Wake-on-LAN using magic packets
> - basic ethtool operations (link, counters, message level)
> - MAC destination address filtering (promiscuous, ALL_MULTI, etc.)
>
> Reviewed-by: Simon Horman <[email protected]>
> Signed-off-by: Florian Fainelli <[email protected]>
> Signed-off-by: Justin Chen <[email protected]>
> ---
[snip]
> +static const struct net_device_ops bcmasp_netdev_ops = {
> + .ndo_open = bcmasp_open,
> + .ndo_stop = bcmasp_stop,
> + .ndo_start_xmit = bcmasp_xmit,
> + .ndo_tx_timeout = bcmasp_tx_timeout,
> + .ndo_set_rx_mode = bcmasp_set_rx_mode,
> + .ndo_get_phys_port_name = bcmasp_get_phys_port_name,
> + .ndo_get_stats = bcmasp_get_stats,
> + .ndo_do_ioctl = bcmasp_ioctl,
This needs to be:
@@ -1207,7 +1196,7 @@ static const struct net_device_ops
bcmasp_netdev_ops = {
.ndo_set_rx_mode = bcmasp_set_rx_mode,
.ndo_get_phys_port_name = bcmasp_get_phys_port_name,
.ndo_get_stats = bcmasp_get_stats,
- .ndo_do_ioctl = bcmasp_ioctl,
+ .ndo_eth_ioctl = phy_do_ioctl_running,
.ndo_set_mac_address = bcmasp_set_mac_address,
};
such that MII ioctls work properly.
--
Florian
On 5/31/23 12:18 PM, Krzysztof Kozlowski wrote:
> On 25/05/2023 01:01, Justin Chen wrote:
>> From: Florian Fainelli <[email protected]>
>>
>> Add a binding document for the Broadcom ASP 2.0 Ethernet
>> controller.
>>
>> Signed-off-by: Florian Fainelli <[email protected]>
>> Signed-off-by: Justin Chen <[email protected]>
>> ---
>> v5
>> - Fix compatible string yaml format to properly capture what we want
>>
>> v4
>> - Adjust compatible string example to reference SoC and HW ver
>>
>> v3
>> - Minor formatting issues
>> - Change channel prop to brcm,channel for vendor specific format
>> - Removed redundant v2.0 from compat string
>> - Fix ranges field
>>
>> v2
>> - Minor formatting issues
>>
>> .../devicetree/bindings/net/brcm,asp-v2.0.yaml | 149 +++++++++++++++++++++
>> 1 file changed, 149 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml b/Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml
>> new file mode 100644
>> index 000000000000..c4cd24492bfd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml
>> @@ -0,0 +1,149 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/net/brcm,asp-v2.0.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Broadcom ASP 2.0 Ethernet controller
>> +
>> +maintainers:
>> + - Justin Chen <[email protected]>
>> + - Florian Fainelli <[email protected]>
>> +
>> +description: Broadcom Ethernet controller first introduced with 72165
>> +
>> +properties:
>> + '#address-cells':
>
> Judging by more comments, there will be a v6, thus please also use
> consistent quotes - either ' or ".
>
>> + const: 1
>> + '#size-cells':
>> + const: 1
>> +
>> + compatible:
>
> As Conor pointed out, compatible is always first.
>
>> + oneOf:
>> + - items:
>> + - enum:
>> + - brcm,bcm74165-asp
>> + - const: brcm,asp-v2.1
>> + - items:
>> + - enum:
>> + - brcm,bcm72165-asp
>> + - const: brcm,asp-v2.0
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + ranges: true
>> +
>> + interrupts:
>> + minItems: 1
>> + items:
>> + - description: RX/TX interrupt
>> + - description: Port 0 Wake-on-LAN
>> + - description: Port 1 Wake-on-LAN
>> +
>> + clocks:
>> + maxItems: 1
>> +
>> + ethernet-ports:
>> + type: object
>> + properties:
>> + '#address-cells':
>> + const: 1
>> + '#size-cells':
>> + const: 0
>> +
>> + patternProperties:
>> + "^port@[0-9]+$":
>> + type: object
>> +
>> + $ref: ethernet-controller.yaml#
>> +
>> + properties:
>> + reg:
>> + maxItems: 1
>> + description: Port number
>> +
>> + brcm,channel:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + description: ASP channel number
>
> Why do you need it? reg defines it. Your description does not explain
> here much, except copying property name. Can we please avoid
> descriptions which just copy name?
>
Will add a better description. The values may be different.
Thanks,
Justin