diff mbox series

[v6,3/5] arm64: dts: sdm845: add Inline Crypto Engine registers and clock

Message ID 20200710072013.177481-4-ebiggers@kernel.org (mailing list archive)
State New, archived
Headers show
Series Inline crypto support on DragonBoard 845c | expand

Commit Message

Eric Biggers July 10, 2020, 7:20 a.m. UTC
From: Eric Biggers <ebiggers@google.com>

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.

Use a separate register range rather than extending the main UFS range
because there's a gap between the two, and the ICE registers are
vendor-specific.  (Actually, the hardware claims that the ICE range also
includes the array of standard crypto configuration registers; however,
on this SoC the Linux kernel isn't permitted to access them directly.)

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

Martin K. Petersen July 14, 2020, 2:16 p.m. UTC | #1
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.
Eric Biggers July 14, 2020, 4:15 p.m. UTC | #2
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?

- Eric
Rob Herring July 14, 2020, 4:35 p.m. UTC | #3
On Tue, Jul 14, 2020 at 10:15 AM Eric Biggers <ebiggers@kernel.org> 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.

Rob
Eric Biggers July 14, 2020, 4:43 p.m. UTC | #4
On Tue, Jul 14, 2020 at 10:35:12AM -0600, Rob Herring wrote:
> On Tue, Jul 14, 2020 at 10:15 AM Eric Biggers <ebiggers@kernel.org> 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.

But the driver patches can be applied without the DTS patch.

Martin: how about you take patches 1-2 and 4-5 through the scsi tree for 5.9,
and then we get the DTS patch taken through the QCOM tree for 5.10?

- Eric
Martin K. Petersen July 14, 2020, 4:46 p.m. UTC | #5
Eric,

> Martin: how about you take patches 1-2 and 4-5 through the scsi tree
> for 5.9, and then we get the DTS patch taken through the QCOM tree for
> 5.10?

Sure!
Rob Herring July 14, 2020, 4:59 p.m. UTC | #6
On Tue, Jul 14, 2020 at 10:43 AM Eric Biggers <ebiggers@kernel.org> 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 <ebiggers@kernel.org> 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.

Rob
Eric Biggers July 14, 2020, 5:12 p.m. UTC | #7
On Tue, Jul 14, 2020 at 10:59:44AM -0600, Rob Herring wrote:
> On Tue, Jul 14, 2020 at 10:43 AM Eric Biggers <ebiggers@kernel.org> 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 <ebiggers@kernel.org> 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?

- Eric
Bjorn Andersson July 14, 2020, 5:31 p.m. UTC | #8
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 <ebiggers@kernel.org> 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 <ebiggers@kernel.org> 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.


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).

Regards,
Bjorn
Rob Herring July 14, 2020, 5:36 p.m. UTC | #9
On Tue, Jul 14, 2020 at 11:12 AM Eric Biggers <ebiggers@kernel.org> 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 <ebiggers@kernel.org> 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 <ebiggers@kernel.org> 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.)

dtb <-> kernel is an ABI and not an internal interface. The dts files
are hosted in the kernel for convenience as that's where the vast
majority of h/w support is. The DT parts are also generated as a
standalone repo[1] using git-filter-branch for other projects to use.

>  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?

With the above being said, it's up to the individual platform
maintainers to decide whether breaking the ABI is okay or how long to
worry about compatibility. My only requirement is documenting that a
change does break compatibility. Given this is probably an optional
driver, they may not care.

BTW, we have broken (and fixed) 1998 era PowerMac G3s not too long
ago. That's the opposite direction with old DT (firmware really) and
new kernel.

Rob

[1] https://git.kernel.org/pub/scm/linux/kernel/git/devicetree/devicetree-rebasing.git/
Bjorn Andersson July 14, 2020, 5:43 p.m. UTC | #10
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 <ebiggers@kernel.org> 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 <ebiggers@kernel.org> 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).
> 
> Regards,
> Bjorn
Eric Biggers July 14, 2020, 5:57 p.m. UTC | #11
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 <ebiggers@kernel.org> 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 <ebiggers@kernel.org> 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?

- Eric
Bjorn Andersson July 14, 2020, 8 p.m. UTC | #12
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 <ebiggers@kernel.org> 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 <ebiggers@kernel.org> 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
Eric Biggers July 15, 2020, 3 a.m. UTC | #13
On Tue, Jul 14, 2020 at 01:00:27PM -0700, Bjorn Andersson wrote:
> 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 <ebiggers@kernel.org> 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 <ebiggers@kernel.org> 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.
> 

Okay, great.  So an old DTS with the new driver isn't a problem because no DTS
has ever declared dev_ref_clk_ctrl.  And a new DTS with an old driver is a less
important case, and also not really a problem here since breakage would only
occur if we added the ICE registers to an older SoC that has hw_ver.major == 1.

Maybe you'd like to provide your Acked-by on patches 2 and 5?

My instinct is always to remove code that has never been used.  But sure, if you
think the dev_ref_clk_ctrl code might be used soon, we can keep it for now.

