* [PATCH 5/8] pinctrl: add driver for MB86S7x
From: Linus Walleij @ 2014-07-22 16:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1405233067-4725-1-git-send-email-mollie.wu@linaro.org>
On Sun, Jul 13, 2014 at 8:31 AM, Mollie Wu <mollie.wu@linaro.org> wrote:
> The mb86s70 and mb86s73 Fujitsu SoCs differ in that the latter provide a pinmux.
> GPIOs are supported on top of Pinctrl api.
>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Tetsuya Takinishi <t.takinishi@jp.fujitsu.com>
> Signed-off-by: Mollie Wu <mollie.wu@linaro.org>
This doesn't seem to be just about muxing but a full featured pin control
driver including pin config for electrical portions?
> +++ b/Documentation/devicetree/bindings/gpio/fujitsu,mb86s7x-gpio.txt
This looks OK.
> diff --git a/Documentation/devicetree/bindings/pinctrl/fujitsu,mb86s7x-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/fujitsu,mb86s7x-pinctrl.txt
> new file mode 100644
> index 0000000..ce2011b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/fujitsu,mb86s7x-pinctrl.txt
> @@ -0,0 +1,30 @@
> +Fujitsu MB86S7x Pin Controller
> +------------------------------
> +
> +Required properties:
> +- compatible: Should be "fujitsu,mb86s70-pinctrl" or "fujitsu,mb86s73-pinctrl"
> +- reg: Should contain the register physical address and length for the
> + pin controller.
> +- #gpio-range-cells: Should be 3
> +
> +Optional subnode-properties:
> +- mb86s7x,function: String specifying the function name.
> +- mb86s7x,drvst: Drive strength needed for pins to operate in that mode.
> + 0 (Hi-z), 2mA, 4mA, 6mA or 8mA
> +- mb86s7x,pullup: Should be 0 for Pull-Down or non-zero for Pull-Up
For all of these please switch the driver to using generic pin configuration.
We have stopped letting drivers use custom props for these things, even
functions are nowadays standardized to be just the function = "foo";
strings.
Consult
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
for the bindings,
Since the driver selects GENERIC_PINCONF, utilize the core
support for parsing DT info etc for these settings in
drivers/pinctrl/pinconf-generic.c and look how other drivers
exploit these helpers.
> +++ b/drivers/pinctrl/pinctrl-mb86s7x.c
(...)
> +#define PDR(x) (0x0 + x)
> +#define DDR(x) (0x10 + x)
> +#define PFR(x) (0x20 + x)
> +
> +#define CDRV_PDG(x) (0x0 + ((x) / 16) * 4)
> +#define FORCEZ_PDG(x) (0x100 + ((x) / 16) * 4)
> +#define PULL_PDG(x) (0x200 + ((x) / 16) * 4)
> +#define PIN_OFF(x) (((x) % 16) * 2)
I think the 0x0, 0x10, 0x20, 0x100, 0x200 etc offsets should
be named #defines telling us what kind of registers they will be
hitting.
> +#define PMUX_MB86S70 0
> +#define PMUX_MB86S73 1
More something like MB86S70_ID, MB86S73_ID or something are
better names for this?
> +/* Virtual pin numbers start so that reg offset calculations get simple */
> +enum {
> + PINS_VIRT = 128,
> + PINS_PCIE0 = PINS_VIRT,
> + PINS_HOLE1,
> + PINS_USB3H0,
> + PINS_HOLE2,
> + PINS_USB2H0,
> + PINS_USB2D0,
> + PINS_SDH30,
> + PINS_HOLE3,
> + PINS_EMMC0,
> + PINS_HSSPI0,
> + PINS_GMACD0,
> + PINS_GMACM0,
> + PINS_I2S0,
> + PINS_UART0,
> + PINS_OTHER0,
> + PINS_JTAG0,
> + PINS_PCIE1,
> + PINS_HOLE4,
> + PINS_USB3H1,
> + MB86S70_PMUX_PINS,
> +};
> +
> +struct mb86s70_gpio_chip {
> + spinlock_t lock; /* for goio regs */
> + struct clk *clk;
> + void __iomem *base;
> + struct gpio_chip gc;
> +};
> +
> +static int mb86s70_gpio_request(struct gpio_chip *gc, unsigned offset)
> +{
> + int ret = pinctrl_request_gpio(gc->base + offset);
> +
> + if (!ret) {
> + struct mb86s70_gpio_chip *gchip = container_of(gc,
> + struct mb86s70_gpio_chip, gc);
> + unsigned long flags;
> + u32 val;
> +
> + spin_lock_irqsave(&gchip->lock, flags);
> + val = readl(gchip->base + PFR(offset / 8 * 4));
> + val &= ~(1 << (offset % 8)); /* as gpio-port */
> + writel(val, gchip->base + PFR(offset / 8 * 4));
When PFR is used like this I don't see how those macros are
really helpful. It's just very hard to read:
readl(base + PFR(offset / 8 * 4));
(Whis is already scary unless you know expression evaluation order by heart...)
Expands to
readl(base + 0x20 + (offset / 8 * 4));
Is it possible to just get rid of the PFR macro and create an inline
function like this:
static inline unsigned mb86s70_pfr(unsigned offset)
{
return 0x20 + (offset / 8 * 4);
}
And the above becomes:
#include <linux/bitops.h>
val = readl(gchip->base + mb86s70_pfr(offset));
val &= ~BIT(offset % 8);
writel(val, gchip->base + mb86s70_pfr(offset));
This way it's less cluttered IMO. But it's not a strong preference.
> +static int mb86s70_gpio_direction_output(struct gpio_chip *gc,
> + unsigned offset, int value)
> +{
> + struct mb86s70_gpio_chip *gchip =
> + container_of(gc, struct mb86s70_gpio_chip, gc);
> + unsigned long flags;
> + unsigned char val;
> +
> + spin_lock_irqsave(&gchip->lock, flags);
> +
> + val = readl(gchip->base + PDR(offset / 8 * 4));
> + if (value)
> + val |= (1 << (offset % 8));
> + else
> + val &= ~(1 << (offset % 8));
In this and other places I'd just
val |= BIT(offset % 8); (etc)
> + writel(val, gchip->base + PDR(offset / 8 * 4));
> +
> + val = readl(gchip->base + DDR(offset / 8 * 4));
> + val |= (1 << (offset % 8));
> + writel(val, gchip->base + DDR(offset / 8 * 4));
> +
> + spin_unlock_irqrestore(&gchip->lock, flags);
> +
> + return 0;
> +}
Don't you want to use pinctrl_gpio_direction_output()
and pinctrl_gpio_direction_input() in these?
(Well maybe not.)
> +static int mb86s70_gpio_get(struct gpio_chip *gc, unsigned offset)
> +{
> + struct mb86s70_gpio_chip *gchip =
> + container_of(gc, struct mb86s70_gpio_chip, gc);
> + unsigned char val;
> +
> + val = readl(gchip->base + PDR(offset / 8 * 4));
> + val &= (1 << (offset % 8));
> + return val ? 1 : 0;
I usually just do this:
#include <linux/bitops.h>
return !!(readl(...) & BIT(offset % 8));
This applied in a few places simplifies the code I think.
> +static int mb86s70_gpio_probe(struct platform_device *pdev)
> +{
> + struct mb86s70_gpio_chip *gchip;
> + struct resource *res;
> + int ret;
> +
> + gchip = devm_kzalloc(&pdev->dev, sizeof(*gchip), GFP_KERNEL);
> + if (gchip == NULL)
> + return -ENOMEM;
Just
if (!gchip)
> +static int func_count, grp_count;
> +static const struct mb86s70_pmx_grp *mb86s7x_pgrps;
> +static const struct mb86s70_pmx_function *mb86s7x_pmx_funcs;
No static locals please. Add these to struct mb86s70_pmux_chip or
similar state container as they seem to be dynamic. Use
pinctrl_dev_get_drvdata()
to get from struct pinctrl_dev *pctl to the state container.
> +static int mb86s70_pctrl_get_groups_count(struct pinctrl_dev *pctl)
> +{
> + return grp_count;
> +}
> +
> +static const char *
> +mb86s70_pctrl_get_group_name(struct pinctrl_dev *pctl, unsigned selector)
> +{
> + return mb86s7x_pgrps[selector].name;
> +}
>
> +static int
> +mb86s70_pctrl_get_group_pins(struct pinctrl_dev *pctl, unsigned selector,
> + const unsigned **pins, unsigned *num_pins)
> +{
> + *pins = mb86s7x_pgrps[selector].pins;
> + *num_pins = mb86s7x_pgrps[selector].num_pins;
> +
> + return 0;
> +}
So these should use that method with pinctrl_dev_get_drvdata().
> +static int
> +mb86s70_pctrl_dt_node_to_map(struct pinctrl_dev *pctl,
> + struct device_node *node,
> + struct pinctrl_map **map, unsigned *num_maps)
> +{
Rewrite this to use pinconf_generic_dt_node_to_map()
and friends. Read other drivers doing this, for example pinctrl-msm.c
> + ret = of_property_read_string(node, "mb86s7x,function", &function);
> + if (ret) {
> + dev_err(pchip->dev,
> + "missing mb86s7x,function property in node %s\n",
> + node->name);
> + return -EINVAL;
> + }
For generic function parsing use just the name "function" rather
than "mb86s7x,function".
> +static const char * const pcie0_groups[] = {"pcie0_grp"};
> +static const char * const usb3h0_groups[] = {"usb3h0_grp"};
> +static const char * const usb2h0_groups[] = {"usb2h0_grp"};
> +static const char * const usb2d0_groups[] = {"usb2d0_grp"};
> +static const char * const sdh30_groups[] = {"sdh30_grp"};
> +static const char * const emmc0_groups[] = {"emmc0_grp"};
> +static const char * const hsspi0_groups[] = {"hsspi0_grp"};
> +static const char * const gmacd0_groups[] = {"gmacd0_grp"};
> +static const char * const gmacm0_groups[] = {"gmacm0_grp"};
> +static const char * const i2s0_groups[] = {"i2s0_grp"};
> +static const char * const other0_groups[] = {"other0_grp"};
> +static const char * const jtag0_groups[] = {"jtag0_grp"};
> +static const char * const pcie1_groups[] = {"pcie1_grp"};
> +static const char * const usb3h1_groups[] = {"usb3h1_grp"};
> +static const char * const extint16_groups[] = {"extint16_grp"};
> +static const char * const extint5_groups[] = {"extint5_grp"};
> +static const char * const tsif0_groups[] = {"tsif0_grp"};
> +static const char * const tsif1_groups[] = {"tsif1_grp"};
> +static const char * const cfg_groups[] = {"cfg_grp"};
> +static const char * const uart0_groups[] = {"uart0_grp"};
> +static const char * const uart1_groups[] = {"uart1_grp"};
> +static const char * const uart2_groups[] = {"uart2_grp"};
> +static const char * const pl244_groups[] = {"pl244_grp"};
> +static const char * const trace_groups[] = {"trace_grp"};
> +static const char * const memcs_groups[] = {"memcs_grp"};
> +static const char * const cap_groups[] = {"cap_grp"};
> +static const char * const smt_groups[] = {"smt_grp"};
> +
> +static const struct mb86s70_pmx_function mb86s73_pmx_funcs[] = {
> + {
> + .prog_val = 0x1,
> + .name = "extint5",
> + .groups = extint5_groups,
> + .num_groups = ARRAY_SIZE(extint5_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "pcie0",
> + .groups = pcie0_groups,
> + .num_groups = ARRAY_SIZE(pcie0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "usb3h0",
> + .groups = usb3h0_groups,
> + .num_groups = ARRAY_SIZE(usb3h0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "usb2h0",
> + .groups = usb2h0_groups,
> + .num_groups = ARRAY_SIZE(usb2h0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "usb2d0",
> + .groups = usb2d0_groups,
> + .num_groups = ARRAY_SIZE(usb2d0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "sdh30",
> + .groups = sdh30_groups,
> + .num_groups = ARRAY_SIZE(sdh30_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "emmc0",
> + .groups = emmc0_groups,
> + .num_groups = ARRAY_SIZE(emmc0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "hsspi0",
> + .groups = hsspi0_groups,
> + .num_groups = ARRAY_SIZE(hsspi0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "gmacd0",
> + .groups = gmacd0_groups,
> + .num_groups = ARRAY_SIZE(gmacd0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "gmacm0",
> + .groups = gmacm0_groups,
> + .num_groups = ARRAY_SIZE(gmacm0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "i2s0",
> + .groups = i2s0_groups,
> + .num_groups = ARRAY_SIZE(i2s0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "other0",
> + .groups = other0_groups,
> + .num_groups = ARRAY_SIZE(other0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "jtag0",
> + .groups = jtag0_groups,
> + .num_groups = ARRAY_SIZE(jtag0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "pcie1",
> + .groups = pcie1_groups,
> + .num_groups = ARRAY_SIZE(pcie1_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "usb3h1",
> + .groups = usb3h1_groups,
> + .num_groups = ARRAY_SIZE(usb3h1_groups),
> + },
> + {
> + .prog_val = 0x1,
> + .name = "cfg",
> + .groups = cfg_groups,
> + .num_groups = ARRAY_SIZE(cfg_groups),
> + },
> + {
> + .prog_val = 0x1,
> + .name = "uart0",
> + .groups = uart0_groups,
> + .num_groups = ARRAY_SIZE(uart0_groups),
> + },
> + {
> + .prog_val = 0x1,
> + .name = "uart1",
> + .groups = uart1_groups,
> + .num_groups = ARRAY_SIZE(uart1_groups),
> + },
> + {
> + .prog_val = 0x1,
> + .name = "uart2",
> + .groups = uart2_groups,
> + .num_groups = ARRAY_SIZE(uart2_groups),
> + },
> + {
> + .prog_val = 0x2,
> + .name = "trace",
> + .groups = trace_groups,
> + .num_groups = ARRAY_SIZE(trace_groups),
> + },
> + {
> + .prog_val = 0x2,
> + .name = "pl244",
> + .groups = pl244_groups,
> + .num_groups = ARRAY_SIZE(pl244_groups),
> + },
> + {
> + .prog_val = 0x3,
> + .name = "smt",
> + .groups = smt_groups,
> + .num_groups = ARRAY_SIZE(smt_groups),
> + },
> + {
> + .prog_val = 0x3,
> + .name = "memcs",
> + .groups = memcs_groups,
> + .num_groups = ARRAY_SIZE(memcs_groups),
> + },
> +};
Open-coding the function tables like this is certainly OK, but most
drivers will use macros to compress the tables. But I'm happy with
this.
> +static int
> +mb86s70_pmx_get_functions_count(struct pinctrl_dev *pctl)
> +{
> + return func_count;
> +}
> +
> +static const char *
> +mb86s70_pmx_get_function_name(struct pinctrl_dev *pctl, unsigned selector)
> +{
> + return mb86s7x_pmx_funcs[selector].name;
> +}
> +
> +static int
> +mb86s70_pmx_get_function_groups(struct pinctrl_dev *pctl,
> + unsigned selector, const char * const **groups,
> + unsigned * const num_groups)
> +{
> + *groups = mb86s7x_pmx_funcs[selector].groups;
> + *num_groups = mb86s7x_pmx_funcs[selector].num_groups;
> +
> + return 0;
> +}
Again get these things from the state container.
> +static int
> +mb86s70_pmx_enable(struct pinctrl_dev *pctl,
> + unsigned func_selector, unsigned group_selector)
> +{
> + struct mb86s70_pmux_chip *pchip = pinctrl_dev_get_drvdata(pctl);
> + unsigned long flags;
> + const unsigned *pins;
> + int i, j;
> +
> + if (group_selector >= grp_count) {
> + pr_err("%s:%d\n", __func__, __LINE__);
> + return -EINVAL;
> + }
> +
> + j = mb86s7x_pgrps[group_selector].num_pins;
> + pins = mb86s7x_pgrps[group_selector].pins;
> +
> + spin_lock_irqsave(&pchip->lock, flags);
> +
> + /* Busy if any pin in the same 'bunch' is taken */
> + for (i = 0; i < j; i++) {
> + u32 val;
> + int p = pins[i] / 4 * 4;
> +
> + if (pins[i] >= PINS_VIRT) /* Not for virtual pins */
> + continue;
> +
> + val = readl(pchip->base + p);
> + /* skip if no change needed */
> + if (val == mb86s7x_pmx_funcs[func_selector].prog_val)
> + continue;
> +
> + if (pchip->pin_busy[p]) {
> + pr_err("%s:%d:%d %d busy\n",
> + __func__, __LINE__, pins[i], p);
> + goto busy_exit;
> + }
> +
> + if (pchip->pin_busy[p + 1]) {
> + pr_err("%s:%d:%d %d busy\n",
> + __func__, __LINE__, pins[i], p+1);
> + goto busy_exit;
> + }
I don't see why you are doing this and keeping track of
pins as "busy" or not. One thing the pin control core does
is to make sure pins do not collide in different use cases,
it seems like you're re-implementing this check again.
> + if (p == 64)
> + continue;
This if-clause seems dubious. At least add a comment as
to what is happening here and why.
> + if (pchip->pin_busy[p + 2]) {
> + pr_err("%s:%d:%d %d busy\n",
> + __func__, __LINE__, pins[i], p+2);
> + goto busy_exit;
> + }
> +
> + if (pchip->pin_busy[p + 3]) {
> + pr_err("%s:%d:%d %d busy\n",
> + __func__, __LINE__, pins[i], p+3);
> + goto busy_exit;
> + }
I don't understand this either.
Why all this fuzz around the four pins from p ... p+3?
> + continue;
> +busy_exit:
> + spin_unlock_irqrestore(&pchip->lock, flags);
> + pr_err("%s:%d Take a look!\n", __func__, __LINE__);
> + return -EBUSY;
> + }
You don't have to have the busy_exit: inside the for-loop right?
Just push it below the return 0; in the end of this function
so you can get rid of the dangling continue;
> + pr_debug("Going to enable %s on pins -",
> + mb86s7x_pmx_funcs[func_selector].name);
> + for (i = 0; i < j; i++) {
> + int p = pins[i];
> +
> + pr_debug(" %d", p);
> + pchip->pin_busy[p] = true;
So I'm questioning this....
> + if (p < PINS_VIRT) /* Not for virtual pins */
We need an explanation somewhere about what "virtual pins"
means in this driver, I have never seen that before.
> + writel(mb86s7x_pmx_funcs[func_selector].prog_val,
> + pchip->base + p / 4 * 4);
This may need some static inline like described above to simplify
the code and make it more readable, but I see what is going on.
> + }
> +
> + spin_unlock_irqrestore(&pchip->lock, flags);
> + pr_debug("\n");
> +
> + return 0;
> +}
> +
> +static void
> +mb86s70_pmx_disable(struct pinctrl_dev *pctl,
> + unsigned func_selector, unsigned group_selector)
> +{
> + struct mb86s70_pmux_chip *pchip = pinctrl_dev_get_drvdata(pctl);
> + unsigned long flags;
> + const unsigned *pins;
> + int i, j;
> +
> + if (group_selector >= grp_count) {
> + pr_err("%s:%d\n", __func__, __LINE__);
> + return;
> + }
> +
> + j = mb86s7x_pgrps[group_selector].num_pins;
> + pins = mb86s7x_pgrps[group_selector].pins;
> +
> + pr_debug("Going to disable %s on pins -",
> + mb86s7x_pmx_funcs[func_selector].name);
> + spin_lock_irqsave(&pchip->lock, flags);
> + for (i = 0; i < j; i++) {
> + pr_debug(" %d", pins[i]);
> + pchip->pin_busy[pins[i]] = false;
> + }
> + spin_unlock_irqrestore(&pchip->lock, flags);
> + pr_debug("\n");
> +}
Remove this function. The .disable member is gone from
struct pinmux_ops, as it was ambiguous, see commit
2243a87d90b42eb38bc281957df3e57c712b5e56
"pinctrl: avoid duplicated calling enable_pinmux_setting for a pin"
> +static int
> +mb86s70_pmx_gpio_set_direction(struct pinctrl_dev *pctl,
> + struct pinctrl_gpio_range *range,
> + unsigned pin, bool input)
> +{
> + struct gpio_chip *gc = range->gc;
> + struct mb86s70_gpio_chip *gchip = container_of(gc,
> + struct mb86s70_gpio_chip, gc);
> + unsigned long flags;
> + u32 off, bit, val;
> +
> + if (pin >= 64)
> + return -EINVAL;
This is a bit strange again...
> + spin_lock_irqsave(&gchip->lock, flags);
> + bit = (pin - gc->base) % 8;
> + off = (pin - gc->base) / 8 * 4;
> + val = readl(gchip->base + DDR(off));
> + if (input)
> + val &= ~(1 << bit);
> + else
> + val |= (1 << bit);
> + writel(val, gchip->base + DDR(off));
> + spin_unlock_irqrestore(&gchip->lock, flags);
> +
> + return 0;
> +}
So .set_direction is implemented but not called from the
gpiochip functions as pointed out above.
> +static int
> +mb86s70_pin_config_group_get(struct pinctrl_dev *pctl, unsigned group,
> + unsigned long *config)
> +{
> + return -EINVAL;
> +}
Don't implement stubs for this, just leave the vtable entry
unassigned, the core will return an error.
However it doesn't seem so hard to implement this
actually: the set code just below does exactly this, retrieves
the right bits and alters them, so why is not get implemented?
> +static int
> +mb86s70_pin_config_group_set(struct pinctrl_dev *pctl, unsigned group,
> + unsigned long *configs, unsigned num_configs)
> +{
> + struct mb86s70_pmux_chip *pchip = pinctrl_dev_get_drvdata(pctl);
> + unsigned long flags;
> + const unsigned *pin;
> + int i, j, p;
> + u32 val, ds;
> +
> + p = mb86s7x_pgrps[group].num_pins;
> + pin = mb86s7x_pgrps[group].pins;
> +
> + spin_lock_irqsave(&pchip->lock, flags);
> +
> + for (i = 0; i < num_configs; i++) {
> + switch (pinconf_to_config_param(configs[i])) {
> + case PIN_CONFIG_DRIVE_STRENGTH:
> + /* Drive Strength should be 2, 4, 6 or 8 mA */
> + ds = pinconf_to_config_argument(configs[i]);
> + ds = (ds - 2) / 2;
I like this use of SI units, so use the generic bindings too!
(...)
> + pchip->desc.name = dev_name(&pdev->dev);
> + pchip->desc.npins = MB86S70_PMUX_PINS;
> + pchip->desc.pins = pchip->pins;
> + pchip->desc.pctlops = &mb86s70_pctrl_ops;
> + pchip->desc.pmxops = &mb86s70_pmx_ops;
> + pchip->desc.owner = THIS_MODULE;
> + if (type == PMUX_MB86S73) {
> + pchip->desc.confops = &mb86s70_conf_ops;
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + pchip->conf_base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(pchip->conf_base))
> + return PTR_ERR(pchip->conf_base);
> + mb86s7x_pmx_funcs = mb86s73_pmx_funcs;
> + func_count = ARRAY_SIZE(mb86s73_pmx_funcs);
> + mb86s7x_pgrps = mb86s73_pgrps;
> + grp_count = ARRAY_SIZE(mb86s73_pgrps);
> + } else {
> + mb86s7x_pmx_funcs = mb86s70_pmx_funcs;
> + func_count = ARRAY_SIZE(mb86s70_pmx_funcs);
> + mb86s7x_pgrps = mb86s70_pgrps;
> + grp_count = ARRAY_SIZE(mb86s70_pgrps);
> + }
As mentioned don't use static locals for these things, add these
to the state container.
> +static const struct of_device_id mb86s70_gpio_dt_ids[] = {
> + { .compatible = "fujitsu,mb86s7x-gpio" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mb86s70_gpio_dt_ids);
"fujitsu" does not seem to exist in
Documentation/devicetree/bindings/vendor-prefixes.txt
Do you want to add it?
> +static struct platform_driver mb86s70_gpio_driver = {
> + .probe = mb86s70_gpio_probe,
> + .driver = {
> + .name = "mb86s70-gpio",
> + .owner = THIS_MODULE,
> + .of_match_table = mb86s70_gpio_dt_ids,
> + },
> +};
> +
> +static struct platform_driver mb86s70_pinmux_driver = {
> + .probe = mb86s70_pinmux_probe,
> + .driver = {
> + .name = "mb86s70-pinmux",
> + .owner = THIS_MODULE,
> + .of_match_table = mb86s70_pinmux_dt_ids,
> + },
> +};
> +
> +static int __init mb86s70_pins_init(void)
> +{
> + int ret;
> +
> + ret = platform_driver_register(&mb86s70_pinmux_driver);
> + if (ret)
> + return ret;
> +
> + return platform_driver_register(&mb86s70_gpio_driver);
> +}
> +subsys_initcall(mb86s70_pins_init);
It's not really necessary to split this up in two drivers in one
file, it is possible to just use one driver: mb86s70_pinmux_driver,
have just one .probe() function, and register *both* the gpiochip
*and* pin control driver from that probe().
(Well pin control first the gpiochip I guess, but you get the
idea.)
This driver needs some work, but I like the start, please keep at it.
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH v4] pcie: Add Xilinx PCIe Host Bridge IP driver
From: Bjorn Helgaas @ 2014-07-22 16:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CA+mB=1JupzQy+R14kKAk7mTJR2FcZ+cQv-uXQSB6jEVBW95aGQ@mail.gmail.com>
On Mon, Jul 21, 2014 at 11:10 PM, Srikanth Thokala <sthokal@xilinx.com> wrote:
> On Wed, Jul 16, 2014 at 11:08 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> ...
>> I see I forgot to ask for a MAINTAINERS entry for this driver. Can
>> you add one?
>
> There was a discussion on this earlier and Michal mentioned it is not
> required as it is
> handled by our Xilinx record.
>
> Here is the reply from Michal to the MAINTAINERS update comment,
>
> < Reply from Michal >
>
>> Please also include a MAINTAINERS update for drivers/pci/host/pci-xilinx.c.
>
> This should be handle by our record that's why MAINTAINERS update is
> not necessary.
> (N: xilinx below)
That's technically true in the sense that get_maintainer.pl will do
the right thing, but I often review patches in email (without
extracting them), so it's more convenient to just look in MAINTAINERS
to see if they have the right acks. But I guess I can deal with it
either way.
> ARM/ZYNQ ARCHITECTURE
> M: Michal Simek <michal.simek@xilinx.com>
> L: linux-arm-kernel at lists.infradead.org (moderated for non-subscribers)
> W: http://wiki.xilinx.com
> T: git git://git.xilinx.com/linux-xlnx.git
> S: Supported
> F: arch/arm/mach-zynq/
> F: drivers/cpuidle/cpuidle-zynq.c
> N: zynq
> N: xilinx
^ permalink raw reply
* [PATCH v2] bus: ARM CCN PMU driver
From: Olof Johansson @ 2014-07-22 16:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1406023886.25343.48.camel@hornet>
On Tue, Jul 22, 2014 at 3:11 AM, Pawel Moll <pawel.moll@arm.com> wrote:
> Hi Arnd, Kevin, Olof,
>
> On Fri, 2014-07-11 at 16:06 +0100, Pawel Moll wrote:
>> Driver providing perf backend for ARM Cache Coherent Network
>> interconnect. Supports counting all hardware events and crosspoint
>> watchpoints.
>>
>> Currently works with CCN-504 only, although there should be
>> no changes required for CCN-508 (just impossible to test it now).
>>
>> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
>
> Would you consider taking this through arm-soc tree? There isn't
> anything in MAINTAINERS, but I vaguely remember Arnd proposing this
> directory in the first place...
Yeah, drivers/bus lacks an owner and we've normally been the ones
merging them. Please send a fresh version of the patch over.
-Olof
^ permalink raw reply
* [PATCH 2/2] ARM: multi_v7_defconfig: Enable MiPHY365x - ST's Generic (SATA & PCIe) PHY
From: Olof Johansson @ 2014-07-22 16:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1406021985-8763-2-git-send-email-lee.jones@linaro.org>
On Tue, Jul 22, 2014 at 10:39:45AM +0100, Lee Jones wrote:
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
Appled both. In the future, feel free to batch them up in one commit,
no need to keep them separate since they're related.
-Olof
^ permalink raw reply
* [PATCHv4 3/5] common: dma-mapping: Introduce common remapping functions
From: Catalin Marinas @ 2014-07-22 16:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53CD6B1C.2010801@codeaurora.org>
On Mon, Jul 21, 2014 at 08:33:48PM +0100, Laura Abbott wrote:
> On 7/18/2014 6:53 AM, Catalin Marinas wrote:
> > On Wed, Jul 02, 2014 at 07:03:36PM +0100, Laura Abbott wrote:
> >> +void *dma_common_pages_remap(struct page **pages, size_t size,
> >> + unsigned long vm_flags, pgprot_t prot,
> >> + const void *caller)
> >> +{
> >> + struct vm_struct *area;
> >> +
> >> + area = get_vm_area_caller(size, vm_flags, caller);
> >> + if (!area)
> >> + return NULL;
> >> +
> >> + if (map_vm_area(area, prot, &pages)) {
> >> + vunmap(area->addr);
> >> + return NULL;
> >> + }
> >> +
> >> + return area->addr;
> >> +}
> >
> > Why not just replace this function with vmap()? It is nearly identical.
>
> With this version, the caller stored and printed via /proc/vmallocinfo
> is the actual caller of the DMA API whereas if we just call vmap we
> don't get any useful caller information. Going to vmap would change
> the existing behavior on ARM so it seems unwise to switch.
OK.
> Another option is to move this into vmalloc.c and add vmap_caller.
Maybe as a subsequent clean-up (once this series gets merged).
--
Catalin
^ permalink raw reply
* [STLinux Kernel] [PATCH v3 10/10] MAINTAINERS: Add sdhci-st file to ARCH/STI architecture
From: Olof Johansson @ 2014-07-22 16:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53CE556C.10607@st.com>
On Tue, Jul 22, 2014 at 5:13 AM, Maxime Coquelin <maxime.coquelin@st.com> wrote:
> Hi Olof, Arnd, Kevin,
>
> I am preparing two tags for v3.17 regarding STi platform.
> One for DT, and one for defconfigs.
>
> Except this one, I have no patches for ARM-SoC.
> Can you take this patch directly?
>
> Or should I send a pull request with this one only?
We can apply directly. I'll do that in a moment here.
-Olof
^ permalink raw reply
* [PATCH v2 1/3] usb: dwc3: add ST dwc3 glue layer to manage dwc3 HC
From: Lee Jones @ 2014-07-22 15:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140722155117.GF20588@saruman.home>
On Tue, 22 Jul 2014, Felipe Balbi wrote:
> On Tue, Jul 22, 2014 at 04:45:03PM +0100, Lee Jones wrote:
> > > > > > +static void st_dwc3_init(struct st_dwc3 *dwc3_data)
> > > > > > +{
> > > > > > + u32 reg = st_dwc3_readl(dwc3_data->glue_base, USB2_CLKRST_CTRL);
> > > > > > +
> > > > > > + reg |= aux_clk_en(1) | ext_cfg_reset_n(1) | xhci_revision(1);
> > > > > > + reg &= ~sw_pipew_reset_n(1);
> > > > >
> > > > > 1? Better to add defines for these magic numbers. What is 1 anyway?
> > > >
> > > > They are just bit setting macros, I've converted them over to use BIT macro now,
> > > > so it no longer takes a parameter.
> > >
> > > the macros are better, but make them upper case as everybody else.
> >
> > When you say that the macros are better, what do you mean?
> >
> > Defines do the job in a manner which is a great deal cleaner:
> >
> > #define AUX_CLK_EN BIT(0)
> > #define SW_PIPEW_RESET_N BIT(4)
> > #define EXT_CFG_RESET_N BIT(8)
> > #define XHCI_REVISION BIT(12)
> >
> > reg = AUX_CLK_EN | SW_PIPEW_RESET_N | XHCI_REVISION;
> > reg &= ~SW_PIPEW_RESET_N
>
> this is what I meant ;-) I see what I wrote can be very ambiguous :-p
Okay great, thanks for clarifying. :)
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCHv4 5/5] arm64: Add atomic pool for non-coherent and CMA allocations.
From: Catalin Marinas @ 2014-07-22 15:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53CD9601.5070001@codeaurora.org>
On Mon, Jul 21, 2014 at 11:36:49PM +0100, Laura Abbott wrote:
> On 7/18/2014 6:43 AM, Catalin Marinas wrote:
> > On Wed, Jul 02, 2014 at 07:03:38PM +0100, Laura Abbott wrote:
> >> @@ -73,50 +124,56 @@ static void __dma_free_coherent(struct device *dev, size_t size,
> >> void *vaddr, dma_addr_t dma_handle,
> >> struct dma_attrs *attrs)
> >> {
> >> + bool freed;
> >> + phys_addr_t paddr = dma_to_phys(dev, dma_handle);
> >> +
> >> if (dev == NULL) {
> >> WARN_ONCE(1, "Use an actual device structure for DMA allocation\n");
> >> return;
> >> }
> >>
> >> - if (IS_ENABLED(CONFIG_DMA_CMA)) {
> >> - phys_addr_t paddr = dma_to_phys(dev, dma_handle);
> >>
> >> - dma_release_from_contiguous(dev,
> >> + freed = dma_release_from_contiguous(dev,
> >> phys_to_page(paddr),
> >> size >> PAGE_SHIFT);
> >> - } else {
> >> + if (!freed)
> >> swiotlb_free_coherent(dev, size, vaddr, dma_handle);
> >> - }
> >> }
> >
> > Is __dma_free_coherent() ever called in atomic context? If yes, the
> > dma_release_from_contiguous() may not like it since it tries to acquire
> > a mutex. But since we don't have the gfp flags here, we don't have an
> > easy way to know what to call.
> >
> > So the initial idea of always calling __alloc_from_pool() for both
> > coherent/non-coherent cases would work better (but still with a single
> > shared pool, see below).
>
> We should be okay
>
> __dma_free_coherent -> dma_release_from_contiguous -> cma_release which
> bounds checks the CMA region before taking any mutexes unless I missed
> something.
Ah, good point. I missed the pfn range check in
dma_release_from_contiguous.
> The existing behavior on arm is to not allow non-atomic allocations to be
> freed atomic context when CMA is enabled so we'd be giving arm64 more
> leeway there. Is being able to free non-atomic allocations in atomic
> context really necessary?
No. I was worried that an atomic coherent allocation (falling back to
swiotlb) would trigger some CMA mutex in atomic context on the freeing
path. But you are right, it shouldn't happen.
> >> + page = dma_alloc_from_contiguous(NULL, nr_pages,
> >> + get_order(atomic_pool_size));
> >> + else
> >> + page = alloc_pages(GFP_KERNEL, get_order(atomic_pool_size));
> >
> > One problem here is that the atomic pool wouldn't be able to honour
> > GFP_DMA (in the latest kernel, CMA is by default in ZONE_DMA). You
> > should probably pass GFP_KERNEL|GFP_DMA here. You could also use the
> > swiotlb_alloc_coherent() which, with a NULL dev, assumes 32-bit DMA mask
> > but it still expects GFP_DMA to be passed.
> >
>
> I think I missed updating this to GFP_DMA. The only advantage I would see
> to using swiotlb_alloc_coherent vs. alloc_pages directly would be to
> allow the fallback to using a bounce buffer if __get_free_pages failed.
> I'll keep this as alloc_pages for now; it can be changed later if there
> is a particular need for swiotlb behavior.
That's fine. Since we don't have a device at this point, I don't see how
swiotlb could fall back to the bounce buffer.
Thanks.
--
Catalin
^ permalink raw reply
* [PATCH v2 1/3] usb: dwc3: add ST dwc3 glue layer to manage dwc3 HC
From: Felipe Balbi @ 2014-07-22 15:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140722154503.GC23210@lee--X1>
On Tue, Jul 22, 2014 at 04:45:03PM +0100, Lee Jones wrote:
> > > > > +static void st_dwc3_init(struct st_dwc3 *dwc3_data)
> > > > > +{
> > > > > + u32 reg = st_dwc3_readl(dwc3_data->glue_base, USB2_CLKRST_CTRL);
> > > > > +
> > > > > + reg |= aux_clk_en(1) | ext_cfg_reset_n(1) | xhci_revision(1);
> > > > > + reg &= ~sw_pipew_reset_n(1);
> > > >
> > > > 1? Better to add defines for these magic numbers. What is 1 anyway?
> > >
> > > They are just bit setting macros, I've converted them over to use BIT macro now,
> > > so it no longer takes a parameter.
> >
> > the macros are better, but make them upper case as everybody else.
>
> When you say that the macros are better, what do you mean?
>
> Defines do the job in a manner which is a great deal cleaner:
>
> #define AUX_CLK_EN BIT(0)
> #define SW_PIPEW_RESET_N BIT(4)
> #define EXT_CFG_RESET_N BIT(8)
> #define XHCI_REVISION BIT(12)
>
> reg = AUX_CLK_EN | SW_PIPEW_RESET_N | XHCI_REVISION;
> reg &= ~SW_PIPEW_RESET_N
this is what I meant ;-) I see what I wrote can be very ambiguous :-p
cheers
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140722/b62b0981/attachment.sig>
^ permalink raw reply
* [PATCH 13/11] arm64: Add support for 48-bit VA space with 64KB page configuration
From: Jungseok Lee @ 2014-07-22 15:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140722153738.GJ2219@arm.com>
On Jul 23, 2014, at 12:37 AM, Catalin Marinas wrote:
> On Tue, Jul 22, 2014 at 04:13:27PM +0100, Jungseok Lee wrote:
>> On Jul 22, 2014, at 00:09 +900 Catalin Marinas wrote:
>>> This patch allows support for 3 levels of page tables with 64KB page
>>> configuration allowing 48-bit VA space. The pgd is no longer a full
>>> PAGE_SIZE (PTRS_PER_PGD is 64) and (swapper|idmap)_pg_dir are not fully
>>> populated (pgd_alloc falls back to kzalloc).
>>>
>>> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
>>> ---
>>
>> One more step :)
>
> The last before the upcoming merging window. I also updated the
> pgtable-4levels branch with the latest.
I've checked it out. I will leave a comment ASAP if needed.
>> Are you going to post the next version of this series including [12/11]
>> and [13/11] or finalize the series?
>
> I wasn't planning to post another series. It's good to get the clean-up
> merged with the change that 48-bit VA now depends on BROKEN until KVM is
> sorted (for 3.18).
I see.
>> In addition, could you put my gmail address in CC in coming patch or
>> the next version? It help me follow up the series easily.
>
> Do you want me to add any reviewed etc. tags from you?
No, it's okay. What I mean is mailing thread, not tags.
- Jungseok Lee
^ permalink raw reply
* [PATCHv4 2/5] lib/genalloc.c: Add genpool range check function
From: Catalin Marinas @ 2014-07-22 15:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53CD6F28.3080203@codeaurora.org>
On Mon, Jul 21, 2014 at 08:51:04PM +0100, Laura Abbott wrote:
> On 7/9/2014 3:33 PM, Olof Johansson wrote:
> > On Wed, Jul 2, 2014 at 11:03 AM, Laura Abbott <lauraa@codeaurora.org> wrote:
> >>
> >> After allocating an address from a particular genpool,
> >> there is no good way to verify if that address actually
> >> belongs to a genpool. Introduce addr_in_gen_pool which
> >> will return if an address plus size falls completely
> >> within the genpool range.
> >>
> >> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> >
> > Reviewed-by: Olof Johansson <olof@lixom.net>
> >
> > What's the merge path for this code? Part of the arm64 code that needs
> > it, I presume?
>
> My plan was to have the entire series go through the arm64 tree unless
> someone has a better idea.
It's fine by me. But since it touches core arch/arm code, I would like
to see an Ack from Russell.
--
Catalin
^ permalink raw reply
* [PATCH] pinctrl: dra: dt-bindings: Fix pull enable/disable
From: Nishanth Menon @ 2014-07-22 15:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1406043594-14181-1-git-send-email-nm@ti.com>
> 1: dra72x-evm: Boot ok: http://slexy.org/raw/s20I6QXQa (needs MMC filesystem that current dts does not have.
Oops - missed a character in the link:
http://slexy.org/view/s20I6QXQal
Sorry about the spam.
--
Regards,
Nishanth Menon
^ permalink raw reply
* [PATCH v2 1/3] usb: dwc3: add ST dwc3 glue layer to manage dwc3 HC
From: Lee Jones @ 2014-07-22 15:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140722145711.GC20588@saruman.home>
> > > > +static void st_dwc3_init(struct st_dwc3 *dwc3_data)
> > > > +{
> > > > + u32 reg = st_dwc3_readl(dwc3_data->glue_base, USB2_CLKRST_CTRL);
> > > > +
> > > > + reg |= aux_clk_en(1) | ext_cfg_reset_n(1) | xhci_revision(1);
> > > > + reg &= ~sw_pipew_reset_n(1);
> > >
> > > 1? Better to add defines for these magic numbers. What is 1 anyway?
> >
> > They are just bit setting macros, I've converted them over to use BIT macro now,
> > so it no longer takes a parameter.
>
> the macros are better, but make them upper case as everybody else.
When you say that the macros are better, what do you mean?
Defines do the job in a manner which is a great deal cleaner:
#define AUX_CLK_EN BIT(0)
#define SW_PIPEW_RESET_N BIT(4)
#define EXT_CFG_RESET_N BIT(8)
#define XHCI_REVISION BIT(12)
reg = AUX_CLK_EN | SW_PIPEW_RESET_N | XHCI_REVISION;
reg &= ~SW_PIPEW_RESET_N
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH v5 00/14] Clock support for rk3066,rk3188 and rk3288
From: Heiko Stübner @ 2014-07-22 15:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <2878354.yVHl3h6vxm@diego>
Hi Mike,
Am Montag, 14. Juli 2014, 10:07:01 schrieb Heiko St?bner:
> Hi Olof,
>
> Am Sonntag, 13. Juli 2014, 20:12:07 schrieb Olof Johansson:
> > Hi,
> >
> > On Sun, Jul 13, 2014 at 12:46 PM, Mike Turquette <mturquette@linaro.org>
>
> wrote:
> > > Quoting Heiko St?bner (2014-07-02 16:53:24)
> > >
> > >> This series adds a clock driver infrastructure for Rockchip SoCs in
> > >> general and clock-definitions for the RK3188 and RK3288 in particular.
> > >>
> > >> Apart from the arch/arm patches included here, there are some more
> > >> in the waiting line, like adding the i2c nodes and possibly the pwm,
> > >> i2s
> > >> and spi nodes if the relevant drivers get accepted, where the ids
> > >> defined in the dt-binding headers are needed.
> > >>
> > >> So if the whole thing is acceptable could we either offer a branch
> > >> from the clk-tree that can get merged or take the whole series through
> > >> the arm tree?
> > >
> > > Heiko,
> > >
> > > This version looks good. I've pushed a branch with all 14 of these
> > > patches based on 3.16-rc3 to the clk tree. See:
> > >
> > > git://git.linaro.org/people/mike.turquette/linux.git clk-rockchip
> > >
> > > If everyone is OK to merge that stable branch then I will merge
> > > clk-rockchip into clk-next and we're all good. If the arm-soc guys want
> > > to carve it up a bit further or handle it in a different way then we can
> > > do that. I've Cc'd arm-soc for clarification.
> >
> > Traditionally we usually take the DT changes through arm-soc, but as
> > long as we share the branch we might be ok. We tend to stick them in
> > different branches in our tree though, so rockchip will be a little
> > mis-sorted this release. Not a big deal, and we can deal with it.
> >
> > So, I'll leave it to Heiko to choose here, he knows best what other DT
> > changes are coming this release. If he wants to carry the DT changes
> > separately, then patches 1-10 would be for the clk tree, so it should
> > be easy to reset the branch back a bit (and 11-14 would be for
> > arm-soc).
>
> hmm, there are essentially two changeset/branches upcoming:
> - the collected dts changes for rk3066 and rk3188 I posted saturday
> These essentially extend patches 12-14
> - rk3288 support which would start off patch 11 (the reset controller)
>
> So if we were to separate the series I would say patches 1-11 in the shared
> branch and patch 12-14 as the first ones for a rk3066/rk3188 specific
> branch.
are you ok with simply resetting the clk-rockchip branch to
"ARM: rockchip: Select ARCH_HAS_RESET_CONTROLLER"
being the last commit and the last 3 patches going through arm-soc?
As we're already at -rc6 now, it would be very cool if we could get this
sorted soon.
Thanks
Heiko
^ permalink raw reply
* [PATCH v6 4/5] PCI: add PCI controller for keystone PCIe h/w
From: Rob Herring @ 2014-07-22 15:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53CD424B.1090806@ti.com>
On Mon, Jul 21, 2014 at 11:39 AM, Murali Karicheri <m-karicheri2@ti.com> wrote:
> On 07/20/2014 09:44 PM, Jingoo Han wrote:
>>
>> On Saturday, July 19, 2014 5:29 AM, Murali Karicheri wrote:
>>>
>>> On 07/18/2014 03:31 PM, Rob Herring wrote:
>>>>
>>>> On Fri, Jul 18, 2014 at 10:14 AM, Murali Karicheri<m-karicheri2@ti.com>
>>>> wrote:
>>>
>>> --- Cut ---
>>>>>
>>>>> +
>>>>> +Optional properties:-
>>>>> + phys: phandle to Generic Keystone SerDes phy for PCI
>>>>> + phy-names: name of the Generic Keystine SerDes phy for PCI
>>>>> + - If boot loader already does PCI link establishment, then
>>>>> phys and
>>>>> + phy-names shouldn't be present.
>>>>> + ti,enable-linktrain - Enable Link training.
>>>>> + - If boot loader already does PCI link establishment, then
>>>>> this
>>>>> + shouldn't be present.
>>>>
>>>>
>>>> Can't you read from the h/w if the link is trained?
>>
>>
>> I agree with Rob Herring's suggestion.
>>
>>>
>>> Yes.
>>>
>>> There are customers who use this driver with PCI Link setup done in the
>>> boot loader. This property tells the driver to bypass Link setup
>>> procedure in that case. Is this undesirable and if so. how other
>>> platforms handle it? Check first if link is trained or start the link
>>> setup procedure? Let me know. If this is fine, please provide your Ack.
>>
>>
>> Please, check the following code of Exynos PCIe diver.
>>
>> ./drivers/pci/host/pci-exynos.c
>>
>> static int exynos_pcie_establish_link(struct pcie_port *pp)
>> {
>> struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp);
>> void __iomem *elbi_base = exynos_pcie->elbi_base;
>> void __iomem *pmu_base = exynos_pcie->pmu_base;
>>
>> if (dw_pcie_link_up(pp)) {
>> dev_err(pp->dev, "Link already up\n");
>> return 0;
>> }
>> .....
>>
>> In the case of Exynos PCIe, the Exynos PCIe driver checks the
>> h/w bit such as PCIE_ELBI_LTSSM_ENABLE bit of PCIE_ELBI_RDLH_LINKUP
>> offset register.
>>
>> If the link is already set up by the boot loader or other reasons,
>> the driver will skip some initialization codes.
>>
>> The first step is that you find such h/w bit for checking link up.
>> If so, please add the code for skipping, when the link is already
>> set up.
>>
> Rob, Jingoo,
>
> We have similar bit to check for Link status and I have removed the DT
> property and skip Link retrain if PCIe Link is already Up. I will be
> resending the series with Patch 4/5 updated.
For both keystone and exynos, is this status bit in the phy? If so,
sounds like this should be a standard phy function op.
Rob
^ permalink raw reply
* [PATCH] pinctrl: dra: dt-bindings: Fix pull enable/disable
From: Nishanth Menon @ 2014-07-22 15:39 UTC (permalink / raw)
To: linux-arm-kernel
The DRA74/72 control module pins have a weak pull up and pull down.
This is configured by bit offset 17. if BIT(17) is 1, a pull up is
selected, else a pull down is selected.
However, this pull resisstor is applied based on BIT(16) -
PULLUDENABLE - if BIT(18) is *0*, then pull as defined in BIT(17) is
applied, else no weak pulls are applied. We defined this in reverse.
Reference: Table 18-5 (Description of the pad configuration register
bits) in Technical Reference Manual Revision (DRA74x revision Q:
SPRUHI2Q Revised June 2014 and DRA72x revision F: SPRUHP2F - Revised
June 2014)
Fixes: 6e58b8f1daaf1a ("ARM: dts: DRA7: Add the dts files for dra7 SoC and dra7-evm board")
Signed-off-by: Nishanth Menon <nm@ti.com>
---
Patch based on v3.16-rc5 tag
1: dra72x-evm: Boot ok: http://slexy.org/raw/s20I6QXQa (needs MMC filesystem that current dts does not have.
- Fails in plain Vanilla 3.16-rc5 kernel due to missing patch to handle USB IP instance delta (between dra72x and dra74x) appropriately.
- Tested with fixes needed: https://patchwork.kernel.org/patch/4565431/ and https://patchwork.kernel.org/patch/4565461/
2: dra7xx-evm: Boot PASS: http://slexy.org/raw/s21c6X2wOd
Equivalent testing on 3.14 based product kernel:
dra72x-evm: Boot PASS: http://slexy.org/raw/s21yIgttJw
dra7xx-evm: Boot PASS: http://slexy.org/raw/s20w7OZaJJ
It is obvious that current users of padconf have'nt had trouble with
the wrong definitions. I think I might have been the first to discover
this as emmc on beagleboard-X15 (an upcoming platform) exposed this problem.
include/dt-bindings/pinctrl/dra.h | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/include/dt-bindings/pinctrl/dra.h b/include/dt-bindings/pinctrl/dra.h
index 002a285..3d33794 100644
--- a/include/dt-bindings/pinctrl/dra.h
+++ b/include/dt-bindings/pinctrl/dra.h
@@ -30,7 +30,8 @@
#define MUX_MODE14 0xe
#define MUX_MODE15 0xf
-#define PULL_ENA (1 << 16)
+#define PULL_ENA (0 << 16)
+#define PULL_DIS (1 << 16)
#define PULL_UP (1 << 17)
#define INPUT_EN (1 << 18)
#define SLEWCONTROL (1 << 19)
@@ -38,10 +39,10 @@
#define WAKEUP_EVENT (1 << 25)
/* Active pin states */
-#define PIN_OUTPUT 0
+#define PIN_OUTPUT (0 | PULL_DIS)
#define PIN_OUTPUT_PULLUP (PIN_OUTPUT | PULL_ENA | PULL_UP)
#define PIN_OUTPUT_PULLDOWN (PIN_OUTPUT | PULL_ENA)
-#define PIN_INPUT INPUT_EN
+#define PIN_INPUT (INPUT_EN | PULL_DIS)
#define PIN_INPUT_SLEW (INPUT_EN | SLEWCONTROL)
#define PIN_INPUT_PULLUP (PULL_ENA | INPUT_EN | PULL_UP)
#define PIN_INPUT_PULLDOWN (PULL_ENA | INPUT_EN)
--
1.7.9.5
^ permalink raw reply related
* [PATCH 13/11] arm64: Add support for 48-bit VA space with 64KB page configuration
From: Catalin Marinas @ 2014-07-22 15:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <2C360BFC-E4CF-448D-9110-4197DCAEAD4F@gmail.com>
On Tue, Jul 22, 2014 at 04:13:27PM +0100, Jungseok Lee wrote:
> On Jul 22, 2014, at 00:09 +900 Catalin Marinas wrote:
> > This patch allows support for 3 levels of page tables with 64KB page
> > configuration allowing 48-bit VA space. The pgd is no longer a full
> > PAGE_SIZE (PTRS_PER_PGD is 64) and (swapper|idmap)_pg_dir are not fully
> > populated (pgd_alloc falls back to kzalloc).
> >
> > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> > ---
>
> One more step :)
The last before the upcoming merging window. I also updated the
pgtable-4levels branch with the latest.
> Are you going to post the next version of this series including [12/11]
> and [13/11] or finalize the series?
I wasn't planning to post another series. It's good to get the clean-up
merged with the change that 48-bit VA now depends on BROKEN until KVM is
sorted (for 3.18).
> In addition, could you put my gmail address in CC in coming patch or
> the next version? It help me follow up the series easily.
Do you want me to add any reviewed etc. tags from you?
--
Catalin
^ permalink raw reply
* [PATCH v6 4/5] PCI: add PCI controller for keystone PCIe h/w
From: Rob Herring @ 2014-07-22 15:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5467203.eMVRoNeSx1@wuerfel>
On Fri, Jul 18, 2014 at 2:50 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 18 July 2014 14:31:39 Rob Herring wrote:
>> > +
>> > + Example:
>> > + pcie_msi_intc: msi-interrupt-controller {
>> > + interrupt-controller;
>> > + #interrupt-cells = <1>;
>> > + interrupt-parent = <&gic>;
>> > + interrupts = <GIC_SPI 30 IRQ_TYPE_EDGE_RISING>,
>> > + <GIC_SPI 31 IRQ_TYPE_EDGE_RISING>,
>> > + <GIC_SPI 32 IRQ_TYPE_EDGE_RISING>,
>> > + <GIC_SPI 33 IRQ_TYPE_EDGE_RISING>,
>> > + <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>,
>> > + <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>,
>> > + <GIC_SPI 36 IRQ_TYPE_EDGE_RISING>,
>> > + <GIC_SPI 37 IRQ_TYPE_EDGE_RISING>;
>> > + };
>> > +
>> > +pcie_intc: Interrupt controller device node for Legacy irq chip
>> > + interrupt-cells: should be set to 1
>> > + interrupt-parent: Parent interrupt controller phandle
>> > + interrupts: GIC interrupt lines connected to PCI Legacy interrupt lines
>> > +
>> > + Example:
>> > + pcie_intc: legacy-interrupt-controller {
>> > + interrupt-controller;
>> > + #interrupt-cells = <1>;
>> > + interrupt-parent = <&gic>;
>> > + interrupts = <GIC_SPI 26 IRQ_TYPE_EDGE_RISING>,
>> > + <GIC_SPI 27 IRQ_TYPE_EDGE_RISING>,
>> > + <GIC_SPI 28 IRQ_TYPE_EDGE_RISING>,
>> > + <GIC_SPI 29 IRQ_TYPE_EDGE_RISING>;
>> > + };
>>
>> This seems wrong. Legacy interrupts should be described with
>> interrupt-map and then PCI child devices have a standard interrupt
>> specifier.
>>
>> I'm not sure about MSIs, but I would think they would have a standard
>> format too.
>>
>
> IIRC, it's actually the correct way to do this here: the problem is that
> the PCI IRQs are not directly connected to the GIC, but instead there is
> a nested irqchip that has each PCI IRQ routed to it and that requires
> an extra EOI for each interrupt.
>
> The interrupt-map in the PCI host points to this special irqchip rather
> than to the GIC.
Okay, if there is still an interrupt-map property, then I agree. This
wasn't clear in the example.
Rob
^ permalink raw reply
* [PATCH 2/2] ARM: LPAE: reduce damage caused by idmap to virtual memory layout
From: Konstantin Khlebnikov @ 2014-07-22 15:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140722153623.25088.37742.stgit@buzz>
idmap layout combines both phisical and virtual addresses.
Everything works fine if ram physically lays below PAGE_OFFSET.
Otherwise idmap starts punching huge holes in virtual memory layout.
It maps ram by 2MiB sections, but when it allocates new pmd page it
cuts 1GiB at once.
This patch makes a copy of all affected pmds from init_mm.
Only few (usually one) 2MiB sections will be lost.
This is not eliminates problem but makes it 512 times less likely.
Usually idmap is used only for a short period. For examle for booting
secondary cpu __turn_mmu_on (executed in physical addresses) jumps to
virtual address of __secondary_switched which calls secondary_start_kernel
which in turn calls cpu_switch_mm and switches to normal pgd from init_mm.
So everything will be fine if these functions aren't intersect with idmap.
Signed-off-by: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
---
arch/arm/mm/idmap.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c
index 8e0e52e..dcd023c 100644
--- a/arch/arm/mm/idmap.c
+++ b/arch/arm/mm/idmap.c
@@ -25,6 +25,9 @@ static void idmap_add_pmd(pud_t *pud, unsigned long addr, unsigned long end,
pr_warning("Failed to allocate identity pmd.\n");
return;
}
+ if (!pud_none(*pud))
+ memcpy(pmd, pmd_offset(pud, 0),
+ PTRS_PER_PMD * sizeof(pmd_t));
pud_populate(&init_mm, pud, pmd);
pmd += pmd_index(addr);
} else
^ permalink raw reply related
* [PATCH 1/2] ARM: LPAE: load upper bits of early TTBR0/TTBR1
From: Konstantin Khlebnikov @ 2014-07-22 15:36 UTC (permalink / raw)
To: linux-arm-kernel
This patch fixes booting when idmap pgd lays above 4gb. Commit
4756dcbfd37 mostly had fixed this, but it'd failed to load upper bits.
Also this fixes adding TTBR1_OFFSET to TTRR1: if lower part overflows
carry flag must be added to the upper part.
Signed-off-by: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
Cc: Cyril Chemparathy <cyril@ti.com>
Cc: Vitaly Andrianov <vitalya@ti.com>
---
arch/arm/mm/proc-v7-3level.S | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mm/proc-v7-3level.S b/arch/arm/mm/proc-v7-3level.S
index 22e3ad6..f0481dd 100644
--- a/arch/arm/mm/proc-v7-3level.S
+++ b/arch/arm/mm/proc-v7-3level.S
@@ -140,12 +140,11 @@ ENDPROC(cpu_v7_set_pte_ext)
mov \tmp, \ttbr1, lsr #(32 - ARCH_PGD_SHIFT) @ upper bits
mov \ttbr1, \ttbr1, lsl #ARCH_PGD_SHIFT @ lower bits
addls \ttbr1, \ttbr1, #TTBR1_OFFSET
- mcrr p15, 1, \ttbr1, \zero, c2 @ load TTBR1
+ adcls \tmp, \tmp, #0
+ mcrr p15, 1, \ttbr1, \tmp, c2 @ load TTBR1
mov \tmp, \ttbr0, lsr #(32 - ARCH_PGD_SHIFT) @ upper bits
mov \ttbr0, \ttbr0, lsl #ARCH_PGD_SHIFT @ lower bits
- mcrr p15, 0, \ttbr0, \zero, c2 @ load TTBR0
- mcrr p15, 1, \ttbr1, \zero, c2 @ load TTBR1
- mcrr p15, 0, \ttbr0, \zero, c2 @ load TTBR0
+ mcrr p15, 0, \ttbr0, \tmp, c2 @ load TTBR0
.endm
/*
^ permalink raw reply related
* [PATCH] Documentation/arm64/memory.txt: fix typo
From: Alex Bennée @ 2014-07-22 15:14 UTC (permalink / raw)
To: linux-arm-kernel
There is no swapper_pgd_dir, it meant swapper_pg_dir.
Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
---
Documentation/arm64/memory.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/arm64/memory.txt b/Documentation/arm64/memory.txt
index d50fa61..dd724c8 100644
--- a/Documentation/arm64/memory.txt
+++ b/Documentation/arm64/memory.txt
@@ -17,7 +17,7 @@ User addresses have bits 63:39 set to 0 while the kernel addresses have
the same bits set to 1. TTBRx selection is given by bit 63 of the
virtual address. The swapper_pg_dir contains only kernel (global)
mappings while the user pgd contains only user (non-global) mappings.
-The swapper_pgd_dir address is written to TTBR1 and never written to
+The swapper_pg_dir address is written to TTBR1 and never written to
TTBR0.
--
2.0.2
^ permalink raw reply related
* [PATCH 13/11] arm64: Add support for 48-bit VA space with 64KB page configuration
From: Jungseok Lee @ 2014-07-22 15:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <AABDB416-D194-4825-8113-4748C10BC339@gmail.com>
On Jul 22, 2014, at 00:09 +900 Catalin Marinas wrote:
> This patch allows support for 3 levels of page tables with 64KB page
> configuration allowing 48-bit VA space. The pgd is no longer a full
> PAGE_SIZE (PTRS_PER_PGD is 64) and (swapper|idmap)_pg_dir are not fully
> populated (pgd_alloc falls back to kzalloc).
>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
One more step :)
Are you going to post the next version of this series including [12/11]
and [13/11] or finalize the series?
In addition, could you put my gmail address in CC in coming patch or
the next version? It help me follow up the series easily.
- Jungseok Lee
^ permalink raw reply
* [RFC PATCH] usb: dwc3: core: allow vendor drivers to check probe status
From: Felipe Balbi @ 2014-07-22 15:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140722135534.GB6817@griffinp-ThinkPad-X1-Carbon-2nd>
On Tue, Jul 22, 2014 at 02:55:34PM +0100, Peter Griffin wrote:
> Hi Felipe,
>
> Sorry for the delay in replying. I've been trying to get to the root cause
> of this problem so I could reply which took longer than I had hoped.
>
> The problem manifested itself as a hang on register read/write access if dwc3-st
> probed before the usb3 phy. Even though dwc3 core would bail and return
> -EPROBE_DEFER that is not propogated up through of_platform_populate.
>
> <snip>
> >
> > yeah, because glue layers are not supposed to know. There should be no
> > coupling what so ever between glue layer and core driver, other than the
> > fact that glue layer is the one which triggers platform_device creation
> > through of_platform_population(). But the glue layer has (or should
> > have) no interest in exactly when the core driver finishes probing.
>
> Thanks for this clue :-) As it got me debugging why there was this dependency
> between the usb3 phy IP and the ST glue register wrapper around the dwc3 usb core.
>
> The reason for the depedency / hang is that there is a shared reset signal
> for the dwc3 core, glue registers and usb3 phy. This reset signal was only
> being managed in the USB3 phy driver, which is why if dwc3-st or dwc3 did
> any register access it would cause a hang.
>
> So the solution is in addition to taking the devm_reset_control for the powerdown
> signal, in V3 of the dwc3-st glue layer, it also gets the softreset signal,
> and deasserts this before any register accesses.
>
> This is now working properly without any init ordering hacks etc.
AWESOME! :-) Thanks for finding that out, it really helps us keep dwc3
clean without platform-specific hacks ;-)
> > > commit message, another way of ensuring the PHYs are available is to
> > > request them, but this would mean an awful lot of code duplication.
> > >
> > > In your opinion, what's the best way to handle this?
> >
> > How can I know ? You still haven't fully explained what you need. All
> > you said was that you're trying to "configure through the glue-layer".
>
> We can forget about this now. Having dwc3-st take a reference on the usb3 phys was
> just another method I was experimenting with to find out whether the usb3
> PHY had probed or not.
>
> >
> > Care to further explain what the problem really is ? I'm assuming below
> > is what you're concerned about which I had to go dig in the archives
> > because there was no reference to that patch anywhere here.
>
> Hopefully I have now above, and the proposed solution.
>
> >
> > > +static void st_dwc3_init(struct st_dwc3 *dwc3_data)
> > > +{
> > > + u32 reg = st_dwc3_readl(dwc3_data->glue_base, USB2_CLKRST_CTRL);
> > > +
> > > + reg |= aux_clk_en(1) | ext_cfg_reset_n(1) | xhci_revision(1);
> >
> > so you have auxiliary clock, an external config reset, what's this
> > xhci_revision ?
>
> xhci_revision is an input signal to the dwc3 core, if it is asserted
> then the host controller compiles with the xHCI revision 1.0 spec, if
> not it complies with xHCI revision 0.96 spec. This input signal to
> dwc3 core is exposed in the CLKRST_CTRL glue register wrapped around
> the controller by ST.
I wonder why would HW folks give SW access to that, though. Oh well,
I've seen weirder things ;-)
> Looking through the docs, it was present until 2.40a, then removed as
> an input signal to the core from 2.50a onwards.
>
> To make this clearer I have also added a comment above the
> xhci_revision macro in V3 of the dwc3-st patches explaining the
> bitfield and what it does.
thanks
> > looks like it should be split between a CCF and reset drivers. Or maybe
> > a single driver which does both. Do you have a clock/reset control for
> > all IPs ?
>
> Yes most IPs which have reset or powerdown signals are already
> controlled by a driver in drivers/reset/sti. These reset and powerdown
> signals are all exposed in the sysconfig registers of the SoC. Indeed
> it was a shared reset signal which wasn't being properly managed and
> causing the hang.
>
> However the reset signal and clock gate here is controlling a small
> piece of wrapper IP called pipew which sits between the dwc3 core and
> usb3 phy. I believe this pipew protocol wrapper hardware is designed
> internally by ST, and has some special contriants which is why these
> reset signals are being exposed here in the glue logic (see below).
>
> > That might be a good way to hide stuff, driver would simply
> > call clk_get()/clk_prepare_enable() and reset_assert()/deassert() when
> > necessary (sure, this doesn't solve the 'when has that guy probe' but
> > you still haven't explained why you need it).
> >
> > > + reg = st_dwc3_readl(dwc3_data->glue_base, USB2_VBUS_MNGMNT_SEL1);
> > > + reg |= SEL_OVERRIDE_VBUSVALID(1) | SEL_OVERRIDE_POWERPRESENT(1) |
> > > + SEL_OVERRIDE_BVALID(1);
> >
> > this is not correct. You don't know if VBUS is really valid at this
> > time. We have used a gpio which gets pull high/low depending on the
> > state of VBUS/ID.
>
> This isn't stating that VBUS is valid, it is configuring a mux to
> select where the vbus / bvalid / powerpresent signals will be selected
> from.
/me now notices the "SEL_" prefix :-)
> I have added a better comment in V3 which hopefully makes the function
> of VBUS_MNGMNT_SEL register clearer.
thanks
> > > + st_dwc3_writel(dwc3_data->glue_base, USB2_VBUS_MNGMNT_SEL1, reg);
> > > + udelay(100);
> > > +
> > > + reg = st_dwc3_readl(dwc3_data->glue_base, USB2_CLKRST_CTRL);
> > > + reg |= sw_pipew_reset_n(1);
> > > + st_dwc3_writel(dwc3_data->glue_base, USB2_CLKRST_CTRL, reg);
> >
> > let me ask you something else. Isn't the DWC3_GUSB3PIPECTL_PHYSOFTRST
> > bit functional for you guys ? This sw_pipe2_reset_n looks suspicious.
>
> Your right to be suspicious ;-)
>
> Due to a constriant on the pipew hardware, they have provided two
> extra software controlled resets ext_cfg_reset and sw_pipew_reset in
> the CLKRST_CTRL glue reg.
>
> These two software controlled resets are ANDED with the nominal
> cfg_reset_n and pipe_reset_n resets to the pipew hardware.
>
> So yes DWC3_GUSB3PIPECTL_PHYSOFTRST is functional, but it will only
> actually issue a reset to pipew and then onto MiPHY if
> sw_pipew_reset_n is also set in the glue. The same goes with the
> global bus_reset_n signal which is subsystem wide reset, it will only
> bepropogated to pipew if ext_cfg_reset is also set in CLKRST_CTRL.
oh man, what a mess :-)
> Hopefully that makes things clearer and I've answered everything. I
> intend to send a V3 shortly.
sure, thanks
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140722/5eba7994/attachment.sig>
^ permalink raw reply
* [PATCH 13/11] arm64: Add support for 48-bit VA space with 64KB page configuration
From: Jungseok Lee @ 2014-07-22 14:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <ADA477FA-165D-4679-A38F-C82C901BEE4D@gmail.com>
On Jul 22, 2014, at 00:09 +900 Catalin Marinas wrote:
> This patch allows support for 3 levels of page tables with 64KB page
> configuration allowing 48-bit VA space. The pgd is no longer a full
> PAGE_SIZE (PTRS_PER_PGD is 64) and (swapper|idmap)_pg_dir are not fully
> populated (pgd_alloc falls back to kzalloc).
>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
One more step :)
Are you going to post the next version of this series including [12/11]
and [13/11] or finalize the series?
In addition, could you put my gmail address in CC in coming patch or
the next version? It help me follow up the series easily.
- Jungseok Lee
^ permalink raw reply
* [PATCH v2 1/3] usb: dwc3: add ST dwc3 glue layer to manage dwc3 HC
From: Felipe Balbi @ 2014-07-22 14:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140722091800.GB8233@griffinp-ThinkPad-X1-Carbon-2nd>
Hi,
On Tue, Jul 22, 2014 at 10:18:00AM +0100, Peter Griffin wrote:
> > > +static inline u32 st_dwc3_readl(void __iomem *base, u32 offset)
> > > +{
> > > + return readl_relaxed(base + offset);
> > > +}
> > > +
> > > +static inline void st_dwc3_writel(void __iomem *base, u32 offset, u32 value)
> > > +{
> > > + writel_relaxed(value, base + offset);
> > > +}
> >
> > Why are these being abstracted?
> >
> > Just use {read,write}l_relaxed() directly.
>
> Ok, unabstracted in v3
no, no... all other glues add their own local helpers for register
access. This is good for tracing, it's very easy to add a tracepoint to
this sort of function and get very low overhead tracing of every
register access.
> > > + if (dwc3_data->drd_device_conf)
> > > + val |= USB_SET_PORT_DEVICE;
> > > + else
> > > + val &= USB_HOST_DEFAULT_MASK;
> > > +
> > > + return regmap_write(dwc3_data->regmap, dwc3_data->syscfg_reg_off, val);
> > > +}
> > > +
> > > +/**
> > > + * st_dwc3_init: init the controller via glue logic
> > > + * @dwc3_data: driver private structure
> > > + */
> > > +static void st_dwc3_init(struct st_dwc3 *dwc3_data)
> > > +{
> > > + u32 reg = st_dwc3_readl(dwc3_data->glue_base, USB2_CLKRST_CTRL);
> > > +
> > > + reg |= aux_clk_en(1) | ext_cfg_reset_n(1) | xhci_revision(1);
> > > + reg &= ~sw_pipew_reset_n(1);
> >
> > 1? Better to add defines for these magic numbers. What is 1 anyway?
>
> They are just bit setting macros, I've converted them over to use BIT macro now,
> so it no longer takes a parameter.
the macros are better, but make them upper case as everybody else.
> > > + st_dwc3_writel(dwc3_data->glue_base, USB2_CLKRST_CTRL, reg);
> > > +
> > > + reg = st_dwc3_readl(dwc3_data->glue_base, USB2_VBUS_MNGMNT_SEL1);
> > > + reg |= SEL_OVERRIDE_VBUSVALID(1) | SEL_OVERRIDE_POWERPRESENT(1) |
> > > + SEL_OVERRIDE_BVALID(1);
> > > + st_dwc3_writel(dwc3_data->glue_base, USB2_VBUS_MNGMNT_SEL1, reg);
> > > + udelay(100);
> >
> > Why 100?
>
> I've asked ST how this value was derirved and why. It came from validation.
> The docs don't mention that it is necessary, and removing it
> seems to have no ill effects. So I've removed this udelay in v3.
make sure to test with many, many iterations just to make sure.
> > > + reg = st_dwc3_readl(dwc3_data->glue_base, USB2_CLKRST_CTRL);
> > > + reg |= sw_pipew_reset_n(1);
> > > + st_dwc3_writel(dwc3_data->glue_base, USB2_CLKRST_CTRL, reg);
> > > +}
> > > +
> > > +static void st_dwc3_dt_get_pdata(struct platform_device *pdev,
> > > + struct st_dwc3 *dwc3_data)
> > > +{
> > > + struct device_node *np = pdev->dev.of_node;
> > > +
> > > + dwc3_data->drd_device_conf =
> > > + of_property_read_bool(np, "st,dwc3-drd-device");
> >
> > This requires a DT Ack.
>
> Ok. Do the DT folks have any comment on this?
look at the child's dr-mode property instead of adding your own.
> > > + dwc3_data->glue_base = devm_request_and_ioremap(dev, res);
use devm_ioremap_resource()
> > > + regmap = syscon_regmap_lookup_by_phandle(node, "st,syscfg");
> > > + if (IS_ERR(regmap))
> > > + return PTR_ERR(regmap);
> > > +
> > > + dwc3 = platform_device_alloc("st-dwc3", PLATFORM_DEVID_AUTO);
> > > + if (!dwc3) {
> > > + dev_err(&pdev->dev, "couldn't allocate dwc3 device\n");
> > > + return -ENOMEM;
> > > + }
> >
> > I'm confused. What is this doing? Isn't this the DWC3 driver, which
> > already had a platform device structure associated to it? Perhaps a
> > small ASCII diagram describing the layers might be useful.
>
> Your right, this was wrong. It was some legacy code which is
> unnecessary and I've removed this in v3.
if you're going for DT, why don't you create the parent and the child
from DT as omap/exynos/qcom are doing ?
> > > + dma_set_coherent_mask(&dwc3->dev, dev->coherent_dma_mask);
> > > +
> > > + dwc3->dev.parent = &pdev->dev;
> > > + dwc3->dev.dma_mask = pdev->dev.dma_mask;
> > > + dwc3->dev.dma_parms = pdev->dev.dma_parms;
stick to DT device creation. Look into dwc3-omap.c
> > > +static int st_dwc3_remove(struct platform_device *pdev)
> > > +{
> > > + struct st_dwc3 *dwc3_data = platform_get_drvdata(pdev);
> > > +
> > > + platform_device_unregister(dwc3_data->dwc3);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +#ifdef CONFIG_PM_SLEEP
> > > +static int st_dwc3_suspend(struct device *dev)
> > > +{
> > > + struct st_dwc3 *dwc3_data = dev_get_drvdata(dev);
> > > +
> > > + reset_control_assert(dwc3_data->rstc_pwrdn);
> > > +
> > > + pinctrl_pm_select_sleep_state(dev);
pinctrl will select sleep and default states automatically for you.
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140722/9f43e8df/attachment.sig>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox