Message-ID: <MW3PR12MB4459E69C4CE32CDD14C93929C1089@MW3PR12MB4459.namprd12.prod.outlook.com>
Date:   Wed, 23 Jun 2021 16:02:00 +0000
From:   Nelson Costa <Nelson.Costa@...opsys.com>
To:     Vinod Koul <vkoul@...nel.org>
CC:     "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Kishon Vijay Abraham I <kishon@...com>,
        Rob Herring <robh+dt@...nel.org>,
        Jose Abreu <Jose.Abreu@...opsys.com>
Subject: RE: [PATCH 5/9] phy: dwc: Add Synopsys DesignWare HDMI RX PHYs e405
 and e406 Driver

Hi Vinod,

Thanks for your initial comments and feedback!

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

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

I agree! It will be addressed in v2.

> > +obj-$(CONFIG_VIDEO_DWC_HDMI_PHY_E40X)	+= phy-dw-hdmi-e40x.o
> > diff --git a/drivers/phy/dwc/phy-dw-hdmi-e405.c b/drivers/phy/dwc/phy-dw-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
> 
> 2018 - 2021 ?
> 

It will be addressed in v2.

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

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)
> 
> Please align this to preceding line open brace (checkpatch with --strict would warn you about this)
> 

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 = 0; i < DW_PHY_EQ_WAIT_TIME_START; i++) {
> > +		main_fsm_status = 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 == DW_PHY_EQ_WAIT_TIME_START) {
> > +		dev_dbg(dw_dev->dev, "PHY start conditions not achieved\n");
> 
> not error?
> 

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

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

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

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

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

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

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

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

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 = 0;
> > +	ch->acq = 0;
> > +	ch->last_acq = 0;
> > +	ch->valid_long_setting = 0;
> > +	ch->valid_short_setting = 0;
> 
> memset() ?
> 

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 = 0x1 << setting;
> > +	unsigned int i;
> > +
> > +	ch0->out_bound_acq = 0;
> > +	ch1->out_bound_acq = 0;
> > +	ch2->out_bound_acq = 0;
> > +	ch0->acq = 0;
> > +	ch1->acq = 0;
> > +	ch2->acq = 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 = dw_phy_read(dw_dev, DW_PHY_CH0_EQ_STATUS3);
> > +	ch1->read_acq = dw_phy_read(dw_dev, DW_PHY_CH1_EQ_STATUS3);
> > +	ch2->read_acq = dw_phy_read(dw_dev, DW_PHY_CH2_EQ_STATUS3);
> > +
> > +	ch0->acq += ch0->read_acq;
> > +	ch1->acq += ch1->read_acq;
> > +	ch2->acq += ch2->read_acq;
> > +
> > +	ch0->upper_bound_acq = ch0->read_acq + DW_PHY_EQ_BOUNDSPREAD;
> > +	ch0->lower_bound_acq = ch0->read_acq - DW_PHY_EQ_BOUNDSPREAD;
> > +	ch1->upper_bound_acq = ch1->read_acq + DW_PHY_EQ_BOUNDSPREAD;
> > +	ch1->lower_bound_acq = ch1->read_acq - DW_PHY_EQ_BOUNDSPREAD;
> > +	ch2->upper_bound_acq = ch2->read_acq + DW_PHY_EQ_BOUNDSPREAD;
> > +	ch2->lower_bound_acq = ch2->read_acq - DW_PHY_EQ_BOUNDSPREAD;
> > +
> > +	for (i = 1; i < acq; i++) {
> 
> why do we start from 1 here..?
> 

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

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

It will be addressed in v2.

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

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

Regarding the other fields, we can create a common function 
'_dw_phy_eq_init_vars()' in the common files phy-dw-hdmi-e40x.h/c and 
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 = of_device_get_match_data(dw_dev->dev);
> > +
> > +	if (of_data) {
> > +		dw_dev->phy_data = (struct dw_hdmi_phy_data *)of_data;
> > +	} else if (dw_dev->config->version == dw_phy_e405_data.version) {
> > +		dw_dev->phy_data = &dw_phy_e405_data;
> > +	} else if (dw_dev->config->version == dw_phy_e406_data.version) {
> > +		dw_dev->phy_data = &dw_phy_e406_data;
> 
> Driver supports only of, where will these else cases get triggered?
> 

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

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

> -- 
> ~Vinod

Thanks,

BR,

Nelson Costa

Powered by blists - more mailing lists