| Message ID | 20211209094615.329379-3-horatiu.vultur@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: lan966x: Add switchdev and vlan support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 14 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Thu, Dec 09, 2021 at 10:46:11AM +0100, Horatiu Vultur wrote: > Extend dt-bindings for lan966x with analyzer interrupt. > This interrupt can be generated for example when the HW learn/forgets > an entry in the MAC table. > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> > --- Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> Why don't you describe your hardware in the device tree all at once? Doing it piece by piece means that every time when you add a new functionality you need to be compatible with the absence of a certain reg, interrupt etc. > .../devicetree/bindings/net/microchip,lan966x-switch.yaml | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml b/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml > index 5bee665d5fcf..e79e4e166ad8 100644 > --- a/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml > +++ b/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml > @@ -37,12 +37,14 @@ properties: > items: > - description: register based extraction > - description: frame dma based extraction > + - description: analyzer interrupt > > interrupt-names: > minItems: 1 > items: > - const: xtr > - const: fdma > + - const: ana > > resets: > items: > -- > 2.33.0 >
The 12/09/2021 10:58, Vladimir Oltean wrote: > > On Thu, Dec 09, 2021 at 10:46:11AM +0100, Horatiu Vultur wrote: > > Extend dt-bindings for lan966x with analyzer interrupt. > > This interrupt can be generated for example when the HW learn/forgets > > an entry in the MAC table. > > > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> > > --- > > Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > Why don't you describe your hardware in the device tree all at once? > Doing it piece by piece means that every time when you add a new > functionality you need to be compatible with the absence of a certain > reg, interrupt etc. I though it is more clear what is added in the patch series. But then, if for example add more interrupts in DT than what the driver support, that would not be an issue? > > > .../devicetree/bindings/net/microchip,lan966x-switch.yaml | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml b/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml > > index 5bee665d5fcf..e79e4e166ad8 100644 > > --- a/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml > > +++ b/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml > > @@ -37,12 +37,14 @@ properties: > > items: > > - description: register based extraction > > - description: frame dma based extraction > > + - description: analyzer interrupt > > > > interrupt-names: > > minItems: 1 > > items: > > - const: xtr > > - const: fdma > > + - const: ana > > > > resets: > > items: > > -- > > 2.33.0 > >
On Thu, Dec 09, 2021 at 04:42:47PM +0100, Horatiu Vultur wrote: > The 12/09/2021 10:58, Vladimir Oltean wrote: > > > > On Thu, Dec 09, 2021 at 10:46:11AM +0100, Horatiu Vultur wrote: > > > Extend dt-bindings for lan966x with analyzer interrupt. > > > This interrupt can be generated for example when the HW learn/forgets > > > an entry in the MAC table. > > > > > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> > > > --- > > > > Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > > > Why don't you describe your hardware in the device tree all at once? > > Doing it piece by piece means that every time when you add a new > > functionality you need to be compatible with the absence of a certain > > reg, interrupt etc. > > I though it is more clear what is added in the patch series. > But then, if for example add more interrupts in DT than what the > driver support, that would not be an issue? I haven't kept track of the lan966x driver development. It looks like it is pretty new, so I think it's ok in this case. But I've also seen features introduced years after the driver was initially published (see ocelot fdma) where device tree updates were still necessary, due to minor things like these: an interrupt isn't there, the registers for FDMA aren't there, etc. After that kind of time you'd expect the DT to no longer require updates unless there is some unforeseen event (something is broken, a driver is radically rethought). Sure there's a fine line between how much you can add to the device tree and the how many consumers there are in the kernel, but on the other hand the kernel doesn't have to use everything that's in the device tree. For example, at Mark Brown's suggestion, the DSPI nodes in ls1028a.dtsi declare their DMA channels even though the driver does not use them (it could, though, but it would be slower). Similarly, the DSPI driver for LS1021A has had a while in which it ignored the interrupt line from the device tree, because poll mode was simply faster. To me, this kind of approach where the device tree is provisioned even for configurations that aren't supported today makes sense, precisely because the DT blob and the kernel have different lifetimes. It's better to have the interrupt and not use it than to need it and not have it.
diff --git a/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml b/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml index 5bee665d5fcf..e79e4e166ad8 100644 --- a/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml +++ b/Documentation/devicetree/bindings/net/microchip,lan966x-switch.yaml @@ -37,12 +37,14 @@ properties: items: - description: register based extraction - description: frame dma based extraction + - description: analyzer interrupt interrupt-names: minItems: 1 items: - const: xtr - const: fdma + - const: ana resets: items:
Extend dt-bindings for lan966x with analyzer interrupt. This interrupt can be generated for example when the HW learn/forgets an entry in the MAC table. Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> --- .../devicetree/bindings/net/microchip,lan966x-switch.yaml | 2 ++ 1 file changed, 2 insertions(+)