Re: [PATCH 2/2] pinctrl: Introduce TI IOdelay configuration driver

[Nishanth Menon <nm@xxxxxx>]

On Tue, Mar 10, 2015 at 6:03 AM, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> On Wed, Mar 4, 2015 at 1:00 AM, Nishanth Menon <nm@xxxxxx> wrote:
>
>> SoC family such as DRA7 family of processors have, in addition
>> to the regular muxing of pins (as done by pinctrl-single), an
>> additional hardware module called IODelay which is also expected to be
>> configured. This "IODelay" module has it's own register space that is
>> independent of the control module.
>>
>> It is advocated strongly in TI's official documentation considering
>> the existing design of the DRA7 family of processors during mux or
>> IODelay reconfiguration, there is a potential for a significant glitch
>> which may cause functional impairment to certain hardware. It is
>> hence recommended to do as little of muxing as absolutely necessary
>> without I/O isolation (which can only be done in initial stages of
>> bootloader).
>>
>> NOTE: with the system wide I/O isolation scheme present in DRA7 SoC
>> family, it is not reasonable to do stop all I/O operations for every
>> such pad configuration scheme. So, we will let it glitch when used in
>> this mode.
>>
>> Even with the above limitation, certain functionality such as MMC has
>> mandatory need for IODelay reconfiguration requirements, depending on
>> speed of transfer. In these cases, with careful examination of usecase
>> involved, the expected glitch can be controlled such that it does not
>> impact functionality.
>>
>> In short, IODelay module support as a padconf driver being introduced
>> here is not expected to do SoC wide I/O Isolation and is meant for
>> a limited subset of IODelay configuration requirements that need to
>> be dynamic and whose glitchy behavior will not cause functionality
>> failure for that interface.
>>
>> Signed-off-by: Nishanth Menon <nm@xxxxxx>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@xxxxxx>
>
> Pending the conclusion of my remark on the bindings that this
> should use the generic pin config bindings... here are some
> "normal" review comments.
>
>> +config PINCTRL_TI_IODELAY
>> +       bool "TI IODelay Module pinconf driver"
>> +       depends on OF
>> +       select PINCONF
>> +       select GENERIC_PINCONF
>> +       select REGMAP_MMIO
>> +       help
>> +         Say Y here to support Texas Instruments' IODelay pinconf driver.
>> +         IODelay module is used for the DRA7 SoC family. This driver is in
>> +         addition to PINCTRL_SINGLE which controls the mux.
>
> If the driver is "in addition to PINCTRL_SINGLE", then it
> should depend on it.
>
>> +/* Should I change this? Abuse? */
>> +#define IODELAY_MUX_PINS_NAME          "pinctrl-single,pins"
>
> Yeah this is abuse and is why you should use the generic
> bindings with just "pins" instead, and use the generic pinconf
> parameters to set them up.
>
>> + * struct ti_iodelay_conf_vals - Description of each configuration parameters.
>> + * @offset:    Configuration register offset
>> + * @a_delay:   Agnostic Delay (in ps)
>> + * @g_delay:   Gnostic Delay (in ps)
>> + */
>> +struct ti_iodelay_conf_vals {
>> +       u16 offset;
>> +       u16 a_delay;
>> +       u16 g_delay;
>> +};
>
> So I don't think it's a good idea to get the register offset from
> the device tree. The driver should know these.
>
>> +/**
>> + * struct ti_iodelay_reg_values - Computed io_reg configuration values (see TRM)
>> + * @coarse_ref_count:  Coarse reference count
>> + * @coarse_delay_count:        Coarse delay count
>> + * @fine_ref_count:    Fine reference count
>> + * @fine_delay_count:  Fine Delay count
>> + * @ref_clk_period:    Reference Clock period
>> + * @cdpe:              Coarse delay parameter
>> + * @fdpe:              Fine delay parameter
>> + */
>> +struct ti_iodelay_reg_values {
>> +       u16 coarse_ref_count;
>
> If you're doing reference counting, use kref.
> <linux/kref.h>
>
>> +       u16 coarse_delay_count;
>> +
>> +       u16 fine_ref_count;
>
> Dito.
>
>> +       u16 fine_delay_count;
>> +
>> +       u16 ref_clk_period;
>> +
>> +       u32 cdpe;
>> +       u32 fdpe;
>> +};
>
>
>> +/**
>> + * struct ti_iodelay_pin_name - name of the pins
>> + * @name: name
>> + */
>> +struct ti_iodelay_pin_name {
>> +       char name[IODELAY_REG_NAME_LEN];
>> +};
>
> What is this? The pin control subsystem already has a struct
> for naming pins. struct pinctrl_pin_desc.
>
> Are you reimplementing core functionality?
>
> The generic pin config reference pins by name, and since you
> obviously like to encode these into you data you can just
> as well use that method for this too.
>
> Again I think this is a side effect from using the pinctrl-single
> concepts, if you think of this as some other pinctrl driver
> I think this will fall out nicely.
>
>> +/**
>> + * struct ti_iodelay_device - Represents information for a IOdelay instance
>> + * @dev: device pointer
>> + * @reg_base: Remapped virtual address
>> + * @regmap: Regmap for this IOdelay instance
>> + * @pctl: Pinctrl device
>> + * @desc: pinctrl descriptor for pctl
>> + * @pa: pinctrl pin wise description
>> + * @names: names of the pins
>> + * @groups: list of pinconf groups for iodelay instance
>> + * @ngroups: number of groups in the list
>> + * @mutex: mutex to protect group list modification
>> + * @reg_data: Register definition data for the IODelay instance
>> + * @reg_init_conf_values: Initial configuration values.
>> + */
>> +struct ti_iodelay_device {
>> +       struct device *dev;
>> +       void __iomem *reg_base;
>> +       struct regmap *regmap;
>> +
>> +       struct pinctrl_dev *pctl;
>> +       struct pinctrl_desc desc;
>> +       struct pinctrl_pin_desc *pa;
>> +       struct ti_iodelay_pin_name *names;
>
> Unhappy about that. pinctrl_desc contains
> struct pinctrl_pin_desc const *pins; !
>
>> +
>> +       struct list_head groups;
>> +       int ngroups;
>> +       struct mutex mutex; /* list protection */
>
> Why is this needed? Usually the pin control core
> serialize calls into the drivers. Care to explain? Have
> the potential race really triggered?
>
>> +static inline u32 ti_iodelay_compute_dpe(u16 period, u16 ref, u16 delay,
>> +                                        u16 delay_m)
>> +{
>> +       u64 m, d;
>> +
>> +       /* Handle overflow conditions */
>> +       m = 10 * (u64)period * (u64)ref;
>> +       d = 2 * (u64)delay * (u64)delay_m;
>> +
>> +       /* Truncate result back to 32 bits */
>> +       return div64_u64(m, d);
>> +}
>
> Serious business ...
>
>> +/**
>> + * ti_iodelay_pinconf_set() - Configure the pin configuration
>> + * @iod:       IODelay device
>> + * @val:       Configuration value
>> + *
>> + * Update the configuration register as per TRM and lockup once done.
>> + * *IMPORTANT NOTE* SoC TRM does recommend doing iodelay programmation only
>> + * while in Isolation. But, then, isolation also implies that every pin
>> + * on the SoC(including DDR) will be isolated out. The only benefit being
>> + * a glitchless configuration, However, the intent of this driver is purely
>> + * to support a "glitchy" configuration where applicable.
>> + *
>> + * Return: 0 in case of success, else appropriate error value
>> + */
>> +static int ti_iodelay_pinconf_set(struct ti_iodelay_device *iod,
>> +                                 struct ti_iodelay_conf_vals *val)
>> +{
>> +       const struct ti_iodelay_reg_data *reg = iod->reg_data;
>> +       struct ti_iodelay_reg_values *ival = &iod->reg_init_conf_values;
>> +       struct device *dev = iod->dev;
>> +       u32 g_delay_coarse, g_delay_fine;
>> +       u32 a_delay_coarse, a_delay_fine;
>> +       u32 c_elements, f_elements;
>> +       u32 total_delay;
>> +       u32 reg_mask, reg_val, tmp_val;
>> +       int r;
>> +
>> +       /* NOTE: Truncation is expected in all division below */
>> +       g_delay_coarse = val->g_delay / 920;
>> +       g_delay_fine = ((val->g_delay % 920) * 10) / 60;
>> +
>> +       a_delay_coarse = val->a_delay / ival->cdpe;
>> +       a_delay_fine = ((val->a_delay % ival->cdpe) * 10) / ival->fdpe;
>> +
>> +       c_elements = g_delay_coarse + a_delay_coarse;
>> +       f_elements = (g_delay_fine + a_delay_fine) / 10;
>> +
>> +       if (f_elements > 22) {
>> +               total_delay = c_elements * ival->cdpe + f_elements * ival->fdpe;
>> +               c_elements = total_delay / ival->cdpe;
>> +               f_elements = (total_delay % ival->cdpe) / ival->fdpe;
>> +       }
>> +
>> +       reg_mask = reg->signature_mask;
>> +       reg_val = reg->signature_value << __ffs(reg->signature_mask);
>> +
>> +       reg_mask |= reg->binary_data_coarse_mask;
>> +       tmp_val = c_elements << __ffs(reg->binary_data_coarse_mask);
>> +       if (tmp_val & ~reg->binary_data_coarse_mask) {
>> +               dev_err(dev, "Masking overflow of coarse elements %08x\n",
>> +                       tmp_val);
>> +               tmp_val &= reg->binary_data_coarse_mask;
>> +       }
>> +       reg_val |= tmp_val;
>> +
>> +       reg_mask |= reg->binary_data_fine_mask;
>> +       tmp_val = f_elements << __ffs(reg->binary_data_fine_mask);
>> +       if (tmp_val & ~reg->binary_data_fine_mask) {
>> +               dev_err(dev, "Masking overflow of fine elements %08x\n",
>> +                       tmp_val);
>> +               tmp_val &= reg->binary_data_fine_mask;
>> +       }
>> +       reg_val |= tmp_val;
>> +
>> +       /* Write with lock value - we DONOT want h/w updates */
>> +       reg_mask |= reg->lock_mask;
>> +       reg_val |= reg->lock_val << __ffs(reg->lock_mask);
>> +       r = regmap_update_bits(iod->regmap, val->offset, reg_mask, reg_val);
>> +
>> +       dev_dbg(dev, "Set reg 0x%x Delay(a=%d g=%d), Elements(C=%d F=%d)0x%x\n",
>> +               val->offset, val->a_delay, val->g_delay, c_elements,
>> +               f_elements, reg_val);
>> +
>> +       return r;
>> +}
>
> Looking at this terse thing I think it's obviously easier to use
> a more verbose device tree description with
>
> drive-delay = <some SI unit>;
>
>
>> +       /* unlock the IOdelay region */
>> +       r = regmap_update_bits(iod->regmap, reg->reg_global_lock_offset,
>> +                              reg->global_lock_mask, reg->global_unlock_val);
>> +       if (r)
>> +               return r;
>> +
>> +       /* Read up Recalibration sequence done by bootloader */
>> +       r = regmap_read(iod->regmap, reg->reg_refclk_offset, &val);
>> +       if (r)
>> +               return r;
>> +       ival->ref_clk_period = ti_iodelay_extract(val, reg->refclk_period_mask);
>> +       dev_dbg(dev, "refclk_period=0x%04x\n", ival->ref_clk_period);
>
> That sounds hightec. So the bootloader calibrates the delays
> and  computes the refclock period? So is that sime PLL or something?
>
>> +       r = regmap_read(iod->regmap, reg->reg_coarse_offset, &val);
>> +       if (r)
>> +               return r;
>> +       ival->coarse_ref_count =
>> +           ti_iodelay_extract(val, reg->coarse_ref_count_mask);
>> +       ival->coarse_delay_count =
>> +           ti_iodelay_extract(val, reg->coarse_delay_count_mask);
>> +       if (!ival->coarse_delay_count) {
>> +               dev_err(dev, "Invalid Coarse delay count (0) (reg=0x%08x)\n",
>> +                       val);
>> +               return -EINVAL;
>> +       }
>> +       ival->cdpe = ti_iodelay_compute_dpe(ival->ref_clk_period,
>> +                                           ival->coarse_ref_count,
>> +                                           ival->coarse_delay_count, 88);
>
> 88? What does that mean... This should be #defined I think.
>
>> +static int ti_iodelay_dt_node_to_map(struct pinctrl_dev *pctldev,
>> +                                    struct device_node *np,
>> +                                    struct pinctrl_map **map,
>> +                                    unsigned *num_maps)
>
> If you use the generic bindings this quite complex parsing and
> everything associated with it goes away into the core.
>
>> +static void ti_iodelay_dt_free_map(struct pinctrl_dev *pctldev,
>> +                                  struct pinctrl_map *map, unsigned num_maps)
>
> Dito.
>
> BTW even if you proceed this way I suspect these are verbatim
> copies from pinctrl-single so they shold obviouslyt be abstracted
> out as library functions in that case.
>
>> +/**
>> + * ti_iodelay_pinctrl_get_group_pins() - get group pins
>> + * @pctldev:   pinctrl device representing IODelay device
>> + * @gselector: Group selector
>> + * @pins:      pointer to the pins
>> + * @npins:     number of pins
>> + *
>> + * Dummy implementation since we do not track pins, we track configurations
>> + * Forced by pinctrl's pinctrl_check_ops()
>> + *
>> + * Return: -EINVAL
>> + */
>> +static int ti_iodelay_pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
>> +                                            unsigned gselector,
>> +                                            const unsigned **pins,
>> +                                            unsigned *npins)
>> +{
>> +       /* Dummy implementation - we dont do pin mux */
>> +       return -EINVAL;
>> +}
>
> This is not about muxing. It is about enumerating the pins in the
> group that will be affected by the setting, which is something you
> will want to do when using the generic bindings.
>
>> +       group = ti_iodelay_get_group(iod, gselector);
>
> And this seems to be your re-implementation of exactly that pin control
> core functionality. Which again should be shared with pinctrl-single if
> you anyway go down this path.
>
> I'll halt review here pending discussion on the bindings & stuff.

I assume we want to close on binding before looking at implementation
- definitely needs a rewrite - only guidance I'd probably need is
about which model you'd suggest I should follow(pinconf/pinctrl) once
we are done with the bindings discussion.

-- 
---
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html