| Message ID | 20240924065020.2009975-1-quic_mmanikan@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/1] i2c: qcom-geni: add 32MHz I2C SE clock support for IPQ5424 | expand |
On Tue, Sep 24, 2024 at 12:20:20PM +0530, Manikanta Mylavarapu wrote: Subject gives a clear indication that this is specific to IPQ5424, which it isn't. So, please drop that wording from the subject. Perhaps: "i2c: qcom-geni: Support systems with 32MHz SE clock" > The IPQ5424 I2C SE clock operates at a frequency of 32MHz. Since the > existing map table is based on 19.2MHz, this patch incorporate the > clock map table to derive the SCL clock from the 32MHz SE clock. Then here you're doing the right thing of introducing the IPQ5424, so this looks good to me. > > Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com> > --- > drivers/i2c/busses/i2c-qcom-geni.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c > index 212336f724a6..bbd9ecf09f4b 100644 > --- a/drivers/i2c/busses/i2c-qcom-geni.c > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > @@ -71,6 +71,7 @@ enum geni_i2c_err_code { > > #define I2C_AUTO_SUSPEND_DELAY 250 > #define KHZ(freq) (1000 * freq) > +#define MHZ(freq) (1000000 * freq) > #define PACKING_BYTES_PW 4 > > #define ABORT_TIMEOUT HZ > @@ -152,11 +153,21 @@ static const struct geni_i2c_clk_fld geni_i2c_clk_map[] = { > {KHZ(1000), 1, 3, 9, 18}, > }; > > +/* source_clock = 32 MHz */ > +static const struct geni_i2c_clk_fld geni_i2c_clk_map_32M[] = { I'd prefer that you s/32M/32mhz/, and that you rename geni_i2c_clk_map to geni_i2c_clk_map_19p2mhz[]. > + {KHZ(100), 7, 14, 18, 40}, > + {KHZ(400), 4, 3, 11, 20}, > + {KHZ(1000), 4, 3, 6, 15}, > +}; > + > static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c) > { > int i; > const struct geni_i2c_clk_fld *itr = geni_i2c_clk_map; > > + if (clk_get_rate(gi2c->se.clk) == MHZ(32)) > + itr = geni_i2c_clk_map_32M; Leave itr uninitialized above and add an else here with the assignment, to make it clearer that it's one or the other case. (Compared to "It's always 19.2MHz and then in some cases we override that with 32MHz") PS. I wouldn't mind you dropping the addition of the MHZ macro and just compare clk_get_rate() with 32000000 and 19200000. But that's a matter of taste. Regards, Bjorn > + > for (i = 0; i < ARRAY_SIZE(geni_i2c_clk_map); i++, itr++) { > if (itr->clk_freq_out == gi2c->clk_freq_out) { > gi2c->clk_fld = itr; > -- > 2.34.1 >
Hello Manikanta, On 9/24/24 09:50, Manikanta Mylavarapu wrote: > The IPQ5424 I2C SE clock operates at a frequency of 32MHz. Since the > existing map table is based on 19.2MHz, this patch incorporate the > clock map table to derive the SCL clock from the 32MHz SE clock. > > Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com> > --- > drivers/i2c/busses/i2c-qcom-geni.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c > index 212336f724a6..bbd9ecf09f4b 100644 > --- a/drivers/i2c/busses/i2c-qcom-geni.c > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > @@ -71,6 +71,7 @@ enum geni_i2c_err_code { > > #define I2C_AUTO_SUSPEND_DELAY 250 > #define KHZ(freq) (1000 * freq) > +#define MHZ(freq) (1000000 * freq) pleae drop this new macro, please use freq * HZ_PER_MHZ instead of MHZ(freq). KHZ(freq) macro should be also replaced by HZ_PER_KHZ, but it's a separate change. -- Best wishes, Vladimir
On 9/25/2024 2:15 AM, Bjorn Andersson wrote: > On Tue, Sep 24, 2024 at 12:20:20PM +0530, Manikanta Mylavarapu wrote: > > Subject gives a clear indication that this is specific to IPQ5424, which > it isn't. So, please drop that wording from the subject. > Okay, sure. > Perhaps: > "i2c: qcom-geni: Support systems with 32MHz SE clock" > Okay, sure. >> The IPQ5424 I2C SE clock operates at a frequency of 32MHz. Since the >> existing map table is based on 19.2MHz, this patch incorporate the >> clock map table to derive the SCL clock from the 32MHz SE clock. > > Then here you're doing the right thing of introducing the IPQ5424, so > this looks good to me. > >> >> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com> >> --- >> drivers/i2c/busses/i2c-qcom-geni.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c >> index 212336f724a6..bbd9ecf09f4b 100644 >> --- a/drivers/i2c/busses/i2c-qcom-geni.c >> +++ b/drivers/i2c/busses/i2c-qcom-geni.c >> @@ -71,6 +71,7 @@ enum geni_i2c_err_code { >> >> #define I2C_AUTO_SUSPEND_DELAY 250 >> #define KHZ(freq) (1000 * freq) >> +#define MHZ(freq) (1000000 * freq) >> #define PACKING_BYTES_PW 4 >> >> #define ABORT_TIMEOUT HZ >> @@ -152,11 +153,21 @@ static const struct geni_i2c_clk_fld geni_i2c_clk_map[] = { >> {KHZ(1000), 1, 3, 9, 18}, >> }; >> >> +/* source_clock = 32 MHz */ >> +static const struct geni_i2c_clk_fld geni_i2c_clk_map_32M[] = { > > I'd prefer that you s/32M/32mhz/, and that you rename geni_i2c_clk_map > to geni_i2c_clk_map_19p2mhz[]. > Okay, sure. >> + {KHZ(100), 7, 14, 18, 40}, >> + {KHZ(400), 4, 3, 11, 20}, >> + {KHZ(1000), 4, 3, 6, 15}, >> +}; >> + >> static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c) >> { >> int i; >> const struct geni_i2c_clk_fld *itr = geni_i2c_clk_map; >> >> + if (clk_get_rate(gi2c->se.clk) == MHZ(32)) >> + itr = geni_i2c_clk_map_32M; > > Leave itr uninitialized above and add an else here with the assignment, > to make it clearer that it's one or the other case. (Compared to "It's > always 19.2MHz and then in some cases we override that with 32MHz") > > Okay, sure. > PS. I wouldn't mind you dropping the addition of the MHZ macro and just > compare clk_get_rate() with 32000000 and 19200000. But that's a matter > of taste. > Okay, sure. I will drop MHZ macro. Thanks & Regards, Manikanta. > Regards, > Bjorn > >> + >> for (i = 0; i < ARRAY_SIZE(geni_i2c_clk_map); i++, itr++) { >> if (itr->clk_freq_out == gi2c->clk_freq_out) { >> gi2c->clk_fld = itr; >> -- >> 2.34.1 >>
On 9/25/2024 11:09 AM, Vladimir Zapolskiy wrote: > Hello Manikanta, > > On 9/24/24 09:50, Manikanta Mylavarapu wrote: >> The IPQ5424 I2C SE clock operates at a frequency of 32MHz. Since the >> existing map table is based on 19.2MHz, this patch incorporate the >> clock map table to derive the SCL clock from the 32MHz SE clock. >> >> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com> >> --- >> drivers/i2c/busses/i2c-qcom-geni.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c >> index 212336f724a6..bbd9ecf09f4b 100644 >> --- a/drivers/i2c/busses/i2c-qcom-geni.c >> +++ b/drivers/i2c/busses/i2c-qcom-geni.c >> @@ -71,6 +71,7 @@ enum geni_i2c_err_code { >> #define I2C_AUTO_SUSPEND_DELAY 250 >> #define KHZ(freq) (1000 * freq) >> +#define MHZ(freq) (1000000 * freq) > > pleae drop this new macro, please use freq * HZ_PER_MHZ instead of MHZ(freq). > > KHZ(freq) macro should be also replaced by HZ_PER_KHZ, but it's a separate change. > Okay, sure. Thanks & Regards, Manikanta. > -- > Best wishes, > Vladimir
Hi Manikanta, On 9/24/2024 12:20 PM, Manikanta Mylavarapu wrote: > The IPQ5424 I2C SE clock operates at a frequency of 32MHz. Since the would it be better to say , I2C SE is sourced from 32MHZ ? > existing map table is based on 19.2MHz, this patch incorporate the based on 19.2MHz. this patch /,/. > clock map table to derive the SCL clock from the 32MHz SE clock. from the 32MHz Source Clock frequency. SE = Expand OR (I2C Serial Engine Controller) > > Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com> > --- > drivers/i2c/busses/i2c-qcom-geni.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c > index 212336f724a6..bbd9ecf09f4b 100644 > --- a/drivers/i2c/busses/i2c-qcom-geni.c > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > @@ -71,6 +71,7 @@ enum geni_i2c_err_code { > > #define I2C_AUTO_SUSPEND_DELAY 250 > #define KHZ(freq) (1000 * freq) > +#define MHZ(freq) (1000000 * freq) > #define PACKING_BYTES_PW 4 > > #define ABORT_TIMEOUT HZ > @@ -152,11 +153,21 @@ static const struct geni_i2c_clk_fld geni_i2c_clk_map[] = { A thought - Should we rename this appending _19.2M ? In future one or more may come as it evolves speed. > {KHZ(1000), 1, 3, 9, 18}, > }; > > +/* source_clock = 32 MHz */ > +static const struct geni_i2c_clk_fld geni_i2c_clk_map_32M[] = { > + {KHZ(100), 7, 14, 18, 40}, > + {KHZ(400), 4, 3, 11, 20}, > + {KHZ(1000), 4, 3, 6, 15}, > +}; > + > static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c) > { > int i; > const struct geni_i2c_clk_fld *itr = geni_i2c_clk_map; > > + if (clk_get_rate(gi2c->se.clk) == MHZ(32)) > + itr = geni_i2c_clk_map_32M; > + > for (i = 0; i < ARRAY_SIZE(geni_i2c_clk_map); i++, itr++) { > if (itr->clk_freq_out == gi2c->clk_freq_out) { > gi2c->clk_fld = itr;
On 9/25/2024 1:06 PM, Mukesh Kumar Savaliya wrote: > Hi Manikanta, > > On 9/24/2024 12:20 PM, Manikanta Mylavarapu wrote: >> The IPQ5424 I2C SE clock operates at a frequency of 32MHz. Since the > would it be better to say , I2C SE is sourced from 32MHZ ? Okay, sure. >> existing map table is based on 19.2MHz, this patch incorporate the > based on 19.2MHz. this patch /,/. Okay, sure. >> clock map table to derive the SCL clock from the 32MHz SE clock. > from the 32MHz Source Clock frequency. > SE = Expand OR (I2C Serial Engine Controller) Okay, sure. >> >> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com> >> --- >> drivers/i2c/busses/i2c-qcom-geni.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c >> index 212336f724a6..bbd9ecf09f4b 100644 >> --- a/drivers/i2c/busses/i2c-qcom-geni.c >> +++ b/drivers/i2c/busses/i2c-qcom-geni.c >> @@ -71,6 +71,7 @@ enum geni_i2c_err_code { >> #define I2C_AUTO_SUSPEND_DELAY 250 >> #define KHZ(freq) (1000 * freq) >> +#define MHZ(freq) (1000000 * freq) >> #define PACKING_BYTES_PW 4 >> #define ABORT_TIMEOUT HZ >> @@ -152,11 +153,21 @@ static const struct geni_i2c_clk_fld geni_i2c_clk_map[] = { > A thought - Should we rename this appending _19.2M ? In future one or more may come as it evolves speed. Okay, sure. Thanks & Regards, Manikanta. >> {KHZ(1000), 1, 3, 9, 18}, >> }; >> +/* source_clock = 32 MHz */ >> +static const struct geni_i2c_clk_fld geni_i2c_clk_map_32M[] = { >> + {KHZ(100), 7, 14, 18, 40}, >> + {KHZ(400), 4, 3, 11, 20}, >> + {KHZ(1000), 4, 3, 6, 15}, >> +}; >> + >> static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c) >> { >> int i; >> const struct geni_i2c_clk_fld *itr = geni_i2c_clk_map; >> + if (clk_get_rate(gi2c->se.clk) == MHZ(32)) >> + itr = geni_i2c_clk_map_32M; >> + >> for (i = 0; i < ARRAY_SIZE(geni_i2c_clk_map); i++, itr++) { >> if (itr->clk_freq_out == gi2c->clk_freq_out) { >> gi2c->clk_fld = itr;
Thanks manikanta ! On 9/25/2024 2:14 PM, Manikanta Mylavarapu wrote: > > > On 9/25/2024 1:06 PM, Mukesh Kumar Savaliya wrote: >> Hi Manikanta, >> >> On 9/24/2024 12:20 PM, Manikanta Mylavarapu wrote: >>> The IPQ5424 I2C SE clock operates at a frequency of 32MHz. Since the >> would it be better to say , I2C SE is sourced from 32MHZ ? > > Okay, sure. > >>> existing map table is based on 19.2MHz, this patch incorporate the >> based on 19.2MHz. this patch /,/. > > Okay, sure. > >>> clock map table to derive the SCL clock from the 32MHz SE clock. >> from the 32MHz Source Clock frequency. >> SE = Expand OR (I2C Serial Engine Controller) > > Okay, sure. > Please also mention the reason why 32MHz got added ? Need to describe evolution to understand the purpose behind this change. >>> >>> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com> >>> --- >>> drivers/i2c/busses/i2c-qcom-geni.c | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c >>> index 212336f724a6..bbd9ecf09f4b 100644 >>> --- a/drivers/i2c/busses/i2c-qcom-geni.c >>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c >>> @@ -71,6 +71,7 @@ enum geni_i2c_err_code { >>> #define I2C_AUTO_SUSPEND_DELAY 250 >>> #define KHZ(freq) (1000 * freq) >>> +#define MHZ(freq) (1000000 * freq) >>> #define PACKING_BYTES_PW 4 >>> #define ABORT_TIMEOUT HZ >>> @@ -152,11 +153,21 @@ static const struct geni_i2c_clk_fld geni_i2c_clk_map[] = { >> A thought - Should we rename this appending _19.2M ? In future one or more may come as it evolves speed. > > Okay, sure. > > Thanks & Regards, > Manikanta. > >>> {KHZ(1000), 1, 3, 9, 18}, >>> }; >>> +/* source_clock = 32 MHz */ >>> +static const struct geni_i2c_clk_fld geni_i2c_clk_map_32M[] = { >>> + {KHZ(100), 7, 14, 18, 40}, >>> + {KHZ(400), 4, 3, 11, 20}, >>> + {KHZ(1000), 4, 3, 6, 15}, >>> +}; >>> + >>> static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c) >>> { >>> int i; >>> const struct geni_i2c_clk_fld *itr = geni_i2c_clk_map; >>> + if (clk_get_rate(gi2c->se.clk) == MHZ(32)) >>> + itr = geni_i2c_clk_map_32M; >>> + >>> for (i = 0; i < ARRAY_SIZE(geni_i2c_clk_map); i++, itr++) { >>> if (itr->clk_freq_out == gi2c->clk_freq_out) { >>> gi2c->clk_fld = itr; >
On 9/25/2024 3:25 PM, Mukesh Kumar Savaliya wrote: > Thanks manikanta ! > > On 9/25/2024 2:14 PM, Manikanta Mylavarapu wrote: >> >> >> On 9/25/2024 1:06 PM, Mukesh Kumar Savaliya wrote: >>> Hi Manikanta, >>> >>> On 9/24/2024 12:20 PM, Manikanta Mylavarapu wrote: >>>> The IPQ5424 I2C SE clock operates at a frequency of 32MHz. Since the >>> would it be better to say , I2C SE is sourced from 32MHZ ? >> >> Okay, sure. >> >>>> existing map table is based on 19.2MHz, this patch incorporate the >>> based on 19.2MHz. this patch /,/. >> >> Okay, sure. >> >>>> clock map table to derive the SCL clock from the 32MHz SE clock. >>> from the 32MHz Source Clock frequency. >>> SE = Expand OR (I2C Serial Engine Controller) >> >> Okay, sure. >> > Please also mention the reason why 32MHz got added ? Need to describe evolution to understand the purpose behind this change. Okay, sure. Thanks & Regards, Manikanta.
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c index 212336f724a6..bbd9ecf09f4b 100644 --- a/drivers/i2c/busses/i2c-qcom-geni.c +++ b/drivers/i2c/busses/i2c-qcom-geni.c @@ -71,6 +71,7 @@ enum geni_i2c_err_code { #define I2C_AUTO_SUSPEND_DELAY 250 #define KHZ(freq) (1000 * freq) +#define MHZ(freq) (1000000 * freq) #define PACKING_BYTES_PW 4 #define ABORT_TIMEOUT HZ @@ -152,11 +153,21 @@ static const struct geni_i2c_clk_fld geni_i2c_clk_map[] = { {KHZ(1000), 1, 3, 9, 18}, }; +/* source_clock = 32 MHz */ +static const struct geni_i2c_clk_fld geni_i2c_clk_map_32M[] = { + {KHZ(100), 7, 14, 18, 40}, + {KHZ(400), 4, 3, 11, 20}, + {KHZ(1000), 4, 3, 6, 15}, +}; + static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c) { int i; const struct geni_i2c_clk_fld *itr = geni_i2c_clk_map; + if (clk_get_rate(gi2c->se.clk) == MHZ(32)) + itr = geni_i2c_clk_map_32M; + for (i = 0; i < ARRAY_SIZE(geni_i2c_clk_map); i++, itr++) { if (itr->clk_freq_out == gi2c->clk_freq_out) { gi2c->clk_fld = itr;
The IPQ5424 I2C SE clock operates at a frequency of 32MHz. Since the existing map table is based on 19.2MHz, this patch incorporate the clock map table to derive the SCL clock from the 32MHz SE clock. Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com> --- drivers/i2c/busses/i2c-qcom-geni.c | 11 +++++++++++ 1 file changed, 11 insertions(+)