[U-Boot] [PATCH 1/4] i2c: UniPhier: add driver for UniPhier i2c controller

[Simon Glass]

Hi Masahiro,

On 25 December 2014 at 00:15, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
> Simon, Heiko,
>
>
> If you have no objection to this series,
> I will apply it to u-boot-uniphier.
>
> Is this OK?
>
>
> Some replies below to Simon's comment.
>
>
>
> On Tue, 23 Dec 2014 12:54:53 -0700
> Simon Glass <sjg at chromium.org> wrote:
>
>
>> > +
>> > +#define IOBUS_FREQ     100000000
>> > +
>> > +struct uniphier_i2c_dev {
>> > +       void __iomem *base;             /* register base */
>>
>> U-Boot normally uses a struct for register access.
>
>
> IMHO, we can play it by ear.
>
> I notice two disadvantages of struct strategy.
>
> [1] we have to intert dummy registers where no register exist.
>
> For example, there is no register at offset address 0x8
> of fi2c controller.
>
> struct uniphier_fi2c_regs {
>      u32  cr;
>      u32  dttx;
>      u32  __dummy;   <-- necessary
>      u32  slad;

That seems like a minor problem to me. A better term might be 'reserved'.

>         ...
>
> [2] It is difficult to describe alias register name for the same address.
>
> For example,
>
> #define I2C_DTTX        0x04            /* send FIFO */
> #define I2C_DTRX        0x04            /* receive FIFO */
>
> DTTX is write-only register.
> DTRX is read-only register.
>
> They are assigned to the same address.

You could use fifo or rx_tx or data. Of even a #define if you can't avoid it.

There has been quite a bit of discussion about this U-Boot design
decision. I think Albert is the main expert.

>
>
>
>
>
>
>> > +       unsigned long input_clk;        /* master clock (Hz) */
>> > +       unsigned long wait_us;          /* wait for every byte transfer (us) */
>> > +};
>> > +
>> > +static int uniphier_i2c_probe(struct udevice *dev)
>> > +{
>> > +       fdt_addr_t addr;
>> > +       fdt_size_t size;
>> > +       struct uniphier_i2c_dev *priv = dev_get_priv(dev);
>> > +
>> > +       addr = fdtdec_get_addr_size(gd->fdt_blob, dev->of_offset, "reg", &size);
>> > +
>> > +       priv->base = map_sysmem(addr, size);
>> > +
>> > +       if (!priv->base)
>> > +               return -ENOMEM;
>> > +
>> > +       priv->input_clk = IOBUS_FREQ;
>> > +
>> > +       /* deassert reset */
>> > +       writel(0x3, priv->base + I2C_BRST);
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int uniphier_i2c_remove(struct udevice *dev)
>> > +{
>> > +       struct uniphier_i2c_dev *priv = dev_get_priv(dev);
>> > +
>> > +       unmap_sysmem(priv->base);
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int uniphier_i2c_child_pre_probe(struct udevice *dev)
>> > +{
>> > +       struct dm_i2c_chip *i2c_chip = dev_get_parentdata(dev);
>> > +
>> > +       if (dev->of_offset == -1)
>> > +               return 0;
>> > +       return i2c_chip_ofdata_to_platdata(gd->fdt_blob, dev->of_offset,
>> > +                                          i2c_chip);
>> > +}
>> > +
>> > +static int send_and_recv_byte(struct uniphier_i2c_dev *dev, u32 dtrm)
>> > +{
>> > +       writel(dtrm, dev->base + I2C_DTRM);
>> > +
>> > +       /*
>> > +        * U-Boot does not have a good support of interrupt.
>> > +        * Wait for a while.
>> > +        */
>> > +       udelay(dev->wait_us);
>>
>> It might be possible to check a 'ready' bit in the hardware, but I
>> suppose this works and is simple for what sounds like old hardware.
>
>
> Unfortunately, there is no 'ready' bit in this I2C controller.
> This hardware expects intrruption is checked.
>
> It is true that we can check the GIC register of ARM processor,
> but I think it is too much for this driver.

OK.

>
>
>> > +static int uniphier_i2c_set_bus_speed(struct udevice *bus, unsigned int speed)
>> > +{
>> > +       struct uniphier_i2c_dev *priv = dev_get_priv(bus);
>> > +
>> > +       /* max supported frequency is 400 kHz */
>> > +       if (speed > 400000)
>> > +               return -EINVAL;
>> > +
>> > +       /* bus reset: make sure the bus is idle when change the freqency */
>> > +       writel(0x1, priv->base + I2C_BRST);
>> > +
>> > +       writel((priv->input_clk / speed / 2 << 16) | (priv->input_clk / speed),
>> > +              priv->base + I2C_CLK);
>> > +
>> > +       writel(0x3, priv->base + I2C_BRST);
>> > +
>> > +       priv->wait_us = 20000000 / speed;
>>
>> How is that calculated? Perhaps have a comment?
>
> OK, I will add some comments.
> No precise theory for this line.  Rough calculation.

OK.

Regards,
Simon