Message-ID: <Y9E+iXjve8JP6be4@corigine.com>
Date:   Wed, 25 Jan 2023 15:36:57 +0100
From:   Simon Horman <simon.horman@...igine.com>
To:     Min Li <lnimi@...mail.com>
Cc:     richardcochran@...il.com, lee@...nel.org,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        Min Li <min.li.xe@...esas.com>
Subject: Re: [PATCH net RFC 1/1] mfd/ptp: clockmatrix: support 32-bit address
 space

On Tue, Jan 24, 2023 at 10:41:09AM -0500, Min Li wrote:
> From: Min Li <min.li.xe@...esas.com>
> 
> We used to assume 0x2010xxxx address. Now that we
> need to access 0x2011xxxx address, we need to
> support read/write the whole 32-bit address space.
> 
> Signed-off-by: Min Li <min.li.xe@...esas.com>
> ---
> This is a fix that involves both drivers/ptp and drivers/mfd. I made the
> patch right off the net tree but I am not sure if that is OK with everyone.
> Please suggest what to do if you think otherwise. Thanks 
> 
> 
>  drivers/mfd/rsmu.h               |   2 +
>  drivers/mfd/rsmu_i2c.c           | 166 ++++++++--
>  drivers/mfd/rsmu_spi.c           |  52 +--
>  drivers/ptp/ptp_clockmatrix.c    |  51 ++-
>  drivers/ptp/ptp_clockmatrix.h    |  32 +-
>  include/linux/mfd/idt82p33_reg.h |  11 +-
>  include/linux/mfd/idt8a340_reg.h | 546 ++++++++++++++++---------------
>  include/linux/mfd/rsmu.h         |   5 +-
>  8 files changed, 504 insertions(+), 361 deletions(-)

Hi Min Li,

In the same vein as Jakub's comment, this patch is doing a lot of things,
it is large, and it's not clear from the patch description what
it is fixing.

My general feeling is that it seems to be disambiguating support
for two different devices. And correcting support
for at least one of them. If the latter is true, it may well
be a fix. But IMHO it is far to large to be considerd for stable kernels.

My suggestion, FWIIW, would be to:

1. Break the patch into separate patches.
   F.e. 1. for 32bit address support, another to shift #defines around,
   another to add/fix device support.

2. Target this change as an enhancement, i.e. for net-next if it takes
   the networking path. Although I'd also consider if it's more appropriate
   for different portions to be taken into different subsystems.

> --- a/drivers/mfd/rsmu_i2c.c
> +++ b/drivers/mfd/rsmu_i2c.c
> @@ -18,11 +18,12 @@
>  #include "rsmu.h"
>  
>  /*
> - * 16-bit register address: the lower 8 bits of the register address come
> - * from the offset addr byte and the upper 8 bits come from the page register.
> + * 32-bit register address: the lower 8 bits of the register address come
> + * from the offset addr byte and the upper 24 bits come from the page register.
>   */
> -#define	RSMU_CM_PAGE_ADDR		0xFD
> -#define	RSMU_CM_PAGE_WINDOW		256
> +#define	RSMU_CM_PAGE_ADDR		0xFC
> +#define RSMU_CM_PAGE_MASK		0xFFFFFF00
> +#define RSMU_CM_ADDRESS_MASK		0x000000FF

nit: maybe using GENMASK is appropriate here.

...

> +static int rsmu_write_device(struct rsmu_ddata *rsmu, u8 reg, u8 *buf, u16 bytes)
> +{
> +	struct i2c_client *client = to_i2c_client(rsmu->dev);
> +	/* we add 1 byte for device register */
> +	u8 msg[RSMU_MAX_WRITE_COUNT + 1];
> +	int cnt;
> +
> +	if (bytes > RSMU_MAX_WRITE_COUNT)
> +		return -EINVAL;
> +
> +	msg[0] = reg;
> +	memcpy(&msg[1], buf, bytes);

nit: This is easier on my eyes.
     Especially in relation to the other '+ 1' in this function.
     But perhaps it's just me.
     (*untested!*)

	*msg = reg;
	memcpy(msg + 1, buf, bytes);

> +
> +	cnt = i2c_master_send(client, msg, bytes + 1);
> +
> +	if (cnt < 0) {
> +		dev_err(&client->dev,
> +			"i2c_master_send failed at addr: %04x!", reg);
> +		return cnt;
>  	}
> +
> +	return 0;
> +}
> +
> +static int rsmu_write_page_register(struct rsmu_ddata *rsmu, u32 reg)
> +{
> +	u32 page = reg & RSMU_CM_PAGE_MASK;
> +	u8 buf[4];
> +	int err;
> +
> +	/* Do not modify offset register for none-scsr registers */
> +	if (reg < RSMU_CM_SCSR_BASE)
> +		return 0;
> +
> +	/* Simply return if we are on the same page */
> +	if (rsmu->page == page)
> +		return 0;
> +
> +	buf[0] = 0x0;
> +	buf[1] = (u8)((page >> 8) & 0xFF);
> +	buf[2] = (u8)((page >> 16) & 0xFF);
> +	buf[3] = (u8)((page >> 24) & 0xFF);

nit: maybe this is simpler (*untested!*)

	le32 data;

	...

	data = cpu_to_le32(page);
	err = rsmu_write_device(rsmu, RSMU_CM_PAGE_ADDR,
			        (u8 *)&data, sizeof(data));

nit2: I also notice this pattern is repeated.
      Perhaps a helper would be nice.

> +
> +	err = rsmu_write_device(rsmu, RSMU_CM_PAGE_ADDR, buf, sizeof(buf));
> +	if (err)
> +		dev_err(rsmu->dev, "Failed to set page offset 0x%x\n", page);
> +	else
> +		/* Remember the last page */
> +		rsmu->page = page;
> +
> +	return err;
> +}
> +
> +static int rsmu_reg_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +	struct rsmu_ddata *rsmu = i2c_get_clientdata((struct i2c_client *)context);
> +	u8 addr = (u8)(reg & RSMU_CM_ADDRESS_MASK);

