| Message ID | 20230110100003.370917-3-alexander.stein@ew.tq-group.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2,1/4] clk: rs9: Check for vendor/device ID | expand |
On 1/10/23 11:00, Alexander Stein wrote: [...] > static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx) > { > struct i2c_client *client = rs9->client; > + u8 dif = rs9_calc_dif(rs9, idx); > unsigned char name[5] = "DIF0"; > struct device_node *np; > int ret; > u32 sr; > > /* Set defaults */ > - rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx); Are you sure this line ^ should be dropped ? Shouldn't the bitfield be cleared first and modified second? > - rs9->clk_dif_sr |= RS9_REG_SR_3V0_DIF(idx); > + rs9->clk_dif_sr |= dif; > > snprintf(name, 5, "DIF%d", idx); > np = of_get_child_by_name(client->dev.of_node, name); [...]
Hi Marek, thanks for your feedback. Am Dienstag, 10. Januar 2023, 11:31:49 CET schrieb Marek Vasut: > On 1/10/23 11:00, Alexander Stein wrote: > > [...] > > > static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx) > > { > > > > struct i2c_client *client = rs9->client; > > > > + u8 dif = rs9_calc_dif(rs9, idx); > > > > unsigned char name[5] = "DIF0"; > > struct device_node *np; > > int ret; > > u32 sr; > > > > /* Set defaults */ > > > > - rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx); > > Are you sure this line ^ should be dropped ? > Shouldn't the bitfield be cleared first and modified second? Well, I had in my mind that this function is called upon probe with clk_dif_sr being cleared anyway, so this does essentially nothing. And the DIF bit is set unconditionally, so what is the point of masking it before? Best regards, Alexander > > - rs9->clk_dif_sr |= RS9_REG_SR_3V0_DIF(idx); > > + rs9->clk_dif_sr |= dif; > > > > snprintf(name, 5, "DIF%d", idx); > > np = of_get_child_by_name(client->dev.of_node, name); > > [...]
On 1/10/23 14:22, Alexander Stein wrote: > Hi Marek, Hi, > thanks for your feedback. > > Am Dienstag, 10. Januar 2023, 11:31:49 CET schrieb Marek Vasut: >> On 1/10/23 11:00, Alexander Stein wrote: >> >> [...] >> >>> static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx) >>> { >>> >>> struct i2c_client *client = rs9->client; >>> >>> + u8 dif = rs9_calc_dif(rs9, idx); >>> >>> unsigned char name[5] = "DIF0"; >>> struct device_node *np; >>> int ret; >>> u32 sr; >>> >>> /* Set defaults */ >>> >>> - rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx); >> >> Are you sure this line ^ should be dropped ? >> Shouldn't the bitfield be cleared first and modified second? > > Well, I had in my mind that this function is called upon probe with clk_dif_sr > being cleared anyway, so this does essentially nothing. And the DIF bit is set > unconditionally, so what is the point of masking it before? Good point, but then, what's the point of ORRing either ? Just do a plain assignment.
Hello Marek, Am Dienstag, 10. Januar 2023, 14:37:19 CET schrieb Marek Vasut: > On 1/10/23 14:22, Alexander Stein wrote: > > Hi Marek, > > Hi, > > > thanks for your feedback. > > > > Am Dienstag, 10. Januar 2023, 11:31:49 CET schrieb Marek Vasut: > >> On 1/10/23 11:00, Alexander Stein wrote: > >> > >> [...] > >> > >>> static int rs9_get_output_config(struct rs9_driver_data *rs9, int > >>> idx) > >>> { > >>> > >>> struct i2c_client *client = rs9->client; > >>> > >>> + u8 dif = rs9_calc_dif(rs9, idx); > >>> > >>> unsigned char name[5] = "DIF0"; > >>> struct device_node *np; > >>> int ret; > >>> u32 sr; > >>> > >>> /* Set defaults */ > >>> > >>> - rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx); > >> > >> Are you sure this line ^ should be dropped ? > >> Shouldn't the bitfield be cleared first and modified second? > > > > Well, I had in my mind that this function is called upon probe with > > clk_dif_sr being cleared anyway, so this does essentially nothing. And > > the DIF bit is set unconditionally, so what is the point of masking it > > before? > > Good point, but then, what's the point of ORRing either ? Just do a > plain assignment. OR-ring is necessary as this function is called for each DIF output (see idx parameter), so plain assignment will clear the previously set bits. Best regards, Alexander
On 1/10/23 14:47, Alexander Stein wrote: > Hello Marek, > > Am Dienstag, 10. Januar 2023, 14:37:19 CET schrieb Marek Vasut: >> On 1/10/23 14:22, Alexander Stein wrote: >>> Hi Marek, >> >> Hi, >> >>> thanks for your feedback. >>> >>> Am Dienstag, 10. Januar 2023, 11:31:49 CET schrieb Marek Vasut: >>>> On 1/10/23 11:00, Alexander Stein wrote: >>>> >>>> [...] >>>> >>>>> static int rs9_get_output_config(struct rs9_driver_data *rs9, int >>>>> idx) >>>>> { >>>>> >>>>> struct i2c_client *client = rs9->client; >>>>> >>>>> + u8 dif = rs9_calc_dif(rs9, idx); >>>>> >>>>> unsigned char name[5] = "DIF0"; >>>>> struct device_node *np; >>>>> int ret; >>>>> u32 sr; >>>>> >>>>> /* Set defaults */ >>>>> >>>>> - rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx); >>>> >>>> Are you sure this line ^ should be dropped ? >>>> Shouldn't the bitfield be cleared first and modified second? >>> >>> Well, I had in my mind that this function is called upon probe with >>> clk_dif_sr being cleared anyway, so this does essentially nothing. And >>> the DIF bit is set unconditionally, so what is the point of masking it >>> before? >> >> Good point, but then, what's the point of ORRing either ? Just do a >> plain assignment. > > OR-ring is necessary as this function is called for each DIF output (see idx > parameter), so plain assignment will clear the previously set bits. Ah, got it. Reviewed-by: Marek Vasut <marex@denx.de> Thanks !
diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c index bba09a88c2ccc..6b19186228238 100644 --- a/drivers/clk/clk-renesas-pcie.c +++ b/drivers/clk/clk-renesas-pcie.c @@ -18,7 +18,6 @@ #include <linux/regmap.h> #define RS9_REG_OE 0x0 -#define RS9_REG_OE_DIF_OE(n) BIT((n) + 1) #define RS9_REG_SS 0x1 #define RS9_REG_SS_AMP_0V6 0x0 #define RS9_REG_SS_AMP_0V7 0x1 @@ -31,9 +30,6 @@ #define RS9_REG_SS_SSC_MASK (3 << 3) #define RS9_REG_SS_SSC_LOCK BIT(5) #define RS9_REG_SR 0x2 -#define RS9_REG_SR_2V0_DIF(n) 0 -#define RS9_REG_SR_3V0_DIF(n) BIT((n) + 1) -#define RS9_REG_SR_DIF_MASK(n) BIT((n) + 1) #define RS9_REG_REF 0x3 #define RS9_REG_REF_OE BIT(4) #define RS9_REG_REF_OD BIT(5) @@ -160,17 +156,27 @@ static const struct regmap_config rs9_regmap_config = { .reg_read = rs9_regmap_i2c_read, }; +static u8 rs9_calc_dif(const struct rs9_driver_data *rs9, int idx) +{ + enum rs9_model model = rs9->chip_info->model; + + if (model == RENESAS_9FGV0241) + return BIT(idx) + 1; + + return 0; +} + static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx) { struct i2c_client *client = rs9->client; + u8 dif = rs9_calc_dif(rs9, idx); unsigned char name[5] = "DIF0"; struct device_node *np; int ret; u32 sr; /* Set defaults */ - rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx); - rs9->clk_dif_sr |= RS9_REG_SR_3V0_DIF(idx); + rs9->clk_dif_sr |= dif; snprintf(name, 5, "DIF%d", idx); np = of_get_child_by_name(client->dev.of_node, name); @@ -182,11 +188,9 @@ static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx) of_node_put(np); if (!ret) { if (sr == 2000000) { /* 2V/ns */ - rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx); - rs9->clk_dif_sr |= RS9_REG_SR_2V0_DIF(idx); + rs9->clk_dif_sr &= ~dif; } else if (sr == 3000000) { /* 3V/ns (default) */ - rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx); - rs9->clk_dif_sr |= RS9_REG_SR_3V0_DIF(idx); + rs9->clk_dif_sr |= dif; } else ret = dev_err_probe(&client->dev, -EINVAL, "Invalid renesas,slew-rate value\n"); @@ -257,11 +261,13 @@ static void rs9_update_config(struct rs9_driver_data *rs9) } for (i = 0; i < rs9->chip_info->num_clks; i++) { - if (rs9->clk_dif_sr & RS9_REG_SR_3V0_DIF(i)) + u8 dif = rs9_calc_dif(rs9, i); + + if (rs9->clk_dif_sr & dif) continue; - regmap_update_bits(rs9->regmap, RS9_REG_SR, RS9_REG_SR_3V0_DIF(i), - rs9->clk_dif_sr & RS9_REG_SR_3V0_DIF(i)); + regmap_update_bits(rs9->regmap, RS9_REG_SR, dif, + rs9->clk_dif_sr & dif); } }
The calculation DIFx is BIT(n) +1 is only true for 9FGV0241. With additional devices this is getting more complicated. Support a base bit for the DIF calculation, currently only devices with consecutive bits are supported, e.g. the 6-channel device needs additional logic. Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> --- Changes in v2: * Use a common function instead of callback for calculating the DIF bit drivers/clk/clk-renesas-pcie.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)