Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Nelson Costa <Nelson.Costa@synopsys.com>
To:     Vinod Koul <vkoul@kernel.org>
CC:     "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Kishon Vijay Abraham I <kishon@ti.com>,
        Rob Herring <robh+dt@kernel.org>,
        Jose Abreu <Jose.Abreu@synopsys.com>
Subject: RE: [PATCH 5/9] phy: dwc: Add Synopsys DesignWare HDMI RX PHYs e405
 and e406 Driver
Thread-Topic: [PATCH 5/9] phy: dwc: Add Synopsys DesignWare HDMI RX PHYs e405
 and e406 Driver
Thread-Index: AQHXV6Hhw52rFLqJRUefJUBB/2fxaqseFEGAgAOKeVA=
Date:   Wed, 23 Jun 2021 16:02:00 +0000
Message-ID: <MW3PR12MB4459E69C4CE32CDD14C93929C1089@MW3PR12MB4459.namprd12.prod.outlook.com>
References: <cover.1622631488.git.nelson.costa@synopsys.com>
 <ac32f8d433860c5be612b393023329f967e2c058.1622631488.git.nelson.costa@synopsys.com>
 <YNApWS7tNGdWbyCm@vkoul-mobl>
In-Reply-To: <YNApWS7tNGdWbyCm@vkoul-mobl>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?PxDR9AgClLGli+M0DUK8wHFQU0YGR/cMX9HvU4SSXAUI7OFcZCQJJIJS6mm1?=
 =?us-ascii?Q?FBf5S4JtaRlDz5NYrtoXmcV2U7rmYoCz0yhdjrf/g/H2QMK5JuGHfdViIkX3?=
 =?us-ascii?Q?Ej62j8HJGwMEB+9AglgQJhwCKlZ0+qhxBPJJgPcvJdVfwyjGmlO+4HHPv9P/?=
 =?us-ascii?Q?7B8CP9Ra0guDGZDzcJ0lswgkMJi0Nlq9l/KSgtOs6gAMKEXcgw3Paw9Jy0jA?=
 =?us-ascii?Q?8fmXU9WZ3guZeRGikrgfvEWHoScz6X2wipxUCaap8r02AaXBC1jKQ8wmxThX?=
 =?us-ascii?Q?UOCjXH/wc/+3i2pDssXEt5WjpEjqL9vIb1VYlbWx20NnrMG2Fw/KuUXkkDr4?=
 =?us-ascii?Q?hVU4RSAS40RkTyzlEU7k8xuNvN61sLR8pzlej3rh++kxwEJobnaIR3xC/nrb?=
 =?us-ascii?Q?02dAWZi8ylLsk9S2NOrYUJ9LmpJcg5TSL0AqYU/5up7A6ztw8JmWomr3NshW?=
 =?us-ascii?Q?X43JqlVW+4uzR2tZnNvsfBO12vc0PkXChUojLx5IfM2r50WLSeqxK73KWpKa?=
 =?us-ascii?Q?ONyVmx8QAP+89VahD7SYkcL5PBK5uDzMBUoCq72/B/MT27JRUorEriJQHts2?=
 =?us-ascii?Q?igqehRg3wzm9ZgICCZ+dUvupE+LCvqxpT/Gb4P44oVXmu4Kjzt03QhRNNXlC?=
 =?us-ascii?Q?N1ftPmh9bzq7Xfq+lbO8pREk/eqrAzKazuD7HFSSveZinsaiogZaaeNdAge2?=
 =?us-ascii?Q?Mk8i5i1LkWMO85WAtrWaagcFNaINXBI5LL83eW6gmKKAW6uyTI4xCzTOvMlt?=
 =?us-ascii?Q?CPgur1tGWR3YHjfAIfK9C+RQhbzOmc5nwAgBL+zgJsNdu70ufYxYHTAOXXas?=
 =?us-ascii?Q?5eqFV/Uok3N4VJvcSBrWznuhOYH/8agWiTpYEw6f8WCiN4Iq02VBmV/e1KKa?=
 =?us-ascii?Q?SrD9kf6wBDuUNzseFJGrYMaYphOi8zfiFI/wSUBwpXSh6T4LYjz8xa7HYNx9?=
 =?us-ascii?Q?0o33n7UDuO5H7SEEId0pe721GSEXXgmIUPo4iT84r6fQSKk8mDMdNBFsAQBj?=
 =?us-ascii?Q?mMJYsuTbSBaBiFPxpfrBUcdL4ceLnks5Adcu9x/wcPpjpZclRnCXRr1EdTts?=
 =?us-ascii?Q?pKqPgNUuWEuSxsQWrGMt/5lq+BJbsQWlRSP8bmZOUK44kcEV2ZAO7Hh46RYY?=
 =?us-ascii?Q?eOhQ8GhWPxkHKKjHrQ5MCqr4MNcoE7UGatA5bc5md9jLRORL+u052BiOCe62?=
 =?us-ascii?Q?bZqlbh3jKojbH6xq7GqfMMSoj40akdo19pDgmT12H3Rj9PhVwnvoc3vMFTWN?=
 =?us-ascii?Q?GYd+3/dXVXPqnpCNVCgrR22HaVywQYk+qM64tuW3Gs13F/JJZDGoa63dWgjB?=
 =?us-ascii?Q?3cY=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: synopsys.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: MW3PR12MB4459.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9584d674-2450-4d1e-a585-08d936603c7e
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Jun 2021 16:02:00.7278
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: c33c9f88-1eb7-4099-9700-16013fd9e8aa
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 7Q0/h/xrvGiibjjy9yqB1Az1R5JFmlmO4q18dhztz+UrkRl1bpOPC/lQogNcwvDSzQ9uMAQh2Bt0yfOU2W5QvQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR1201MB0094
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Vinod,

