Re: [PATCH 2/2] clk: rs9: Add support for 9FGV0841

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hi,

Am Montag, 6. November 2023, 08:49:10 CET schrieb Biju Das:
> Hi Marek,
> 
> Thanks for the patch.
> 
> > -----Original Message-----
> > From: Marek Vasut <marek.vasut+renesas@mailbox.org>
> > Sent: Sunday, November 5, 2023 8:08 PM
> > Subject: [PATCH 2/2] clk: rs9: Add support for 9FGV0841
> > 
> > This model is similar to 9FGV0441, the DIFx bits start at bit 0 again,
> > except this chip has 8 outputs. Adjust rs9_calc_dif() to special-case the
> > 9FGV0241 where DIFx bits start at 1. Extract only vendor ID from VID
> > register, the top 4 bits are revision ID which are not useful for the
> > vendor ID check.
> > 
> > Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> > ---
> > Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
> > Cc: Conor Dooley <conor+dt@kernel.org>
> > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > Cc: Michael Turquette <mturquette@baylibre.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Stephen Boyd <sboyd@kernel.org>
> > Cc: devicetree@vger.kernel.org
> > Cc: linux-clk@vger.kernel.org
> > Cc: linux-renesas-soc@vger.kernel.org
> > ---
> > 
> >  drivers/clk/clk-renesas-pcie.c | 23 +++++++++++++++++++----
> >  1 file changed, 19 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-
> > pcie.c index 6606aba253c5..f8dd79b18d5a 100644
> > --- a/drivers/clk/clk-renesas-pcie.c
> > +++ b/drivers/clk/clk-renesas-pcie.c
> > @@ -7,6 +7,7 @@
> > 
> >   * Currently supported:
> >   *   - 9FGV0241
> >   *   - 9FGV0441
> > 
> > + *   - 9FGV0841
> > 
> >   *
> >   * Copyright (C) 2022 Marek Vasut <marex@denx.de>
> >   */
> > 
> > @@ -42,6 +43,7 @@
> > 
> >  #define RS9_REG_DID				0x6
> >  #define RS9_REG_BCP				0x7
> > 
> > +#define RS9_REG_VID_MASK			GENMASK(3, 0)
> > 
> >  #define RS9_REG_VID_IDT				0x01
> >  
> >  #define RS9_REG_DID_TYPE_FGV			(0x0 << 
RS9_REG_DID_TYPE_SHIFT)
> > 
> > @@ -53,6 +55,7 @@
> > 
> >  enum rs9_model {
> >  
> >  	RENESAS_9FGV0241,
> >  	RENESAS_9FGV0441,
> > 
> > +	RENESAS_9FGV0841,
> > 
> >  };
> >  
> >  /* Structure to describe features of a particular 9-series model */ @@ -
> > 
> > 162,12 +165,15 @@ static u8 rs9_calc_dif(const struct rs9_driver_data
> > *rs9, int idx)  {
> > 
> >  	enum rs9_model model = rs9->chip_info->model;
> > 
> > +	/*
> > +	 * On 9FGV0241, the DIF OE0 is BIT(1) and DIF OE(1) is BIT(2),
> > +	 * on 9FGV0441 and 9FGV0841 the DIF OE0 is BIT(0) and so on.
> > +	 * Increment the index in the 9FGV0241 special case here.
> > +	 */
> 
> I guess model enum variable in struct rs9_chip_info can be replaced with a
> variable for the above hardware differences(eg: BIT(idx) value in struct
> rs9_chip_inf) . Then you don't need this function at all.

That's true for 9FGV family. If support for 9QXL family will ever be added 
(the header claims the support can be added), this enum is required again as 
the register model is completely different.

Best regards,
Alexander

> Cheers,
> Biju
> 
> >  	if (model == RENESAS_9FGV0241)
> > 
> > -		return BIT(idx + 1);
> > -	else if (model == RENESAS_9FGV0441)
> > -		return BIT(idx);
> > +		idx++;
> > 
> > -	return 0;
> > +	return BIT(idx);
> > 
> >  }
> >  
> >  static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx) @@
> > 
> > -333,6 +339,7 @@ static int rs9_probe(struct i2c_client *client)
> > 
> >  	if (ret < 0)
> >  	
> >  		return ret;
> > 
> > +	vid &= RS9_REG_VID_MASK;
> > 
> >  	if (vid != RS9_REG_VID_IDT || did != rs9->chip_info->did)
> >  	
> >  		return dev_err_probe(&client->dev, -ENODEV,
> >  		
> >  				     "Incorrect VID/DID: %#02x, %#02x.
> > 
> > Expected %#02x, %#02x\n", @@ -391,9 +398,16 @@ static const struct
> > rs9_chip_info renesas_9fgv0441_info = {
> > 
> >  	.did		= RS9_REG_DID_TYPE_FGV | 0x04,
> >  
> >  };
> > 
> > +static const struct rs9_chip_info renesas_9fgv0841_info = {
> > +	.model		= RENESAS_9FGV0841,
> > +	.num_clks	= 8,
> > +	.did		= RS9_REG_DID_TYPE_FGV | 0x08,
> > +};
> > +
> > 
> >  static const struct i2c_device_id rs9_id[] = {
> >  
> >  	{ "9fgv0241", .driver_data =
> > 
> > (kernel_ulong_t)&renesas_9fgv0241_info },
> > 
> >  	{ "9fgv0441", .driver_data =
> > 
> > (kernel_ulong_t)&renesas_9fgv0441_info },
> > +	{ "9fgv0841", .driver_data =
> > (kernel_ulong_t)&renesas_9fgv0841_info },
> > 
> >  	{ }
> >  
> >  };
> >  MODULE_DEVICE_TABLE(i2c, rs9_id);
> > 
> > @@ -401,6 +415,7 @@ MODULE_DEVICE_TABLE(i2c, rs9_id);  static const struct
> > of_device_id clk_rs9_of_match[] = {
> > 
> >  	{ .compatible = "renesas,9fgv0241", .data =
> > 
> > &renesas_9fgv0241_info },
> > 
> >  	{ .compatible = "renesas,9fgv0441", .data =
> > 
> > &renesas_9fgv0441_info },
> > +	{ .compatible = "renesas,9fgv0841", .data =
> > &renesas_9fgv0841_info },
> > 
> >  	{ }
> >  
> >  };
> >  MODULE_DEVICE_TABLE(of, clk_rs9_of_match);
> > 
> > --
> > 2.42.0


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/