| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <71fd47e7a83e2c56c96e2638004a7812@codeaurora.org>
Date: Thu, 12 Aug 2021 12:43:11 +0530
From: rajpat@...eaurora.org
To: Matthias Kaehlcke <mka@...omium.org>
Cc: Andy Gross <agross@...nel.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, rnayak@...eaurora.org,
saiprakash.ranjan@...eaurora.org, msavaliy@....qualcomm.com,
skakit@...eaurora.org
Subject: Re: [PATCH V4 2/4] arm64: dts: sc7280: Add QUPv3 wrapper_0 nodes
On 2021-08-11 20:22, Matthias Kaehlcke wrote:
> On Wed, Aug 11, 2021 at 05:43:48PM +0530, rajpat@...eaurora.org wrote:
>> On 2021-07-26 21:32, Matthias Kaehlcke wrote:
>> > On Mon, Jul 26, 2021 at 07:10:45PM +0530, Rajesh Patil wrote:
>> > > From: Roja Rani Yarubandi <rojay@...eaurora.org>
>> > >
>> > > Add QUPv3 wrapper_0 DT nodes for SC7280 SoC.
>> > >
>> > > Signed-off-by: Roja Rani Yarubandi <rojay@...eaurora.org>
>> > > Signed-off-by: Rajesh Patil <rajpat@...eaurora.org>
>> > > ---
>> > > Changes in V4:
>> > > - As per Bjorn's comment, added QUP Wrapper_0 nodes
>> > > other than debug-uart node
>> > > - Dropped interconnect votes for wrapper_0 node
>> > >
>> > > Changes in V3:
>> > > - Broken the huge V2 patch into 3 smaller patches.
>> > > 1. QSPI DT nodes
>> > > 2. QUP wrapper_0 DT nodes
>> > > 3. QUP wrapper_1 DT nodes
>> > >
>> > > Changes in V2:
>> > > - As per Doug's comments removed pinmux/pinconf subnodes.
>> > > - As per Doug's comments split of SPI, UART nodes has been done.
>> > > - Moved QSPI node before aps_smmu as per the order.
>> > >
>> > > arch/arm64/boot/dts/qcom/sc7280-idp.dts | 84 ++++
>> > > arch/arm64/boot/dts/qcom/sc7280.dtsi | 720
>> > > ++++++++++++++++++++++++++++++++
>> > > 2 files changed, 804 insertions(+)
>> > >
>> > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> > > b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> > > index b0bfd8e..f63cf51 100644
>> > > --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> > > +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> > > @@ -358,6 +358,16 @@
>> > > vdda18-supply = <&vreg_l1c_1p8>;
>> > > };
>> > >
>> > > +&uart7 {
>> > > + status = "okay";
>> > > +
>> > > + /delete-property/interrupts;
>> > > + interrupts-extended = <&intc GIC_SPI 608 IRQ_TYPE_LEVEL_HIGH>,
>> > > + <&tlmm 31 IRQ_TYPE_EDGE_FALLING>;
>> > > + pinctrl-names = "default", "sleep";
>> > > + pinctrl-1 = <&qup_uart7_sleep_cts>, <&qup_uart7_sleep_rts>,
>> > > <&qup_uart7_sleep_tx>, <&qup_uart7_sleep_rx>;
>> > > +};
>> > > +
>> > > /* PINCTRL - additions to nodes defined in sc7280.dtsi */
>> > >
>> > > &qspi_cs0 {
>> > > @@ -428,3 +438,77 @@
>> > > bias-pull-up;
>> > > };
>> > > };
>> > > +&qup_uart7_cts {
>> > > + /*
>> > > + * Configure a pull-down on CTS to match the pull of
>> > > + * the Bluetooth module.
>> > > + */
>> > > + bias-pull-down;
>> > > +};
>> > > +
>> > > +&qup_uart7_rts {
>> > > + /* We'll drive RTS, so no pull */
>> > > + drive-strength = <2>;
>> > > + bias-disable;
>> > > +};
>> > > +
>> > > +&qup_uart7_tx {
>> > > + /* We'll drive TX, so no pull */
>> > > + drive-strength = <2>;
>> > > + bias-disable;
>> > > +};
>> > > +
>> > > +&qup_uart7_rx {
>> > > + /*
>> > > + * Configure a pull-up on RX. This is needed to avoid
>> > > + * garbage data when the TX pin of the Bluetooth module is
>> > > + * in tri-state (module powered off or not driving the
>> > > + * signal yet).
>> > > + */
>> > > + bias-pull-up;
>> > > +};
>> > > +
>> > > +&tlmm {
>> > > + qup_uart7_sleep_cts: qup-uart7-sleep-cts {
>> > > + pins = "gpio28";
>> > > + function = "gpio";
>> > > + /*
>> > > + * Configure a pull-down on CTS to match the pull of
>> > > + * the Bluetooth module.
>> > > + */
>> > > + bias-pull-down;
>> > > + };
>> > > +
>> > > + qup_uart7_sleep_rts: qup-uart7-sleep-rts {
>> > > + pins = "gpio29";
>> > > + function = "gpio";
>> > > + /*
>> > > + * Configure pull-down on RTS. As RTS is active low
>> > > + * signal, pull it low to indicate the BT SoC that it
>> > > + * can wakeup the system anytime from suspend state by
>> > > + * pulling RX low (by sending wakeup bytes).
>> > > + */
>> > > + bias-pull-down;
>> > > + };
>> > > +
>> > > + qup_uart7_sleep_tx: qup-uart7-sleep-tx {
>> > > + pins = "gpio30";
>> > > + function = "gpio";
>> > > + /*
>> > > + * Configure pull-up on TX when it isn't actively driven
>> > > + * to prevent BT SoC from receiving garbage during sleep.
>> > > + */
>> > > + bias-pull-up;
>> > > + };
>> > >
>> > > + qup_uart7_sleep_rx: qup-uart7-sleep-rx {
>> > > + pins = "gpio31";
>> > > + function = "gpio";
>> > > + /*
>> > > + * Configure a pull-up on RX. This is needed to avoid
>> > > + * garbage data when the TX pin of the Bluetooth module
>> > > + * is floating which may cause spurious wakeups.
>> > > + */
>> > > + bias-pull-up;
>> > > + };
>> > > +};
>> >
>> > How the patches of this series are split strikes me as a bit odd.
>> > Supposedly
>> > this patch adds the QUPv3 wrapper_0 DT nodes for the SC7280, however the
>> > above is the pin configuration for the Bluetooth UART of the SC7280 IDP
>> > board.
>> > I don't see a good reason why that should be part of this patch. It
>> > should be
>> > a separate change whose subject indicates that it configures the
>> > Bluetooth UART
>> > of the SC7280 IDP.
>> >
>>
>> Okay will split this up.
>>
>> > Without this conflation of SoC and board DT it would seem perfectly
>> > reasonable
>> > to squash this patch and '[4/4] arm64: dts: sc7280: Add QUPv3 wrapper_1
>> > nodes'
>> > into a single one, they are essentially doing the same thing, I see no
>> > need to
>> > have different patches for the wrapper 0 and 1 nodes.
>>
>> Previously when QUP wrapper 0 and wrapper 1 nodes were added in single
>> patch, we faced some git issues as the patch was huge. Hence we split
>> it up.
>> https://partnerissuetracker.corp.google.com/issues/177045897#comment12
>
> That bug tracker entry isn't public, this is what the comment says:
>
>> I suspect that diff is just having a hard time since your patch
>> touches so
>> much stuff. Presumably you could make it happier if you broke your
>> patch
>> into smaller changes. For instance:
>>
>> One patch that adds the quad SPI. After all the quad SPI isn't really
>> related to the other QUP stuff. One patch that fixes up the existing
>> "uart5" and the QUP it's on to be how it's supposed to be. One patch
>> that adds all the new stuff.
>
> If I understand correctly the problem wasn't that the QUP wrappers are
> added in a single patch (which should be pretty straightforward to
> review), but that the previous patch(es) do too many things at once.
> Adding both QUP wrappers is conceptionally a single thing, the problem
> is intermingling that with adding board specific Bluetooth nodes, board
> flash nodes, pinconf for UART, etc
Even after splitting the patches as suggested(i.e., additions and
modifications separately), if I add qup wrapper0 and wrapper1 nodes in
single patch the git diff is getting messed up. pasted the diff for
reference
+ qup_spi14_cs_gpio: qup-spi14-cs_gpio {
+ pins = "gpio59";
+ function = "gpio";
+ };
- assigned-clocks = <&gcc
GCC_USB30_PRIM_MOCK_UTMI_CLK>,
- <&gcc
GCC_USB30_PRIM_MASTER_CLK>;
- assigned-clock-rates = <19200000>, <200000000>;
+ qup_spi15_data_clk: qup-spi15-data-clk {
+ pins = "gpio60", "gpio61", "gpio62";
+ function = "qup17";
+ };
- interrupts-extended = <&intc GIC_SPI 131
IRQ_TYPE_LEVEL_HIGH>,
- <&pdc 14
IRQ_TYPE_EDGE_BOTH>,
- <&pdc 15
IRQ_TYPE_EDGE_BOTH>,
- <&pdc 17
IRQ_TYPE_LEVEL_HIGH>;
- interrupt-names = "hs_phy_irq", "dp_hs_phy_irq",
- "dm_hs_phy_irq", "ss_phy_irq";
+ qup_spi15_cs: qup-spi15-cs {
+ pins = "gpio63";
+ function = "qup17";
+ };
- power-domains = <&gcc GCC_USB30_PRIM_GDSC>;
+ qup_spi15_cs_gpio: qup-spi15-cs_gpio {
+ pins = "gpio63";
+ function = "gpio";
+ };
Powered by blists - more mailing lists