Thanks for your initial comments and feedback!

From: Vinod Koul <vkoul@kernel.org>
Date: seg, jun 21, 2021 at 06:53:29

> On 02-06-21, 13:24, Nelson Costa wrote:
>=20
> > +# Makefile for the PHY drivers.
> > +#
> > +
> > +phy-dw-hdmi-e40x-y			:=3D phy-dw-hdmi-e40x-core.o
> > +phy-dw-hdmi-e40x-y			+=3D phy-dw-hdmi-e405.o
> > +phy-dw-hdmi-e40x-y			+=3D phy-dw-hdmi-e406.o
>=20
> why not:
> phy-dw-hdmi-e40x-y                   :=3D  phy-dw-hdmi-e40x-core.o phy-dw=
-hdmi-e405.o phy-dw-hdmi-e406.o ?
>=20

I agree! It will be addressed in v2.

> > +obj-$(CONFIG_VIDEO_DWC_HDMI_PHY_E40X)	+=3D phy-dw-hdmi-e40x.o
> > diff --git a/drivers/phy/dwc/phy-dw-hdmi-e405.c b/drivers/phy/dwc/phy-d=
w-hdmi-e405.c
> > new file mode 100644
> > index 0000000..5078a86
> > --- /dev/null
> > +++ b/drivers/phy/dwc/phy-dw-hdmi-e405.c
> > @@ -0,0 +1,497 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 - present Synopsys, Inc. and/or its affiliates.
> > + * Synopsys DesignWare HDMI PHY e405
>=20
> 2018 - 2021 ?
>=20

It will be addressed in v2.

> > +/* PHY e405 mpll configuration */
> > +static const struct dw_phy_mpll_config dw_phy_e405_mpll_cfg[] =3D {
> > +	{ 0x27, 0x1B94 },
>=20
> Lowercase please
>=20

It will be addressed in v2.

> > +	{ 0x28, 0x16D2 },
> > +	{ 0x29, 0x12D9 },
> > +	{ 0x2A, 0x3249 },
> > +	{ 0x2B, 0x3653 },
> > +	{ 0x2C, 0x3436 },
> > +	{ 0x2D, 0x124D },
> > +	{ 0x2E, 0x0001 },
> > +	{ 0xCE, 0x0505 },
> > +	{ 0xCF, 0x0505 },
> > +	{ 0xD0, 0x0000 },
> > +	{ 0x00, 0x0000 },
> > +};
> > +
> > +/* PHY e405 equalization functions */
> > +static int dw_phy_eq_test(struct dw_phy_dev *dw_dev,
> > +			  u16 *fat_bit_mask, int *min_max_length)
>=20
> Please align this to preceding line open brace (checkpatch with --strict =
would warn you about this)
>=20

