* Re: [PATCH net-next 2/2] dt-bindings: net: dsa: realtek: add reset controller
2023-10-24 20:58 ` [PATCH net-next 2/2] dt-bindings: net: dsa: realtek: add reset controller Luiz Angelo Daros de Luca
@ 2023-10-24 22:39 ` Vladimir Oltean
2023-10-26 10:34 ` Arınç ÜNAL
2023-10-26 20:58 ` Rob Herring
2 siblings, 0 replies; 6+ messages in thread
From: Vladimir Oltean @ 2023-10-24 22:39 UTC (permalink / raw)
To: Luiz Angelo Daros de Luca
Cc: netdev, linus.walleij, alsi, andrew, vivien.didelot, f.fainelli,
davem, kuba, pabeni, robh+dt, krzk+dt, arinc.unal, devicetree
On Tue, Oct 24, 2023 at 05:58:06PM -0300, Luiz Angelo Daros de Luca wrote:
> Realtek switches can now be reset using a reset controller.
>
> The 'reset-gpios' were never mandatory for the driver, although they
> are required for some devices if the switch reset was left asserted by
> a previous driver, such as the bootloader.
>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> Cc: devicetree@vger.kernel.org
> ---
I believe there was a rule that device tree binding patches should come
before the user of those bindings.
> Documentation/devicetree/bindings/net/dsa/realtek.yaml | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> index cce692f57b08..070821eae2a7 100644
> --- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> @@ -59,6 +59,12 @@ properties:
> description: GPIO to be used to reset the whole device
> maxItems: 1
>
> + resets:
> + maxItems: 1
> +
> + reset-names:
> + const: switch
> +
> realtek,disable-leds:
> type: boolean
> description: |
> @@ -127,7 +133,6 @@ else:
> - mdc-gpios
> - mdio-gpios
> - mdio
> - - reset-gpios
Ideally, the change that makes the reset-gpios optional should not be
named "add reset controller", unless it is actually the addition of the
reset controller which makes it optional. Which you say is not the case.
So, I think it should be a separate change.
>
> required:
> - compatible
> --
> 2.42.0
>
I also commented this on the other change: please move the example here.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next 2/2] dt-bindings: net: dsa: realtek: add reset controller
2023-10-24 20:58 ` [PATCH net-next 2/2] dt-bindings: net: dsa: realtek: add reset controller Luiz Angelo Daros de Luca
2023-10-24 22:39 ` Vladimir Oltean
@ 2023-10-26 10:34 ` Arınç ÜNAL
2023-10-26 20:09 ` Luiz Angelo Daros de Luca
2023-10-26 20:58 ` Rob Herring
2 siblings, 1 reply; 6+ messages in thread
From: Arınç ÜNAL @ 2023-10-26 10:34 UTC (permalink / raw)
To: Luiz Angelo Daros de Luca, netdev
Cc: linus.walleij, alsi, andrew, vivien.didelot, f.fainelli, olteanv,
davem, kuba, pabeni, robh+dt, krzk+dt, devicetree
Nice work Luiz.
On 24.10.2023 23:58, Luiz Angelo Daros de Luca wrote:
> Realtek switches can now be reset using a reset controller.
The switch could always be reset using a reset controller. The fact that
the Linux driver lacked the ability to do so is irrelevant here. The
abilities or the features of the hardware had never changed. You should get
rid of the "now" above.
>
> The 'reset-gpios' were never mandatory for the driver, although they
> are required for some devices if the switch reset was left asserted by
> a previous driver, such as the bootloader.
dt-bindings are for documenting hardware. The Linux driver details are
irrelevant here. Also, from what I read above, I deduce that for the switch
to be properly controlled in all possible states that it would be found in,
the switch must be reset.
So instead of above I'd say:
Resetting the switch is mandatory. Resetting the switch with reset-gpios is
not mandatory. Therefore require one of reset-gpios or resets and
reset-names.
For dt-bindings changes, I'd remove reset-gpios from else of
if:required:reg as you already do with this patch, and add below to the
root of the schema.
oneOf:
- required:
- reset-gpios
- required:
- resets
- reset-names
And, like Vladimir said, this should be a separate patch.
Also, please put the dt-bindings patches first in the patch series order.
Arınç
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next 2/2] dt-bindings: net: dsa: realtek: add reset controller
2023-10-26 10:34 ` Arınç ÜNAL
@ 2023-10-26 20:09 ` Luiz Angelo Daros de Luca
0 siblings, 0 replies; 6+ messages in thread
From: Luiz Angelo Daros de Luca @ 2023-10-26 20:09 UTC (permalink / raw)
To: Arınç ÜNAL
Cc: netdev, linus.walleij, alsi, andrew, vivien.didelot, f.fainelli,
olteanv, davem, kuba, pabeni, robh+dt, krzk+dt, devicetree
> On 24.10.2023 23:58, Luiz Angelo Daros de Luca wrote:
> > Realtek switches can now be reset using a reset controller.
>
> The switch could always be reset using a reset controller. The fact that
> the Linux driver lacked the ability to do so is irrelevant here. The
> abilities or the features of the hardware had never changed. You should get
> rid of the "now" above.
Yes, I need to avoid thinking about where it will be used.
> > The 'reset-gpios' were never mandatory for the driver, although they
> > are required for some devices if the switch reset was left asserted by
> > a previous driver, such as the bootloader.
>
> dt-bindings are for documenting hardware. The Linux driver details are
> irrelevant here. Also, from what I read above, I deduce that for the switch
> to be properly controlled in all possible states that it would be found in,
> the switch must be reset.
I don't believe the switch must be reset by the HW. It will only be
necessary if the reset was kept asserted by the previous driver or its
initial state. And even in that case, we would only need to deassert
the reset, not assert it.
The driver will sw reset the switch during setup, leaving it in a
pristine state.
This is the current code flow:
realtek-smi/mdio probe()
HW reset assert
sleep
HW reset deassert
sleep
rtl8365mb/rtl8366rb detect()
SW reset
In fact, if we could make sure the hw reset was actually performed,
like checking a switch uptime register, we could avoid resetting it
again.
If you must not consider how the driver was implemented, I must assume
that an advanced driver might be able to configure every aspect of the
switch without a reset. Reset is the easiest solution but I believe
there is a narrow window between the reset and the switch is properly
configured where the switch might act like a dump switch, forwarding
some packets to all ports and possibly leaking traffic.
> So instead of above I'd say:
>
> Resetting the switch is mandatory. Resetting the switch with reset-gpios is
> not mandatory. Therefore require one of reset-gpios or resets and
> reset-names.
>
> For dt-bindings changes, I'd remove reset-gpios from else of
> if:required:reg as you already do with this patch, and add below to the
> root of the schema.
>
> oneOf:
> - required:
> - reset-gpios
> - required:
> - resets
> - reset-names
As I said, I don't believe a way to HW reset is mandatory.
> And, like Vladimir said, this should be a separate patch.
OK. I'm just waiting some days for a v2.
>
> Also, please put the dt-bindings patches first in the patch series order.
OK
>
> Arınç
Regards,
Luiz
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next 2/2] dt-bindings: net: dsa: realtek: add reset controller
2023-10-24 20:58 ` [PATCH net-next 2/2] dt-bindings: net: dsa: realtek: add reset controller Luiz Angelo Daros de Luca
2023-10-24 22:39 ` Vladimir Oltean
2023-10-26 10:34 ` Arınç ÜNAL
@ 2023-10-26 20:58 ` Rob Herring
2023-10-26 22:03 ` Luiz Angelo Daros de Luca
2 siblings, 1 reply; 6+ messages in thread
From: Rob Herring @ 2023-10-26 20:58 UTC (permalink / raw)
To: Luiz Angelo Daros de Luca
Cc: netdev, linus.walleij, alsi, andrew, vivien.didelot, f.fainelli,
olteanv, davem, kuba, pabeni, krzk+dt, arinc.unal, devicetree
On Tue, Oct 24, 2023 at 05:58:06PM -0300, Luiz Angelo Daros de Luca wrote:
> Realtek switches can now be reset using a reset controller.
>
> The 'reset-gpios' were never mandatory for the driver, although they
> are required for some devices if the switch reset was left asserted by
> a previous driver, such as the bootloader.
>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> Cc: devicetree@vger.kernel.org
> ---
> Documentation/devicetree/bindings/net/dsa/realtek.yaml | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> index cce692f57b08..070821eae2a7 100644
> --- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> @@ -59,6 +59,12 @@ properties:
> description: GPIO to be used to reset the whole device
> maxItems: 1
>
> + resets:
> + maxItems: 1
> +
> + reset-names:
> + const: switch
$block-name is not really a useful name for resources. Generally, you
don't need -names if there's only 1 entry.
> +
> realtek,disable-leds:
> type: boolean
> description: |
> @@ -127,7 +133,6 @@ else:
> - mdc-gpios
> - mdio-gpios
> - mdio
> - - reset-gpios
>
> required:
> - compatible
> --
> 2.42.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next 2/2] dt-bindings: net: dsa: realtek: add reset controller
2023-10-26 20:58 ` Rob Herring
@ 2023-10-26 22:03 ` Luiz Angelo Daros de Luca
0 siblings, 0 replies; 6+ messages in thread
From: Luiz Angelo Daros de Luca @ 2023-10-26 22:03 UTC (permalink / raw)
To: Rob Herring
Cc: netdev, linus.walleij, alsi, andrew, vivien.didelot, f.fainelli,
olteanv, davem, kuba, pabeni, krzk+dt, arinc.unal, devicetree
> > + resets:
> > + maxItems: 1
> > +
> > + reset-names:
> > + const: switch
>
> $block-name is not really a useful name for resources. Generally, you
> don't need -names if there's only 1 entry.
I didn't know the reset-control name was optional. Yes, it is not
useful. I'll get rid of it.
It looks like there are more bindings where it is not necessary.
Regards,
Luiz
^ permalink raw reply [flat|nested] 6+ messages in thread