Message-ID: <CAFBinCA_B9psNGBeDyhkewhoutNh6HsLUN+TRfO_8vuNqhis4Q@mail.gmail.com>
Date:   Mon, 26 Aug 2019 23:49:24 +0200
From:   Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To:     "Chuan Hua, Lei" <chuanhua.lei@...ux.intel.com>
Cc:     eswara.kota@...ux.intel.com, cheol.yong.kim@...el.com,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        p.zabel@...gutronix.de, qi-ming.wu@...el.com, robh@...nel.org,
        Hauke Mehrtens <hauke@...ke-m.de>
Subject: Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC

Hi,

On Mon, Aug 26, 2019 at 6:01 AM Chuan Hua, Lei
<chuanhua.lei@...ux.intel.com> wrote:
>
> Hi Martin,
>
> Thanks for your comment.
thank you for the quick reply

> On 8/25/2019 5:11 AM, Martin Blumenstingl wrote:
> > Hi Dilip,
> >
> >> Add driver for the reset controller present on Intel
> >> Lightening Mountain (LGM) SoC for performing reset
> >> management of the devices present on the SoC. Driver also
> >> registers a reset handler to peform the entire device reset.
> > [...]
> >> +static const struct of_device_id intel_reset_match[] = {
> >> +    { .compatible = "intel,rcu-lgm" },
> >> +    {}
> >> +};
> > how is this IP block differnet from the one used in many Lantiq SoCs?
> > there is already an upstream driver for the RCU IP block on the Lantiq
> > SoCs: drivers/reset/reset-lantiq.c
> >
> > some background:
> > Lantiq was started as a spinoff from Infineon in 2009. Intel then
> > acquired Lantiq in 2015. source: [0]
> > Intel is re-using some of the IP blocks from the MIPS Lantiq SoCs
> > (Intel even has some own MIPS SoCs as part of the Lantiq acquisition,
> > typically used for PON/GPON/ADSL/VDSL capable network devices).
> > Thus I think it is likely that the new "Lightening Mountain" SoCs use
> > an updated version of the Lantiq RCU IP.
>
> I would not say there is a fundamental difference since reset is a
> really simple
> stuff from all reset drivers.  However, it did have some difference
> from existing reset-lantiq.c since SoC becomes more and more complex.
OK, let me go through your detailed list

> 1. reset-lantiq.c use index instead of register offset + bit position.
> index reset is good for a small system (< 64). However, it will become very
> difficult to use if you have  > 100 reset. So we use register offset +
> bit position
reset-lantiq uses bit bit positions for specifying the reset line.
for example this is from OpenWrt's vr9.dtsi:
  reset0: reset-controller@10 {
    ...
    reg = <0x10 4>, <0x14 4>;
    #reset-cells = <2>;
  };

  gphy0: gphy@20 {
    ...
    resets = <&reset0 31 30>, <&reset1 7 7>;
    reset-names = "gphy", "gphy2";
  };

in my own words this means:
- all reset0 reset bits are at offset 0x10 (parent is RCU)
- all reset0 status bits are at offset 0x14 (parent is RCU)
- the first reset line uses reset bit 31 and status bit 30
- the second reset line uses reset bit 7 and status bit 7
- there can be multiple reset-controller instances, each taking the
reset and status offsets (OpenWrt's vr9.dtsi specifies the second RCU
reset controller "reset1" with reset offset 0x48 and status offset
0x24)

> 2. reset-lantiq.c does not support device restart which is part of the
> reset in
> old lantiq SoC. It moved this part into arch/mips/lantiq directory.
it was moved to the .dts instead of the arch code. again from
OpenWrt's vr9.dtsi [0]:
  reboot {
    compatible = "syscon-reboot";
    regmap = <&rcu0>;
    offset = <0x10>;
    mask = <0xe0000000>;
  };

this sets the reset0 reset bits 31, 30 and 29 at reboot

> 3. reset-lantiqc reset callback doesn't implement what hardware implemented
> function. In old SoCs, some bits in the same register can be hardware
> reset clear.
>
> It just call assert + assert. For these SoCs, we should only call
> assert, hardware will auto deassert.
I didn't know that. so to confirm I understand this correctly:
- some reset lines must be asserted and de-asserted manually (setting
and clearing the bit manually). the _assert and _deassert functions
should be used in this case
- other reset lines only support reset pulses. the _reset function
should be used in this case
- the _reset function should only assert the reset line, then wait
until the hardware automatically de-asserts it (without any further
write)

is this the same for all, old and new SoCs?

only two mainline Lantiq drivers are using reset lines - both are
using the _assert and _deassert variants:
- drivers/net/dsa/lantiq_gswip.c
- drivers/phy/lantiq/phy-lantiq-rcu-usb2.c

> 4. Code not optimized and intel internal review not assessed.
insights from you (like the issue with the reset callback) are very
valuable - this shows that we should focus on having one driver.

> Based on the above findings, I would suggest reset-lantiq.c to move to
> reset-intel-syscon.c
my concern with having two separate drivers is that it will be hard to
migrate from reset-lantiq to the "optimized" reset-intel-syscon
driver.
I don't have access to the datasheets for the any Lantiq/Intel SoC
(VRX200 and even older).
so debugging issues after switching from one driver to another is
tedious because I cannot tell which part of the driver is causing a
problem (it's either "all code from driver A" vs "all code from driver
B", meaning it's hard to narrow it down).
with separate commits/patches that are improving the reset-lantiq
driver I can do git bisect to find the cause of a problem on the older
SoCs (VRX200 for example)

> What is your opinion?
I explained why I would like to avoid having two separate drivers
(even just for a limited amount of time)


Martin


[0] https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/lantiq/files/arch/mips/boot/dts/vr9.dtsi;h=e8b87dbcc7de2fb928a4e602f1a650030d2f7c35;hb=c3a78955f34a61d402044f357f54f21c75a19ff9#l103

Powered by blists - more mailing lists