| Message ID | 20220721033918.v3.1.I10519ca1bf88233702a90e296088808d18cdc7b1@changeid |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/2] arm64: dts: qcom: Add SKU6 for sc7180-trogdor-pazquel-lte-parade | expand |
On 21/07/2022 05:58, Yunlong Jia wrote: > The difference between sku6 and sku4 is that there is no esim > > The different SKUs are: > > LTE with physical SIM _and_ eSIM > LTE with only a physical SIM > WiFi only > Both sku4 and sku6 are LTE SKUs. > One has the eSIM stuffed and one doesn't. > There is a single shared device tree for the two. > > Signed-off-by: Yunlong Jia <yunlong.jia@ecs.corp-partner.google.com> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
Hi, On Wed, Jul 20, 2022 at 8:59 PM Yunlong Jia <yunlong.jia@ecs.corp-partner.google.com> wrote: > > SKU6 is LTE(w/o eSIM)+WIFI+Parade > > Signed-off-by: Yunlong Jia <yunlong.jia@ecs.corp-partner.google.com> > --- > > Changes in v3: > - Bindings and dts in the same series. > > Changes in v2: > - Put sku6 before sku4. > > arch/arm64/boot/dts/qcom/sc7180-trogdor-pazquel-lte-parade.dts | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Douglas Anderson <dianders@chromium.org>
On 21/07/2022 15:37, Doug Anderson wrote: > > Not worth sending a new version for, but normally I expect the > bindings to be patch #1 and the dts change to be patch #2. In any > case: > > Reviewed-by: Douglas Anderson <dianders@chromium.org> I would say worth v4, because otherwise patches is not bisectable. Best regards, Krzysztof
Hi, On Thu, Jul 21, 2022 at 9:33 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 21/07/2022 15:37, Doug Anderson wrote: > > > > Not worth sending a new version for, but normally I expect the > > bindings to be patch #1 and the dts change to be patch #2. In any > > case: > > > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > I would say worth v4, because otherwise patches is not bisectable. You're saying because `dtbs_check` will fail between the two? How does flipping the order help? If `dtbs_check` needs to be bisectable then these two need to be one patch, but I was always under the impression that we wanted bindings patches separate from dts patches. -Doug
On Thu, Jul 21, 2022 at 11:29:13AM -0700, Doug Anderson wrote: > Hi, > > On Thu, Jul 21, 2022 at 9:52 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: > > > > On 21/07/2022 18:43, Doug Anderson wrote: > > > Hi, > > > > > > On Thu, Jul 21, 2022 at 9:33 AM Krzysztof Kozlowski > > > <krzysztof.kozlowski@linaro.org> wrote: > > >> > > >> On 21/07/2022 15:37, Doug Anderson wrote: > > >>> > > >>> Not worth sending a new version for, but normally I expect the > > >>> bindings to be patch #1 and the dts change to be patch #2. In any > > >>> case: > > >>> > > >>> Reviewed-by: Douglas Anderson <dianders@chromium.org> > > >> > > >> I would say worth v4, because otherwise patches is not bisectable. > > > > > > You're saying because `dtbs_check` will fail between the two? > > > > Yes > > OK. Then I assume you agree that reversing the order of the patches > won't help, only combining the two patches into one. > > > > > How does > > > flipping the order help? If `dtbs_check` needs to be bisectable then > > > these two need to be one patch, but I was always under the impression > > > that we wanted bindings patches separate from dts patches. > > > > I don't think anyone said that bindings patches must be separate from > > DTS. The only restriction is DTS cannot go with drivers. > > I have always heard that best practice is to have bindings in a patch > by themselves. If I've misunderstood and/or folks have changed their > minds, that's fine, but historically I've been told to keep them > separate. Correct. > > Bindings for boards go pretty often with DTS (subarch). This is exactly > > what maintainers do, e.g.: > > https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git/log/?h=arm64-for-5.20 > > Bindings for hardware should go via subsystem maintainer (drivers). > > OK, fair that in this case both the bindings and the yaml will land > through the Qualcomm tree. I guess it's really up to Bjorn and whether > he'd prefer "make dtbs_check" to be bisectable or whether he'd prefer > the bindings and dts change to be in separate patches from each other. Bindings go first if applied together because you have to define the binding before you use it. But sometimes things go via multiple trees and that's fine because it's just easier. In that case, the subsystem tree is preferred for bindings (i.e. with the driver). But in this case, Bjorn is the subsystem tree. Rob
Hi, On Thu, Jul 21, 2022 at 5:22 PM Rob Herring <robh@kernel.org> wrote: > > On Thu, Jul 21, 2022 at 11:29:13AM -0700, Doug Anderson wrote: > > Hi, > > > > On Thu, Jul 21, 2022 at 9:52 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > > > > > > On 21/07/2022 18:43, Doug Anderson wrote: > > > > Hi, > > > > > > > > On Thu, Jul 21, 2022 at 9:33 AM Krzysztof Kozlowski > > > > <krzysztof.kozlowski@linaro.org> wrote: > > > >> > > > >> On 21/07/2022 15:37, Doug Anderson wrote: > > > >>> > > > >>> Not worth sending a new version for, but normally I expect the > > > >>> bindings to be patch #1 and the dts change to be patch #2. In any > > > >>> case: > > > >>> > > > >>> Reviewed-by: Douglas Anderson <dianders@chromium.org> > > > >> > > > >> I would say worth v4, because otherwise patches is not bisectable. > > > > > > > > You're saying because `dtbs_check` will fail between the two? > > > > > > Yes > > > > OK. Then I assume you agree that reversing the order of the patches > > won't help, only combining the two patches into one. > > > > > > > > How does > > > > flipping the order help? If `dtbs_check` needs to be bisectable then > > > > these two need to be one patch, but I was always under the impression > > > > that we wanted bindings patches separate from dts patches. > > > > > > I don't think anyone said that bindings patches must be separate from > > > DTS. The only restriction is DTS cannot go with drivers. > > > > I have always heard that best practice is to have bindings in a patch > > by themselves. If I've misunderstood and/or folks have changed their > > minds, that's fine, but historically I've been told to keep them > > separate. > > Correct. > > > > > Bindings for boards go pretty often with DTS (subarch). This is exactly > > > what maintainers do, e.g.: > > > https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git/log/?h=arm64-for-5.20 > > > Bindings for hardware should go via subsystem maintainer (drivers). > > > > OK, fair that in this case both the bindings and the yaml will land > > through the Qualcomm tree. I guess it's really up to Bjorn and whether > > he'd prefer "make dtbs_check" to be bisectable or whether he'd prefer > > the bindings and dts change to be in separate patches from each other. > > Bindings go first if applied together because you have to define the > binding before you use it. But sometimes things go via multiple trees > and that's fine because it's just easier. In that case, the subsystem > tree is preferred for bindings (i.e. with the driver). But in this case, > Bjorn is the subsystem tree. Thanks! I'll interpret your response as: 1. Keep this as two patches and it's more important to keep dts and bindings separate than it is to avoid breaking bisectability of "make dtbs_check". 2. Bindings should have been patch #1, but it's not a massive deal. 3. I'll assume that Bjorn will yell if he'd like this series re-posted with the reverse order. -Doug
On 21/07/2022 20:29, Doug Anderson wrote: > Hi, > > On Thu, Jul 21, 2022 at 9:52 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 21/07/2022 18:43, Doug Anderson wrote: >>> Hi, >>> >>> On Thu, Jul 21, 2022 at 9:33 AM Krzysztof Kozlowski >>> <krzysztof.kozlowski@linaro.org> wrote: >>>> >>>> On 21/07/2022 15:37, Doug Anderson wrote: >>>>> >>>>> Not worth sending a new version for, but normally I expect the >>>>> bindings to be patch #1 and the dts change to be patch #2. In any >>>>> case: >>>>> >>>>> Reviewed-by: Douglas Anderson <dianders@chromium.org> >>>> >>>> I would say worth v4, because otherwise patches is not bisectable. >>> >>> You're saying because `dtbs_check` will fail between the two? >> >> Yes > > OK. Then I assume you agree that reversing the order of the patches > won't help, only combining the two patches into one. > > >>> How does >>> flipping the order help? If `dtbs_check` needs to be bisectable then >>> these two need to be one patch, but I was always under the impression >>> that we wanted bindings patches separate from dts patches. >> >> I don't think anyone said that bindings patches must be separate from >> DTS. The only restriction is DTS cannot go with drivers. > > I have always heard that best practice is to have bindings in a patch > by themselves. Yes, bindings must be separate patch, no one here objects this. You said they cannot go together via one maintainer tree or I misunderstood? > If I've misunderstood and/or folks have changed their > minds, that's fine, but historically I've been told to keep them > separate. Nothing changed. Bindings must be separate. They will be applied by maintainer and, if correctly ordered, this is bisectable. > > >> Bindings for boards go pretty often with DTS (subarch). This is exactly >> what maintainers do, e.g.: >> https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git/log/?h=arm64-for-5.20 >> Bindings for hardware should go via subsystem maintainer (drivers). > > OK, fair that in this case both the bindings and the yaml will land > through the Qualcomm tree. I guess it's really up to Bjorn and whether > he'd prefer "make dtbs_check" to be bisectable or whether he'd prefer > the bindings and dts change to be in separate patches from each other. ??? The patches must be separate commits and if properly ordered in one branch they are bisectable. Best regards, Krzysztof
On 22/07/2022 17:41, Doug Anderson wrote: > Hi, > > On Thu, Jul 21, 2022 at 5:22 PM Rob Herring <robh@kernel.org> wrote: >> >> On Thu, Jul 21, 2022 at 11:29:13AM -0700, Doug Anderson wrote: >>> Hi, >>> >>> On Thu, Jul 21, 2022 at 9:52 AM Krzysztof Kozlowski >>> <krzysztof.kozlowski@linaro.org> wrote: >>>> >>>> On 21/07/2022 18:43, Doug Anderson wrote: >>>>> Hi, >>>>> >>>>> On Thu, Jul 21, 2022 at 9:33 AM Krzysztof Kozlowski >>>>> <krzysztof.kozlowski@linaro.org> wrote: >>>>>> >>>>>> On 21/07/2022 15:37, Doug Anderson wrote: >>>>>>> >>>>>>> Not worth sending a new version for, but normally I expect the >>>>>>> bindings to be patch #1 and the dts change to be patch #2. In any >>>>>>> case: >>>>>>> >>>>>>> Reviewed-by: Douglas Anderson <dianders@chromium.org> >>>>>> >>>>>> I would say worth v4, because otherwise patches is not bisectable. >>>>> >>>>> You're saying because `dtbs_check` will fail between the two? >>>> >>>> Yes >>> >>> OK. Then I assume you agree that reversing the order of the patches >>> won't help, only combining the two patches into one. >>> >>> >>>>> How does >>>>> flipping the order help? If `dtbs_check` needs to be bisectable then >>>>> these two need to be one patch, but I was always under the impression >>>>> that we wanted bindings patches separate from dts patches. >>>> >>>> I don't think anyone said that bindings patches must be separate from >>>> DTS. The only restriction is DTS cannot go with drivers. >>> >>> I have always heard that best practice is to have bindings in a patch >>> by themselves. If I've misunderstood and/or folks have changed their >>> minds, that's fine, but historically I've been told to keep them >>> separate. >> >> Correct. >> >> >>>> Bindings for boards go pretty often with DTS (subarch). This is exactly >>>> what maintainers do, e.g.: >>>> https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git/log/?h=arm64-for-5.20 >>>> Bindings for hardware should go via subsystem maintainer (drivers). >>> >>> OK, fair that in this case both the bindings and the yaml will land >>> through the Qualcomm tree. I guess it's really up to Bjorn and whether >>> he'd prefer "make dtbs_check" to be bisectable or whether he'd prefer >>> the bindings and dts change to be in separate patches from each other. >> >> Bindings go first if applied together because you have to define the >> binding before you use it. But sometimes things go via multiple trees >> and that's fine because it's just easier. In that case, the subsystem >> tree is preferred for bindings (i.e. with the driver). But in this case, >> Bjorn is the subsystem tree. > > Thanks! I'll interpret your response as: > > 1. Keep this as two patches and it's more important to keep dts and > bindings separate than it is to avoid breaking bisectability of "make > dtbs_check". No one questioned this here... > > 2. Bindings should have been patch #1, but it's not a massive deal. This started our discussion and I said it should be a v4 with a proper order. It's not massive deal, but hopefully the submitter will learn something. > > 3. I'll assume that Bjorn will yell if he'd like this series re-posted > with the reverse order. Best regards, Krzysztof
Hi, On Fri, Jul 22, 2022 at 10:14 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 21/07/2022 20:29, Doug Anderson wrote: > > Hi, > > > > On Thu, Jul 21, 2022 at 9:52 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 21/07/2022 18:43, Doug Anderson wrote: > >>> Hi, > >>> > >>> On Thu, Jul 21, 2022 at 9:33 AM Krzysztof Kozlowski > >>> <krzysztof.kozlowski@linaro.org> wrote: > >>>> > >>>> On 21/07/2022 15:37, Doug Anderson wrote: > >>>>> > >>>>> Not worth sending a new version for, but normally I expect the > >>>>> bindings to be patch #1 and the dts change to be patch #2. In any > >>>>> case: > >>>>> > >>>>> Reviewed-by: Douglas Anderson <dianders@chromium.org> > >>>> > >>>> I would say worth v4, because otherwise patches is not bisectable. > >>> > >>> You're saying because `dtbs_check` will fail between the two? > >> > >> Yes > > > > OK. Then I assume you agree that reversing the order of the patches > > won't help, only combining the two patches into one. > > > > > >>> How does > >>> flipping the order help? If `dtbs_check` needs to be bisectable then > >>> these two need to be one patch, but I was always under the impression > >>> that we wanted bindings patches separate from dts patches. > >> > >> I don't think anyone said that bindings patches must be separate from > >> DTS. The only restriction is DTS cannot go with drivers. > > > > I have always heard that best practice is to have bindings in a patch > > by themselves. > > Yes, bindings must be separate patch, no one here objects this. You said > they cannot go together via one maintainer tree or I misunderstood? > > > If I've misunderstood and/or folks have changed their > > minds, that's fine, but historically I've been told to keep them > > separate. > > Nothing changed. Bindings must be separate. They will be applied by > maintainer and, if correctly ordered, this is bisectable. OK, I think this is the disconnect here. No matter what order Jimmy's patches land in, it won't be bisectable from the standpoint of "make dtbs_check". This is what I've been trying to say. * If the bindings land first then the device tree won't have sku6 and will fail "make dtbs_check" * If the dts lands first then the bindings won't have sku6 and will fail "make dtbs_check". Am I missing something? So when you said "I don't think anyone said that bindings patches must be separate from DTS" and that you cared about "make dtbs_check" being bisectable that you were saying you wanted these squashed into one patch. I guess that's not the case. -Doug
On 22/07/2022 19:23, Doug Anderson wrote: > Hi, > > On Fri, Jul 22, 2022 at 10:14 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 21/07/2022 20:29, Doug Anderson wrote: >>> Hi, >>> >>> On Thu, Jul 21, 2022 at 9:52 AM Krzysztof Kozlowski >>> <krzysztof.kozlowski@linaro.org> wrote: >>>> >>>> On 21/07/2022 18:43, Doug Anderson wrote: >>>>> Hi, >>>>> >>>>> On Thu, Jul 21, 2022 at 9:33 AM Krzysztof Kozlowski >>>>> <krzysztof.kozlowski@linaro.org> wrote: >>>>>> >>>>>> On 21/07/2022 15:37, Doug Anderson wrote: >>>>>>> >>>>>>> Not worth sending a new version for, but normally I expect the >>>>>>> bindings to be patch #1 and the dts change to be patch #2. In any >>>>>>> case: >>>>>>> >>>>>>> Reviewed-by: Douglas Anderson <dianders@chromium.org> >>>>>> >>>>>> I would say worth v4, because otherwise patches is not bisectable. >>>>> >>>>> You're saying because `dtbs_check` will fail between the two? >>>> >>>> Yes >>> >>> OK. Then I assume you agree that reversing the order of the patches >>> won't help, only combining the two patches into one. >>> >>> >>>>> How does >>>>> flipping the order help? If `dtbs_check` needs to be bisectable then >>>>> these two need to be one patch, but I was always under the impression >>>>> that we wanted bindings patches separate from dts patches. >>>> >>>> I don't think anyone said that bindings patches must be separate from >>>> DTS. The only restriction is DTS cannot go with drivers. >>> >>> I have always heard that best practice is to have bindings in a patch >>> by themselves. >> >> Yes, bindings must be separate patch, no one here objects this. You said >> they cannot go together via one maintainer tree or I misunderstood? >> >>> If I've misunderstood and/or folks have changed their >>> minds, that's fine, but historically I've been told to keep them >>> separate. >> >> Nothing changed. Bindings must be separate. They will be applied by >> maintainer and, if correctly ordered, this is bisectable. > > OK, I think this is the disconnect here. > > No matter what order Jimmy's patches land in, it won't be bisectable > from the standpoint of "make dtbs_check". This is what I've been > trying to say. > > * If the bindings land first then the device tree won't have sku6 and > will fail "make dtbs_check" > > * If the dts lands first then the bindings won't have sku6 and will > fail "make dtbs_check". > > Am I missing something? Ah, you're right... The patch changes the bindings of a board instead of bringing a new variant. Yeah, this cannot be bisectable if kept separate, thus order does no matter. > > So when you said "I don't think anyone said that bindings patches must > be separate from DTS" and that you cared about "make dtbs_check" being > bisectable that you were saying you wanted these squashed into one > patch. I guess that's not the case. > Best regards, Krzysztof
Hi, On Thu, Jul 21, 2022 at 6:36 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Wed, Jul 20, 2022 at 8:59 PM Yunlong Jia > <yunlong.jia@ecs.corp-partner.google.com> wrote: > > > > SKU6 is LTE(w/o eSIM)+WIFI+Parade > > > > Signed-off-by: Yunlong Jia <yunlong.jia@ecs.corp-partner.google.com> > > --- > > > > Changes in v3: > > - Bindings and dts in the same series. > > > > Changes in v2: > > - Put sku6 before sku4. > > > > arch/arm64/boot/dts/qcom/sc7180-trogdor-pazquel-lte-parade.dts | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > Reviewed-by: Douglas Anderson <dianders@chromium.org> We're at a point now where this won't be able to land for at least 2.5 weeks. As an experiment, I created a staging tree atop the current arm64 dts tree and put this there. I'll try to put only things that I believe are truly ready to land there, but git hashes won't be stable since it's just a staging tree: https://github.com/dianders/kernel-staging/commits/qcom/arm64-staging I reversed the order of patch #1 and patch #2 when applying as per discussion in patch #2. -Doug
On Thu, 21 Jul 2022 03:58:42 +0000, Yunlong Jia wrote: > SKU6 is LTE(w/o eSIM)+WIFI+Parade > > Applied, thanks! [1/2] arm64: dts: qcom: Add SKU6 for sc7180-trogdor-pazquel-lte-parade commit: 030a7bfb365fd19714e25e9547764bff690cb227 [2/2] dt-bindings: arm: qcom: Document additional sku6 for sc7180 pazquel commit: 07603a1c17cf9eec5c963b470daba780cd7b9981 Best regards,
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pazquel-lte-parade.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pazquel-lte-parade.dts index 764c451c1a85..767cb7450c0d 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pazquel-lte-parade.dts +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pazquel-lte-parade.dts @@ -14,7 +14,7 @@ / { model = "Google Pazquel (Parade,LTE)"; - compatible = "google,pazquel-sku4", "qcom,sc7180"; + compatible = "google,pazquel-sku6", "google,pazquel-sku4", "qcom,sc7180"; }; &ap_sar_sensor_i2c {
SKU6 is LTE(w/o eSIM)+WIFI+Parade Signed-off-by: Yunlong Jia <yunlong.jia@ecs.corp-partner.google.com> --- Changes in v3: - Bindings and dts in the same series. Changes in v2: - Put sku6 before sku4. arch/arm64/boot/dts/qcom/sc7180-trogdor-pazquel-lte-parade.dts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)