+Heiko Schocher who might know more about this I2C question
On Mon, 9 Nov 2020 at 15:09, Duffin, CooperX <cooperx.duf...@intel.com> wrote:
>
> -----Original Message-----
> From: Simon Glass <s...@chromium.org>
> Sent: Saturday, November 7, 2020 1:33 PM
> To: Duffin, CooperX <cooperx.duf...@intel.com>
> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; uboot-snps-...@synopsys.com;
> Tom Rini <tr...@konsulko.com>; Robert Beckett <bob.beck...@collabora.com>;
> Heiko Schocher <h...@denx.de>; Wolgang Denk <w...@denx.de>; Ian Ray
> <ian....@ge.com>
> Subject: Re: [dwi2c PATCH v1] dwi2c add offsets to reads
>
> Hi CooperX,
>
> On Fri, 6 Nov 2020 at 16:08, Duffin, CooperX <cooperx.duf...@intel.com> wrote:
> >
> > Hello Simon,
> >
> > I wasn’t using the test/dm/i2c I this was tested using hardware where I was
> > using the dm_i2c_read() function. I just tried to use the test/dm/i2c where
> > I am following the README but I keep getting "sdl2-config: Command not
> > found". I suspect it would also fail where it is trying to read but I will
> > have to get test/dm/i2c working before I know for sure.
> >
> > Essentially my test was
> >
> > int uboot_app (int argc, char * const argv[]) {
> > uint8_t buf[10];
> > uint8_t read_buf[5];
> > uint8_t dev_addr;
> > uint32_t bus_speed;
> > //i2c_bus device pointer
> > struct udevice *i2c_led;
> > struct udevice *bus;
> > app_startup(argv);
> > /* Print the ABI version */
> > printf ("Example expects ABI version %d\n", XF_VERSION);
> > printf ("Actual U-Boot ABI version %d\n", (int)get_version());
> >
> > dev_addr = 0x60;
> > bus_speed = 100000; //100KHz
> > printf("starting test i2c\n");
> > buf[0] = 'b';
> > buf[1] = 'o';
> > buf[2] = 'o';
> > buf[3] = 't';
> > //Get i2c chip, init the code
> > printf("init starting\n");
> > if(i2c_get_chip_for_busnum(0 , 0x60, 1, &i2c_led)!=0){
> > printf("ERROR: no device found at %x \n",dev_addr);
> > return (0);
> > }
> > uclass_get_device_by_seq(UCLASS_I2C, 0, &bus);
> > printf("Setting bus speed to %d\n", bus_speed);
> > if(dm_i2c_set_bus_speed(bus,bus_speed)!=0){
> > printf("ERROR: Cannot set buspeed\n");
> > return (0);
> > }
> > printf("i2c_led name is %s\n",i2c_led->name);
> > if(i2c_led == NULL){
> > printf("ERROR i2c_led 0 is null\n");
> > return (0);
> > }
> > printf("Writing\n");
> > for(unsigned int a =0; a< 4; a++){
> > if(dm_i2c_write(i2c_led,a,&buf[a],1)!=0){
> > printf("ERROR writing\n");
> > return (0);
> > }
> > }
> >
> > printf("Reading\n");
> > for(unsigned int a =0; a< 4; a++){
> > if(dm_i2c_read(i2c_led,a,&read_buf[a],1)!=0){
> > printf("ERROR writing\n");
> > return (0);
> > }
> > }
> > printf("read buffer is %c, %c, %c, %c \n",read_buf[0],read_buf[1],
> > read_buf[2],read_buf[3]);
> > printf ("\n\n");
> > return (0);
> > }
> >
> > ## Starting application at 0x0C100000 ...
> > Example expects ABI version 10
> > Actual U-Boot ABI version 10
> > starting test i2c
> > init starting
> > Setting bus speed to 100000
> > i2c_led name is generic_60
> > Writing
> > Reading
> > read buffer is o, o, o, o
> >
> > with the fix I get:
> >
> > ## Starting application at 0x0C100000 ...
> > Example expects ABI version 10
> > Actual U-Boot ABI version 10
> > starting test i2c
> > init starting
> > Setting bus speed to 100000
> > i2c_led name is generic_60
> > Writing
> > Reading
> > read buffer is b, o, o, t
> >
> > which is correct. Similarly if I stop auto boot I get a similar result:
> >
> > Hit any key to stop autoboot: 0
> > device # i2c
> > i2c - I2C sub-system
> >
> > Assuming I write "boot" to the device
> > device # i2c dev 0
> > Setting bus to 0
> > device # i2c md 0x60 0 4
> > 0000: 6f 6f 6f 6f oooo
> >
> > With the fix/patch I get:
> >
> > device# i2c md 0x60 0 4
> > 0000: 62 6f 6f 74 boot
> >
> > Which is correct. Also the reads were working in linux leading to my
> > original suspicion that there might be something going on in the driver.
> > Hopefully that answers your question let me know if you have anymore.
>
> I had a bit of a look at this. If you look at dm_i2c_read() it actually
> builds the address into the buffer it sends. So when it gets to
> __dw_i2c_write() the alen parameter is always 0. That function is actually a
> holdover from before driver model, so one day the alen and addr parameters
> will go away.
>
> I wonder if your device does not support multiple-byte reads or writes? Can
> you try the i2c md with a single byte? There is a DM_I2C_CHIP_WR_ADDRESS
> option to handle this - see the 'i2c flags'
> command.
>
> In the designware_i2c_xfer(), try printing out the bytes that it gets in each
> message using i2c_dump_msgs().
>
> Also check the i2c_get_chip() function which you are using. See if
> chip->offset_len is set to 1 as it should be, for your device. If not,
> perhaps something is missing.
>
> In short I am not really sure what is going on, but I think it needs more
> investigation.
>
> BTW with this mailing list we like people to reply inline or at the bottom,
> not at the top.
>
> Regards,
> Simon
>
> >
> > Best regards,
> >
> > -Cooper Duffin
> > -----Original Message-----
> > From: Simon Glass <s...@chromium.org>
> > Sent: Friday, November 6, 2020 10:51 AM
> > To: Duffin, CooperX <cooperx.duf...@intel.com>
> > Cc: U-Boot Mailing List <u-boot@lists.denx.de>;
> > uboot-snps-...@synopsys.com; Tom Rini <tr...@konsulko.com>; Robert
> > Beckett <bob.beck...@collabora.com>; Heiko Schocher <h...@denx.de>;
> > Wolgang Denk <w...@denx.de>; Ian Ray <ian....@ge.com>
> > Subject: Re: [dwi2c PATCH v1] dwi2c add offsets to reads
> >
> > Hi Cduffinx,
> >
> > On Thu, 5 Nov 2020 at 14:26, cduffinx <cooperx.duf...@intel.com> wrote:
> > >
> > > modify the designware_i2c_xfer function to use 1 byte per address,
> > > it was set to 0 before which makes it think its reading a register type.
> > > Added offset of where it is supposed to read from. Before it was
> > > always reading from offset 0 despite specifying the offset in the
> > > higher level function.
> > >
> > > Signed-off-by: Cooper Duffin <cooperx.duf...@intel.com>
> > > Signed-off-by: cduffinx <cooperx.duf...@intel.com>
> > > ---
> > >
> > > drivers/i2c/designware_i2c.c | 5 +++--
> > > drivers/i2c/i2c-uclass.c | 1 +
> > > include/i2c.h | 1 +
> > > 3 files changed, 5 insertions(+), 2 deletions(-)
> > >
> >
> > Thanks for the patch!
> >
> > Is there a test that was failing before (test/dm/i2c.c) or should we add a
> > new one to catch this?
> >
> > Regards,
> > Simon
>
> [Duffin, CooperX]
> Thanks for the reply Simon. I tried a couple more things.
>
> Simon Glass <s...@chromium.org> wrote:
> > I wonder if your device does not support multiple-byte reads or writes? Can
> > you try the i2c md with a single byte? There is a DM_I2C_CHIP_WR_ADDRESS
> > option to handle this - see the 'i2c flags'
> >command.
>
> I tried with the flag DM_I2C_CHIP_WR_ADDRESS but it did not make a
> difference. A single byte read will yield the following:
>
> Device # i2c md 0x60 0 1
> 0000: 6f o
>
> Which is not correct in fact it doesn’t seem to matter how much I read it
> only picks up the second character I wrote to it. For example if I read 16
> characters I get:
> device# i2c flags 0x60 4
> device # i2c md 0x60 0 10
> 0000: 6f 6f 6f 6f 6f 6f 6f 6f 6f 6f 6f 6f 6f 6f 6f 6f oooooooooooooooo
>
> I can tell the writes are working because I am writing to an LED display and
> I can see the proper characters get displayed. Also I think my i2c device
> needs restarts in-between each byte for it to work properly, so I need to be
> able to read individual bytes with there corresponding offsets, also
> i2c-tools supports this.
>
> Simon Glass <s...@chromium.org> wrote:
> >Also check the i2c_get_chip() function which you are using. See if
> >chip->offset_len is set to 1 as it should be, for your device. If not,
> >perhaps something is missing.
> If I use this command I get:
>
> device # i2c olen 0x60
> 1
>
> This is without the patch BTW.
