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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxxx).
> > > 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



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux