Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221213124854.3779-1-sinthu.raja@ti.com> <20221213124854.3779-3-sinthu.raja@ti.com>
 <47c159b6-a1d3-dd7f-265e-6acbfc06ead0@kernel.org>
In-Reply-To: <47c159b6-a1d3-dd7f-265e-6acbfc06ead0@kernel.org>
From:   Sinthu Raja M <sinthu.raja@mistralsolutions.com>
Date:   Wed, 4 Jan 2023 12:52:04 +0530
Message-ID: <CAEd-yTSgABwuohxg=-GxDkJYkW1hSLHmtW=LUuSnzYN3X0AXLw@mail.gmail.com>
Subject: Re: [PATCH 2/2] phy: ti: j721e-wiz: Add support to enable LN23 Type-C swap
To:     Roger Quadros <rogerq@kernel.org>
Cc:     Vinod Koul <vkoul@kernel.org>,
        Ravi Gunasekaran <r-gunasekaran@ti.com>,
        Siddharth Vadapalli <s-vadapalli@ti.com>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        linux-phy@lists.infradead.org, linux-kernel@vger.kernel.org,
        Sinthu Raja <sinthu.raja@ti.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Roger,
On Wed, Dec 14, 2022 at 2:47 PM Roger Quadros <rogerq@kernel.org> wrote:
>
>
>
> On 13/12/2022 14:48, Sinthu Raja wrote:
> > Serdes wiz supports both LN23 and LN10 Type-C swap. Add support to
>
> SerDes?
>
> what is wiz?
The WIZ acts as a wrapper for the SerDes and can send control signals
to and report status signals from the SerDes, and muxes SerDes to
peripherals.
>
> It has nothing to do with Type-C. It is just a lane swap.
> There may or may not be a Type-C port.
According to the SerDes design, in the case of 4 lanes SerDes, Lane 0
and Lane 2 are reserved for USB for type-C lane swap if Lane 1 and
Lane 3 are integrated into USB3 PHY. The C-type lane swap is
responsible for swapping lanes 0 and 1 or lanes 2 and 3 based on the
configuration register. This allows a Type C USB connector to deal
with the connector orientation.
>
> > configure LN23 bit to swap between lane2 or lane3 if required.
>
> What do you mean by "swap between lane2 or lane3"?
>
> Do you mean "swap lanes 2 and 3"?
Yes.
>
> Is LN23 bit supported on all variants?
Yes, it is supported in all J7 variants if it is a 4 Lanes SerDes and
USB3 PHY is supported.
>
> >
> > Signed-off-by: Sinthu Raja <sinthu.raja@ti.com>
> > ---
> >  drivers/phy/ti/phy-j721e-wiz.c | 33 +++++++++++++++++++++++++++++----
> >  1 file changed, 29 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
> > index b17eec632d49..0091892af0b0 100644
> > --- a/drivers/phy/ti/phy-j721e-wiz.c
> > +++ b/drivers/phy/ti/phy-j721e-wiz.c
> > @@ -58,6 +58,11 @@ enum wiz_lane_standard_mode {
> >       LANE_MODE_GEN4,
> >  };
> >
> > +enum wiz_lane_typec_swap_mode {
> > +     LANE10_SWAP = 0,
> > +     LANE23_SWAP = 2,
> > +};
>
> What is this? Is it a register setting?
These are the master lane numbers that support the C-type lane swap.
Will change the enum name relatable and also shall add a comment for
more clarification.
>
> > +
> >  enum wiz_refclk_mux_sel {
> >       PLL0_REFCLK,
> >       PLL1_REFCLK,
> > @@ -194,6 +199,9 @@ static const struct reg_field p_mac_div_sel1[WIZ_MAX_LANES] = {
> >  static const struct reg_field typec_ln10_swap =
> >                                       REG_FIELD(WIZ_SERDES_TYPEC, 30, 30);
> >
> > +static const struct reg_field typec_ln23_swap =
> > +                                     REG_FIELD(WIZ_SERDES_TYPEC, 31, 31);
> > +
> >  struct wiz_clk_mux {
> >       struct clk_hw           hw;
> >       struct regmap_field     *field;
> > @@ -366,6 +374,7 @@ struct wiz {
> >       struct regmap_field     *mux_sel_field[WIZ_MUX_NUM_CLOCKS];
> >       struct regmap_field     *div_sel_field[WIZ_DIV_NUM_CLOCKS_16G];
> >       struct regmap_field     *typec_ln10_swap;
> > +     struct regmap_field     *typec_ln23_swap;
> >       struct regmap_field     *sup_legacy_clk_override;
> >
> >       struct device           *dev;
> > @@ -675,6 +684,13 @@ static int wiz_regfield_init(struct wiz *wiz)
> >               return PTR_ERR(wiz->typec_ln10_swap);
> >       }
> >
> > +     wiz->typec_ln23_swap = devm_regmap_field_alloc(dev, regmap,
> > +                                                    typec_ln23_swap);
> > +     if (IS_ERR(wiz->typec_ln23_swap)) {
> > +             dev_err(dev, "LN23_SWAP reg field init failed\n");
> > +             return PTR_ERR(wiz->typec_ln23_swap);
> > +     }
> > +
> >       wiz->phy_en_refclk = devm_regmap_field_alloc(dev, regmap, phy_en_refclk);
> >       if (IS_ERR(wiz->phy_en_refclk)) {
> >               dev_err(dev, "PHY_EN_REFCLK reg field init failed\n");
> > @@ -1242,15 +1258,24 @@ static int wiz_phy_reset_deassert(struct reset_controller_dev *rcdev,
> >                               regmap_field_write(wiz->typec_ln10_swap, 0);
> >               } else {
> >                       /* if no typec-dir gpio was specified, and USB lines
> > -                      * are connected to Lane 0 then set LN10 SWAP bit to 1.
> > +                      * are connected to SWAP lanes '0' or '2' then set LN10 SWAP
> > +                      * or LN23 bit to 1 respectively.
> >                        */
> >                       u32 num_lanes = wiz->num_lanes;
> >                       int i;
> >
> >                       for (i = 0; i < num_lanes; i++) {
> > -                             if ((wiz->lane_phy_type[i] == PHY_TYPE_USB3) \
> > -                                             && wiz->lane_phy_reg[i] == 0) {
> > -                                     regmap_field_write(wiz->typec_ln10_swap, 1);
> > +                             if (wiz->lane_phy_type[i] == PHY_TYPE_USB3) {
> > +                                     switch (wiz->lane_phy_reg[i]) {
> > +                                     case LANE10_SWAP:
> > +                                             regmap_field_write(wiz->typec_ln10_swap, 1);
> > +                                             break;
> > +                                     case LANE23_SWAP:
> > +                                             regmap_field_write(wiz->typec_ln23_swap, 1);
> > +                                             break;
> > +                                     default:
> > +                                             break;
> > +                                     }
>
> Could you please explain what is going on here?
> What is the basis for deciding if LN10 or LN23 bit must be set or not?
This snippet is used to configure the SerDes Type C control register
that allows the external lanes selection to be swapped. Based on the
master lane number and if the PHY type is USB3, we are configuring the
lane swap bits.
>

> What about clearing those bits?
According to the design this does not need to be cleared if it is set.
By default, it is set to 0.
>
> >                               }
> >                       }
> >               }
>
> cheers,
> -roger


-- 
With Regards
Sinthu Raja