This is aligned in the original source code, and checkpatch clean. :)
Seems that only in patch format due the char '+' gives that effect.

> > +{
> > +	u16 main_fsm_status, val;
> > +	unsigned int i;
> > +
> > +	for (i =3D 0; i < DW_PHY_EQ_WAIT_TIME_START; i++) {
> > +		main_fsm_status =3D dw_phy_read(dw_dev, DW_PHY_MAINFSM_STATUS1);
> > +		if (main_fsm_status & DW_PHY_CLOCK_STABLE)
> > +			break;
> > +		mdelay(DW_PHY_EQ_SLEEP_TIME_CDR);
> > +	}
> > +
> > +	if (i =3D=3D DW_PHY_EQ_WAIT_TIME_START) {
> > +		dev_dbg(dw_dev->dev, "PHY start conditions not achieved\n");
>=20
> not error?
>=20

In the reality this is not an error scenario.
The 'ETIMEDOUT' return code is used by the HDMI RX Controller driver to=20
know that the equalization was not performed due the lack of Clock=20
Stable, and in this case the driver will keep forcing the equalization.

This scenario is something that can happen during HDMI Source Video=20
transitions, or even in scenarios that the Source is not sending video at=20
all.

That's why the message is not an error but instead only useful for debug=20
purposes.

BTW, the print message will be changed in v2 to make more clear it's not=20
an error scenario:
"PHY start conditions not achieved\n" -> "Clock received is not stable=20
yet\n"

> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	if (main_fsm_status & DW_PHY_PLL_RATE_BIT1) {
> > +		dev_dbg(dw_dev->dev, "invalid pll rate\n");
>=20
> error?
>=20

This is not an error scenario also.
The 'EINVAL' return code is used by the HDMI RX Controller driver to know=20
that the Equalization is not required and only needs to wait for tmds=20
valid to proceed.

BTW, the print message will be changed in v2 to make more clear it's not=20
an error scenario:
"invalid pll rate\n" -> "Equalization not required\n"

> > +		return -EINVAL;
> > +	}
> > +
> > +	val =3D dw_phy_read(dw_dev, DW_PHY_CDR_CTRL_CNT) &
> > +		DW_PHY_HDMI_MHL_MODE_MASK;
>=20
> can be single line
>=20

But in single line seems that would not fit in the 80 chars.

> > +static void dw_phy_eq_init_vars(struct dw_phy_eq_ch *ch)
> > +{
> > +	ch->acc =3D 0;
> > +	ch->acq =3D 0;
> > +	ch->last_acq =3D 0;
> > +	ch->valid_long_setting =3D 0;
> > +	ch->valid_short_setting =3D 0;
>=20
> memset() ?
>=20

In this function we only initialize some fields of 'ch' with 0.
There are other fields that are not supposed to be zeroed.

> > +static bool dw_phy_eq_acquire_early_cnt(struct dw_phy_dev *dw_dev,
> > +					u16 setting, u16 acq,
> > +					struct dw_phy_eq_ch *ch0,
> > +					struct dw_phy_eq_ch *ch1,
> > +					struct dw_phy_eq_ch *ch2)
> > +{
> > +	u16 lock_vector =3D 0x1 << setting;
> > +	unsigned int i;
> > +
> > +	ch0->out_bound_acq =3D 0;
> > +	ch1->out_bound_acq =3D 0;
> > +	ch2->out_bound_acq =3D 0;
> > +	ch0->acq =3D 0;
> > +	ch1->acq =3D 0;
> > +	ch2->acq =3D 0;
> > +
> > +	dw_phy_eq_equal_setting(dw_dev, lock_vector);
> > +	dw_phy_eq_auto_calib(dw_dev);
> > +
> > +	mdelay(DW_PHY_EQ_SLEEP_TIME_CDR);
> > +	if (!dw_phy_tmds_valid(dw_dev))
> > +		dev_dbg(dw_dev->dev, "TMDS is NOT valid\n");
> > +
> > +	ch0->read_acq =3D dw_phy_read(dw_dev, DW_PHY_CH0_EQ_STATUS3);
> > +	ch1->read_acq =3D dw_phy_read(dw_dev, DW_PHY_CH1_EQ_STATUS3);
> > +	ch2->read_acq =3D dw_phy_read(dw_dev, DW_PHY_CH2_EQ_STATUS3);
> > +
> > +	ch0->acq +=3D ch0->read_acq;
> > +	ch1->acq +=3D ch1->read_acq;
> > +	ch2->acq +=3D ch2->read_acq;
> > +
> > +	ch0->upper_bound_acq =3D ch0->read_acq + DW_PHY_EQ_BOUNDSPREAD;
> > +	ch0->lower_bound_acq =3D ch0->read_acq - DW_PHY_EQ_BOUNDSPREAD;
> > +	ch1->upper_bound_acq =3D ch1->read_acq + DW_PHY_EQ_BOUNDSPREAD;
> > +	ch1->lower_bound_acq =3D ch1->read_acq - DW_PHY_EQ_BOUNDSPREAD;
> > +	ch2->upper_bound_acq =3D ch2->read_acq + DW_PHY_EQ_BOUNDSPREAD;
> > +	ch2->lower_bound_acq =3D ch2->read_acq - DW_PHY_EQ_BOUNDSPREAD;
> > +
> > +	for (i =3D 1; i < acq; i++) {
>=20
> why do we start from 1 here..?
>=20

Because the first acquisition is required to be done before and out of=20
the for loop.

> > +static const struct dw_phy_mpll_config dw_phy_e406_mpll_cfg[] =3D {
> > +	{ 0x27, 0x1C94 },
> > +	{ 0x28, 0x3713 },
> > +	{ 0x29, 0x24DA },
> > +	{ 0x2A, 0x5492 },
> > +	{ 0x2B, 0x4B0D },
> > +	{ 0x2C, 0x4760 },
> > +	{ 0x2D, 0x008C },
> > +	{ 0x2E, 0x0010 },
> > +	{ 0x00, 0x0000 },
>=20
> lower case here too please
>=20

It will be addressed in v2.

> > +static void dw_phy_eq_init_vars(struct dw_phy_eq_ch *ch)
> > +{
> > +	ch->acc =3D 0;
> > +	ch->acq =3D 0;
> > +	ch->last_acq =3D 0;
> > +	ch->valid_long_setting =3D 0;
> > +	ch->valid_short_setting =3D 0;
> > +	ch->best_setting =3D DW_PHY_EQ_SHORT_CABLE_SETTING;
> > +}
>=20
> duplicate, it would make sense to create a common lib of such functions
> and use them across these files
>=20

This function initializes the 'ch->best_setting =3D=20
DW_PHY_EQ_SHORT_CABLE_SETTING' where the define can be tuned=20
independently for each PHY.

Regarding the other fields, we can create a common function=20
'_dw_phy_eq_init_vars()' in the common files phy-dw-hdmi-e40x.h/c and=20
call it inside the original function.

> > +static int dw_phy_set_data(struct dw_phy_dev *dw_dev)
> > +{
> > +	const struct dw_hdmi_phy_data *of_data;
> > +
> > +	of_data =3D of_device_get_match_data(dw_dev->dev);
> > +
> > +	if (of_data) {
> > +		dw_dev->phy_data =3D (struct dw_hdmi_phy_data *)of_data;
> > +	} else if (dw_dev->config->version =3D=3D dw_phy_e405_data.version) {
> > +		dw_dev->phy_data =3D &dw_phy_e405_data;
> > +	} else if (dw_dev->config->version =3D=3D dw_phy_e406_data.version) {
> > +		dw_dev->phy_data =3D &dw_phy_e406_data;
>=20
> Driver supports only of, where will these else cases get triggered?
>=20

The driver supports the following two use cases:
 1. Device Tree support: where the configuration data is supported by the=20
DT itself
 2. Non Device Tree support: where the configuration data is passed=20
through the platform_data

So, the "else cases" are used for the second use case (Non Device Tree=20
support).

> --=20
> ~Vinod

Thanks,

BR,

Nelson Costa