| Message ID | 20140505120810.GB16461@amd.pavel.ucw.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/05/2014 02:08 PM, Pavel Machek wrote: > Non-TI chips (including socfpga) needs different raminit > sequence. Implement it. > > Tested-by: Thor Thayer <tthayer@altera.com> > Signed-off-by: Thor Thayer <tthayer@altera.com> > Signed-off-by: Pavel Machek <pavel@denx.de> > > diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c > index 1df0b32..476d1e0 100644 > --- a/drivers/net/can/c_can/c_can_platform.c > +++ b/drivers/net/can/c_can/c_can_platform.c > @@ -40,6 +40,7 @@ > #define CAN_RAMINIT_START_MASK(i) (0x001 << (i)) > #define CAN_RAMINIT_DONE_MASK(i) (0x100 << (i)) > #define CAN_RAMINIT_ALL_MASK(i) (0x101 << (i)) > +#define DCAN_RAM_INIT_BIT (1 << 3) > static DEFINE_SPINLOCK(raminit_lock); > /* > * 16-bit c_can registers can be arranged differently in the memory > @@ -80,7 +81,7 @@ static void c_can_hw_raminit_wait(const struct c_can_priv *priv, u32 mask, > udelay(1); > } > > -static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable) > +static void c_can_hw_raminit_ti(const struct c_can_priv *priv, bool enable) > { > u32 mask = CAN_RAMINIT_ALL_MASK(priv->instance); > u32 ctrl; > @@ -88,7 +89,8 @@ static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable) > spin_lock(&raminit_lock); > > ctrl = readl(priv->raminit_ctrlreg); > - /* We clear the done and start bit first. The start bit is > + /* > + * We clear the done and start bit first. The start bit is Please don't reformat comments. > * looking at the 0 -> transition, but is not self clearing; > * And we clear the init done bit as well. > */ > @@ -108,6 +110,54 @@ static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable) > spin_unlock(&raminit_lock); > } > > +static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable) > +{ > + u32 ctrl; > + > + spin_lock(&raminit_lock); Why do you need this spinlock? > + > + ctrl = readl(priv->raminit_ctrlreg); > + ctrl &= ~DCAN_RAM_INIT_BIT; > + writel(ctrl, priv->raminit_ctrlreg); Why don't use use the reg directly? Have you read my previous review? > + c_can_hw_raminit_wait(priv, ctrl, 0); > + > + if (enable) { > + /* Set start bit. */ > + ctrl |= DCAN_RAM_INIT_BIT; > + writel(ctrl, priv->raminit_ctrlreg); > + c_can_hw_raminit_wait(priv, ctrl, 0); > + } > + spin_unlock(&raminit_lock); > +} > + > +static u32 c_can_plat_read_reg32(struct c_can_priv *priv, enum reg index) > +{ > + u32 val; > + > + val = priv->read_reg(priv, index); > + val |= ((u32) priv->read_reg(priv, index + 1)) << 16; > + > + return val; > +} Why are you adding the read32() support here? Your commit message doesn't mention it. Please move this into a different patch. > + > +static void c_can_plat_write_reg32(struct c_can_priv *priv, enum reg index, > + u32 val) > +{ > + priv->write_reg(priv, index + 1, val>>16); > + priv->write_reg(priv, index, val); > +} > + > +static u32 d_can_plat_read_reg32(struct c_can_priv *priv, enum reg index) > +{ > + return readl(priv->base + priv->regs[index]); > +} > + > +static void d_can_plat_write_reg32(struct c_can_priv *priv, enum reg index, > + u32 val) > +{ > + writel(val, priv->base + priv->regs[index]); > +} > + > static struct platform_device_id c_can_id_table[] = { > [BOSCH_C_CAN_PLATFORM] = { > .name = KBUILD_MODNAME, > @@ -201,11 +251,15 @@ static int c_can_plat_probe(struct platform_device *pdev) > case IORESOURCE_MEM_32BIT: > priv->read_reg = c_can_plat_read_reg_aligned_to_32bit; > priv->write_reg = c_can_plat_write_reg_aligned_to_32bit; > + priv->read_reg32 = c_can_plat_read_reg32; > + priv->write_reg32 = c_can_plat_write_reg32; > break; > case IORESOURCE_MEM_16BIT: > default: > priv->read_reg = c_can_plat_read_reg_aligned_to_16bit; > priv->write_reg = c_can_plat_write_reg_aligned_to_16bit; > + priv->read_reg32 = c_can_plat_read_reg32; > + priv->write_reg32 = c_can_plat_write_reg32; > break; > } > break; > @@ -214,6 +268,8 @@ static int c_can_plat_probe(struct platform_device *pdev) > priv->can.ctrlmode_supported |= CAN_CTRLMODE_3_SAMPLES; > priv->read_reg = c_can_plat_read_reg_aligned_to_16bit; > priv->write_reg = c_can_plat_write_reg_aligned_to_16bit; > + priv->read_reg32 = d_can_plat_read_reg32; > + priv->write_reg32 = d_can_plat_write_reg32; > > if (pdev->dev.of_node) > priv->instance = of_alias_get_id(pdev->dev.of_node, "d_can"); > @@ -221,11 +277,22 @@ static int c_can_plat_probe(struct platform_device *pdev) > priv->instance = pdev->id; > > res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + /* > + * Not all D_CAN module have a separate register for the D_CAN > + * RAM initialization. Use default RAM init bit in D_CAN module > + * if not specified in DT. > + */ > + if (!res) { > + priv->raminit = c_can_hw_raminit; > + priv->raminit_ctrlreg = addr + priv->regs[C_CAN_FUNCTION_REG]; > + break; > + } > + > priv->raminit_ctrlreg = devm_ioremap_resource(&pdev->dev, res); > if (IS_ERR(priv->raminit_ctrlreg) || priv->instance < 0) > dev_info(&pdev->dev, "control memory is not used for raminit\n"); > else > - priv->raminit = c_can_hw_raminit; > + priv->raminit = c_can_hw_raminit_ti; > break; > default: > ret = -EINVAL; > Marc
On 05/05/2014 02:21 PM, Marc Kleine-Budde wrote: > On 05/05/2014 02:08 PM, Pavel Machek wrote: >> Non-TI chips (including socfpga) needs different raminit >> sequence. Implement it. >> >> Tested-by: Thor Thayer <tthayer@altera.com> >> Signed-off-by: Thor Thayer <tthayer@altera.com> >> Signed-off-by: Pavel Machek <pavel@denx.de> >> >> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c >> index 1df0b32..476d1e0 100644 >> --- a/drivers/net/can/c_can/c_can_platform.c >> +++ b/drivers/net/can/c_can/c_can_platform.c >> @@ -40,6 +40,7 @@ >> #define CAN_RAMINIT_START_MASK(i) (0x001 << (i)) >> #define CAN_RAMINIT_DONE_MASK(i) (0x100 << (i)) >> #define CAN_RAMINIT_ALL_MASK(i) (0x101 << (i)) >> +#define DCAN_RAM_INIT_BIT (1 << 3) >> static DEFINE_SPINLOCK(raminit_lock); >> /* >> * 16-bit c_can registers can be arranged differently in the memory >> @@ -80,7 +81,7 @@ static void c_can_hw_raminit_wait(const struct c_can_priv *priv, u32 mask, >> udelay(1); >> } >> >> -static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable) >> +static void c_can_hw_raminit_ti(const struct c_can_priv *priv, bool enable) >> { >> u32 mask = CAN_RAMINIT_ALL_MASK(priv->instance); >> u32 ctrl; >> @@ -88,7 +89,8 @@ static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable) >> spin_lock(&raminit_lock); >> >> ctrl = readl(priv->raminit_ctrlreg); >> - /* We clear the done and start bit first. The start bit is >> + /* >> + * We clear the done and start bit first. The start bit is > > Please don't reformat comments. > >> * looking at the 0 -> transition, but is not self clearing; >> * And we clear the init done bit as well. >> */ >> @@ -108,6 +110,54 @@ static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable) >> spin_unlock(&raminit_lock); >> } >> >> +static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable) >> +{ >> + u32 ctrl; >> + >> + spin_lock(&raminit_lock); > > Why do you need this spinlock? > >> + >> + ctrl = readl(priv->raminit_ctrlreg); >> + ctrl &= ~DCAN_RAM_INIT_BIT; >> + writel(ctrl, priv->raminit_ctrlreg); > > Why don't use use the reg directly? Have you read my previous review? > >> + c_can_hw_raminit_wait(priv, ctrl, 0); >> + >> + if (enable) { >> + /* Set start bit. */ >> + ctrl |= DCAN_RAM_INIT_BIT; >> + writel(ctrl, priv->raminit_ctrlreg); >> + c_can_hw_raminit_wait(priv, ctrl, 0); >> + } >> + spin_unlock(&raminit_lock); >> +} >> + >> +static u32 c_can_plat_read_reg32(struct c_can_priv *priv, enum reg index) >> +{ >> + u32 val; >> + >> + val = priv->read_reg(priv, index); >> + val |= ((u32) priv->read_reg(priv, index + 1)) << 16; >> + >> + return val; >> +} > > Why are you adding the read32() support here? Your commit message > doesn't mention it. Please move this into a different patch. > >> + >> +static void c_can_plat_write_reg32(struct c_can_priv *priv, enum reg index, >> + u32 val) >> +{ >> + priv->write_reg(priv, index + 1, val>>16); >> + priv->write_reg(priv, index, val); >> +} >> + >> +static u32 d_can_plat_read_reg32(struct c_can_priv *priv, enum reg index) >> +{ >> + return readl(priv->base + priv->regs[index]); >> +} >> + >> +static void d_can_plat_write_reg32(struct c_can_priv *priv, enum reg index, >> + u32 val) >> +{ >> + writel(val, priv->base + priv->regs[index]); >> +} >> + >> static struct platform_device_id c_can_id_table[] = { >> [BOSCH_C_CAN_PLATFORM] = { >> .name = KBUILD_MODNAME, >> @@ -201,11 +251,15 @@ static int c_can_plat_probe(struct platform_device *pdev) >> case IORESOURCE_MEM_32BIT: >> priv->read_reg = c_can_plat_read_reg_aligned_to_32bit; >> priv->write_reg = c_can_plat_write_reg_aligned_to_32bit; >> + priv->read_reg32 = c_can_plat_read_reg32; >> + priv->write_reg32 = c_can_plat_write_reg32; This will not compile. Marc
On Mon 2014-05-05 14:21:33, Marc Kleine-Budde wrote: > On 05/05/2014 02:08 PM, Pavel Machek wrote: > > Non-TI chips (including socfpga) needs different raminit > > sequence. Implement it. > > > > Tested-by: Thor Thayer <tthayer@altera.com> > > Signed-off-by: Thor Thayer <tthayer@altera.com> > > Signed-off-by: Pavel Machek <pavel@denx.de> > > > > @@ -88,7 +89,8 @@ static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable) > > spin_lock(&raminit_lock); > > > > ctrl = readl(priv->raminit_ctrlreg); > > - /* We clear the done and start bit first. The start bit is > > + /* > > + * We clear the done and start bit first. The start bit is > > Please don't reformat comments. Previous one is not correct coding style. I'd like to get it fixed. priv, bool enable) > > spin_unlock(&raminit_lock); > > } > > > > +static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable) > > +{ > > + u32 ctrl; > > + > > + spin_lock(&raminit_lock); > > Why do you need this spinlock? _ti() used spinlock, so I assume I need it, too. > > + ctrl = readl(priv->raminit_ctrlreg); > > + ctrl &= ~DCAN_RAM_INIT_BIT; > > + writel(ctrl, priv->raminit_ctrlreg); > > Why don't use use the reg directly? Have you read my previous > review? Can you repost it? I don't think I seen it. > > + c_can_hw_raminit_wait(priv, ctrl, 0); > > + > > + if (enable) { > > + /* Set start bit. */ > > + ctrl |= DCAN_RAM_INIT_BIT; > > + writel(ctrl, priv->raminit_ctrlreg); > > + c_can_hw_raminit_wait(priv, ctrl, 0); > > + } > > + spin_unlock(&raminit_lock); > > +} > > + > > +static u32 c_can_plat_read_reg32(struct c_can_priv *priv, enum reg index) > > +{ > > + u32 val; > > + > > + val = priv->read_reg(priv, index); > > + val |= ((u32) priv->read_reg(priv, index + 1)) << 16; > > + > > + return val; > > +} > > Why are you adding the read32() support here? Your commit message > doesn't mention it. Please move this into a different patch. Should be different patch, yes. Pavel
On 05/05/2014 02:58 PM, Pavel Machek wrote: > On Mon 2014-05-05 14:21:33, Marc Kleine-Budde wrote: >> On 05/05/2014 02:08 PM, Pavel Machek wrote: >>> Non-TI chips (including socfpga) needs different raminit >>> sequence. Implement it. >>> >>> Tested-by: Thor Thayer <tthayer@altera.com> >>> Signed-off-by: Thor Thayer <tthayer@altera.com> >>> Signed-off-by: Pavel Machek <pavel@denx.de> >>> >>> @@ -88,7 +89,8 @@ static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable) >>> spin_lock(&raminit_lock); >>> >>> ctrl = readl(priv->raminit_ctrlreg); >>> - /* We clear the done and start bit first. The start bit is >>> + /* >>> + * We clear the done and start bit first. The start bit is >> >> Please don't reformat comments. > > Previous one is not correct coding style. I'd like to get it fixed. net/ and drivers/net use a different multiline commenting style. > > priv, bool enable) >>> spin_unlock(&raminit_lock); >>> } >>> >>> +static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable) >>> +{ >>> + u32 ctrl; >>> + >>> + spin_lock(&raminit_lock); >> >> Why do you need this spinlock? > > _ti() used spinlock, so I assume I need it, too. It's not a shared ressource you're working on. TI does. > >>> + ctrl = readl(priv->raminit_ctrlreg); >>> + ctrl &= ~DCAN_RAM_INIT_BIT; >>> + writel(ctrl, priv->raminit_ctrlreg); >> >> Why don't use use the reg directly? Have you read my previous >> review? > > Can you repost it? I don't think I seen it. Marc
> > + > > + ctrl = readl(priv->raminit_ctrlreg); > > + ctrl &= ~DCAN_RAM_INIT_BIT; > > + writel(ctrl, priv->raminit_ctrlreg); > > Why don't use use the reg directly? Have you read my previous > review? Not really. I did not notice that there are more comments below all that quoted text. Standby. Pavel
diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c index 1df0b32..476d1e0 100644 --- a/drivers/net/can/c_can/c_can_platform.c +++ b/drivers/net/can/c_can/c_can_platform.c @@ -40,6 +40,7 @@ #define CAN_RAMINIT_START_MASK(i) (0x001 << (i)) #define CAN_RAMINIT_DONE_MASK(i) (0x100 << (i)) #define CAN_RAMINIT_ALL_MASK(i) (0x101 << (i)) +#define DCAN_RAM_INIT_BIT (1 << 3) static DEFINE_SPINLOCK(raminit_lock); /* * 16-bit c_can registers can be arranged differently in the memory @@ -80,7 +81,7 @@ static void c_can_hw_raminit_wait(const struct c_can_priv *priv, u32 mask, udelay(1); } -static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable) +static void c_can_hw_raminit_ti(const struct c_can_priv *priv, bool enable) { u32 mask = CAN_RAMINIT_ALL_MASK(priv->instance); u32 ctrl; @@ -88,7 +89,8 @@ static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable) spin_lock(&raminit_lock); ctrl = readl(priv->raminit_ctrlreg); - /* We clear the done and start bit first. The start bit is + /* + * We clear the done and start bit first. The start bit is * looking at the 0 -> transition, but is not self clearing; * And we clear the init done bit as well. */ @@ -108,6 +110,54 @@ static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable) spin_unlock(&raminit_lock); } +static void c_can_hw_raminit(const struct c_can_priv *priv, bool enable) +{ + u32 ctrl; + + spin_lock(&raminit_lock); + + ctrl = readl(priv->raminit_ctrlreg); + ctrl &= ~DCAN_RAM_INIT_BIT; + writel(ctrl, priv->raminit_ctrlreg); + c_can_hw_raminit_wait(priv, ctrl, 0); + + if (enable) { + /* Set start bit. */ + ctrl |= DCAN_RAM_INIT_BIT; + writel(ctrl, priv->raminit_ctrlreg); + c_can_hw_raminit_wait(priv, ctrl, 0); + } + spin_unlock(&raminit_lock); +} + +static u32 c_can_plat_read_reg32(struct c_can_priv *priv, enum reg index) +{ + u32 val; + + val = priv->read_reg(priv, index); + val |= ((u32) priv->read_reg(priv, index + 1)) << 16; + + return val; +} + +static void c_can_plat_write_reg32(struct c_can_priv *priv, enum reg index, + u32 val) +{ + priv->write_reg(priv, index + 1, val>>16); + priv->write_reg(priv, index, val); +} + +static u32 d_can_plat_read_reg32(struct c_can_priv *priv, enum reg index) +{ + return readl(priv->base + priv->regs[index]); +} + +static void d_can_plat_write_reg32(struct c_can_priv *priv, enum reg index, + u32 val) +{ + writel(val, priv->base + priv->regs[index]); +} + static struct platform_device_id c_can_id_table[] = { [BOSCH_C_CAN_PLATFORM] = { .name = KBUILD_MODNAME, @@ -201,11 +251,15 @@ static int c_can_plat_probe(struct platform_device *pdev) case IORESOURCE_MEM_32BIT: priv->read_reg = c_can_plat_read_reg_aligned_to_32bit; priv->write_reg = c_can_plat_write_reg_aligned_to_32bit; + priv->read_reg32 = c_can_plat_read_reg32; + priv->write_reg32 = c_can_plat_write_reg32; break; case IORESOURCE_MEM_16BIT: default: priv->read_reg = c_can_plat_read_reg_aligned_to_16bit; priv->write_reg = c_can_plat_write_reg_aligned_to_16bit; + priv->read_reg32 = c_can_plat_read_reg32; + priv->write_reg32 = c_can_plat_write_reg32; break; } break; @@ -214,6 +268,8 @@ static int c_can_plat_probe(struct platform_device *pdev) priv->can.ctrlmode_supported |= CAN_CTRLMODE_3_SAMPLES; priv->read_reg = c_can_plat_read_reg_aligned_to_16bit; priv->write_reg = c_can_plat_write_reg_aligned_to_16bit; + priv->read_reg32 = d_can_plat_read_reg32; + priv->write_reg32 = d_can_plat_write_reg32; if (pdev->dev.of_node) priv->instance = of_alias_get_id(pdev->dev.of_node, "d_can"); @@ -221,11 +277,22 @@ static int c_can_plat_probe(struct platform_device *pdev) priv->instance = pdev->id; res = platform_get_resource(pdev, IORESOURCE_MEM, 1); + /* + * Not all D_CAN module have a separate register for the D_CAN + * RAM initialization. Use default RAM init bit in D_CAN module + * if not specified in DT. + */ + if (!res) { + priv->raminit = c_can_hw_raminit; + priv->raminit_ctrlreg = addr + priv->regs[C_CAN_FUNCTION_REG]; + break; + } + priv->raminit_ctrlreg = devm_ioremap_resource(&pdev->dev, res); if (IS_ERR(priv->raminit_ctrlreg) || priv->instance < 0) dev_info(&pdev->dev, "control memory is not used for raminit\n"); else - priv->raminit = c_can_hw_raminit; + priv->raminit = c_can_hw_raminit_ti; break; default: ret = -EINVAL;