On Tue, 23 May 2023 14:53:43 -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]>
> ---
> v3
> - 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 | 145 +++++++++++++++++++++
> 1 file changed, 145 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/brcm,asp-v2.0.example.dtb: ethernet@9c00000: compatible: ['brcm,bcm72165-asp', 'brcm,asp-v2.0'] is too long
From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Hey Justin,
On Tue, May 23, 2023 at 02:53:43PM -0700, Justin Chen wrote:
> + compatible:
> + enum:
> + - brcm,asp-v2.0
> + - brcm,bcm72165-asp
> + - brcm,asp-v2.1
> + - brcm,bcm74165-asp
> + compatible = "brcm,bcm72165-asp", "brcm,asp-v2.0";
You can't do this, as Rob's bot has pointed out. Please test the
bindings :( You need one of these type of constructs:
compatible:
oneOf:
- items:
- const: brcm,bcm72165-asp
- const: brcm,asp-v2.0
- items:
- const: brcm,bcm74165-asp
- const: brcm,asp-v2.1
Although, given either you or Florian said there are likely to be
multiple parts, going for an enum, rather than const for the brcm,bcm..
entry will prevent some churn. Up to you.
Cheers,
Conor.
On Tue, May 23, 2023 at 3:55 PM Conor Dooley <[email protected]> wrote:
>
> Hey Justin,
>
> On Tue, May 23, 2023 at 02:53:43PM -0700, Justin Chen wrote:
>
> > + compatible:
> > + enum:
> > + - brcm,asp-v2.0
> > + - brcm,bcm72165-asp
> > + - brcm,asp-v2.1
> > + - brcm,bcm74165-asp
>
> > + compatible = "brcm,bcm72165-asp", "brcm,asp-v2.0";
>
> You can't do this, as Rob's bot has pointed out. Please test the
> bindings :( You need one of these type of constructs:
>
> compatible:
> oneOf:
> - items:
> - const: brcm,bcm72165-asp
> - const: brcm,asp-v2.0
> - items:
> - const: brcm,bcm74165-asp
> - const: brcm,asp-v2.1
>
> Although, given either you or Florian said there are likely to be
> multiple parts, going for an enum, rather than const for the brcm,bcm..
> entry will prevent some churn. Up to you.
>
Urg so close. Thought it was a trivial change, so didn't bother
retesting the binding. I think I have it right now...
compatible:
oneOf:
- items:
- enum:
- brcm,bcm72165-asp
- brcm,bcm74165-asp
- enum:
- brcm,asp-v2.0
- brcm,asp-v2.1
Something like this look good? Will submit a v5 tomorrow.
Thanks,
Justin
> Cheers,
> Conor.
Hey Justin,
On Tue, May 23, 2023 at 04:27:12PM -0700, Justin Chen wrote:
> On Tue, May 23, 2023 at 3:55 PM Conor Dooley <[email protected]> wrote:
> > On Tue, May 23, 2023 at 02:53:43PM -0700, Justin Chen wrote:
> >
> > > + compatible:
> > > + enum:
> > > + - brcm,asp-v2.0
> > > + - brcm,bcm72165-asp
> > > + - brcm,asp-v2.1
> > > + - brcm,bcm74165-asp
> >
> > > + compatible = "brcm,bcm72165-asp", "brcm,asp-v2.0";
> >
> > You can't do this, as Rob's bot has pointed out. Please test the
> > bindings :( You need one of these type of constructs:
> >
> > compatible:
> > oneOf:
> > - items:
> > - const: brcm,bcm72165-asp
> > - const: brcm,asp-v2.0
> > - items:
> > - const: brcm,bcm74165-asp
> > - const: brcm,asp-v2.1
> >
> > Although, given either you or Florian said there are likely to be
> > multiple parts, going for an enum, rather than const for the brcm,bcm..
> > entry will prevent some churn. Up to you.
> >
> Urg so close. Thought it was a trivial change, so didn't bother
> retesting the binding. I think I have it right now...
>
> compatible:
> oneOf:
> - items:
> - enum:
> - brcm,bcm72165-asp
> - brcm,bcm74165-asp
> - enum:
> - brcm,asp-v2.0
> - brcm,asp-v2.1
>
> Something like this look good?
I am still caffeine-less, but this implies that both of
"brcm,bcm72165-asp", "brcm,asp-v2.0"
_and_
"brcm,bcm72165-asp", "brcm,asp-v2.1"
are. I suspect that that is not the case, unless "brcm,asp-v2.0" is a
valid fallback for "brcm,asp-v2.1"?
The oneOf: also becomes redundant since you only have one items:.
> Will submit a v5 tomorrow.
BTW, when you do, could you use the address listed in MAINTAINERS rather
than the one you used for this version?
Cheers,
Conor.
On Wed, May 24, 2023 at 07:51:07AM +0100, Conor Dooley wrote:
> Hey Justin,
> On Tue, May 23, 2023 at 04:27:12PM -0700, Justin Chen wrote:
> > On Tue, May 23, 2023 at 3:55 PM Conor Dooley <[email protected]> wrote:
> > > On Tue, May 23, 2023 at 02:53:43PM -0700, Justin Chen wrote:
> > >
> > > > + compatible:
> > > > + enum:
> > > > + - brcm,asp-v2.0
> > > > + - brcm,bcm72165-asp
> > > > + - brcm,asp-v2.1
> > > > + - brcm,bcm74165-asp
> > >
> > > > + compatible = "brcm,bcm72165-asp", "brcm,asp-v2.0";
> > >
> > > You can't do this, as Rob's bot has pointed out. Please test the
> > > bindings :( You need one of these type of constructs:
> > >
> > > compatible:
> > > oneOf:
> > > - items:
> > > - const: brcm,bcm72165-asp
> > > - const: brcm,asp-v2.0
> > > - items:
> > > - const: brcm,bcm74165-asp
> > > - const: brcm,asp-v2.1
> > >
> > > Although, given either you or Florian said there are likely to be
> > > multiple parts, going for an enum, rather than const for the brcm,bcm..
> > > entry will prevent some churn. Up to you.
> > >
> > Urg so close. Thought it was a trivial change, so didn't bother
> > retesting the binding. I think I have it right now...
> >
> > compatible:
> > oneOf:
> > - items:
> > - enum:
> > - brcm,bcm72165-asp
> > - brcm,bcm74165-asp
> > - enum:
> > - brcm,asp-v2.0
> > - brcm,asp-v2.1
> >
> > Something like this look good?
>
> I am still caffeine-less, but this implies that both of
> "brcm,bcm72165-asp", "brcm,asp-v2.0"
> _and_
> "brcm,bcm72165-asp", "brcm,asp-v2.1"
> are. I suspect that that is not the case, unless "brcm,asp-v2.0" is a
I a word. s/are/are valid/
> valid fallback for "brcm,asp-v2.1"?
> The oneOf: also becomes redundant since you only have one items:.
>
> > Will submit a v5 tomorrow.
>
> BTW, when you do, could you use the address listed in MAINTAINERS rather
> than the one you used for this version?
>
> Cheers,
> Conor.
On Tue, May 23, 2023 at 02:53:44PM -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.)
>
> Signed-off-by: Florian Fainelli <[email protected]>
> Signed-off-by: Justin Chen <[email protected]>
Hi Justin,
Reviewed-by: Simon Horman <[email protected]>
As I see there will be a v5 I have added a few nits from my side.
Feel free to ignore them as you see fit.
...
> +int bcmasp_netfilt_check_dup(struct bcmasp_intf *intf,
> + struct ethtool_rx_flow_spec *fs)
nit: the return type of this function could be bool
> +{
> + struct bcmasp_priv *priv = intf->parent;
> + struct ethtool_rx_flow_spec *cur;
> + size_t fs_size = 0;
> + int i;
> +
> + for (i = 0; i < NUM_NET_FILTERS; i++) {
> + if (!priv->net_filters[i].claimed ||
> + priv->net_filters[i].port != intf->port)
> + continue;
> +
> + cur = &priv->net_filters[i].fs;
> +
> + if (cur->flow_type != fs->flow_type ||
> + cur->ring_cookie != fs->ring_cookie)
> + continue;
> +
> + switch (fs->flow_type & ~(FLOW_EXT | FLOW_MAC_EXT)) {
> + case ETHER_FLOW:
> + fs_size = sizeof(struct ethhdr);
> + break;
> + case IP_USER_FLOW:
> + fs_size = sizeof(struct ethtool_usrip4_spec);
> + break;
> + case TCP_V6_FLOW:
> + case UDP_V6_FLOW:
> + fs_size = sizeof(struct ethtool_tcpip6_spec);
> + break;
> + case TCP_V4_FLOW:
> + case UDP_V4_FLOW:
> + fs_size = sizeof(struct ethtool_tcpip4_spec);
> + break;
> + default:
> + continue;
> + }
> +
> + if (memcmp(&cur->h_u, &fs->h_u, fs_size) ||
> + memcmp(&cur->m_u, &fs->m_u, fs_size))
> + continue;
> +
> + if (cur->flow_type & FLOW_EXT) {
> + if (cur->h_ext.vlan_etype != fs->h_ext.vlan_etype ||
> + cur->m_ext.vlan_etype != fs->m_ext.vlan_etype ||
> + cur->h_ext.vlan_tci != fs->h_ext.vlan_tci ||
> + cur->m_ext.vlan_tci != fs->m_ext.vlan_tci ||
> + cur->h_ext.data[0] != fs->h_ext.data[0])
> + continue;
> + }
> + if (cur->flow_type & FLOW_MAC_EXT) {
> + if (memcmp(&cur->h_ext.h_dest,
> + &fs->h_ext.h_dest, ETH_ALEN) ||
> + memcmp(&cur->m_ext.h_dest,
> + &fs->m_ext.h_dest, ETH_ALEN))
> + continue;
> + }
> +
> + return 1;
> + }
> +
> + return 0;
> +}
...
> +static int bcmasp_is_port_valid(struct bcmasp_priv *priv, int port)
> +{
> + /* Quick sanity check
> + * Ports 0/1 reserved for unimac
> + * Max supported ports is 2
> + */
> + return (port == 0 || port == 1);
nit: unnecessary parentheses
> +}
...
On Tue, May 23, 2023 at 02:53:47PM -0700, Justin Chen wrote:
> Add maintainers entry for ASP 2.0 Ethernet driver.
>
> Signed-off-by: Florian Fainelli <[email protected]>
> Signed-off-by: Justin Chen <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
On Tue, May 23, 2023 at 02:53:42PM -0700, Justin Chen wrote:
> The ASP 2.0 Ethernet controller uses a brcm unimac.
>
> Acked-by: Conor Dooley <[email protected]>
> Signed-off-by: Florian Fainelli <[email protected]>
> Signed-off-by: Justin Chen <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
On Tue, May 23, 2023 at 11:56 PM Conor Dooley
<[email protected]> wrote:
>
> On Wed, May 24, 2023 at 07:51:07AM +0100, Conor Dooley wrote:
> > Hey Justin,
> > On Tue, May 23, 2023 at 04:27:12PM -0700, Justin Chen wrote:
> > > On Tue, May 23, 2023 at 3:55 PM Conor Dooley <[email protected]> wrote:
> > > > On Tue, May 23, 2023 at 02:53:43PM -0700, Justin Chen wrote:
> > > >
> > > > > + compatible:
> > > > > + enum:
> > > > > + - brcm,asp-v2.0
> > > > > + - brcm,bcm72165-asp
> > > > > + - brcm,asp-v2.1
> > > > > + - brcm,bcm74165-asp
> > > >
> > > > > + compatible = "brcm,bcm72165-asp", "brcm,asp-v2.0";
> > > >
> > > > You can't do this, as Rob's bot has pointed out. Please test the
> > > > bindings :( You need one of these type of constructs:
> > > >
> > > > compatible:
> > > > oneOf:
> > > > - items:
> > > > - const: brcm,bcm72165-asp
> > > > - const: brcm,asp-v2.0
> > > > - items:
> > > > - const: brcm,bcm74165-asp
> > > > - const: brcm,asp-v2.1
> > > >
> > > > Although, given either you or Florian said there are likely to be
> > > > multiple parts, going for an enum, rather than const for the brcm,bcm..
> > > > entry will prevent some churn. Up to you.
> > > >
> > > Urg so close. Thought it was a trivial change, so didn't bother
> > > retesting the binding. I think I have it right now...
> > >
> > > compatible:
> > > oneOf:
> > > - items:
> > > - enum:
> > > - brcm,bcm72165-asp
> > > - brcm,bcm74165-asp
> > > - enum:
> > > - brcm,asp-v2.0
> > > - brcm,asp-v2.1
> > >
> > > Something like this look good?
> >
> > I am still caffeine-less, but this implies that both of
> > "brcm,bcm72165-asp", "brcm,asp-v2.0"
> > _and_
> > "brcm,bcm72165-asp", "brcm,asp-v2.1"
> > are. I suspect that that is not the case, unless "brcm,asp-v2.0" is a
>
> I a word. s/are/are valid/
>
Gotcha. I got something like this now.
compatible:
oneOf:
- items:
- enum:
- brcm,bcm74165-asp
- const: brcm,asp-v2.1
- items:
- enum:
- brcm,bcm72165-asp
- const: brcm,asp-v2.0
Apologies, still getting used to this yaml stuff. Starting to make a
bit more sense to me now.
> > valid fallback for "brcm,asp-v2.1"?
> > The oneOf: also becomes redundant since you only have one items:.
> >
> > > Will submit a v5 tomorrow.
> >
> > BTW, when you do, could you use the address listed in MAINTAINERS rather
> > than the one you used for this version?
> >
I changed the address listed in MAINTAINERS from the previous versions
of this patchset. The current version should match the address that
this patch set was sent from. Looks like I forgot to add a changelog
for that in v4.
Thanks,
Justin
> > Cheers,
> > Conor.
On Wed, May 24, 2023 at 02:47:59PM -0700, Justin Chen wrote:
> On Tue, May 23, 2023 at 11:56 PM Conor Dooley <[email protected]> wrote:
> Gotcha. I got something like this now.
>
> compatible:
> oneOf:
> - items:
> - enum:
> - brcm,bcm74165-asp
> - const: brcm,asp-v2.1
> - items:
> - enum:
> - brcm,bcm72165-asp
> - const: brcm,asp-v2.0
Yes, this is what I had in mind.
> Apologies, still getting used to this yaml stuff. Starting to make a
> bit more sense to me now.
No worries.
> > > valid fallback for "brcm,asp-v2.1"?
> > > The oneOf: also becomes redundant since you only have one items:.
> > >
> > > > Will submit a v5 tomorrow.
> > >
> > > BTW, when you do, could you use the address listed in MAINTAINERS rather
> > > than the one you used for this version?
> > >
> I changed the address listed in MAINTAINERS from the previous versions
> of this patchset. The current version should match the address that
> this patch set was sent from. Looks like I forgot to add a changelog
> for that in v4.
Hmm, I must not have been clear. You sent it to <[email protected]> and I
was hoping that you would use <[email protected]> instead so that you
end up hitting the right mail filters :) It's not a problem, I was just
added to it in -rc1 so get_maintainer.pl probably didn't spit my name
out for your original revision.
Thanks,
Conor.