| Message ID | 20220518200939.689308-5-clabbe@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | arm64: add ethernet to orange pi 3 | expand |
On Wed, 18 May 2022 20:09:38 +0000, Corentin Labbe wrote: > Add entries for the new optional regulators. > > Signed-off-by: Corentin Labbe <clabbe@baylibre.com> > --- > Documentation/devicetree/bindings/net/ethernet-phy.yaml | 9 +++++++++ > 1 file changed, 9 insertions(+) > 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: ./Documentation/devicetree/bindings/net/ethernet-phy.yaml:149:8: [warning] wrong indentation: expected 6 but found 7 (indentation) dtschema/dtc warnings/errors: doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. 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.
On 18/05/2022 22:09, Corentin Labbe wrote: > Add entries for the new optional regulators. Please explain why do you need it. > > Signed-off-by: Corentin Labbe <clabbe@baylibre.com> > --- > Documentation/devicetree/bindings/net/ethernet-phy.yaml | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml > index ed1415a4381f..bd59e5cc0a55 100644 > --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml > +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml > @@ -153,6 +153,15 @@ properties: > used. The absence of this property indicates the muxers > should be configured so that the external PHY is used. > > + regulators: > + description: > + List of phandle to regulators needed for the PHY I don't understand that... is your PHY defining the regulators or using supplies? If it needs a regulator (as a supply), you need to document supplies, using existing bindings. > + > + regulator-names: > + description: > + List of regulator name strings sorted in the same order as the > + regulators property. > + > resets: > maxItems: 1 > Best regards, Krzysztof
On Thu, May 19, 2022 at 11:55:28AM +0200, Krzysztof Kozlowski wrote: > On 18/05/2022 22:09, Corentin Labbe wrote: > > + regulators: > > + description: > > + List of phandle to regulators needed for the PHY > I don't understand that... is your PHY defining the regulators or using > supplies? If it needs a regulator (as a supply), you need to document > supplies, using existing bindings. They're trying to have a generic driver which works with any random PHY so the binding has no idea what supplies it might need.
On 19/05/2022 13:31, Mark Brown wrote: > On Thu, May 19, 2022 at 11:55:28AM +0200, Krzysztof Kozlowski wrote: >> On 18/05/2022 22:09, Corentin Labbe wrote: > >>> + regulators: >>> + description: >>> + List of phandle to regulators needed for the PHY > >> I don't understand that... is your PHY defining the regulators or using >> supplies? If it needs a regulator (as a supply), you need to document >> supplies, using existing bindings. > > They're trying to have a generic driver which works with any random PHY > so the binding has no idea what supplies it might need. OK, that makes sense, but then question is why not using existing naming, so "supplies" and "supply-names"? Best regards, Krzysztof
On Thu, May 19, 2022 at 01:33:21PM +0200, Krzysztof Kozlowski wrote: > On 19/05/2022 13:31, Mark Brown wrote: > > On Thu, May 19, 2022 at 11:55:28AM +0200, Krzysztof Kozlowski wrote: > >> On 18/05/2022 22:09, Corentin Labbe wrote: > > > >>> + regulators: > >>> + description: > >>> + List of phandle to regulators needed for the PHY > > > >> I don't understand that... is your PHY defining the regulators or using > >> supplies? If it needs a regulator (as a supply), you need to document > >> supplies, using existing bindings. > > > > They're trying to have a generic driver which works with any random PHY > > so the binding has no idea what supplies it might need. > > OK, that makes sense, but then question is why not using existing > naming, so "supplies" and "supply-names"? I'm not saying it is not possible, but in general, the names are not interesting. All that is needed is that they are all on, or potentially all off to save power on shutdown. We don't care how many there are, or what order they are enabled. Ethernet PHY can have multiple supplies. For example there can be two digital voltages and one analogue. Most designs just hard wire them always on. It would not be unreasonable to have one GPIO which controls all three. Or there could be one GPIO for the two digital supplies, and one for the analogue. Or potentially, three GPIOs. Given all the different ways the board could be designed, i doubt any driver is going to want to control its supplies in an way other than all on, or all off. 802.3 clause 22 defines a standardized way to put a PHY into a low power mode. Using that one bit is much simpler than trying to figure out how a board is wired. However, the API/binding should be generic, usable for other use cases. Nobody has needed an API like this before, but it is not to say it might have other uses in the future. So maybe "supplies" and "supply-names" is useful, but we still need a way to enumerate them as a list without caring how many there are, or what their names are. Andrew
On Thu, May 19, 2022 at 01:58:18PM +0200, Andrew Lunn wrote: > On Thu, May 19, 2022 at 01:33:21PM +0200, Krzysztof Kozlowski wrote: > > On 19/05/2022 13:31, Mark Brown wrote: > > > On Thu, May 19, 2022 at 11:55:28AM +0200, Krzysztof Kozlowski wrote: > > >> On 18/05/2022 22:09, Corentin Labbe wrote: > > >>> + regulators: > > >>> + description: > > >>> + List of phandle to regulators needed for the PHY > > >> I don't understand that... is your PHY defining the regulators or using > > >> supplies? If it needs a regulator (as a supply), you need to document > > >> supplies, using existing bindings. > > > They're trying to have a generic driver which works with any random PHY > > > so the binding has no idea what supplies it might need. > > OK, that makes sense, but then question is why not using existing > > naming, so "supplies" and "supply-names"? > I'm not saying it is not possible, but in general, the names are not > interesting. All that is needed is that they are all on, or > potentially all off to save power on shutdown. We don't care how many > there are, or what order they are enabled. I think Krzysztof is referring to the name of the property rather than the contents of the -names property there.
On Thu, May 19, 2022 at 01:58:18PM +0200, Andrew Lunn wrote: > On Thu, May 19, 2022 at 01:33:21PM +0200, Krzysztof Kozlowski wrote: > > On 19/05/2022 13:31, Mark Brown wrote: > > > On Thu, May 19, 2022 at 11:55:28AM +0200, Krzysztof Kozlowski wrote: > > >> On 18/05/2022 22:09, Corentin Labbe wrote: > > > > > >>> + regulators: > > >>> + description: > > >>> + List of phandle to regulators needed for the PHY > > > > > >> I don't understand that... is your PHY defining the regulators or using > > >> supplies? If it needs a regulator (as a supply), you need to document > > >> supplies, using existing bindings. > > > > > > They're trying to have a generic driver which works with any random PHY > > > so the binding has no idea what supplies it might need. > > > > OK, that makes sense, but then question is why not using existing > > naming, so "supplies" and "supply-names"? > > I'm not saying it is not possible, but in general, the names are not > interesting. All that is needed is that they are all on, or > potentially all off to save power on shutdown. We don't care how many > there are, or what order they are enabled. > > Ethernet PHY can have multiple supplies. For example there can be two > digital voltages and one analogue. Most designs just hard wire them > always on. It would not be unreasonable to have one GPIO which > controls all three. Or there could be one GPIO for the two digital > supplies, and one for the analogue. Or potentially, three GPIOs. Again, it's not just supplies... > > Given all the different ways the board could be designed, i doubt any > driver is going to want to control its supplies in an way other than > all on, or all off. 802.3 clause 22 defines a standardized way to put > a PHY into a low power mode. Using that one bit is much simpler than > trying to figure out how a board is wired. > > However, the API/binding should be generic, usable for other use > cases. The binding should not be generic as I explained here and many times before... > Nobody has needed an API like this before, but it is not to say > it might have other uses in the future. So maybe "supplies" and > "supply-names" is useful, but we still need a way to enumerate them as > a list without caring how many there are, or what their names are. There's 2 standard patterns for how producer/consumer bindings work There's how gpio and regulators are done and then there's the foo/foo-names style. Regulators when with the former and we're not going to do both. You can still do what you want by retrieving all properties ending with '-supply'. Not as easy to implement, but works for existing users. Rob
On 19/05/2022 17:49, Mark Brown wrote: > On Thu, May 19, 2022 at 01:58:18PM +0200, Andrew Lunn wrote: >> On Thu, May 19, 2022 at 01:33:21PM +0200, Krzysztof Kozlowski wrote: >>> On 19/05/2022 13:31, Mark Brown wrote: >>>> On Thu, May 19, 2022 at 11:55:28AM +0200, Krzysztof Kozlowski wrote: >>>>> On 18/05/2022 22:09, Corentin Labbe wrote: > >>>>>> + regulators: >>>>>> + description: >>>>>> + List of phandle to regulators needed for the PHY > >>>>> I don't understand that... is your PHY defining the regulators or using >>>>> supplies? If it needs a regulator (as a supply), you need to document >>>>> supplies, using existing bindings. > >>>> They're trying to have a generic driver which works with any random PHY >>>> so the binding has no idea what supplies it might need. > >>> OK, that makes sense, but then question is why not using existing >>> naming, so "supplies" and "supply-names"? > >> I'm not saying it is not possible, but in general, the names are not >> interesting. All that is needed is that they are all on, or >> potentially all off to save power on shutdown. We don't care how many >> there are, or what order they are enabled. > > I think Krzysztof is referring to the name of the property rather than > the contents of the -names property there. Yes, exactly. Existing pattern for single regulator supply is "xxx-supply", so why this uses a bit different pattern instead of something more consistent ("supplies" and "supply-names")? Best regards, Krzysztof
Le Fri, May 20, 2022 at 09:57:26AM +0200, Krzysztof Kozlowski a écrit : > On 19/05/2022 17:49, Mark Brown wrote: > > On Thu, May 19, 2022 at 01:58:18PM +0200, Andrew Lunn wrote: > >> On Thu, May 19, 2022 at 01:33:21PM +0200, Krzysztof Kozlowski wrote: > >>> On 19/05/2022 13:31, Mark Brown wrote: > >>>> On Thu, May 19, 2022 at 11:55:28AM +0200, Krzysztof Kozlowski wrote: > >>>>> On 18/05/2022 22:09, Corentin Labbe wrote: > > > >>>>>> + regulators: > >>>>>> + description: > >>>>>> + List of phandle to regulators needed for the PHY > > > >>>>> I don't understand that... is your PHY defining the regulators or using > >>>>> supplies? If it needs a regulator (as a supply), you need to document > >>>>> supplies, using existing bindings. > > > >>>> They're trying to have a generic driver which works with any random PHY > >>>> so the binding has no idea what supplies it might need. > > > >>> OK, that makes sense, but then question is why not using existing > >>> naming, so "supplies" and "supply-names"? > > > >> I'm not saying it is not possible, but in general, the names are not > >> interesting. All that is needed is that they are all on, or > >> potentially all off to save power on shutdown. We don't care how many > >> there are, or what order they are enabled. > > > > I think Krzysztof is referring to the name of the property rather than > > the contents of the -names property there. > > Yes, exactly. Existing pattern for single regulator supply is > "xxx-supply", so why this uses a bit different pattern instead of > something more consistent ("supplies" and "supply-names")? > I agree that supplies and supply-names are better. But in another answer Rob is against it, so if I understand well, we are stuck to use individual xxx-supply. I will try to create a new regulator_get_bulk_all() which scan all properties matching xxx-supply
On 20/05/2022 10:15, LABBE Corentin wrote: > > I agree that supplies and supply-names are better. > But in another answer Rob is against it, so if I understand well, we are stuck to use individual xxx-supply. > I will try to create a new regulator_get_bulk_all() which scan all properties matching xxx-supply Yep. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml index ed1415a4381f..bd59e5cc0a55 100644 --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml @@ -153,6 +153,15 @@ properties: used. The absence of this property indicates the muxers should be configured so that the external PHY is used. + regulators: + description: + List of phandle to regulators needed for the PHY + + regulator-names: + description: + List of regulator name strings sorted in the same order as the + regulators property. + resets: maxItems: 1
Add entries for the new optional regulators. Signed-off-by: Corentin Labbe <clabbe@baylibre.com> --- Documentation/devicetree/bindings/net/ethernet-phy.yaml | 9 +++++++++ 1 file changed, 9 insertions(+)