| Message ID | 20240206114745.1388491-3-quic_kriskura@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add DT support for Multiport controller on SC8280XP | expand |
On Tue, 6 Feb 2024 at 14:03, Krishna Kurapati <quic_kriskura@quicinc.com> wrote: > > Enable tertiary controller for SA8295P (based on SC8280XP). > Add pinctrl support for usb ports to provide VBUS to connected peripherals. These are not just pinctrl entries. They hide VBUS regulators. Please implement them properly as corresponding vbus regulators. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 49 ++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts > index fd253942e5e5..6da444042f82 100644 > --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts > +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts > @@ -9,6 +9,7 @@ > #include <dt-bindings/gpio/gpio.h> > #include <dt-bindings/regulator/qcom,rpmh-regulator.h> > #include <dt-bindings/spmi/spmi.h> > +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h> > > #include "sa8540p.dtsi" > #include "sa8540p-pmics.dtsi" > @@ -584,6 +585,16 @@ &usb_1_qmpphy { > status = "okay"; > }; > > +&usb_2 { > + pinctrl-0 = <&usb2_en>, > + <&usb3_en>, > + <&usb4_en>, > + <&usb5_en>; > + pinctrl-names = "default"; > + > + status = "okay"; > +}; > + > &usb_2_hsphy0 { > vdda-pll-supply = <&vreg_l5a>; > vdda18-supply = <&vreg_l7g>; > @@ -636,6 +647,44 @@ &xo_board_clk { > > /* PINCTRL */ > > +&pmm8540c_gpios { > + usb2_en: usb2-en-state { > + pins = "gpio9"; > + function = "normal"; > + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>; > + output-high; > + power-source = <0>; > + }; > +}; > + > +&pmm8540e_gpios { > + usb3_en: usb3-en-state { > + pins = "gpio5"; > + function = "normal"; > + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>; > + output-high; > + power-source = <0>; > + }; > +}; > + > +&pmm8540g_gpios { > + usb4_en: usb4-en-state { > + pins = "gpio5"; > + function = "normal"; > + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>; > + output-high; > + power-source = <0>; > + }; > + > + usb5_en: usb5-en-state { > + pins = "gpio9"; > + function = "normal"; > + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>; > + output-high; > + power-source = <0>; > + }; > +}; > + > &tlmm { > pcie2a_default: pcie2a-default-state { > clkreq-n-pins { > -- > 2.34.1 > >
On 2/6/2024 5:43 PM, Dmitry Baryshkov wrote: > On Tue, 6 Feb 2024 at 14:03, Krishna Kurapati <quic_kriskura@quicinc.com> wrote: >> >> Enable tertiary controller for SA8295P (based on SC8280XP). >> Add pinctrl support for usb ports to provide VBUS to connected peripherals. > > These are not just pinctrl entries. They hide VBUS regulators. Please > implement them properly as corresponding vbus regulators. > Hi Dmitry. Apologies, can you elaborate on your comment. I thought this implementation was fine as Konrad reviewed it in v13 [1]. I removed his RB tag as I made one change of dropping "_state" in labels. [1]: https://lore.kernel.org/all/7141c2dd-9dcd-4186-ba83-829fe925e464@linaro.org/ Regards, Krishna, >> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 49 ++++++++++++++++++++++++ >> 1 file changed, 49 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts >> index fd253942e5e5..6da444042f82 100644 >> --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts >> +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts >> @@ -9,6 +9,7 @@ >> #include <dt-bindings/gpio/gpio.h> >> #include <dt-bindings/regulator/qcom,rpmh-regulator.h> >> #include <dt-bindings/spmi/spmi.h> >> +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h> >> >> #include "sa8540p.dtsi" >> #include "sa8540p-pmics.dtsi" >> @@ -584,6 +585,16 @@ &usb_1_qmpphy { >> status = "okay"; >> }; >> >> +&usb_2 { >> + pinctrl-0 = <&usb2_en>, >> + <&usb3_en>, >> + <&usb4_en>, >> + <&usb5_en>; >> + pinctrl-names = "default"; >> + >> + status = "okay"; >> +}; >> + >> &usb_2_hsphy0 { >> vdda-pll-supply = <&vreg_l5a>; >> vdda18-supply = <&vreg_l7g>; >> @@ -636,6 +647,44 @@ &xo_board_clk { >> >> /* PINCTRL */ >> >> +&pmm8540c_gpios { >> + usb2_en: usb2-en-state { >> + pins = "gpio9"; >> + function = "normal"; >> + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>; >> + output-high; >> + power-source = <0>; >> + }; >> +}; >> + >> +&pmm8540e_gpios { >> + usb3_en: usb3-en-state { >> + pins = "gpio5"; >> + function = "normal"; >> + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>; >> + output-high; >> + power-source = <0>; >> + }; >> +}; >> + >> +&pmm8540g_gpios { >> + usb4_en: usb4-en-state { >> + pins = "gpio5"; >> + function = "normal"; >> + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>; >> + output-high; >> + power-source = <0>; >> + }; >> + >> + usb5_en: usb5-en-state { >> + pins = "gpio9"; >> + function = "normal"; >> + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>; >> + output-high; >> + power-source = <0>; >> + }; >> +}; >> + >> &tlmm { >> pcie2a_default: pcie2a-default-state { >> clkreq-n-pins { >> -- >> 2.34.1 >> >> > >
On Tue, 6 Feb 2024 at 14:28, Krishna Kurapati PSSNV <quic_kriskura@quicinc.com> wrote: > > > > On 2/6/2024 5:43 PM, Dmitry Baryshkov wrote: > > On Tue, 6 Feb 2024 at 14:03, Krishna Kurapati <quic_kriskura@quicinc.com> wrote: > >> > >> Enable tertiary controller for SA8295P (based on SC8280XP). > >> Add pinctrl support for usb ports to provide VBUS to connected peripherals. > > > > These are not just pinctrl entries. They hide VBUS regulators. Please > > implement them properly as corresponding vbus regulators. > > > > Hi Dmitry. Apologies, can you elaborate on your comment. I thought this > implementation was fine as Konrad reviewed it in v13 [1]. I removed his > RB tag as I made one change of dropping "_state" in labels. My comment is pretty simple: if I'm not mistaken, your DT doesn't reflect your hardware design. You have actual VBUS regulators driven by these GPIO pins. Is this correct? If so, you should describe them properly in the device tree rather than describing them just as USB host's pinctrl state. > > [1]: > https://lore.kernel.org/all/7141c2dd-9dcd-4186-ba83-829fe925e464@linaro.org/ > > Regards, > Krishna, > > >> > >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > >> --- > >> arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 49 ++++++++++++++++++++++++ > >> 1 file changed, 49 insertions(+) > >> > >> diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts > >> index fd253942e5e5..6da444042f82 100644 > >> --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts > >> +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts > >> @@ -9,6 +9,7 @@ > >> #include <dt-bindings/gpio/gpio.h> > >> #include <dt-bindings/regulator/qcom,rpmh-regulator.h> > >> #include <dt-bindings/spmi/spmi.h> > >> +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h> > >> > >> #include "sa8540p.dtsi" > >> #include "sa8540p-pmics.dtsi" > >> @@ -584,6 +585,16 @@ &usb_1_qmpphy { > >> status = "okay"; > >> }; > >> > >> +&usb_2 { > >> + pinctrl-0 = <&usb2_en>, > >> + <&usb3_en>, > >> + <&usb4_en>, > >> + <&usb5_en>; > >> + pinctrl-names = "default"; > >> + > >> + status = "okay"; > >> +}; > >> + > >> &usb_2_hsphy0 { > >> vdda-pll-supply = <&vreg_l5a>; > >> vdda18-supply = <&vreg_l7g>; > >> @@ -636,6 +647,44 @@ &xo_board_clk { > >> > >> /* PINCTRL */ > >> > >> +&pmm8540c_gpios { > >> + usb2_en: usb2-en-state { > >> + pins = "gpio9"; > >> + function = "normal"; > >> + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>; > >> + output-high; > >> + power-source = <0>; > >> + }; > >> +}; > >> + > >> +&pmm8540e_gpios { > >> + usb3_en: usb3-en-state { > >> + pins = "gpio5"; > >> + function = "normal"; > >> + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>; > >> + output-high; > >> + power-source = <0>; > >> + }; > >> +}; > >> + > >> +&pmm8540g_gpios { > >> + usb4_en: usb4-en-state { > >> + pins = "gpio5"; > >> + function = "normal"; > >> + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>; > >> + output-high; > >> + power-source = <0>; > >> + }; > >> + > >> + usb5_en: usb5-en-state { > >> + pins = "gpio9"; > >> + function = "normal"; > >> + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>; > >> + output-high; > >> + power-source = <0>; > >> + }; > >> +}; > >> + > >> &tlmm { > >> pcie2a_default: pcie2a-default-state { > >> clkreq-n-pins { > >> -- > >> 2.34.1 > >> > >> > > > >
On 2/6/2024 6:54 PM, Dmitry Baryshkov wrote: > On Tue, 6 Feb 2024 at 14:28, Krishna Kurapati PSSNV > <quic_kriskura@quicinc.com> wrote: >> >> >> >> On 2/6/2024 5:43 PM, Dmitry Baryshkov wrote: >>> On Tue, 6 Feb 2024 at 14:03, Krishna Kurapati <quic_kriskura@quicinc.com> wrote: >>>> >>>> Enable tertiary controller for SA8295P (based on SC8280XP). >>>> Add pinctrl support for usb ports to provide VBUS to connected peripherals. >>> >>> These are not just pinctrl entries. They hide VBUS regulators. Please >>> implement them properly as corresponding vbus regulators. >>> >> >> Hi Dmitry. Apologies, can you elaborate on your comment. I thought this >> implementation was fine as Konrad reviewed it in v13 [1]. I removed his >> RB tag as I made one change of dropping "_state" in labels. > > My comment is pretty simple: if I'm not mistaken, your DT doesn't > reflect your hardware design. > You have actual VBUS regulators driven by these GPIO pins. Is this correct? > If so, you should describe them properly in the device tree rather > than describing them just as USB host's pinctrl state. > Hi Dmitry, I have very little idea about the gpio controller regulators. I will go through it and see how I can implement it. I just found this : https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/gpio-regulator.txt One query. If we model it as a regulator, do we need to add it as a supply and call regulator_enable in dwc3_qcom probe again ? Regards, Krishna,
On Thu, 8 Feb 2024 at 04:40, Krishna Kurapati PSSNV <quic_kriskura@quicinc.com> wrote: > On 2/6/2024 6:54 PM, Dmitry Baryshkov wrote: > > On Tue, 6 Feb 2024 at 14:28, Krishna Kurapati PSSNV > > <quic_kriskura@quicinc.com> wrote: > >> > >> > >> > >> On 2/6/2024 5:43 PM, Dmitry Baryshkov wrote: > >>> On Tue, 6 Feb 2024 at 14:03, Krishna Kurapati <quic_kriskura@quicinc.com> wrote: > >>>> > >>>> Enable tertiary controller for SA8295P (based on SC8280XP). > >>>> Add pinctrl support for usb ports to provide VBUS to connected peripherals. > >>> > >>> These are not just pinctrl entries. They hide VBUS regulators. Please > >>> implement them properly as corresponding vbus regulators. > >>> > >> > >> Hi Dmitry. Apologies, can you elaborate on your comment. I thought this > >> implementation was fine as Konrad reviewed it in v13 [1]. I removed his > >> RB tag as I made one change of dropping "_state" in labels. > > > > My comment is pretty simple: if I'm not mistaken, your DT doesn't > > reflect your hardware design. > > You have actual VBUS regulators driven by these GPIO pins. Is this correct? > > If so, you should describe them properly in the device tree rather > > than describing them just as USB host's pinctrl state. > > > > Hi Dmitry, > > I have very little idea about the gpio controller regulators. I will > go through it and see how I can implement it. I just found this : > https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/gpio-regulator.txt Much simpler, it can be found at Documentation/devicetree/bindings/regulator/fixed-regulator.yaml > One query. If we model it as a regulator, do we need to add it as a > supply and call regulator_enable in dwc3_qcom probe again ? Not in probe(), but yes. It needs to be enabled when the VBUS has to be powered up, when the device is initialised or switched to the host mode, and disabled when the VBUS has to be powered down, if the device is being switched to the device mode.
On 2/8/2024 10:11 AM, Dmitry Baryshkov wrote: > On Thu, 8 Feb 2024 at 04:40, Krishna Kurapati PSSNV > <quic_kriskura@quicinc.com> wrote: >> On 2/6/2024 6:54 PM, Dmitry Baryshkov wrote: >>> On Tue, 6 Feb 2024 at 14:28, Krishna Kurapati PSSNV >>> <quic_kriskura@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 2/6/2024 5:43 PM, Dmitry Baryshkov wrote: >>>>> On Tue, 6 Feb 2024 at 14:03, Krishna Kurapati <quic_kriskura@quicinc.com> wrote: >>>>>> >>>>>> Enable tertiary controller for SA8295P (based on SC8280XP). >>>>>> Add pinctrl support for usb ports to provide VBUS to connected peripherals. >>>>> >>>>> These are not just pinctrl entries. They hide VBUS regulators. Please >>>>> implement them properly as corresponding vbus regulators. >>>>> >>>> >>>> Hi Dmitry. Apologies, can you elaborate on your comment. I thought this >>>> implementation was fine as Konrad reviewed it in v13 [1]. I removed his >>>> RB tag as I made one change of dropping "_state" in labels. >>> >>> My comment is pretty simple: if I'm not mistaken, your DT doesn't >>> reflect your hardware design. >>> You have actual VBUS regulators driven by these GPIO pins. Is this correct? >>> If so, you should describe them properly in the device tree rather >>> than describing them just as USB host's pinctrl state. >>> >> >> Hi Dmitry, >> >> I have very little idea about the gpio controller regulators. I will >> go through it and see how I can implement it. I just found this : >> https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/gpio-regulator.txt > > Much simpler, it can be found at > Documentation/devicetree/bindings/regulator/fixed-regulator.yaml Thanks for the reference. > >> One query. If we model it as a regulator, do we need to add it as a >> supply and call regulator_enable in dwc3_qcom probe again ? > > Not in probe(), but yes. It needs to be enabled when the VBUS has to > be powered up, when the device is initialised or switched to the host > mode, and disabled when the VBUS has to be powered down, if the device > is being switched to the device mode. > Actually since we never go to device mode, can't we just stick to this pinctrl approach and skip turning on regulator in driver ? Regards, Krishna,
On Thu, 8 Feb 2024 at 06:48, Krishna Kurapati PSSNV <quic_kriskura@quicinc.com> wrote: > > > > On 2/8/2024 10:11 AM, Dmitry Baryshkov wrote: > > On Thu, 8 Feb 2024 at 04:40, Krishna Kurapati PSSNV > > <quic_kriskura@quicinc.com> wrote: > >> On 2/6/2024 6:54 PM, Dmitry Baryshkov wrote: > >>> On Tue, 6 Feb 2024 at 14:28, Krishna Kurapati PSSNV > >>> <quic_kriskura@quicinc.com> wrote: > >>>> > >>>> > >>>> > >>>> On 2/6/2024 5:43 PM, Dmitry Baryshkov wrote: > >>>>> On Tue, 6 Feb 2024 at 14:03, Krishna Kurapati <quic_kriskura@quicinc.com> wrote: > >>>>>> > >>>>>> Enable tertiary controller for SA8295P (based on SC8280XP). > >>>>>> Add pinctrl support for usb ports to provide VBUS to connected peripherals. > >>>>> > >>>>> These are not just pinctrl entries. They hide VBUS regulators. Please > >>>>> implement them properly as corresponding vbus regulators. > >>>>> > >>>> > >>>> Hi Dmitry. Apologies, can you elaborate on your comment. I thought this > >>>> implementation was fine as Konrad reviewed it in v13 [1]. I removed his > >>>> RB tag as I made one change of dropping "_state" in labels. > >>> > >>> My comment is pretty simple: if I'm not mistaken, your DT doesn't > >>> reflect your hardware design. > >>> You have actual VBUS regulators driven by these GPIO pins. Is this correct? > >>> If so, you should describe them properly in the device tree rather > >>> than describing them just as USB host's pinctrl state. > >>> > >> > >> Hi Dmitry, > >> > >> I have very little idea about the gpio controller regulators. I will > >> go through it and see how I can implement it. I just found this : > >> https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/gpio-regulator.txt > > > > Much simpler, it can be found at > > Documentation/devicetree/bindings/regulator/fixed-regulator.yaml > > Thanks for the reference. > > > > >> One query. If we model it as a regulator, do we need to add it as a > >> supply and call regulator_enable in dwc3_qcom probe again ? > > > > Not in probe(), but yes. It needs to be enabled when the VBUS has to > > be powered up, when the device is initialised or switched to the host > > mode, and disabled when the VBUS has to be powered down, if the device > > is being switched to the device mode. > > > > Actually since we never go to device mode, can't we just stick to this > pinctrl approach and skip turning on regulator in driver ? Scroll several emails back. DT should describe the hardware. Hardware has VBUS regulators.
On 2/8/2024 10:37 AM, Dmitry Baryshkov wrote: > On Thu, 8 Feb 2024 at 06:48, Krishna Kurapati PSSNV > <quic_kriskura@quicinc.com> wrote: >> >> >> >> On 2/8/2024 10:11 AM, Dmitry Baryshkov wrote: >>> On Thu, 8 Feb 2024 at 04:40, Krishna Kurapati PSSNV >>> <quic_kriskura@quicinc.com> wrote: >>>> On 2/6/2024 6:54 PM, Dmitry Baryshkov wrote: >>>>> On Tue, 6 Feb 2024 at 14:28, Krishna Kurapati PSSNV >>>>> <quic_kriskura@quicinc.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 2/6/2024 5:43 PM, Dmitry Baryshkov wrote: >>>>>>> On Tue, 6 Feb 2024 at 14:03, Krishna Kurapati <quic_kriskura@quicinc.com> wrote: >>>>>>>> >>>>>>>> Enable tertiary controller for SA8295P (based on SC8280XP). >>>>>>>> Add pinctrl support for usb ports to provide VBUS to connected peripherals. >>>>>>> >>>>>>> These are not just pinctrl entries. They hide VBUS regulators. Please >>>>>>> implement them properly as corresponding vbus regulators. >>>>>>> >>>>>> >>>>>> Hi Dmitry. Apologies, can you elaborate on your comment. I thought this >>>>>> implementation was fine as Konrad reviewed it in v13 [1]. I removed his >>>>>> RB tag as I made one change of dropping "_state" in labels. >>>>> >>>>> My comment is pretty simple: if I'm not mistaken, your DT doesn't >>>>> reflect your hardware design. >>>>> You have actual VBUS regulators driven by these GPIO pins. Is this correct? >>>>> If so, you should describe them properly in the device tree rather >>>>> than describing them just as USB host's pinctrl state. >>>>> >>>> >>>> Hi Dmitry, >>>> >>>> I have very little idea about the gpio controller regulators. I will >>>> go through it and see how I can implement it. I just found this : >>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/gpio-regulator.txt >>> >>> Much simpler, it can be found at >>> Documentation/devicetree/bindings/regulator/fixed-regulator.yaml >> >> Thanks for the reference. >> >>> >>>> One query. If we model it as a regulator, do we need to add it as a >>>> supply and call regulator_enable in dwc3_qcom probe again ? >>> >>> Not in probe(), but yes. It needs to be enabled when the VBUS has to >>> be powered up, when the device is initialised or switched to the host >>> mode, and disabled when the VBUS has to be powered down, if the device >>> is being switched to the device mode. >>> >> >> Actually since we never go to device mode, can't we just stick to this >> pinctrl approach and skip turning on regulator in driver ? > > Scroll several emails back. DT should describe the hardware. Hardware > has VBUS regulators. > Hi Dmitry, I dug up the schematic and I see that the gpio's we are using in this patch are actually "Enable Pins" to an external chip that provides vbus to the peripherals connected. So from the perspective of SoC, it is a GPIO and not to be represented as a regulator I believe. Regards, Krishna,
On 08/02/2024 10:07, Krishna Kurapati PSSNV wrote: >>>> >>>>> One query. If we model it as a regulator, do we need to add it as a >>>>> supply and call regulator_enable in dwc3_qcom probe again ? >>>> >>>> Not in probe(), but yes. It needs to be enabled when the VBUS has to >>>> be powered up, when the device is initialised or switched to the host >>>> mode, and disabled when the VBUS has to be powered down, if the device >>>> is being switched to the device mode. >>>> >>> >>> Actually since we never go to device mode, can't we just stick to this >>> pinctrl approach and skip turning on regulator in driver ? >> >> Scroll several emails back. DT should describe the hardware. Hardware >> has VBUS regulators. >> > > Hi Dmitry, > > I dug up the schematic and I see that the gpio's we are using in this > patch are actually "Enable Pins" to an external chip that provides vbus > to the peripherals connected. So from the perspective of SoC, it is a > GPIO and not to be represented as a regulator I believe. According to such logic nothing is a regulator... sorry, you just described regulator. Best regards, Krzysztof
On Tue, Feb 06, 2024 at 03:24:45PM +0200, Dmitry Baryshkov wrote: > On Tue, 6 Feb 2024 at 14:28, Krishna Kurapati PSSNV > <quic_kriskura@quicinc.com> wrote: > > > > > > > > On 2/6/2024 5:43 PM, Dmitry Baryshkov wrote: > > > On Tue, 6 Feb 2024 at 14:03, Krishna Kurapati <quic_kriskura@quicinc.com> wrote: > > >> > > >> Enable tertiary controller for SA8295P (based on SC8280XP). > > >> Add pinctrl support for usb ports to provide VBUS to connected peripherals. > > > > > > These are not just pinctrl entries. They hide VBUS regulators. Please > > > implement them properly as corresponding vbus regulators. > > > > > > > Hi Dmitry. Apologies, can you elaborate on your comment. I thought this > > implementation was fine as Konrad reviewed it in v13 [1]. I removed his > > RB tag as I made one change of dropping "_state" in labels. > > My comment is pretty simple: if I'm not mistaken, your DT doesn't > reflect your hardware design. > You have actual VBUS regulators driven by these GPIO pins. Is this correct? > If so, you should describe them properly in the device tree rather > than describing them just as USB host's pinctrl state. > This is correct, these gpios are wired to the enable-pin of a set of regulators providing the VBUS voltage. Please, Krishna, represent them as always-on fixed-regulators instead. Regards, Bjorn
On 2/9/2024 8:11 AM, Bjorn Andersson wrote: > On Tue, Feb 06, 2024 at 03:24:45PM +0200, Dmitry Baryshkov wrote: >> On Tue, 6 Feb 2024 at 14:28, Krishna Kurapati PSSNV >> <quic_kriskura@quicinc.com> wrote: >>> >>> >>> >>> On 2/6/2024 5:43 PM, Dmitry Baryshkov wrote: >>>> On Tue, 6 Feb 2024 at 14:03, Krishna Kurapati <quic_kriskura@quicinc.com> wrote: >>>>> >>>>> Enable tertiary controller for SA8295P (based on SC8280XP). >>>>> Add pinctrl support for usb ports to provide VBUS to connected peripherals. >>>> >>>> These are not just pinctrl entries. They hide VBUS regulators. Please >>>> implement them properly as corresponding vbus regulators. >>>> >>> >>> Hi Dmitry. Apologies, can you elaborate on your comment. I thought this >>> implementation was fine as Konrad reviewed it in v13 [1]. I removed his >>> RB tag as I made one change of dropping "_state" in labels. >> >> My comment is pretty simple: if I'm not mistaken, your DT doesn't >> reflect your hardware design. >> You have actual VBUS regulators driven by these GPIO pins. Is this correct? >> If so, you should describe them properly in the device tree rather >> than describing them just as USB host's pinctrl state. >> > > This is correct, these gpios are wired to the enable-pin of a set of > regulators providing the VBUS voltage. Please, Krishna, represent them > as always-on fixed-regulators instead. > Hi Dmitry, Krzysztof, Bjorn, Thanks for the review on this patch. Will convert the pinctrl DT's to regulator entries. Regards, Krishna,
diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts index fd253942e5e5..6da444042f82 100644 --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts @@ -9,6 +9,7 @@ #include <dt-bindings/gpio/gpio.h> #include <dt-bindings/regulator/qcom,rpmh-regulator.h> #include <dt-bindings/spmi/spmi.h> +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h> #include "sa8540p.dtsi" #include "sa8540p-pmics.dtsi" @@ -584,6 +585,16 @@ &usb_1_qmpphy { status = "okay"; }; +&usb_2 { + pinctrl-0 = <&usb2_en>, + <&usb3_en>, + <&usb4_en>, + <&usb5_en>; + pinctrl-names = "default"; + + status = "okay"; +}; + &usb_2_hsphy0 { vdda-pll-supply = <&vreg_l5a>; vdda18-supply = <&vreg_l7g>; @@ -636,6 +647,44 @@ &xo_board_clk { /* PINCTRL */ +&pmm8540c_gpios { + usb2_en: usb2-en-state { + pins = "gpio9"; + function = "normal"; + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>; + output-high; + power-source = <0>; + }; +}; + +&pmm8540e_gpios { + usb3_en: usb3-en-state { + pins = "gpio5"; + function = "normal"; + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>; + output-high; + power-source = <0>; + }; +}; + +&pmm8540g_gpios { + usb4_en: usb4-en-state { + pins = "gpio5"; + function = "normal"; + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>; + output-high; + power-source = <0>; + }; + + usb5_en: usb5-en-state { + pins = "gpio9"; + function = "normal"; + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>; + output-high; + power-source = <0>; + }; +}; + &tlmm { pcie2a_default: pcie2a-default-state { clkreq-n-pins {
Enable tertiary controller for SA8295P (based on SC8280XP). Add pinctrl support for usb ports to provide VBUS to connected peripherals. Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> --- arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 49 ++++++++++++++++++++++++ 1 file changed, 49 insertions(+)