Message-ID: <20241022081304.d2tpnn4eyf3adxg2@pengutronix.de>
Date: Tue, 22 Oct 2024 10:13:04 +0200
From: Marco Felsch <m.felsch@...gutronix.de>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: POPESCU Catalin <catalin.popescu@...ca-geosystems.com>,
	Sherry Sun <sherry.sun@....com>,
	Amitkumar Karwar <amitkumar.karwar@....com>,
	Neeraj Sanjay Kale <neeraj.sanjaykale@....com>,
	"marcel@...tmann.org" <marcel@...tmann.org>,
	"luiz.dentz@...il.com" <luiz.dentz@...il.com>,
	"robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>,
	"p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
	"linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	GEO-CHHER-bsp-development <bsp-development.geo@...ca-geosystems.com>
Subject: Re: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add support for
 supply and reset

On 24-10-22, Krzysztof Kozlowski wrote:
> On 22/10/2024 09:30, Krzysztof Kozlowski wrote:
> > On 22/10/2024 09:12, Marco Felsch wrote:
> >> On 24-10-22, Krzysztof Kozlowski wrote:
> >>> On 21/10/2024 12:25, Marco Felsch wrote:
> >>>> On 24-10-21, Krzysztof Kozlowski wrote:
> >>>>> On 21/10/2024 08:41, Marco Felsch wrote:
> >>>>>> On 24-10-07, Krzysztof Kozlowski wrote:
> >>
> >> ...
> >>
> >>>>>>> Based on earlier message:
> >>>>>>>
> >>>>>>> "For NXP WIFI/BT chip, WIFI and BT share the one PDn pin, which means
> >>>>>>> that both wifi and BT controller will be powered on and off at the same
> >>>>>>> time."
> >>>>>>>
> >>>>>>> but maybe that's not needed. No clue, I don't know the hardware. But be
> >>>>>>> carefully what you write in the bindings, because then it will be ABI.
> >>>>>>
> >>>>>> We noticed the new power-sequencing infrastructure which is part of 6.11
> >>>>>> too but I don't think that this patch is wrong. The DT ABI won't break
> >>>>>> if we switch to the power-sequencing later on since the "reset-gpios"
> >>>>>> are not marked as required. So it is up to the driver to handle it
> >>>>>> either via a separate power-sequence driver or via "power-supply" and
> >>>>>> "reset-gpios" directly.
> >>>>>
> >>>>> That's not the point. We expect correct hardware description. If you say
> >>>>> now it has "reset-gpios" but later say "actually no, because it has
> >>>>> PMU", I respond: no. Describe the hardware, not current Linux.
> >>>>
> >>>> I know that DT abstracts the HW. That said I don't see the problem with
> >>>> this patch. The HW is abstracted just fine:
> >>>>
> >>>> shared PDn          -> reset-gpios
> >>>> shared power-supply -> vcc-supply
> >>>>
> >>>> Right now the DT ABI for the BT part is incomplete since it assume a
> >>>> running WLAN part or some hog-gpios to pull the device out-of-reset
> >>>> which is addressed by this patchset.
> >>>>
> >>>> Making use of the new power-sequencing fw is a Linux detail and I don't
> >>>> see why the DT can't be extended later on. We always extend the DT if
> >>>> something is missing or if we found a better way to handle devices.
> >>>
> >>> Sure, although I am not really confident that you understand the
> >>> implications - you will not be able to switch to proper power-sequencing
> >>> with above bindings, because it will not be just possible without
> >>> breaking the ABI or changing hardware description (which you say it is
> >>> "fine", so complete/done). I am fine with it, just mind the implications.
> >>
> >> Sorry can you please share your concerns? I don't get the point yet why
> >> we do break the DT ABI if we are going from
> > 
> > Not necessarily breaking ABI, but changing the description.
> >>
> >> bt {
> >> 	reset-gpios = <&gpio 4 0>;
> >> 	vcc-supply = <&supply>;
> >> };
> >>
> >> to
> >>
> >> bt {
> >> 	vcc-supply = <&pmu_supply>;
> > 
> > ...because you just removed reset-gpios which is a property of this device.

An optional property. That beeing said, dropping the *-gpios was the
solution for the Qualcomm DTS as well:

bd37ce2eeb84 ("arm64: dts: qcom: qrb5165-rb5: add the Wifi node")

> >> };
> >>
> >> or:
> >>
> >> bt {
> >> 	pmu = <&pmu>;
> 
> Ah, and I forgot here: this also might not be correct, because if you
> have PMU, then the PMU consumes VCC, not the Bluetooth. Therefore both
> of above two solutions might be inaccurate description if you decide to
> go with PMU.
> 
> >> };
> >>
> >> Of course the driver need to support all 2/3 cases due to backward
> >> compatibility but from DT pov I don't see any breakage since we already
> >> need to define the power handling properties (gpio & supply) as
> >> optional.
> > 
> > Either existing binding is complete or not. Not half-done.

As I remember DT ABI must be backward compatible. I understand this as
followed: In our current use-case the dt-bindings don't describe any
required hw resource so we need to mark them as optional to be backward
compatible.

Regarding your above comment: "complete or not. Not half-done". Do you
see the current dt-bindings for this particular device as complete or
not? In other words can we mark the reset-gpios and vcc-supply
properties as required albeit this would break the DT ABI since all
current users don't specify it?

> >> That beeing said I don't see the need for a PMU driver for this WLAN/BT
> >> combi chip which is way simpler than the Qualcomm one from Bartosz. Also
> >> there is physically no PMU device which powers the chip unlike the
> >> Qualcomm one. I'm not sure if you would accept virtual PMU devices.
> > 
> > Virtual PMU, of course not. I would like to have complete hardware
> > description, not something which matches your current driver model.

Okay so PMU is no option and we don't have to consider this idea any
longer. So reset-gpios and vcc-supply it is :) and I don't expect this
to change.

Regards,
  Marco

Powered by blists - more mailing lists