- Eric
Eric Biggers July 20, 2020, 5:07 p.m. UTC | #14
On Tue, Jul 14, 2020 at 08:00:04PM -0700, Eric Biggers wrote:
> On Tue, Jul 14, 2020 at 01:00:27PM -0700, Bjorn Andersson wrote:
> > 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 <ebiggers@kernel.org> 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 <ebiggers@kernel.org> 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.
> > 
> 
> Okay, great.  So an old DTS with the new driver isn't a problem because no DTS
> has ever declared dev_ref_clk_ctrl.  And a new DTS with an old driver is a less
> important case, and also not really a problem here since breakage would only
> occur if we added the ICE registers to an older SoC that has hw_ver.major == 1.
> 
> Maybe you'd like to provide your Acked-by on patches 2 and 5?
> 
> My instinct is always to remove code that has never been used.  But sure, if you
> think the dev_ref_clk_ctrl code might be used soon, we can keep it for now.
> 

Ping?
Eric Biggers July 21, 2020, 6:20 p.m. UTC | #15
On Mon, Jul 20, 2020 at 10:07:13AM -0700, Eric Biggers wrote:
> > > 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.
> > > 
> > 
> > Okay, great.  So an old DTS with the new driver isn't a problem because no DTS
> > has ever declared dev_ref_clk_ctrl.  And a new DTS with an old driver is a less
> > important case, and also not really a problem here since breakage would only
> > occur if we added the ICE registers to an older SoC that has hw_ver.major == 1.
> > 
> > Maybe you'd like to provide your Acked-by on patches 2 and 5?
> > 
> > My instinct is always to remove code that has never been used.  But sure, if you
> > think the dev_ref_clk_ctrl code might be used soon, we can keep it for now.
> > 

Martin,

As per the above discussion, Bjorn has included this device tree patch
in his pull request for 5.9:
https://lore.kernel.org/linux-arm-msm/20200721044934.3430084-1-bjorn.andersson@linaro.org/

Could you apply patches 1-2 and 4-5 to the scsi tree now?

Thanks!

- Eric
Bjorn Andersson July 22, 2020, 5:11 a.m. UTC | #16
On Mon 20 Jul 10:07 PDT 2020, Eric Biggers wrote:

> On Tue, Jul 14, 2020 at 08:00:04PM -0700, Eric Biggers wrote:
> > On Tue, Jul 14, 2020 at 01:00:27PM -0700, Bjorn Andersson wrote:
> > > 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 <ebiggers@kernel.org> 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 <ebiggers@kernel.org> 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.
> > > 
> > 
> > Okay, great.  So an old DTS with the new driver isn't a problem because no DTS
> > has ever declared dev_ref_clk_ctrl.  And a new DTS with an old driver is a less
> > important case, and also not really a problem here since breakage would only
> > occur if we added the ICE registers to an older SoC that has hw_ver.major == 1.
> > 
> > Maybe you'd like to provide your Acked-by on patches 2 and 5?
> > 
> > My instinct is always to remove code that has never been used.  But sure, if you
> > think the dev_ref_clk_ctrl code might be used soon, we can keep it for now.
> > 
> 
> Ping?

Sorry, I thought Martin already did pick up the SCSI patches. I've acked
the two patches now.

Let's see what happens on MSM8994 to see if we should alter or remove
the external ref_clk handling.

Regards,
Bjorn
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 8eb5a31346d2..0affb0041724 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1692,7 +1692,9 @@  mmss_noc: interconnect@1740000 {
 		ufs_mem_hc: ufshc@1d84000 {
 			compatible = "qcom,sdm845-ufshc", "qcom,ufshc",
 				     "jedec,ufs-2.0";
-			reg = <0 0x01d84000 0 0x2500>;
+			reg = <0 0x01d84000 0 0x2500>,
+			      <0 0x01d90000 0 0x8000>;
+			reg-names = "std", "ice";
 			interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>;
 			phys = <&ufs_mem_phy_lanes>;
 			phy-names = "ufsphy";
@@ -1712,7 +1714,8 @@  ufs_mem_hc: ufshc@1d84000 {
 				"ref_clk",
 				"tx_lane0_sync_clk",
 				"rx_lane0_sync_clk",
-				"rx_lane1_sync_clk";
+				"rx_lane1_sync_clk",
+				"ice_core_clk";
 			clocks =
 				<&gcc GCC_UFS_PHY_AXI_CLK>,
 				<&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>,
@@ -1721,7 +1724,8 @@  ufs_mem_hc: ufshc@1d84000 {
 				<&rpmhcc RPMH_CXO_CLK>,
 				<&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>,
 				<&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>,
-				<&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>;
+				<&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>,
+				<&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
 			freq-table-hz =
 				<50000000 200000000>,
 				<0 0>,
@@ -1730,7 +1734,8 @@  ufs_mem_hc: ufshc@1d84000 {
 				<0 0>,
 				<0 0>,
 				<0 0>,
-				<0 0>;
+				<0 0>,
+				<0 300000000>;
 
 			status = "disabled";
 		};