nit: open coding this twice seems like it could be improved on.
Perhaps a helper (macro) would be a good option?

...

> @@ -136,7 +232,11 @@ static int rsmu_i2c_probe(struct i2c_client *client)
>  		dev_err(rsmu->dev, "Unsupported RSMU device type: %d\n", rsmu->type);
>  		return -ENODEV;
>  	}
> -	rsmu->regmap = devm_regmap_init_i2c(client, cfg);
> +
> +	if (rsmu->type == RSMU_CM)
> +		rsmu->regmap = devm_regmap_init(&client->dev, NULL, client, cfg);

Why?

> +	else
> +		rsmu->regmap = devm_regmap_init_i2c(client, cfg);
>  	if (IS_ERR(rsmu->regmap)) {
>  		ret = PTR_ERR(rsmu->regmap);
>  		dev_err(rsmu->dev, "Failed to allocate register map: %d\n", ret);
> diff --git a/drivers/mfd/rsmu_spi.c b/drivers/mfd/rsmu_spi.c
> index d2f3d8f1e05a..efece6f764a9 100644
> --- a/drivers/mfd/rsmu_spi.c
> +++ b/drivers/mfd/rsmu_spi.c
> @@ -19,19 +19,21 @@
>  
>  #define	RSMU_CM_PAGE_ADDR		0x7C
>  #define	RSMU_SABRE_PAGE_ADDR		0x7F
> -#define	RSMU_HIGHER_ADDR_MASK		0xFF80
> -#define	RSMU_HIGHER_ADDR_SHIFT		7
> -#define	RSMU_LOWER_ADDR_MASK		0x7F
> +#define	RSMU_PAGE_MASK			0xFFFFFF80
> +#define	RSMU_ADDR_MASK			0x7F

nit: GENMASK

...

> diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c
> index c9d451bf89e2..f626efd034e6 100644
> --- a/drivers/ptp/ptp_clockmatrix.c
> +++ b/drivers/ptp/ptp_clockmatrix.c

...

> @@ -576,21 +577,21 @@ static int _sync_pll_output(struct idtcm *idtcm,
>  
>  	/* PLL5 can have OUT8 as second additional output. */
>  	if (pll == 5 && qn_plus_1 != 0) {
> -		err = idtcm_read(idtcm, 0, HW_Q8_CTRL_SPARE,
> +		err = idtcm_read(idtcm, HW_Q8_CTRL_SPARE, 0,

Is this correct. The type of the module parameter (2nd argument)
of idtcm_read has changed from u16 to u32. And likewise,
HW_Q8_CTRL_SPARE now has more bits. But the order idtcm_read's
parameters hasn't changed. Why are the arguments changing
order here (and elsewhere)?

I see that idtcm_read is a think wrapper that adds together it's 2nd and
3rd parameters, so I guess this has no runtime effect. But still, is this
change correct?

> @@ -1395,6 +1396,20 @@ static int idtcm_set_pll_mode(struct idtcm_channel *channel,
>  	struct idtcm *idtcm = channel->idtcm;
>  	int err;
>  	u8 dpll_mode;
> +	u8 timeout = 0;
> +
> +	/* Setup WF/WP timer for phase pull-in to work correctly */
> +	err = idtcm_write(idtcm, channel->dpll_n, DPLL_WF_TIMER,
> +			  &timeout, sizeof(timeout));
> +	if (err)
> +		return err;
> +
> +	if (mode == PLL_MODE_WRITE_PHASE)
> +		timeout = 160;
> +	err = idtcm_write(idtcm, channel->dpll_n, DPLL_WP_TIMER,
> +			  &timeout, sizeof(timeout));
> +	if (err)
> +		return err;
>  
>  	err = idtcm_read(idtcm, channel->dpll_n,
>  			 IDTCM_FW_REG(idtcm->fw_ver, V520, DPLL_MODE),

I think this change warrants an explanation.

> diff --git a/drivers/ptp/ptp_clockmatrix.h b/drivers/ptp/ptp_clockmatrix.h
> index bf1e49409844..2dc7f3c1edf2 100644
> --- a/drivers/ptp/ptp_clockmatrix.h
> +++ b/drivers/ptp/ptp_clockmatrix.h
> @@ -54,21 +54,9 @@
>  #define LOCK_TIMEOUT_MS			(2000)
>  #define LOCK_POLL_INTERVAL_MS		(10)
>  
> -#define IDTCM_MAX_WRITE_COUNT		(512)
> -
>  #define PHASE_PULL_IN_MAX_PPB		(144000)
>  #define PHASE_PULL_IN_MIN_THRESHOLD_NS	(2)
>  
> -/*
> - * Return register address based on passed in firmware version
> - */
> -#define IDTCM_FW_REG(FW, VER, REG)	(((FW) < (VER)) ? (REG) : (REG##_##VER))
> -enum fw_version {
> -	V_DEFAULT = 0,
> -	V487 = 1,
> -	V520 = 2,
> -};
> -

There seems to be a lot of churn going on around #defines for much of the
remainder of the patch. I think motivation needs to be given for that.

...

Powered by blists - more mailing lists