Re: [PATCH v2 1/1] i2c: qcom-geni: Support systems with 32MHz serial engine clock

[Vladimir Zapolskiy]

On 9/30/24 17:45, Manikanta Mylavarapu wrote:
> On 9/29/2024 12:06 PM, Manikanta Mylavarapu wrote:
> > On 9/26/2024 3:58 PM, Vladimir Zapolskiy wrote:
> > > Hello Manikanta.
> > >
> > > On 9/26/24 06:43, Manikanta Mylavarapu wrote:
> > > > In existing socs, I2C serial engine is sourced from XO (19.2MHz).
> > > > Where as in IPQ5424, I2C serial engine is sourced from GPLL0 (32MHz).
> > > >
> > > > The existing map table is based on 19.2MHz. This patch incorporate
> > > > the clock map table to derive the SCL clock from the 32MHz source
> > > > clock frequency.
> > > >
> > > > Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@xxxxxxxxxxx>
> > > > ---
> > > > Changes in v2:
> > > >      - Dropped IPQ5424 from the commit title
> > > >      - Added else part to assign geni_i2c_clk_map_19p2mhz to itr
> > > >      - Dropped MHZ macro and used HZ_PER_MHZ macro
> > > >      - Expanded SE to serial engine
> > > >      - Added the reason for 32MHz clock in commit message
> > > >
> > > >    drivers/i2c/busses/i2c-qcom-geni.c | 19 ++++++++++++++++---
> > > >    1 file changed, 16 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> > > > index 212336f724a6..22f2a0d83641 100644
> > > > --- a/drivers/i2c/busses/i2c-qcom-geni.c
> > > > +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> > > > @@ -16,6 +16,7 @@
> > > >    #include <linux/pm_runtime.h>
> > > >    #include <linux/soc/qcom/geni-se.h>
> > > >    #include <linux/spinlock.h>
> > > > +#include <linux/units.h>
> > > >      #define SE_I2C_TX_TRANS_LEN        0x26c
> > > >    #define SE_I2C_RX_TRANS_LEN        0x270
> > > > @@ -146,18 +147,30 @@ struct geni_i2c_clk_fld {
> > > >     * clk_freq_out = t / t_cycle
> > > >     * source_clock = 19.2 MHz
> > > >     */
> > > > -static const struct geni_i2c_clk_fld geni_i2c_clk_map[] = {
> > > > +static const struct geni_i2c_clk_fld geni_i2c_clk_map_19p2mhz[] = {
> > > >        {KHZ(100), 7, 10, 11, 26},
> > > >        {KHZ(400), 2,  5, 12, 24},
> > > >        {KHZ(1000), 1, 3,  9, 18},
> > > >    };
> > > >    +/* source_clock = 32 MHz */
> > > > +static const struct geni_i2c_clk_fld geni_i2c_clk_map_32mhz[] = {
> > > > +    {KHZ(100), 7, 14, 18, 40},
> > > > +    {KHZ(400), 4,  3, 11, 20},
> > > > +    {KHZ(1000), 4, 3,  6, 15},
> > > > +};
> > >
> > > Please double check the values.
> > >
> > > This is what I get:
> > > * for 100KHz: 32000000 / (40 * 7) ~ 114286, apparently 32000000 / (40 * 8) would
> > > be a better fit, however it's unclear what would be proper t_high / t_low values,
> > > * for 400KHz: it seems good,
> > > * for 1000KHz: 32000000 / (15 * 4) ~ 533333, which is almost 1/2 of the wanted
> > > bus frequency, so this one looks very wrong.
> > >
> > > Do you have any ideas how to get better bus frequency settings?
> >
> > Thanks for pointing this out.
> >
> > I will double check and get back with the proper data.
> >
> > Thanks & Regards,
> > Manikanta.
>
> Based on Qualcomm's internal hardware programming guide, below values need to be used for 100K & 1000K.
> {KHZ(100), 8, 14, 18, 40}
> {KHZ(1000), 2, 3,  6, 15}
>
> I will update these values in next version.

Great! These values make much more sence, thank you.

--
Best wishes,
Vladimir