Re: [PATCH v3 3/3] arm64: dts: rockchip: k3566-quartz64-a: adds sata variant

[Alessandro Carminati]

Hello Peter,

Thank you for the valuable details you added to this thread.

If I understand correctly, the SATA controller has hardware-related 
issues if some electrical conditions are met by devices connected to 
the two SoC ports.

Here an example of a faulty device layout would be helpful.

But I guess this is not such common situation if you just connect a 
SATA device to the board.



On Sat, Sep 17, 2022 at 07:23:39AM -0400, Peter Geis wrote:
> On Sat, Sep 17, 2022 at 2:42 AM Heiko Stuebner <heiko@xxxxxxxxx> wrote:
> 
> Good Morning Heiko,
> 
> 
> >
> > Hi Peter,
> >
> > Am Samstag, 17. September 2022, 03:40:07 CEST schrieb Peter Geis:
> > > On Fri, Sep 16, 2022 at 12:06 PM Alessandro Carminati
> > > <alessandro.carminati@xxxxxxxxx> wrote:
> > > >
> > > > The Quartz64 board is built upon Rockchip RK3566.
> > > > Rockchip RK3566 has two combo phys.
> > > > The first connects USB3 and SATA ctrl1, and the second PCIe lane and SATA
> > > > ctrl2.
> > > > The second combo phy is hardwired to the PCIe slot, where for the first,
> > > > the hardware on the board provides both the USB3 connector and the SATA
> > > > connector.
> > > > This DT allows the users to switch the combo phy to the SATA connector.
> > >
> > > Good Evening,
> > >
> > > NACK to this whole series. Neither works correctly in the hardware as
> > > is,
> >
> > Just for my understanding for the future, sata not working is that a bug
> > in the soc or the board?
> 
> This is a board level problem. Attempting to build a device that had
> both ports electrically connected without a switch chip created a
> device where neither worked correctly. The SATA controllers themselves
> are amazing. I've used both nvme and sata m2 drives on the model b for
> example.
> 
> >
> > > and USB3 was decided to be left enabled as the SATA port will be
> > > removed completely in the next revision.
> >
> > That is good to know. Thanks for the heads up :-)
> 
> In regards to this sort of stuff in the future, we're working on
> fragment overlay support in U-Boot to work around the kernel's lack of
> support. If I remember correctly EDK2 will be implementing the switch
> in firmware as well. Devices that support both (at least ones I
> maintain) will have both in the dts, with the less likely use case
> left disabled. End users can simply switch which one is enabled if
> they want.

Reading through your message, I have the impression that you are trying 
to solve the problem on the firmware side.
I want to express my admiration for this effort.
I think that this is the right approach to solve this kind of problem, 
and that the more appropriate place to be for device trees is on the 
firmware and not in the kernel.
Currently, the kernel includes a considerable amount of device trees, 
and the Quarttz64-a device tree is already upstream.

As I understand, there's currently an effort to standardize the already 
existing device trees and give direction to the newcomers.
In a recent interaction with Krzysztof Kozlowski, I learned the already 
existing device trees are likely not to respond to these regulations. 

Sooner or later, each upstream device tree will need to be adjusted, 
and the currently upstreamed quartz64-a DTS is one of these.

I understand you are working on the u-boot side, possibly the EDK2. 
They alone are more than 80% of all the firmware running at this moment, 
but there's still a non-neglectable number of boots that use something 
else.

All these words to say: 

* Krzysztof confirmed the upstreamed device tree for the quartz64 needs 
  to be adjusted to meet the device trees node name regulation.

* The work needed to add the SATA support is minimal.

* Having this SATA DTS is not completely useless since numerous SATA 
  configurations work smoothly.

I am willing to work on this patch to make it suitable to be upstreamed.

Regards
Alessandro

> 
> Very Respectfully,
> Peter
> 
> >
> > Heiko
> >
> >
> > > > Signed-off-by: Alessandro Carminati <alessandro.carminati@xxxxxxxxx>
> > > > ---
> > > >  arch/arm64/boot/dts/rockchip/Makefile                   | 1 +
> > > >  arch/arm64/boot/dts/rockchip/rk3566-quartz64-a-sata.dts | 9 +++++++++
> > > >  2 files changed, 10 insertions(+)
> > > >  create mode 100644 arch/arm64/boot/dts/rockchip/rk3566-quartz64-a-sata.dts
> > > >
> > > > diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> > > > index 8c843f6fc3cc..1d5dd91d1a34 100644
> > > > --- a/arch/arm64/boot/dts/rockchip/Makefile
> > > > +++ b/arch/arm64/boot/dts/rockchip/Makefile
> > > > @@ -60,6 +60,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399pro-rock-pi-n10.dtb
> > > >  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-pinenote-v1.1.dtb
> > > >  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-pinenote-v1.2.dtb
> > > >  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-quartz64-a-usb3.dts
> > > > +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-quartz64-a-sata.dts
> > > >  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-quartz64-b.dtb
> > > >  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-roc-pc.dtb
> > > >  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-soquartz-cm4.dtb
> > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a-sata.dts b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a-sata.dts
> > > > new file mode 100644
> > > > index 000000000000..8620df7ec01e
> > > > --- /dev/null
> > > > +++ b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a-sata.dts
> > > > @@ -0,0 +1,9 @@
> > > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > > +
> > > > +/dts-v1/;
> > > > +
> > > > +#include "rk3566-quartz64-a.dtsi"
> > > > +
> > > > +&sata1 {
> > > > +       status = "okay";
> > > > +};
> > > > --
> > > > 2.34.1
> > > >
> > > >
> > > > _______________________________________________
> > > > Linux-rockchip mailing list
> > > > Linux-rockchip@xxxxxxxxxxxxxxxxxxx
> > > > http://lists.infradead.org/mailman/listinfo/linux-rockchip
> > >
> >
> >
> >
> >