From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1A1C0C433E1 for ; Tue, 14 Jul 2020 20:02:36 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id EE1D920720 for ; Tue, 14 Jul 2020 20:02:35 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="EJQo63Fg" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726634AbgGNUCf (ORCPT ); Tue, 14 Jul 2020 16:02:35 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50272 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729426AbgGNUCe (ORCPT ); Tue, 14 Jul 2020 16:02:34 -0400 Received: from mail-pg1-x544.google.com (mail-pg1-x544.google.com [IPv6:2607:f8b0:4864:20::544]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C3FC8C08C5C1 for ; Tue, 14 Jul 2020 13:02:34 -0700 (PDT) Received: by mail-pg1-x544.google.com with SMTP id n5so687958pgf.7 for ; Tue, 14 Jul 2020 13:02:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=Rj7n2FEi3btNvRiXOO95ZgZW2mJPBVEmXrIlW5n92HE=; b=EJQo63FgUyR1rd5T3rjkMbtG3Y3uJp/ZJF1UiISxu+YQSssbpPE+uB6yPx+AV8ZgT0 aUiNAocQQw4V2Kg3Q7RYjIqxJF9zxusE9OTzJ9kkaxZeScYnY4kP5os+IMQcunyG8arU i8M5BMmcdes2oYK8RpGAzLHAoDEqfpf11/brt0Ftgii5nRAQdRnJdomX7Lzhc2ydwOQn yQRQHWf64Te5x1y2UPwEzEtX/8g48OC02IgG23NWiAVqceB0wLpC4YF11VA5zGBqF65G 7BHKiq5A2qbqE02EibFGQQu9V6Js0Iog8tjUrlvfL6qvH1pVGc2IHm2Ys4ZlcWlGMpVy 6LKA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=Rj7n2FEi3btNvRiXOO95ZgZW2mJPBVEmXrIlW5n92HE=; b=IoL32MN0O0t4Hh6j0TnD28c1uL38Tz/ggNgU36XRQbmTrg6HTuWuwCqq78FwtVlALu R2X2M7gGIt6UUnQQbr7EfEG3bwjMODrFnPldhIJsA/i9365je0kitJYHQzVuufGpv7PP w3ICz+q6aHybgdpK8WWRf8RbtnFv7t7GDkQLDGnHRQsPeA7tOEkh3iTqS1u2PbQTjyTJ lFy0gjWjTO9g4+849T2asmgBLrLsFrpIlxZcRZ4CbfkPRsUa0FgAXwkfqFh4OU1LivjP XWD8CM1X3eCBLfgSlGL/DPorfPQTl9Mzy3SCMd7i6cewcZVfdPj1rUW+P99jMxP/fo6D 13dg== X-Gm-Message-State: AOAM532lIYd1wZ/00k3Yh0DkeHvHWCoLXzgqlCz39SxuengdUDfBd9BR yrauta30feLPUX6/uH0cihaIQA== X-Google-Smtp-Source: ABdhPJwJy7HkTTdvIuylyd0YSn5b3RgvXjveXkOJmC0QWWm2KuLx8toxk9W4hVduZo2tCSHPhkrfEw== X-Received: by 2002:a62:52cd:: with SMTP id g196mr5690485pfb.178.1594756953980; Tue, 14 Jul 2020 13:02:33 -0700 (PDT) Received: from builder.lan (104-188-17-28.lightspeed.sndgca.sbcglobal.net. [104.188.17.28]) by smtp.gmail.com with ESMTPSA id i66sm39133pfc.12.2020.07.14.13.02.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 14 Jul 2020 13:02:33 -0700 (PDT) Date: Tue, 14 Jul 2020 13:00:27 -0700 From: Bjorn Andersson To: Eric Biggers Cc: Rob Herring , Andy Gross , SCSI , linux-arm-msm , linux-fscrypt@vger.kernel.org, Alim Akhtar , Avri Altman , Barani Muthukumaran , Can Guo , Elliot Berman , John Stultz , "Martin K. Petersen" , Satya Tangirala , Steev Klimaszewski , Thara Gopinath Subject: Re: [PATCH v6 3/5] arm64: dts: sdm845: add Inline Crypto Engine registers and clock Message-ID: <20200714200027.GH388985@builder.lan> References: <20200710072013.177481-4-ebiggers@kernel.org> <20200714161516.GA1064009@gmail.com> <20200714164353.GB1064009@gmail.com> <20200714171203.GC1064009@gmail.com> <20200714173111.GG388985@builder.lan> <20200714174345.GE1218486@builder.lan> <20200714175718.GD1064009@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200714175718.GD1064009@gmail.com> Sender: linux-arm-msm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org On Tue 14 Jul 10:57 PDT 2020, Eric Biggers wrote: > On Tue, Jul 14, 2020 at 10:43:45AM -0700, Bjorn Andersson wrote: > > On Tue 14 Jul 10:31 PDT 2020, Bjorn Andersson wrote: > > > > > On Tue 14 Jul 10:12 PDT 2020, Eric Biggers wrote: > > > > > > > On Tue, Jul 14, 2020 at 10:59:44AM -0600, Rob Herring wrote: > > > > > On Tue, Jul 14, 2020 at 10:43 AM Eric Biggers wrote: > > > > > > > > > > > > On Tue, Jul 14, 2020 at 10:35:12AM -0600, Rob Herring wrote: > > > > > > > On Tue, Jul 14, 2020 at 10:15 AM Eric Biggers wrote: > > > > > > > > > > > > > > > > On Tue, Jul 14, 2020 at 10:16:04AM -0400, Martin K. Petersen wrote: > > > > > > > > > > > > > > > > > > Eric, > > > > > > > > > > > > > > > > > > > Add the vendor-specific registers and clock for Qualcomm ICE (Inline > > > > > > > > > > Crypto Engine) to the device tree node for the UFS host controller on > > > > > > > > > > sdm845, so that the ufs-qcom driver will be able to use inline crypto. > > > > > > > > > > > > > > > > > > I would like to see an Acked-by for this patch before I merge it. > > > > > > > > > > > > > > > > > > > > > > > > > Andy, Bjorn, or Rob: can you give Acked-by? > > > > > > > > > > > > > > DTS changes should go in via the QCom tree. > > > > > > > > > > > > > > > > > > > So, the DTS patch can't be applied without the driver patches since then the > > > > > > driver would misinterpret the ICE registers as the dev_ref_clk_ctrl registers. > > > > > > > > > > That sounds broken, but there's no context here for me to comment > > > > > further. DTS changes should work with old/stable kernels. I'd suggest > > > > > you get a review from Bjorn on the driver first. > > > > > > > > > > > > > The "breaking" change is that the dev_ref_clk_ctrl registers are now identified > > > > by name instead of assumed to be index 1. > > > > > > > > A reviewer had complained about the device-mapper bindings of this driver before > > > > (https://lkml.kernel.org/r/158334171487.7173.5606223900174949177@swboyd.mtv.corp.google.com). > > > > Changing to identifying the registers by name seemed like an improvement. > > > > > > > > If needed I can add a hole at index 1 to make the DTS changes work with > > > > old/stable kernels too, but I didn't know that is a requirement. (Normally for > > > > Linux kernel development, kernel-internal refactoring is always allowed > > > > upstream.) If I do this, would this hack have to be carried forever, or would > > > > we be able to fix it up eventually? Is there any deprecation period for DTS, or > > > > do the latest DTS have to work with a 20 year old kernel? > > > > > > > > > > The problem here is that DT binding refactoring is not kernel-internal. > > > It's two different projects living in the same git. > > > > > > There's a wish from various people that we make sure that new DTS > > > continues to work with existing kernels. This is a nice in theory > > > there's a lot of examples where we simply couldn't anticipate how future > > > bindings would look. A particular example is that this prohibits most > > > advancement in power management. > > > > > > > > > But afaict what you describe above would make a new kernel failing to > > > operate with the old DTS and that we have agreed to avoid. > > > So I think the appropriate way to deal with this is to request the reg > > > byname to detect the new binding and if that fails then assume that > > > index 1 is dev_ref_clk_ctrl. > > > > > > > I took another look at the git history and I can't find a single dts - > > either upstream or in any downstream tree - that specifies that second > > reg. > > > > So per my argument below, if you could include a patch that just removes > > the "dev_ref_clk_ctrl_mem" reference from the binding and driver I would > > be happy to r-b that and ack this patch. > > > > Regards, > > Bjorn > > > > > > > > There are cases where we just decide not to be backwards compatible, but > > > it's pretty rare. As for deprecation, I think 1-2 LTS releases is > > > sufficient, at that time scale it doesn't make sense to sit with an old > > > DTB anyways (given the current pace of advancements in the kernel). > > > > > Great, I'll remove the driver support for "dev_ref_clk_ctrl" then. However, > that doesn't solve the problem of the new DTS breaking old drivers, since old > drivers assume that reg[1] is dev_ref_clk_ctrl. > > This patch makes the DTS look like: > > reg = <0 0x01d84000 0 0x2500>, > <0 0x01d90000 0 0x8000>; > reg-names = "std", "ice"; > > The "ice" registers are new and are accessed by name instead of by index. > > But these also happen to be in reg[1]. Old drivers will see that reg[1] is > present and assume it is dev_ref_clk_ctrl. > > To work around this, I could leave a blank reg[1] entry: > > reg = <0 0x01d84000 0 0x2500>, > <0 0 0 0>, > <0 0x01d90000 0 0x8000>; > reg-names = "std", "dev_ref_clk_ctrl", "ice"; > > Do I need to do that? > No, let's not complicate it without good reason. SDM845 has hw_ver.major == 3, so we're not taking the else-path in ufs_qcom_init(). So I should be able to just merge this patch for 5.9 through the qcom tree after all (your code handles that it's not there and the existing code doesn't care). The two platforms that I can find that has UFS controller of hw_ver.major == 1 is APQ8084 and MSM8994, so I simply didn't look at an old enough downstream tree (msm-3.10) to find anyone specifying reg[1]. The reg specified is however coming from the TLMM (pinctrl-msm) hardware block, so it should not be directly remapped in the UFS driver... But regardless, that has not been seen in an upstream dts and per your patch 2 we would add that reg by name when that happens. There's recent activity on upstreaming more of the MSM8994 support, so perhaps then it's best to leave this snippet in the driver for now. Summary: Martin merges (merged?) patch 1, 2, 4 and 5 in the scsi tree, I'll merge this patch as is in the qcom tree and we'll just leave the dev_ref_clk handling as is for now then. Regards, Bjorn