Re: [PATCH 2/2] can: sja1000: of: add compatibility with Technologic Systems version
On 12/18/2015 10:02 PM, Damien Riegel wrote:
> On Fri, Dec 18, 2015 at 09:41:47PM +0100, Marc Kleine-Budde wrote:
>> On 12/18/2015 09:17 PM, Damien Riegel wrote:
>>> Technologic Systems provides an IP compatible with the SJA1000,
>>> instantiated in an FPGA. Because of some bus widths issue, access to
>>> registers is made through a "window" that works like this:
>>>
>>> base + 0x0: address to read/write
>>> base + 0x2: 8-bit register value
>>>
>>> This commit adds a new compatible device, "technologic,sja1000", with
>>> read and write functions using the window mechanism.
>>>
>>> Signed-off-by: Damien Riegel
>>> ---
>>> drivers/net/can/sja1000/sja1000_platform.c | 30
>>> --
>>> 1 file changed, 28 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/can/sja1000/sja1000_platform.c
>>> b/drivers/net/can/sja1000/sja1000_platform.c
>>> index 0552ed4..6cbf251 100644
>>> --- a/drivers/net/can/sja1000/sja1000_platform.c
>>> +++ b/drivers/net/can/sja1000/sja1000_platform.c
>>> @@ -70,6 +70,18 @@ static void sp_write_reg32(const struct sja1000_priv
>>> *priv, int reg, u8 val)
>>> iowrite8(val, priv->reg_base + reg * 4);
>>> }
>>>
>>> +static u8 ts4800_read_reg16(const struct sja1000_priv *priv, int reg)
>>> +{
>>> + sp_write_reg16(priv, 0, reg);
>>> + return sp_read_reg16(priv, 2);
>>
>> This is racy, please add a spinlock.
>>
>>> +}
>>> +
>>> +static void ts4800_write_reg16(const struct sja1000_priv *priv, int reg,
>>> u8 val)
>>> +{
>>> + sp_write_reg16(priv, 0, reg);
>>> + sp_write_reg16(priv, 2, val);
>>
>> This is racy, too.
>>
>> Have a look at https://marc.info/?l=linux-can&m=137149497403825&w=2
>
> Thank you for the link. In my situation, I don't think there is a race
> issue at the bus level, so a per-device spinlock should be enough. Would
> that be an acceptable patch? It would look a lot like the patch you
> suggested in the thread you linked, just rebased on current version.
Yes, but for a device spinlock you probably have to remove "const" from
the priv pointer.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 2/2] can: sja1000: of: add compatibility with Technologic Systems version
On 12/18/2015 09:17 PM, Damien Riegel wrote:
> Technologic Systems provides an IP compatible with the SJA1000,
> instantiated in an FPGA. Because of some bus widths issue, access to
> registers is made through a "window" that works like this:
>
> base + 0x0: address to read/write
> base + 0x2: 8-bit register value
>
> This commit adds a new compatible device, "technologic,sja1000", with
> read and write functions using the window mechanism.
>
> Signed-off-by: Damien Riegel
> ---
> drivers/net/can/sja1000/sja1000_platform.c | 30
> --
> 1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/can/sja1000/sja1000_platform.c
> b/drivers/net/can/sja1000/sja1000_platform.c
> index 0552ed4..6cbf251 100644
> --- a/drivers/net/can/sja1000/sja1000_platform.c
> +++ b/drivers/net/can/sja1000/sja1000_platform.c
> @@ -70,6 +70,18 @@ static void sp_write_reg32(const struct sja1000_priv
> *priv, int reg, u8 val)
> iowrite8(val, priv->reg_base + reg * 4);
> }
>
> +static u8 ts4800_read_reg16(const struct sja1000_priv *priv, int reg)
> +{
> + sp_write_reg16(priv, 0, reg);
> + return sp_read_reg16(priv, 2);
This is racy, please add a spinlock.
> +}
> +
> +static void ts4800_write_reg16(const struct sja1000_priv *priv, int reg, u8
> val)
> +{
> + sp_write_reg16(priv, 0, reg);
> + sp_write_reg16(priv, 2, val);
This is racy, too.
Have a look at https://marc.info/?l=linux-can&m=137149497403825&w=2
Marc
> +}
> +
> static void sp_populate(struct sja1000_priv *priv,
> struct sja1000_platform_data *pdata,
> unsigned long resource_mem_flags)
> @@ -98,21 +110,34 @@ static void sp_populate(struct sja1000_priv *priv,
>
> static void sp_populate_of(struct sja1000_priv *priv, struct device_node *of)
> {
> + int is_technologic;
> int err;
> u32 prop;
>
> + is_technologic = of_device_is_compatible(of, "technologic,sja1000");
> +
> err = of_property_read_u32(of, "reg-io-width", &prop);
> if (err)
> prop = 1; /* 8 bit is default */
>
> + if (is_technologic && prop != 2) {
> + netdev_warn(priv->dev, "forcing reg-io-width to 2\n");
> + prop = 2;
> + }
> +
> switch (prop) {
> case 4:
> priv->read_reg = sp_read_reg32;
> priv->write_reg = sp_write_reg32;
> break;
> case 2:
> - priv->read_reg = sp_read_reg16;
> - priv->write_reg = sp_write_reg16;
> + if (is_technologic) {
> + priv->read_reg = ts4800_read_reg16;
> + priv->write_reg = ts4800_write_reg16;
> + } else {
> + priv->read_reg = sp_read_reg16;
> + priv->write_reg = sp_write_reg16;
> + }
> break;
> case 1: /* fallthrough */
> default:
> @@ -244,6 +269,7 @@ static int sp_remove(struct platform_device *pdev)
>
> static const struct of_device_id sp_of_table[] = {
> {.compatible = "nxp,sja1000"},
> + {.compatible = "technologic,sja1000"},
> {},
> };
> MODULE_DEVICE_TABLE(of, sp_of_table);
>
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 11/12] MAINTAINERS: add Lucas Stach as maintainer for the etnaviv DRM driver
On 12/04/2015 06:07 PM, Russell King - ARM Linux wrote: > On Fri, Dec 04, 2015 at 03:00:03PM +0100, Lucas Stach wrote: >> Signed-off-by: Lucas Stach > > Acked-by: Russell King > > Although, I would like to be copied on patches, I don't think we > have a way to encode that information in MAINTAINERS. I think you can just add another "M: Russell King " next to Lucas. We have this for the CAN drivers and it seems to work. > >> --- >> MAINTAINERS | 7 +++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index e9caa4b28828..1c69563c61ed 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -3741,6 +3741,13 @@ S:Maintained >> F: drivers/gpu/drm/sti >> F: Documentation/devicetree/bindings/display/st,stih4xx.txt >> >> +DRM DRIVERS FOR VIVANTE GPU IP >> +M: Lucas Stach >> +L: dri-de...@lists.freedesktop.org >> +S: Maintained >> +F: drivers/gpu/drm/etnaviv >> +F: Documentation/devicetree/bindings/display/etnaviv >> + >> DSBR100 USB FM RADIO DRIVER >> M: Alexey Klimov >> L: linux-me...@vger.kernel.org >> -- >> 2.6.2 >> > Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH] net, can, ti_hecc: add DT support for the ti,hecc controller
On 10/20/2015 05:18 PM, Marc Kleine-Budde wrote: > On 10/20/2015 05:09 PM, Anton.Glukhov wrote: >>>> It's OMAP3 based arch, but HECC is implemented only in AM3505 and AM3517 >>>> SoCs. >>>> So, I'm confused about what's "name" should I use. >>> >>> Which SoC was available first? Pick that. > >> What do you mean available? I know only that HECC appear in AM3505 and >> AM3517. Nowhere else. > > Available on the market. Which one was produced first or was soldered > first to some PCB? A colleague just looked it up, a AM3517 is a AM3505 with 3d graphics. So pick AM3505. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH] net, can, ti_hecc: add DT support for the ti,hecc controller
On 10/20/2015 05:09 PM, Anton.Glukhov wrote: >>> It's OMAP3 based arch, but HECC is implemented only in AM3505 and AM3517 >>> SoCs. >>> So, I'm confused about what's "name" should I use. >> >> Which SoC was available first? Pick that. > What do you mean available? I know only that HECC appear in AM3505 and > AM3517. Nowhere else. Available on the market. Which one was produced first or was soldered first to some PCB? Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH] net, can, ti_hecc: add DT support for the ti,hecc controller
On 10/20/2015 04:57 PM, Anton.Glukhov wrote:
> Hello Marc, Heiko!
> I'm sorry for the delay!
>
> On 19.10.2015 10:31, Marc Kleine-Budde wrote:
>> On 10/19/2015 09:27 AM, Heiko Schocher wrote:
>>>>> .../devicetree/bindings/net/can/ti_hecc-can.txt| 20 ++
>>>>> arch/arm/boot/dts/am3517.dtsi | 13 +++
>>>>> drivers/net/can/ti_hecc.c | 45
>>>>> +-
>>>>> 3 files changed, 76 insertions(+), 2 deletions(-)
>>>>> create mode 100644
>>>>> Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>>>> b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>>>> new file mode 100644
>>>>> index 000..09fab59
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>>>> @@ -0,0 +1,20 @@
>>>>> +* TI HECC CAN *
>>>>> +
>>>>> +Required properties:
>>>>> + - compatible: Should be "ti,hecc"
>>>>
>>>> We usually put the name of the first SoC this IP core appears in to the
>>>> compatible.
>>>
>>> Ok, so "ti,am335xx-hecc" would be OK?
>>> @Anton: you used "am35x" ... it should be "am35xx"
>>
>> The "xx" is not okay. Give precisely the first SoC Version this IP core
>> was implemented in.
>>
>
> It's OMAP3 based arch, but HECC is implemented only in AM3505 and AM3517 SoCs.
> So, I'm confused about what's "name" should I use.
Which SoC was available first? Pick that.
>>>>> + - reg: Should contain CAN controller registers location and length
>>>>> + - interrupts: Should contain IRQ line for the CAN controller
>>>>
>>>> I'm missing the description of the ti,* properties. I think they are
>>>> required, too. Although the code doesn't enforce it.
>>>
>>> Ok.
>>>
>>>>> +
>>>>> +Example:
>>>>> +
>>>>> + can0: hecc@5c05 {
>>>>> + compatible = "ti,hecc";
>>>>> + reg = <0x5c05 0x4000>;
>>>>> + interrupts = <24>;
>>>>> + ti,hecc_scc_offset = <0>;
>>>>> + ti,hecc_scc_ram_offset = <0x3000>;
>>>>> + ti,hecc_ram_offset = <0x3000>;
>>>>> + ti,hecc_mbx_offset = <0x2000>;
>>>>> + ti,hecc_int_line = <0>;
>>>>> + ti,hecc_version = <1>;
>>>>
>>>> Versioning in the OF world is done via the compatible. Are the offsets a
>>>> per SoC parameter? I'm not sure if it's better to put
>>>> the offsets into the driver.
>>>
>>> I am unsure here too..
>>
>> The devicetree people will hopefully help here.
>>
>
> I added offsets here just make it consistent with platform data in machine
> file.
> Actually it seems that it's not necessary to put offsets in DT file and I can
> move it to driver.
> But again, it was added to keep consistency.
The DT is supposed to be OS independent, copying from platform data to
DT is sometimes not the best way to go. Make yourself heard on the
devicetree mailinglist and figure out what's the best way to go here.
Are the offsets for the AM3505 and AM3517 identical?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH] net, can, ti_hecc: add DT support for the ti,hecc controller
On 10/19/2015 09:27 AM, Heiko Schocher wrote:
>>> .../devicetree/bindings/net/can/ti_hecc-can.txt| 20 ++
>>> arch/arm/boot/dts/am3517.dtsi | 13 +++
>>> drivers/net/can/ti_hecc.c | 45
>>> +-
>>> 3 files changed, 76 insertions(+), 2 deletions(-)
>>> create mode 100644
>>> Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>> b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>> new file mode 100644
>>> index 000..09fab59
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt
>>> @@ -0,0 +1,20 @@
>>> +* TI HECC CAN *
>>> +
>>> +Required properties:
>>> + - compatible: Should be "ti,hecc"
>>
>> We usually put the name of the first SoC this IP core appears in to the
>> compatible.
>
> Ok, so "ti,am335xx-hecc" would be OK?
> @Anton: you used "am35x" ... it should be "am35xx"
The "xx" is not okay. Give precisely the first SoC Version this IP core
was implemented in.
>
>>> + - reg: Should contain CAN controller registers location and length
>>> + - interrupts: Should contain IRQ line for the CAN controller
>>
>> I'm missing the description of the ti,* properties. I think they are
>> required, too. Although the code doesn't enforce it.
>
> Ok.
>
>>> +
>>> +Example:
>>> +
>>> + can0: hecc@5c05 {
>>> + compatible = "ti,hecc";
>>> + reg = <0x5c05 0x4000>;
>>> + interrupts = <24>;
>>> + ti,hecc_scc_offset = <0>;
>>> + ti,hecc_scc_ram_offset = <0x3000>;
>>> + ti,hecc_ram_offset = <0x3000>;
>>> + ti,hecc_mbx_offset = <0x2000>;
>>> + ti,hecc_int_line = <0>;
>>> + ti,hecc_version = <1>;
>>
>> Versioning in the OF world is done via the compatible. Are the offsets a
>> per SoC parameter? I'm not sure if it's better to put
>> the offsets into the driver.
>
> I am unsure here too..
The devicetree people will hopefully help here.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH] net, can, ti_hecc: add DT support for the ti,hecc controller
&data->hecc_ram_offset);
> + of_property_read_u32(np, "ti,hecc_mbx_offset",
> + &data->mbx_offset);
> + of_property_read_u32(np, "ti,hecc_int_line",
> + &data->int_line);
> + of_property_read_u32(np, "ti,hecc_version",
> + &data->version);
I'm missing error handling here.
> + return data;
> + }
> + return (const struct ti_hecc_platform_data *)
> + dev_get_platdata(&pdev->dev);
Is this cast needed?
> +}
> +
> static int ti_hecc_probe(struct platform_device *pdev)
> {
> struct net_device *ndev = (struct net_device *)0;
> struct ti_hecc_priv *priv;
> - struct ti_hecc_platform_data *pdata;
> + const struct ti_hecc_platform_data *pdata;
> struct resource *mem, *irq;
> void __iomem *addr;
> int err = -ENODEV;
>
> - pdata = dev_get_platdata(&pdev->dev);
> + pdata = ti_hecc_can_get_driver_data(pdev);
> if (!pdata) {
> dev_err(&pdev->dev, "No platform data\n");
> goto probe_exit;
> @@ -1040,6 +1080,7 @@ static int ti_hecc_resume(struct platform_device *pdev)
> static struct platform_driver ti_hecc_driver = {
> .driver = {
> .name= DRV_NAME,
> + .of_match_table = of_match_ptr(ti_hecc_can_dt_ids),
> },
> .probe = ti_hecc_probe,
> .remove = ti_hecc_remove,
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v4 2/2] usb: chipidea: imx: refine clock operations to adapt for all platforms
On 10/14/2015 03:57 AM, Peter Chen wrote:
> Some i.mx platforms need three clocks to let controller work, but
> others only need one, refine clock operation to adapt for all
> platforms, it fixes a regression found at i.mx27.
>
> Signed-off-by: Peter Chen
> Tested-by: Fabio Estevam
> Cc: #v4.1+
> ---
> drivers/usb/chipidea/ci_hdrc_imx.c | 132
> -
> 1 file changed, 114 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> b/drivers/usb/chipidea/ci_hdrc_imx.c
> index 6ccbf60..82b1dfe 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -84,6 +84,12 @@ struct ci_hdrc_imx_data {
> struct imx_usbmisc_data *usbmisc_data;
> bool supports_runtime_pm;
> bool in_lpm;
> + /* SoC before i.mx6 (except imx23/imx28) needs three clks */
> + bool need_three_clks;
> + struct clk *clk_ipg;
> + struct clk *clk_ahb;
> + struct clk *clk_per;
> + /* - */
> };
>
> /* Common functions shared by usbmisc drivers */
> @@ -135,6 +141,103 @@ static struct imx_usbmisc_data
> *usbmisc_get_init_data(struct device *dev)
> }
>
> /* End of common functions shared by usbmisc drivers*/
> +static int imx_get_clks(struct device *dev)
> +{
> + struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
> + int ret = 0;
> +
> + data->clk_ipg = devm_clk_get(dev, "ipg");
> + if (IS_ERR(data->clk_ipg)) {
> + /* If the platform only needs one clocks */
> + data->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(data->clk)) {
> + ret = PTR_ERR(data->clk);
> + dev_err(dev,
> + "Failed to get clock, err=%d\n", ret);
> + dev_err(dev,
> + "Failed to get ipg clock, err=%d\n", ret);
Nitpick, one error message should be enough.
> + return ret;
> + }
> + return ret;
> + }
> +
> + data->clk_ahb = devm_clk_get(dev, "ahb");
> + if (IS_ERR(data->clk_ahb)) {
> + ret = PTR_ERR(data->clk_ahb);
> + dev_err(dev,
> + "Failed to get ahb clock, err=%d\n", ret);
> + return ret;
> + }
> +
> + data->clk_per = devm_clk_get(dev, "per");
> + if (IS_ERR(data->clk_per)) {
> + ret = PTR_ERR(data->clk_per);
> + dev_err(dev,
> + "Failed to get per clock, err=%d\n", ret);
> + return ret;
> + }
> +
> + data->need_three_clks = true;
> + return ret;
> +}
> +
> +static int imx_prepare_enable_clks(struct device *dev)
> +{
> + struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
> + int ret = 0;
> +
> + if (data->need_three_clks) {
> + ret = clk_prepare_enable(data->clk_ipg);
> + if (ret) {
> + dev_err(dev,
> + "Failed to prepare/enable ipg clk, err=%d\n",
> + ret);
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(data->clk_ahb);
> + if (ret) {
> + dev_err(dev,
> + "Failed to prepare/enable ahb clk, err=%d\n",
> + ret);
> + clk_disable_unprepare(data->clk_ipg);
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(data->clk_per);
> + if (ret) {
> + dev_err(dev,
> + "Failed to prepare/enable per clk, err=%d\n",
> + ret);
> + clk_disable_unprepare(data->clk_ahb);
> + clk_disable_unprepare(data->clk_ipg);
> + return ret;
> + }
> + } else {
> + ret = clk_prepare_enable(data->clk);
> + if (ret) {
> + dev_err(dev,
> + "Failed to prepare/enable clk, err=%d\n",
> + ret);
> + return ret;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static void imx_disable_unprepare_clks(struct device *dev)
> +{
> + struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
> +
> + if (data->need_three_clks) {
> +
Re: [PATCH v7 1/7] ARM: dts: imx: imx7d-sbd remove fixed can2-3v3 regulator
On 10/13/2015 02:04 PM, Dong Aisheng wrote: > On Tue, Sep 29, 2015 at 5:56 AM, Adrian Alonso wrote: >> Remove incorrect can2-3v3 fixed regulator, imx7d-sdb doesn't >> have a dedicated can2 fixed regulator instead it shares PERI_3V3 >> fixed regulator (RT8070ZS) which is enabled by default (hardwired) >> from pmic pfuze3000 NVCC_3V3 power rail. >> >> Signed-off-by: Adrian Alonso >> --- >> Changes for V7: New patch in series >> > > It is a correct regulator here, but i have to admit that it's a bit confusing. > Here the regulator is an abstract regulator which is actually used to > control CAN > transceiver STBY signals, it is introduced by below patch: > commit b7c4114b07bbacfe0aee1d04ad1ade9e42309620 > Author: Fabio Estevam > Date: Mon Jun 10 23:12:57 2013 -0300 > > can: flexcan: Use a regulator to control the CAN transceiver > > Instead of using a GPIO to turn on/off the CAN transceiver, it is better > to > use a regulator as some systems may use a PMIC to power the CAN > transceiver. > > Acked-by: Shawn Guo > Signed-off-by: Fabio Estevam > Signed-off-by: Marc Kleine-Budde > > From the commit messages, it should be likely to control the > transceiver external suppy, > not control signals, but it does do that unproper work currently. > > Besides the unproperly handling control signals, this solution also can't > handle > multiple CAN transceiver control signals ways. You can chain several transceivers together if you need to control more than one GPIO line. > I was trying to write a common transceiver driver to handling this issue. > But get interrupted by a few other FSL official works, will resume soon. Have a look at drivers/phy. > I would suggest keep the regulator first until the new solution got in. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH v6 3/3] I2C: mediatek: Add driver for MediaTek MT8173 I2C controller
On 04/28/2015 10:31 AM, Eddie Huang wrote:
> Add mediatek MT8173 I2C controller driver. Compare to I2C controller
> of earlier mediatek SoC, MT8173 fix write-then-read limitation, and
> also increase message size to 64kb.
>
> Signed-off-by: Xudong Chen
> Signed-off-by: Liguo Zhang
> Signed-off-by: Eddie Huang
> ---
> drivers/i2c/busses/i2c-mt65xx.c | 106
> +---
> 1 file changed, 77 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> index 2ecf0d1..c501421 100644
> --- a/drivers/i2c/busses/i2c-mt65xx.c
> +++ b/drivers/i2c/busses/i2c-mt65xx.c
[...]
> static const struct of_device_id mtk_i2c_of_match[] = {
> { .compatible = "mediatek,mt6577-i2c", .data = (void *)&mt6577_compat },
> { .compatible = "mediatek,mt6589-i2c", .data = (void *)&mt6589_compat },
> + { .compatible = "mediatek,mt8173-i2c", .data = (void *)&mt8173_compat },
> {}
> };
> -MODULE_DEVICE_TABLE(of, mtk_i2c_match);
> +MODULE_DEVICE_TABLE(of, mtk_i2c_of_match);
This should go into the previous patch, as 2/3 will probably not compile
without this change.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/2] SPI: control CS via standard GPIO operations instead of SPI-HW
On 03/04/2015 05:40 PM, ker...@martin.sperl.org wrote:
> From: Martin Sperl
>
> Allow the cs-gpios property in DT to be used instead of the
> fixed two chip-selects provided by the SPI-HW itself
>
> Signed-off-by: Martin Sperl
>
> ---
>
> There is the question if we still need to support the chip_selects
> provided by the hardware (plus the buggy CSPOL_HIGH support for those cases)
> or if we could just make the cs-gpios a required setting for this driver.
> Going with the GPIO only solution would clean up the code a bit.
>
> drivers/spi/spi-bcm2835.c | 53
> +++--
> 1 file changed, 46 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
> index 419a782..128a152 100644
> --- a/drivers/spi/spi-bcm2835.c
> +++ b/drivers/spi/spi-bcm2835.c
> @@ -3,6 +3,7 @@
> *
> * Copyright (C) 2012 Chris Boot
> * Copyright (C) 2013 Stephen Warren
> + * Copyright (C) 2015 Martin Sperl
> *
> * This driver is inspired by:
> * spi-ath79.c, Copyright (C) 2009-2011 Gabor Juhos
> @@ -30,6 +31,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> /* SPI register offsets */
> @@ -116,6 +118,18 @@ static inline void bcm2835_wr_fifo(struct bcm2835_spi
> *bs, int len)
> }
> }
>
> +/* ideally spi_set_cs would be exported by spi-core */
> +static inline void bcm2835_set_cs(struct spi_device *spi, bool enable)
> +{
> +
> + if (spi->mode & SPI_CS_HIGH)
> + enable = !enable;
> +
> + if (spi->cs_gpio >= 0)
You might want to use gpio_is_valid() instead of open coding it.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v5] can: Convert to runtime_pm
On 01/12/2015 04:04 PM, Kedareswara rao Appana wrote: > Instead of enabling/disabling clocks at several locations in the driver, > Use the runtime_pm framework. This consolidates the actions for runtime PM > In the appropriate callbacks and makes the driver more readable and > mantainable. > > Signed-off-by: Soren Brinkmann > Signed-off-by: Kedareswara rao Appana > --- > Changes for v5: > - Updated with the review comments. >Updated the remove fuction to use runtime_pm. > Chnages for v4: > - Updated with the review comments. > Changes for v3: > - Converted the driver to use runtime_pm. > Changes for v2: > - Removed the struct platform_device* from suspend/resume > as suggest by Lothar. Any plans to submit a v6? Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH v5] can: Convert to runtime_pm
On 01/13/2015 06:49 PM, Sören Brinkmann wrote:
> On Tue, 2015-01-13 at 06:44PM +0100, Marc Kleine-Budde wrote:
>> On 01/13/2015 06:24 PM, Sören Brinkmann wrote:
>>> On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote:
>>>> On 01/13/2015 06:08 PM, Sören Brinkmann wrote:
>>>>> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
>>>>>> On 01/12/2015 07:45 PM, Sören Brinkmann wrote:
>>>>>>> On Mon, 2015-01-12 at 08:34PM +0530, Kedareswara rao Appana wrote:
>>>>>>>> Instead of enabling/disabling clocks at several locations in the
>>>>>>>> driver,
>>>>>>>> Use the runtime_pm framework. This consolidates the actions for
>>>>>>>> runtime PM
>>>>>>>> In the appropriate callbacks and makes the driver more readable and
>>>>>>>> mantainable.
>>>>>>>>
>>>>>>>> Signed-off-by: Soren Brinkmann
>>>>>>>> Signed-off-by: Kedareswara rao Appana
>>>>>>>> ---
>>>>>>>> Changes for v5:
>>>>>>>> - Updated with the review comments.
>>>>>>>>Updated the remove fuction to use runtime_pm.
>>>>>>>> Chnages for v4:
>>>>>>>> - Updated with the review comments.
>>>>>>>> Changes for v3:
>>>>>>>> - Converted the driver to use runtime_pm.
>>>>>>>> Changes for v2:
>>>>>>>> - Removed the struct platform_device* from suspend/resume
>>>>>>>> as suggest by Lothar.
>>>>>>>>
>>>>>>>> drivers/net/can/xilinx_can.c | 157
>>>>>>>> -
>>>>>>>> 1 files changed, 107 insertions(+), 50 deletions(-)
>>>>>>> [..]
>>>>>>>> +static int __maybe_unused xcan_runtime_resume(struct device *dev)
>>>>>>>> {
>>>>>>>> - struct platform_device *pdev = dev_get_drvdata(dev);
>>>>>>>> - struct net_device *ndev = platform_get_drvdata(pdev);
>>>>>>>> + struct net_device *ndev = dev_get_drvdata(dev);
>>>>>>>>struct xcan_priv *priv = netdev_priv(ndev);
>>>>>>>>int ret;
>>>>>>>> + u32 isr, status;
>>>>>>>>
>>>>>>>>ret = clk_enable(priv->bus_clk);
>>>>>>>>if (ret) {
>>>>>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct
>>>>>>>> device *dev)
>>>>>>>>ret = clk_enable(priv->can_clk);
>>>>>>>>if (ret) {
>>>>>>>>dev_err(dev, "Cannot enable clock.\n");
>>>>>>>> - clk_disable_unprepare(priv->bus_clk);
>>>>>>>> + clk_disable(priv->bus_clk);
>>>>>>> [...]
>>>>>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device
>>>>>>>> *pdev)
>>>>>>>> {
>>>>>>>>struct net_device *ndev = platform_get_drvdata(pdev);
>>>>>>>>struct xcan_priv *priv = netdev_priv(ndev);
>>>>>>>> + int ret;
>>>>>>>> +
>>>>>>>> + ret = pm_runtime_get_sync(&pdev->dev);
>>>>>>>> + if (ret < 0) {
>>>>>>>> + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
>>>>>>>> + __func__, ret);
>>>>>>>> + return ret;
>>>>>>>> + }
>>>>>>>>
>>>>>>>>if (set_reset_mode(ndev) < 0)
>>>>>>>>netdev_err(ndev, "mode resetting failed!\n");
>>>>>>>>
>>>>>>>>unregister_candev(ndev);
>>>>>>>> + pm_runtime_disable(&pdev->dev);
>>>>>>>>netif_napi_del(&priv->napi);
>>>>>>>> + clk_disable_unprepare(priv->bus_clk);
>>>>>>>> + clk_disable_unprepare(priv->can_clk);
>>
Re: [PATCH v5] can: Convert to runtime_pm
On 01/13/2015 06:24 PM, Sören Brinkmann wrote:
> On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote:
>> On 01/13/2015 06:08 PM, Sören Brinkmann wrote:
>>> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
>>>> On 01/12/2015 07:45 PM, Sören Brinkmann wrote:
>>>>> On Mon, 2015-01-12 at 08:34PM +0530, Kedareswara rao Appana wrote:
>>>>>> Instead of enabling/disabling clocks at several locations in the driver,
>>>>>> Use the runtime_pm framework. This consolidates the actions for runtime
>>>>>> PM
>>>>>> In the appropriate callbacks and makes the driver more readable and
>>>>>> mantainable.
>>>>>>
>>>>>> Signed-off-by: Soren Brinkmann
>>>>>> Signed-off-by: Kedareswara rao Appana
>>>>>> ---
>>>>>> Changes for v5:
>>>>>> - Updated with the review comments.
>>>>>>Updated the remove fuction to use runtime_pm.
>>>>>> Chnages for v4:
>>>>>> - Updated with the review comments.
>>>>>> Changes for v3:
>>>>>> - Converted the driver to use runtime_pm.
>>>>>> Changes for v2:
>>>>>> - Removed the struct platform_device* from suspend/resume
>>>>>> as suggest by Lothar.
>>>>>>
>>>>>> drivers/net/can/xilinx_can.c | 157
>>>>>> -
>>>>>> 1 files changed, 107 insertions(+), 50 deletions(-)
>>>>> [..]
>>>>>> +static int __maybe_unused xcan_runtime_resume(struct device *dev)
>>>>>> {
>>>>>> -struct platform_device *pdev = dev_get_drvdata(dev);
>>>>>> -struct net_device *ndev = platform_get_drvdata(pdev);
>>>>>> +struct net_device *ndev = dev_get_drvdata(dev);
>>>>>> struct xcan_priv *priv = netdev_priv(ndev);
>>>>>> int ret;
>>>>>> +u32 isr, status;
>>>>>>
>>>>>> ret = clk_enable(priv->bus_clk);
>>>>>> if (ret) {
>>>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct
>>>>>> device *dev)
>>>>>> ret = clk_enable(priv->can_clk);
>>>>>> if (ret) {
>>>>>> dev_err(dev, "Cannot enable clock.\n");
>>>>>> -clk_disable_unprepare(priv->bus_clk);
>>>>>> +clk_disable(priv->bus_clk);
>>>>> [...]
>>>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device
>>>>>> *pdev)
>>>>>> {
>>>>>> struct net_device *ndev = platform_get_drvdata(pdev);
>>>>>> struct xcan_priv *priv = netdev_priv(ndev);
>>>>>> +int ret;
>>>>>> +
>>>>>> +ret = pm_runtime_get_sync(&pdev->dev);
>>>>>> +if (ret < 0) {
>>>>>> +netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
>>>>>> +__func__, ret);
>>>>>> +return ret;
>>>>>> +}
>>>>>>
>>>>>> if (set_reset_mode(ndev) < 0)
>>>>>> netdev_err(ndev, "mode resetting failed!\n");
>>>>>>
>>>>>> unregister_candev(ndev);
>>>>>> +pm_runtime_disable(&pdev->dev);
>>>>>> netif_napi_del(&priv->napi);
>>>>>> +clk_disable_unprepare(priv->bus_clk);
>>>>>> +clk_disable_unprepare(priv->can_clk);
>>>>>
>>>>> Shouldn't pretty much all these occurrences of clk_disable/enable
>>>>> disappear? This should all be handled by the runtime_pm framework now.
>>>>
>>>> We have:
>>>> - clk_prepare_enable() in probe
>>>
>>> This should become something like pm_runtime_get_sync(), shouldn't it?
>>>
>>>> - clk_disable_unprepare() in remove
>>>
>>> pm_runtime_put()
>>>
>>>> - clk_enable() in runtime_resume
>>>> - clk_disable() in runtime_suspend
>>>
>>> These are the ones
Re: [PATCH v5] can: Convert to runtime_pm
On 01/13/2015 06:08 PM, Sören Brinkmann wrote:
> On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
>> On 01/12/2015 07:45 PM, Sören Brinkmann wrote:
>>> On Mon, 2015-01-12 at 08:34PM +0530, Kedareswara rao Appana wrote:
>>>> Instead of enabling/disabling clocks at several locations in the driver,
>>>> Use the runtime_pm framework. This consolidates the actions for runtime PM
>>>> In the appropriate callbacks and makes the driver more readable and
>>>> mantainable.
>>>>
>>>> Signed-off-by: Soren Brinkmann
>>>> Signed-off-by: Kedareswara rao Appana
>>>> ---
>>>> Changes for v5:
>>>> - Updated with the review comments.
>>>>Updated the remove fuction to use runtime_pm.
>>>> Chnages for v4:
>>>> - Updated with the review comments.
>>>> Changes for v3:
>>>> - Converted the driver to use runtime_pm.
>>>> Changes for v2:
>>>> - Removed the struct platform_device* from suspend/resume
>>>> as suggest by Lothar.
>>>>
>>>> drivers/net/can/xilinx_can.c | 157
>>>> -
>>>> 1 files changed, 107 insertions(+), 50 deletions(-)
>>> [..]
>>>> +static int __maybe_unused xcan_runtime_resume(struct device *dev)
>>>> {
>>>> - struct platform_device *pdev = dev_get_drvdata(dev);
>>>> - struct net_device *ndev = platform_get_drvdata(pdev);
>>>> + struct net_device *ndev = dev_get_drvdata(dev);
>>>>struct xcan_priv *priv = netdev_priv(ndev);
>>>>int ret;
>>>> + u32 isr, status;
>>>>
>>>>ret = clk_enable(priv->bus_clk);
>>>>if (ret) {
>>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct
>>>> device *dev)
>>>>ret = clk_enable(priv->can_clk);
>>>>if (ret) {
>>>>dev_err(dev, "Cannot enable clock.\n");
>>>> - clk_disable_unprepare(priv->bus_clk);
>>>> + clk_disable(priv->bus_clk);
>>> [...]
>>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device
>>>> *pdev)
>>>> {
>>>>struct net_device *ndev = platform_get_drvdata(pdev);
>>>>struct xcan_priv *priv = netdev_priv(ndev);
>>>> + int ret;
>>>> +
>>>> + ret = pm_runtime_get_sync(&pdev->dev);
>>>> + if (ret < 0) {
>>>> + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
>>>> + __func__, ret);
>>>> + return ret;
>>>> + }
>>>>
>>>>if (set_reset_mode(ndev) < 0)
>>>>netdev_err(ndev, "mode resetting failed!\n");
>>>>
>>>>unregister_candev(ndev);
>>>> + pm_runtime_disable(&pdev->dev);
>>>>netif_napi_del(&priv->napi);
>>>> + clk_disable_unprepare(priv->bus_clk);
>>>> + clk_disable_unprepare(priv->can_clk);
>>>
>>> Shouldn't pretty much all these occurrences of clk_disable/enable
>>> disappear? This should all be handled by the runtime_pm framework now.
>>
>> We have:
>> - clk_prepare_enable() in probe
>
> This should become something like pm_runtime_get_sync(), shouldn't it?
>
>> - clk_disable_unprepare() in remove
>
> pm_runtime_put()
>
>> - clk_enable() in runtime_resume
>> - clk_disable() in runtime_suspend
>
> These are the ones needed.
>
> The above makes me suspect that the clocks are always on, regardless of
Define "on" :)
The clocks are prepared after probe() exists, but not enabled. The first
pm_runtime_get_sync() will enable the clocks.
> the runtime suspend state since they are enabled in probe and disabled
> in remove, is that right? Ideally, the usage in probe and remove should
> be migrated to runtime_pm and clocks should really only be running when
> needed and not throughout the whole lifetime of the driver.
The clocks are not en/disabled via pm_runtime, because
pm_runtime_get_sync() is called from atomic contect. We can have another
look into the driver and try to change this.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v5] can: Convert to runtime_pm
On 01/12/2015 07:45 PM, Sören Brinkmann wrote:
> On Mon, 2015-01-12 at 08:34PM +0530, Kedareswara rao Appana wrote:
>> Instead of enabling/disabling clocks at several locations in the driver,
>> Use the runtime_pm framework. This consolidates the actions for runtime PM
>> In the appropriate callbacks and makes the driver more readable and
>> mantainable.
>>
>> Signed-off-by: Soren Brinkmann
>> Signed-off-by: Kedareswara rao Appana
>> ---
>> Changes for v5:
>> - Updated with the review comments.
>>Updated the remove fuction to use runtime_pm.
>> Chnages for v4:
>> - Updated with the review comments.
>> Changes for v3:
>> - Converted the driver to use runtime_pm.
>> Changes for v2:
>> - Removed the struct platform_device* from suspend/resume
>> as suggest by Lothar.
>>
>> drivers/net/can/xilinx_can.c | 157
>> -
>> 1 files changed, 107 insertions(+), 50 deletions(-)
> [..]
>> +static int __maybe_unused xcan_runtime_resume(struct device *dev)
>> {
>> -struct platform_device *pdev = dev_get_drvdata(dev);
>> -struct net_device *ndev = platform_get_drvdata(pdev);
>> +struct net_device *ndev = dev_get_drvdata(dev);
>> struct xcan_priv *priv = netdev_priv(ndev);
>> int ret;
>> +u32 isr, status;
>>
>> ret = clk_enable(priv->bus_clk);
>> if (ret) {
>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device
>> *dev)
>> ret = clk_enable(priv->can_clk);
>> if (ret) {
>> dev_err(dev, "Cannot enable clock.\n");
>> -clk_disable_unprepare(priv->bus_clk);
>> +clk_disable(priv->bus_clk);
> [...]
>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
>> {
>> struct net_device *ndev = platform_get_drvdata(pdev);
>> struct xcan_priv *priv = netdev_priv(ndev);
>> +int ret;
>> +
>> +ret = pm_runtime_get_sync(&pdev->dev);
>> +if (ret < 0) {
>> +netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
>> +__func__, ret);
>> +return ret;
>> +}
>>
>> if (set_reset_mode(ndev) < 0)
>> netdev_err(ndev, "mode resetting failed!\n");
>>
>> unregister_candev(ndev);
>> +pm_runtime_disable(&pdev->dev);
>> netif_napi_del(&priv->napi);
>> +clk_disable_unprepare(priv->bus_clk);
>> +clk_disable_unprepare(priv->can_clk);
>
> Shouldn't pretty much all these occurrences of clk_disable/enable
> disappear? This should all be handled by the runtime_pm framework now.
We have:
- clk_prepare_enable() in probe
- clk_disable_unprepare() in remove
- clk_enable() in runtime_resume
- clk_disable() in runtime_suspend
Which is, as far as I understand the right way to do it. Maybe
Kedareswara can post the clock debug output again with this patch
iteration. Have I missed something?
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v5] can: Convert to runtime_pm
XCAN_SRR_RESET_MASK);
> + } else if ((status & XCAN_SR_ESTAT_MASK) ==
> + XCAN_SR_ESTAT_MASK) {
> + priv->can.state = CAN_STATE_ERROR_PASSIVE;
> + } else if (status & XCAN_SR_ERRWRN_MASK) {
> + priv->can.state = CAN_STATE_ERROR_WARNING;
> + } else {
> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> + }
> netif_device_attach(ndev);
> netif_start_queue(ndev);
> }
> @@ -1030,7 +1059,10 @@ static int __maybe_unused xcan_resume(struct device
> *dev)
> return 0;
> }
>
> -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend, xcan_resume);
> +static const struct dev_pm_ops xcan_dev_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
> + SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL)
> +};
>
> /**
> * xcan_probe - Platform registration call
> @@ -1071,7 +1103,7 @@ static int xcan_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> priv = netdev_priv(ndev);
> - priv->dev = ndev;
> + priv->dev = &pdev->dev;
> priv->can.bittiming_const = &xcan_bittiming_const;
> priv->can.do_set_mode = xcan_do_set_mode;
> priv->can.do_get_berr_counter = xcan_get_berr_counter;
> @@ -1137,6 +1169,16 @@ static int xcan_probe(struct platform_device *pdev)
>
> netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
>
> + pm_runtime_set_active(&pdev->dev);
> + pm_runtime_irq_safe(&pdev->dev);
> + pm_runtime_enable(&pdev->dev);
> + ret = pm_runtime_get_sync(&pdev->dev);
> + if (ret < 0) {
> + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
> + __func__, ret);
> + goto err_pmdisable;
after err_pmdisable is a runtime_put(), your cleanup is broken.
> + }
> +
> ret = register_candev(ndev);
> if (ret) {
> dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret);
> goto err_unprepare_disable_busclk;
I think you have to adjust the jump label.
> }
>
> devm_can_led_init(ndev);
>
> pm_runtime_put(&pdev->dev);
> @@ -1144,15 +1186,19 @@ static int xcan_probe(struct platform_device *pdev)
> }
>
> devm_can_led_init(ndev);
> - clk_disable_unprepare(priv->bus_clk);
> - clk_disable_unprepare(priv->can_clk);
> +
> + pm_runtime_put(&pdev->dev);
> +
> netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo depth:%d\n",
> priv->reg_base, ndev->irq, priv->can.clock.freq,
> priv->tx_max);
>
> return 0;
>
> +err_pmdisable:
> + pm_runtime_disable(&pdev->dev);
> err_unprepare_disable_busclk:
> + pm_runtime_put(priv->dev);
This cleanup is not correct.
> clk_disable_unprepare(priv->bus_clk);
> err_unprepare_disable_dev:
> clk_disable_unprepare(priv->can_clk);
> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
> {
> struct net_device *ndev = platform_get_drvdata(pdev);
> struct xcan_priv *priv = netdev_priv(ndev);
> + int ret;
> +
> + ret = pm_runtime_get_sync(&pdev->dev);
> + if (ret < 0) {
> + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
> + __func__, ret);
> + return ret;
> + }
>
> if (set_reset_mode(ndev) < 0)
> netdev_err(ndev, "mode resetting failed!\n");
>
> unregister_candev(ndev);
> + pm_runtime_disable(&pdev->dev);
> netif_napi_del(&priv->napi);
> + clk_disable_unprepare(priv->bus_clk);
> + clk_disable_unprepare(priv->can_clk);
> free_candev(ndev);
>
> return 0;
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v3] can: Convert to runtime_pm
On 11/27/2014 11:47 PM, Sören Brinkmann wrote:
> On Thu, 2014-11-27 at 10:17PM +0100, Marc Kleine-Budde wrote:
>> On 11/27/2014 02:08 PM, Kedareswara rao Appana wrote:
>>> Instead of enabling/disabling clocks at several locations in the driver,
>>> use the runtime_pm framework. This consolidates the actions for
>>> runtime PM in the appropriate callbacks and makes the driver more
>>> readable and mantainable.
>>>
>>> Signed-off-by: Soren Brinkmann
>>> Signed-off-by: Kedareswara rao Appana
>>> ---
>>> Changes for v3:
>>> - Converted the driver to use runtime_pm.
>>> Changes for v2:
>>> - Removed the struct platform_device* from suspend/resume
>>> as suggest by Lothar.
>>>
>>> drivers/net/can/xilinx_can.c | 119
>>> +
>>> 1 files changed, 72 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
>>> index 8a998e3..1be28ed 100644
>>> --- a/drivers/net/can/xilinx_can.c
>>> +++ b/drivers/net/can/xilinx_can.c
> [...]
>>> @@ -1030,7 +1046,10 @@ static int __maybe_unused xcan_resume(struct device
>>> *dev)
>>> return 0;
>>> }
>>>
>>> -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend, xcan_resume);
>>> +static const struct dev_pm_ops xcan_dev_pm_ops = {
>>> + SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
>>> + SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL)
>>> +};
>>>
>>> /**
>>> * xcan_probe - Platform registration call
>>> @@ -1071,7 +1090,7 @@ static int xcan_probe(struct platform_device *pdev)
>>> return -ENOMEM;
>>>
>>> priv = netdev_priv(ndev);
>>> - priv->dev = ndev;
>>> + priv->dev = &pdev->dev;
>>> priv->can.bittiming_const = &xcan_bittiming_const;
>>> priv->can.do_set_mode = xcan_do_set_mode;
>>> priv->can.do_get_berr_counter = xcan_get_berr_counter;
>>> @@ -1137,6 +1156,11 @@ static int xcan_probe(struct platform_device *pdev)
>>>
>>> netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
>>>
>>> + pm_runtime_set_active(&pdev->dev);
>>> + pm_runtime_irq_safe(&pdev->dev);
>>
>> You use just clock_enable()/disable() in the runtime functions, thus you
>> can say they are irq_safe. On the other the the zync grpio driver uses
>> "full" prepare_enable/disable_unprepare calls. What's best practice here?
>
> IIRC, the prepare/unprepare functions can sleep. xcan_get_berr_counter
> is called from atomic context. So, I think we have to use the
> disable/enable functions without the prepare/unprepare.
> In the GPIO driver the that problem does not exist.
IC, yes, correct.
This is why we introducted in other drivers a __get_berr_counter()
function, that doesn't touch the clocks, which is used from within the
driver (from the atomic contects), while get_berr_counter() will fiddle
with the clocks. This function is used for the
priv->can.do_get_berr_counter callback.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v3] can: Convert to runtime_pm
t;dev);
> + pm_runtime_irq_safe(&pdev->dev);
You use just clock_enable()/disable() in the runtime functions, thus you
can say they are irq_safe. On the other the the zync grpio driver uses
"full" prepare_enable/disable_unprepare calls. What's best practice here?
> + pm_runtime_enable(&pdev->dev);
> + pm_runtime_get_sync(&pdev->dev);
> +
> ret = register_candev(ndev);
> if (ret) {
> dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret);
I think in case of an error, you have to call pm_runtime_put() here
> @@ -1144,8 +1168,9 @@ static int xcan_probe(struct platform_device *pdev)
> }
>
> devm_can_led_init(ndev);
> - clk_disable_unprepare(priv->bus_clk);
> - clk_disable_unprepare(priv->can_clk);
> +
> + pm_runtime_put(&pdev->dev);
> +
> netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo depth:%d\n",
> priv->reg_base, ndev->irq, priv->can.clock.freq,
> priv->tx_max);
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v4 00/10] ARM: dts: TI: Add CAN support
On 11/13/2014 01:22 PM, Roger Quadros wrote: > Hi Tony, > > These patches add CAN support for am33xx, am43xx and dra7 platforms. > They can go in independent of [1] but depend on [1] for functionality. > > Tested on am335x-evm, am437x-gp-evm and dra7-evm. > > NOTE: > - for DCAN to work on DRA7 we need this patch to keep the CAN PHY powered. > https://lkml.org/lkml/2014/10/21/484 > > - on DRA7 there is another issue which causes undesired behaviour if WKUPAON > and > L4CFG clock domains are allowed to HW_AUTO. Undesired behaviour is in the form > of DCAN1 module being stuck-in-transition when disabled and RAMINIT-done bit > not getting set as part of RAMINIT mechanism. To address these issues there > is a patch > to prevent these clockdomains from HW_AUTO. > https://github.com/rogerq/linux/commit/31d6da939dbbaf8ea2ab96ca5680b81cd5607e51 > Even without this second patch DCAN seems to work. FWIW: Acked-by: Marc Kleine-Budde I've send a pull request with the CAN driver changes to David Miller yesterday, the code is supposed to hit mainline in v3.19. regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH v2] can: Fix bug in suspend/resume
On 11/14/2014 04:20 PM, Sören Brinkmann wrote: >>>> Please look the at suspend/resume code and count the >>>> clock_enable/disable manually. After a suspend/resume cycle, you have >>>> enabled the clock twice, but disabled it once. >>>> >>>> I think you have to abstract the clock handling behind runtime PM. I >>>> haven't done this myself yet, but the strong feeling that this is a >>>> possible solution to your problem. These links might help: >>> >>> I agree, the clock handling looks weird. Also the clk_disable calls in >>> xcan_get_berr_counter() look suspicious to me, but I might be wrong. >>> I think you can take a look at gpio-zynq for an example for runtime_pm >>> usage. I think the usage model in that driver is similar to here. >> >> The xcan_get_berr_counter() function is correct, when doing manual (i.e. >> non runtime-pm) clock handling. This function might be called if the >> interface is down, this means clocks are disabled. > > I see, thanks for the clarification. Guess that should become > pm_runtime_get_sync() and pm_runtime_put() when converting to > runtime_pm. Yes, as far as I understand runtime pm. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH v2] can: Fix bug in suspend/resume
On 11/14/2014 04:05 PM, Sören Brinkmann wrote: > On Fri, 2014-11-14 at 09:54AM +0100, Marc Kleine-Budde wrote: >> On 11/14/2014 09:16 AM, Kedareswara rao Appana wrote: >>> The drvdata in the suspend/resume is of type struct net_device, >>> not the platform device.Enable the clocks in the suspend before >>> accessing the registers of the CAN. >>> >>> Signed-off-by: Kedareswara rao Appana >>> --- >>> Changes for v2: >>> - Removed the struct platform_device* from suspend/resume >>> as suggest by Lothar. >>> - The clocks are getting disabled and un prepared at the end of the >>> probe. >>> In the suspend the driver is doing a register write.In order >>> To do that register write we have to again enable and prepare the >>> clocks. >> >> Please look the at suspend/resume code and count the >> clock_enable/disable manually. After a suspend/resume cycle, you have >> enabled the clock twice, but disabled it once. >> >> I think you have to abstract the clock handling behind runtime PM. I >> haven't done this myself yet, but the strong feeling that this is a >> possible solution to your problem. These links might help: > > I agree, the clock handling looks weird. Also the clk_disable calls in > xcan_get_berr_counter() look suspicious to me, but I might be wrong. > I think you can take a look at gpio-zynq for an example for runtime_pm > usage. I think the usage model in that driver is similar to here. The xcan_get_berr_counter() function is correct, when doing manual (i.e. non runtime-pm) clock handling. This function might be called if the interface is down, this means clocks are disabled. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH v2] can: Fix bug in suspend/resume
On 11/14/2014 09:16 AM, Kedareswara rao Appana wrote: > The drvdata in the suspend/resume is of type struct net_device, > not the platform device.Enable the clocks in the suspend before > accessing the registers of the CAN. > > Signed-off-by: Kedareswara rao Appana > --- > Changes for v2: > - Removed the struct platform_device* from suspend/resume > as suggest by Lothar. > - The clocks are getting disabled and un prepared at the end of the probe. > In the suspend the driver is doing a register write.In order > To do that register write we have to again enable and prepare the clocks. Please look the at suspend/resume code and count the clock_enable/disable manually. After a suspend/resume cycle, you have enabled the clock twice, but disabled it once. I think you have to abstract the clock handling behind runtime PM. I haven't done this myself yet, but the strong feeling that this is a possible solution to your problem. These links might help: http://lwn.net/Articles/505683/ http://www.slideshare.net/linaroorg/runtime-pm http://www.slideshare.net/linaroorg/lca14-407-deployingruntimepmonarmsocs http://www.slideshare.net/SamsungOSG/shuah-khan-lpcpmops Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH v4 06/10] ARM: dts: am4372: Add DCAN nodes
On 11/13/2014 03:49 PM, Roger Quadros wrote:
> On 11/13/2014 04:44 PM, Marc Kleine-Budde wrote:
>> On 11/13/2014 03:40 PM, Roger Quadros wrote:
>>> On 11/13/2014 04:07 PM, Marc Kleine-Budde wrote:
>>>> On 11/13/2014 01:22 PM, Roger Quadros wrote:
>>>>> The SoC contains 2 DCAN modules. Add them.
>>>>>
>>>>> Signed-off-by: Roger Quadros
>>>>> ---
>>>>> arch/arm/boot/dts/am4372.dtsi | 22 ++
>>>>> 1 file changed, 22 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
>>>>> index 899c57c..12fb1db 100644
>>>>> --- a/arch/arm/boot/dts/am4372.dtsi
>>>>> +++ b/arch/arm/boot/dts/am4372.dtsi
>>>>> @@ -901,6 +901,28 @@
>>>>> compatible = "mmio-sram";
>>>>> reg = <0x4030 0x4>; /* 256k */
>>>>> };
>>>>> +
>>>>> + dcan0: can@481cc000 {
>>>>> + compatible = "ti,am3352-d_can";
>>>>
>>>> You should add "ti,am4372-d_can" as first compatible here.
>>>>
>>> We don't have a separate compatible id for am4372-d_can
>>> as it the IP exactly same as am3352-d_can.
>>
>> Having the "ti,am4372-d_can" compatible gives you the freedom to add
>> some spacial handling for the IP if there turns that you need to without
>> needing to modify the device tree. (We do this on imx.)
>
> Agreed. On OMAP platforms we don't add new compatible IDs unless we really
> need to.
> Can we add "ti,am4372-d_can" even if it is not mentioned in
> Documentation/devicetree/bindings?
>
> Tony what is your preference?
I don't insist, do it the Omap way :)
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v4 06/10] ARM: dts: am4372: Add DCAN nodes
On 11/13/2014 03:40 PM, Roger Quadros wrote:
> On 11/13/2014 04:07 PM, Marc Kleine-Budde wrote:
>> On 11/13/2014 01:22 PM, Roger Quadros wrote:
>>> The SoC contains 2 DCAN modules. Add them.
>>>
>>> Signed-off-by: Roger Quadros
>>> ---
>>> arch/arm/boot/dts/am4372.dtsi | 22 ++
>>> 1 file changed, 22 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
>>> index 899c57c..12fb1db 100644
>>> --- a/arch/arm/boot/dts/am4372.dtsi
>>> +++ b/arch/arm/boot/dts/am4372.dtsi
>>> @@ -901,6 +901,28 @@
>>> compatible = "mmio-sram";
>>> reg = <0x4030 0x4>; /* 256k */
>>> };
>>> +
>>> + dcan0: can@481cc000 {
>>> + compatible = "ti,am3352-d_can";
>>
>> You should add "ti,am4372-d_can" as first compatible here.
>>
> We don't have a separate compatible id for am4372-d_can
> as it the IP exactly same as am3352-d_can.
Having the "ti,am4372-d_can" compatible gives you the freedom to add
some spacial handling for the IP if there turns that you need to without
needing to modify the device tree. (We do this on imx.)
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v4 06/10] ARM: dts: am4372: Add DCAN nodes
On 11/13/2014 01:22 PM, Roger Quadros wrote:
> The SoC contains 2 DCAN modules. Add them.
>
> Signed-off-by: Roger Quadros
> ---
> arch/arm/boot/dts/am4372.dtsi | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
> index 899c57c..12fb1db 100644
> --- a/arch/arm/boot/dts/am4372.dtsi
> +++ b/arch/arm/boot/dts/am4372.dtsi
> @@ -901,6 +901,28 @@
> compatible = "mmio-sram";
> reg = <0x4030 0x4>; /* 256k */
> };
> +
> + dcan0: can@481cc000 {
> + compatible = "ti,am3352-d_can";
You should add "ti,am4372-d_can" as first compatible here.
> + ti,hwmods = "d_can0";
> + clocks = <&dcan0_fck>;
> + clock-names = "fck";
> + reg = <0x481cc000 0x2000>;
> + syscon-raminit = <&am43xx_control_module 0x644 0>;
> + interrupts = ;
> + status = "disabled";
> + };
> +
> + dcan1: can@481d {
> + compatible = "ti,am3352-d_can";
Same here.
> + ti,hwmods = "d_can1";
> + clocks = <&dcan1_fck>;
> + clock-names = "fck";
> + reg = <0x481d 0x2000>;
> + syscon-raminit = <&am43xx_control_module 0x644 1>;
> + interrupts = ;
> + status = "disabled";
> + };
> };
> };
>
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH] can: Fix bug in suspend/resume
On 11/13/2014 12:32 PM, Lothar Waßmann wrote:
>>>> static int __maybe_unused xcan_suspend(struct device *dev) {
>>>> -struct platform_device *pdev = dev_get_drvdata(dev);
>>>> +struct platform_device *pdev = container_of(dev,
>>>> +struct platform_device, dev);
>>>> struct net_device *ndev = platform_get_drvdata(pdev);
>>>> struct xcan_priv *priv = netdev_priv(ndev);
>>
> Why not simply:
> struct net_device *ndev = dev_get_drvdata(dev);
>
> There is no need for a struct platform_device* at all.
ACK
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH] can: Fix bug in suspend/resume
On 11/13/2014 11:15 AM, Marc Kleine-Budde wrote:
> On 11/13/2014 07:58 AM, Kedareswara rao Appana wrote:
>> When accessing the priv structure use container_of instead of
>> dev_get_drvdata.
>
> Why?
The drvdata here is the struct net_device, not the platform device.
Please state this in the commit message.
If I understand the code correct, you can make use of the existing
helper function to_platform_device():
http://lxr.free-electrons.com/source/include/linux/platform_device.h#L42
>
>> Enable the clocks in the suspend before accessing the registers of the CAN.
>>
>> Signed-off-by: Kedareswara rao Appana
>> ---
>> drivers/net/can/xilinx_can.c | 20 ++--
>> 1 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
>> index 5e8b560..63ef645 100644
>> --- a/drivers/net/can/xilinx_can.c
>> +++ b/drivers/net/can/xilinx_can.c
>> @@ -972,15 +972,30 @@ static const struct net_device_ops xcan_netdev_ops = {
>> */
>> static int __maybe_unused xcan_suspend(struct device *dev)
>> {
>> -struct platform_device *pdev = dev_get_drvdata(dev);
>> +struct platform_device *pdev = container_of(dev,
>> +struct platform_device, dev);
>> struct net_device *ndev = platform_get_drvdata(pdev);
>> struct xcan_priv *priv = netdev_priv(ndev);
>> +int ret;
>>
>> if (netif_running(ndev)) {
>> netif_stop_queue(ndev);
>> netif_device_detach(ndev);
>> }
>>
>> +ret = clk_prepare_enable(priv->can_clk);
>> +if (ret) {
>> +dev_err(dev, "unable to enable device clock\n");
>> +return ret;
>> +}
>> +
>> +ret = clk_prepare_enable(priv->bus_clk);
>> +if (ret) {
>> +dev_err(dev, "unable to enable bus clock\n");
>> +clk_disable_unprepare(priv->can_clk);
>> +return ret;
>> +}
>
> Now you have clock imbalance. Per suspend/resume cycle the clocks are
> enabled twice, but disabled only once.
>
>> +
>> priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_SLEEP_MASK);
>> priv->can.state = CAN_STATE_SLEEPING;
>>
>> @@ -999,7 +1014,8 @@ static int __maybe_unused xcan_suspend(struct device
>> *dev)
>> */
>> static int __maybe_unused xcan_resume(struct device *dev)
>> {
>> -struct platform_device *pdev = dev_get_drvdata(dev);
>> +struct platform_device *pdev = container_of(dev,
>> +struct platform_device, dev);
>> struct net_device *ndev = platform_get_drvdata(pdev);
>> struct xcan_priv *priv = netdev_priv(ndev);
>> int ret;
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH] can: Fix bug in suspend/resume
On 11/13/2014 07:58 AM, Kedareswara rao Appana wrote:
> When accessing the priv structure use container_of instead of dev_get_drvdata.
Why?
> Enable the clocks in the suspend before accessing the registers of the CAN.
>
> Signed-off-by: Kedareswara rao Appana
> ---
> drivers/net/can/xilinx_can.c | 20 ++--
> 1 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 5e8b560..63ef645 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -972,15 +972,30 @@ static const struct net_device_ops xcan_netdev_ops = {
> */
> static int __maybe_unused xcan_suspend(struct device *dev)
> {
> - struct platform_device *pdev = dev_get_drvdata(dev);
> + struct platform_device *pdev = container_of(dev,
> + struct platform_device, dev);
> struct net_device *ndev = platform_get_drvdata(pdev);
> struct xcan_priv *priv = netdev_priv(ndev);
> + int ret;
>
> if (netif_running(ndev)) {
> netif_stop_queue(ndev);
> netif_device_detach(ndev);
> }
>
> + ret = clk_prepare_enable(priv->can_clk);
> + if (ret) {
> + dev_err(dev, "unable to enable device clock\n");
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(priv->bus_clk);
> + if (ret) {
> + dev_err(dev, "unable to enable bus clock\n");
> + clk_disable_unprepare(priv->can_clk);
> + return ret;
> + }
Now you have clock imbalance. Per suspend/resume cycle the clocks are
enabled twice, but disabled only once.
> +
> priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_SLEEP_MASK);
> priv->can.state = CAN_STATE_SLEEPING;
>
> @@ -999,7 +1014,8 @@ static int __maybe_unused xcan_suspend(struct device
> *dev)
> */
> static int __maybe_unused xcan_resume(struct device *dev)
> {
> - struct platform_device *pdev = dev_get_drvdata(dev);
> + struct platform_device *pdev = container_of(dev,
> + struct platform_device, dev);
> struct net_device *ndev = platform_get_drvdata(pdev);
> struct xcan_priv *priv = netdev_priv(ndev);
> int ret;
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: FlexCAN driver triggers kernel BUG in BUSOFF restart
On 09/11/2014 06:08 AM, Keith Lugsden wrote:
> I was asked by Freescale to post this kernel BUG to you and your email
> address appears in the kernel documentation, so please accept my
> apologies if you did not wish to see this.
Thanks for the bug report.
> A kernel BUG is triggered during a CAN send operation (kernel version
> 3.15.7, stack trace follows below). A simple program, such as the one
> included in the kernel's CAN documentation, will cause this problem, as
> will the can-utils programs 'cangen' and 'cansend'.
[...]
> Typical stack trace:
>
> BUG: scheduling while atomic: swapper/0/0/0x0100
> 1 lock held by swapper/0/0:
> #0: (((&priv->restart_timer))){+.-...}, at: [<8002fe60>]
> call_timer_fn+0x0/0xe8
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 3.15.7 #5
> Backtrace:
> [<80011ba4>] (dump_backtrace) from [<80011d40>] (show_stack+0x18/0x1c)
> r6: r5: r4: r3:
> [<80011d28>] (show_stack) from [<805de120>] (dump_stack+0x88/0xa4)
> [<805de098>] (dump_stack) from [<805db3ec>] (__schedule_bug+0x64/0x7c)
> r5:80867268 r4:80867268
> [<805db388>] (__schedule_bug) from [<805e09b0>] (__schedule+0x540/0x5f8)
> r4:bf7c7b00 r3:0002
> [<805e0470>] (__schedule) from [<805e0b5c>] (schedule+0x38/0x88)
> r10:8085c000 r9: r8:0001 r7: r6:4e20 r5:
> r4:2710
> [<805e0b24>] (schedule) from [<805e03c4>]
> (schedule_hrtimeout_range_clock+0xe0/0x158)
> [<805e02e4>] (schedule_hrtimeout_range_clock) from [<805e0450>]
> (schedule_hrtimeout_range+0x14/0x18)
> r10:808644c4 r8:bf335e00 r7: r6:c09b8000 r5:c09b8000 r4:bf335800
> [<805e043c>] (schedule_hrtimeout_range) from [<8002fb58>]
> (usleep_range+0x50/0x58)
> [<8002fb08>] (usleep_range) from [<803f2964>]
> (flexcan_chip_start+0x7c/0x440)
> [<803f28e8>] (flexcan_chip_start) from [<803f3994>]
> (flexcan_set_mode+0x28/0x54)
[...]
David Jander already provided a fix for this problem:
8badd65 can: flexcan: avoid calling usleep_range from interrupt context
It will hit mainline soon and then be backported to the stable trees.
Marc
---
From 8badd65e48c90d66587359d5329c2813088c0f50 Mon Sep 17 00:00:00 2001
From: David Jander
Date: Wed, 27 Aug 2014 12:02:16 +0200
Subject: [PATCH] can: flexcan: avoid calling usleep_range from interrupt
context
Apparently can_restart() runs from a (timer-) interrupt and can call
flexcan_chip_[en|dis]able(), so avoid using usleep_range()
Signed-off-by: David Jander
Cc: linux-stable
Signed-off-by: Marc Kleine-Budde
---
drivers/net/can/flexcan.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 944aa5d..630c7bf 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -298,7 +298,7 @@ static int flexcan_chip_enable(struct flexcan_priv *priv)
flexcan_write(reg, ®s->mcr);
while (timeout-- && (flexcan_read(®s->mcr) & FLEXCAN_MCR_LPM_ACK))
- usleep_range(10, 20);
+ udelay(10);
if (flexcan_read(®s->mcr) & FLEXCAN_MCR_LPM_ACK)
return -ETIMEDOUT;
@@ -317,7 +317,7 @@ static int flexcan_chip_disable(struct flexcan_priv *priv)
flexcan_write(reg, ®s->mcr);
while (timeout-- && !(flexcan_read(®s->mcr) & FLEXCAN_MCR_LPM_ACK))
- usleep_range(10, 20);
+ udelay(10);
if (!(flexcan_read(®s->mcr) & FLEXCAN_MCR_LPM_ACK))
return -ETIMEDOUT;
@@ -336,7 +336,7 @@ static int flexcan_chip_freeze(struct flexcan_priv *priv)
flexcan_write(reg, ®s->mcr);
while (timeout-- && !(flexcan_read(®s->mcr) & FLEXCAN_MCR_FRZ_ACK))
- usleep_range(100, 200);
+ udelay(100);
if (!(flexcan_read(®s->mcr) & FLEXCAN_MCR_FRZ_ACK))
return -ETIMEDOUT;
@@ -355,7 +355,7 @@ static int flexcan_chip_unfreeze(struct flexcan_priv *priv)
flexcan_write(reg, ®s->mcr);
while (timeout-- && (flexcan_read(®s->mcr) & FLEXCAN_MCR_FRZ_ACK))
- usleep_range(10, 20);
+ udelay(10);
if (flexcan_read(®s->mcr) & FLEXCAN_MCR_FRZ_ACK)
return -ETIMEDOUT;
@@ -370,7 +370,7 @@ static int flexcan_chip_softreset(struct flexcan_priv *priv)
flexcan_write(FLEXCAN_MCR_SOFTRST, ®s->mcr);
while (timeout-- && (flexcan_read(®s->mcr) & FLEXCAN_MCR_SOFTRST))
- usleep_range(10, 20);
+ udelay(10);
if (flexcan_read(®s->mcr) & FLEXCAN_MCR_SOFTRST)
return -ETIMEDOUT;
--
2.1.0
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v3 1/2] rcar_can: document device tree bindings
On 07/31/2014 11:31 PM, Marc Kleine-Budde wrote: > On 07/31/2014 11:24 PM, Sergei Shtylyov wrote: >> Document the R-Car CAN device tree bindings. >> >> Signed-off-by: Sergei Shtylyov > > The series looks good from the CAN perpective. I'll apply the patches > after we've got an Acked-by from the DT folks. Applied both to linux-can-next. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH v5 1/2] can: m_can: add device tree binding documentation
On 08/14/2014 11:30 AM, Dong Aisheng wrote: > Ping... > > On Wed, Jul 30, 2014 at 7:52 PM, Marc Kleine-Budde > wrote: >> On 07/16/2014 11:30 AM, Dong Aisheng wrote: >>> add M_CAN device tree binding documentation Applied both to linux-can-next. Thanks, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH v3 1/2] rcar_can: document device tree bindings
On 08/04/2014 07:20 PM, Sergei Shtylyov wrote: > Hello. > > On 08/01/2014 01:31 AM, Marc Kleine-Budde wrote: > >>> Document the R-Car CAN device tree bindings. > >>> Signed-off-by: Sergei Shtylyov > >> The series looks good from the CAN perpective. I'll apply the patches >> after we've got an Acked-by from the DT folks. > >How about the prior CAN clock support patch? That looks good, too. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH v3 1/2] rcar_can: document device tree bindings
On 07/31/2014 11:24 PM, Sergei Shtylyov wrote:
> Document the R-Car CAN device tree bindings.
>
> Signed-off-by: Sergei Shtylyov
The series looks good from the CAN perpective. I'll apply the patches
after we've got an Acked-by from the DT folks.
Marc
>
> ---
> The patch is against the 'linux-can-next.git' repo.
>
> Changes in version 2:
> - new patch, split from the driver's device tree support patch;
> - adjusted "clocks" property to refer to 3 clocks instead of 1;
> - added mandatory "clock-names" property;
> - renamed the "clock-select" property to "renesas,can-clock-select".
>
> Documentation/devicetree/bindings/net/can/rcar_can.txt | 43
> +
> 1 file changed, 43 insertions(+)
>
> Index: linux-can-next/Documentation/devicetree/bindings/net/can/rcar_can.txt
> ===
> --- /dev/null
> +++ linux-can-next/Documentation/devicetree/bindings/net/can/rcar_can.txt
> @@ -0,0 +1,43 @@
> +Renesas R-Car CAN controller Device Tree Bindings
> +-
> +
> +Required properties:
> +- compatible: "renesas,can-r8a7778" if CAN controller is a part of R8A7778
> SoC.
> + "renesas,can-r8a7779" if CAN controller is a part of R8A7779 SoC.
> + "renesas,can-r8a7790" if CAN controller is a part of R8A7790 SoC.
> + "renesas,can-r8a7791" if CAN controller is a part of R8A7791 SoC.
> +- reg: physical base address and size of the R-Car CAN register map.
> +- interrupts: interrupt specifier for the sole interrupt.
> +- clocks: phandles and clock specifiers for 3 CAN clock inputs.
> +- clock-names: 3 clock input name strings: "clkp1", "clkp2", "can_clk".
> +- pinctrl-0: pin control group to be used for this controller.
> +- pinctrl-names: must be "default".
> +
> +Optional properties:
> +- renesas,can-clock-select: R-Car CAN Clock Source Select. Valid values are:
> + <0x0> (default) : Peripheral clock (clkp1)
> + <0x1> : Peripheral clock (clkp2)
> + <0x3> : Externally input clock
> +
> +Example
> +---
> +
> +SoC common .dtsi file:
> +
> + can0: can@e6e8 {
> + compatible = "renesas,can-r8a7791";
> + reg = <0 0xe6e8 0 0x1000>;
> + interrupts = <0 186 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&mstp9_clks R8A7791_CLK_RCAN0>,
> + <&cpg_clocks R8A7791_CLK_RCAN>, <&can_clk>;
> + clock-names = "clkp1", "clkp2", "can_clk";
> + status = "disabled";
> + };
> +
> +Board specific .dts file:
> +
> +&can0 {
> + pinctrl-0 = <&can0_pins>;
> + pinctrl-names = "default";
> + status = "okay";
> +};
>
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v5 1/2] can: m_can: add device tree binding documentation
On 07/16/2014 11:30 AM, Dong Aisheng wrote:
> add M_CAN device tree binding documentation
>
> Cc: Wolfgang Grandegger
> Cc: Marc Kleine-Budde
> Cc: Mark Rutland
> Cc: Oliver Hartkopp
> Cc: Varka Bhadram
> Signed-off-by: Dong Aisheng
Are there any comment from the DT side to this patch?
Preferred an Acked-by :)
The series looks good to me, I'm waiting for an Ack to apply it to the
linux-can-next tree.
Marc
> ---
> Changes since v4:
> - change the name of board dtsi to board dts which is more common
>
> Changes since v3:
> - change mram-cfg property name to bosch,mram-cfg
> - indent change
> ---
> .../devicetree/bindings/net/can/m_can.txt | 67
>
> 1 files changed, 67 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/net/can/m_can.txt
>
> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
> b/Documentation/devicetree/bindings/net/can/m_can.txt
> new file mode 100644
> index 000..9e33177
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
> @@ -0,0 +1,67 @@
> +Bosch MCAN controller Device Tree Bindings
> +-
> +
> +Required properties:
> +- compatible : Should be "bosch,m_can" for M_CAN controllers
> +- reg: physical base address and size of the M_CAN
> + registers map and Message RAM
> +- reg-names : Should be "m_can" and "message_ram"
> +- interrupts : Should be the interrupt number of M_CAN interrupt
> + line 0 and line 1, could be same if sharing
> + the same interrupt.
> +- interrupt-names: Should contain "int0" and "int1"
> +- clocks : Clocks used by controller, should be host clock
> + and CAN clock.
> +- clock-names: Should contain "hclk" and "cclk"
> +- pinctrl-: Pinctrl states as described in
> bindings/pinctrl/pinctrl-bindings.txt
> +- pinctrl-names : Names corresponding to the numbered pinctrl states
> +- bosch,mram-cfg : Message RAM configuration data.
> + Multiple M_CAN instances can share the same Message
> + RAM and each element(e.g Rx FIFO or Tx Buffer and etc)
> + number in Message RAM is also configurable,
> + so this property is telling driver how the shared or
> + private Message RAM are used by this M_CAN controller.
> +
> + The format should be as follows:
> ++rxb_elems txe_elems txb_elems>
> + The 'offset' is an address offset of the Message RAM
> + where the following elements start from. This is
> + usually set to 0x0 if you're using a private Message
> + RAM. The remain cells are used to specify how many
> + elements are used for each FIFO/Buffer.
> +
> + M_CAN includes the following elements according to
> user manual:
> + 11-bit Filter 0-128 elements / 0-128 words
> + 29-bit Filter 0-64 elements / 0-128 words
> + Rx FIFO 0 0-64 elements / 0-1152 words
> + Rx FIFO 1 0-64 elements / 0-1152 words
> + Rx Buffers0-64 elements / 0-1152 words
> + Tx Event FIFO 0-32 elements / 0-64 words
> + Tx Buffers0-32 elements / 0-576 words
> +
> + Please refer to 2.4.1 Message RAM Configuration in
> + Bosch M_CAN user manual for details.
> +
> +Example:
> +SoC dtsi:
> +m_can1: can@020e8000 {
> + compatible = "bosch,m_can";
> + reg = <0x020e8000 0x4000>, <0x02298000 0x4000>;
> + reg-names = "m_can", "message_ram";
> + interrupts = <0 114 0x04>,
> + <0 114 0x04>;
> + interrupt-names = "int0", "int1";
> + clocks = <&clks IMX6SX_CLK_CANFD>,
> + <&clks IMX6SX_CLK_CANFD>;
> + clock-names = "hclk", "cclk";
> + bosch,mram-cfg = <0x0 0 0 32 0 0 0 1>;
> + status = "disabled";
> +};
> +
> +Board dts:
> +&m_can1 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_m_can1>;
> + status = "enabled";
> +};
>
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2 2/2] rcar_can: add device tree support
On 07/30/2014 12:53 AM, Sergei Shtylyov wrote:
> Add support of the device tree probing for the Renesas R-Car CAN controllers.
>
> Signed-off-by: Sergei Shtylyov
>
> ---
> The patch is against the 'linux-can-next.git' repo plus the CAN clock handling
> patch I've just posted.
>
> Changes in version 2:
> - split the device tree bindings document into a separate patch;
> - replaced 0 with CLKR_CLKP1 in the 'clock_select' variable intializer
> - reversed the condition in the *if* stetement handling the reading of the
> CAN clock selector from the device tree or the paltfrom data;
> - renamed the "clock-select" property to "renesas,can-clock-select";
> - rebased the patch on top of the CAN clock handling fix.
>
> drivers/net/can/rcar_can.c | 33 +
> 1 file changed, 25 insertions(+), 8 deletions(-)
>
> Index: linux-can-next/drivers/net/can/rcar_can.c
> ===
> --- linux-can-next.orig/drivers/net/can/rcar_can.c
> +++ linux-can-next/drivers/net/can/rcar_can.c
> @@ -20,6 +20,7 @@
> #include
> #include
> #include
> +#include
>
> #define RCAR_CAN_DRV_NAME"rcar_can"
>
> @@ -738,13 +739,20 @@ static int rcar_can_probe(struct platfor
> struct net_device *ndev;
> struct resource *mem;
> void __iomem *addr;
> + u32 clock_select = CLKR_CLKP1;
If you introduce the variable clock_select in the patch "rcar_can:
support all input clocks", this diff would be smaller.
> int err = -ENODEV;
> int irq;
>
> - pdata = dev_get_platdata(&pdev->dev);
> - if (!pdata) {
> - dev_err(&pdev->dev, "No platform data provided!\n");
> - goto fail;
> + if (pdev->dev.of_node) {
> + of_property_read_u32(pdev->dev.of_node,
> + "renesas,can-clock-select", &clock_select);
> + } else {
> + pdata = dev_get_platdata(&pdev->dev);
> + if (!pdata) {
> + dev_err(&pdev->dev, "No platform data provided!\n");
> + goto fail;
> + }
> + clock_select = pdata->clock_select;
> }
>
> irq = platform_get_irq(pdev, 0);
> @@ -776,13 +784,12 @@ static int rcar_can_probe(struct platfor
> goto fail_clk;
> }
>
> - if (pdata->clock_select > CLKR_CLKEXT) {
> + if (clock_select > CLKR_CLKEXT) {
> err = -EINVAL;
> dev_err(&pdev->dev, "invalid CAN clock selected\n");
> goto fail_clk;
> }
> - priv->can_clk = devm_clk_get(&pdev->dev,
> - clock_names[pdata->clock_select]);
> + priv->can_clk = devm_clk_get(&pdev->dev, clock_names[clock_select]);
> if (IS_ERR(priv->can_clk)) {
> err = PTR_ERR(priv->can_clk);
> dev_err(&pdev->dev, "cannot get CAN clock: %d\n", err);
> @@ -794,7 +801,7 @@ static int rcar_can_probe(struct platfor
> ndev->flags |= IFF_ECHO;
> priv->ndev = ndev;
> priv->regs = addr;
> - priv->clock_select = pdata->clock_select;
> + priv->clock_select = clock_select;
> priv->can.clock.freq = clk_get_rate(priv->can_clk);
> priv->can.bittiming_const = &rcar_can_bittiming_const;
> priv->can.do_set_mode = rcar_can_do_set_mode;
> @@ -887,10 +894,20 @@ static int __maybe_unused rcar_can_resum
>
> static SIMPLE_DEV_PM_OPS(rcar_can_pm_ops, rcar_can_suspend, rcar_can_resume);
>
> +static const struct of_device_id rcar_can_of_table[] __maybe_unused = {
> + { .compatible = "renesas,can-r8a7778" },
> + { .compatible = "renesas,can-r8a7779" },
> + { .compatible = "renesas,can-r8a7790" },
> + { .compatible = "renesas,can-r8a7791" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, rcar_can_of_table);
> +
> static struct platform_driver rcar_can_driver = {
> .driver = {
> .name = RCAR_CAN_DRV_NAME,
> .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(rcar_can_of_table),
> .pm = &rcar_can_pm_ops,
> },
> .probe = rcar_can_probe,
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v5 1/2] can: m_can: add device tree binding documentation
On 07/16/2014 11:30 AM, Dong Aisheng wrote: > add M_CAN device tree binding documentation > > Cc: Wolfgang Grandegger > Cc: Marc Kleine-Budde > Cc: Mark Rutland > Cc: Oliver Hartkopp > Cc: Varka Bhadram > Signed-off-by: Dong Aisheng > --- > Changes since v4: > - change the name of board dtsi to board dts which is more common > > Changes since v3: > - change mram-cfg property name to bosch,mram-cfg > - indent change > --- > .../devicetree/bindings/net/can/m_can.txt | 67 > > 1 files changed, 67 insertions(+), 0 deletions(-) > create mode 100644 Documentation/devicetree/bindings/net/can/m_can.txt I'll apply this series once we've got an Ack for the DT folks. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH v4 2/2] can: m_can: add Bosch M_CAN controller support
On 07/15/2014 11:07 AM, Dong Aisheng wrote:
> On Tue, Jul 15, 2014 at 10:46:32AM +0200, Marc Kleine-Budde wrote:
>> On 07/15/2014 10:26 AM, Dong Aisheng wrote:
>>>>>>> +static void m_can_read_fifo(const struct net_device *dev, struct
>>>>>>> can_frame *cf,
>>>>>>> + u32 rxfs)
>>>>>>> +{
>>>>>>> + struct m_can_priv *priv = netdev_priv(dev);
>>>>>>> + u32 flags, fgi;
>>>>>>> +
>>>>>>> + /* calculate the fifo get index for where to read data */
>>>>>>> + fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
>>>>>>> + flags = m_can_fifo_read(priv, fgi, 0x0);
>>>>>>^^^
>>>>>>
>>>>>> Can you introduce an enum for the offsets, please adjust the signature
>>>>>> of m_can_fifo_read() accordingly.
>>>>>>
>>>>>
>>>>> I wonder enum may not be suitable.
>>>>> The Rx Buffer and FIFO Element is as follows:
>>>>>31 24 23 16 15 8 7 0
>>>>> R0 ESI XTD RTR ID[28:0]
>>>>
>>>> M_CAN_FIFO_ID
>>>>
>>>>> R1 ANMF FIDX[6:0] res EDL BRS DLC[3:0] RXTS[15:0]
>>>>
>>>> M_CAN_FIFO_DLC
>>>>
>>>>> R2 DB3[7:0] DB2[7:0] DB1[7:0] DB0[7:0]
>>>>> R3 DB7[7:0] DB6[7:0] DB5[7:0] DB4[7:0]
>>>>
>>>> M_CAN_FIFO_DATA0
>>>> M_CAN_FIFO_DATA1
>>>>
>>>
>>> You mean as follows?
>>> enum m_can_fifo {
>>> M_CAN_FIFO_ID = 0,
>>> M_CAN_FIFO_DLC,
>> = 0x4,
>>> M_CAN_FIFO_DATA0,
>> = 0x8,
>>> M_CAN_FIFO_DATA1,
>> = 0xc,
>>> };
>>>
>>> static inline u32 m_can_fifo_read(const struct m_can_priv *priv,
>>> u32 fgi, enum m_can_fifo fifo)
>>> {
>>> return readl(priv->mram_base + priv->mcfg[MRAM_RXF0].off +
>>> fgi * RXF0_ELEMENT_SIZE + fifo * 0x4);
>>> }
>>
>> without the * 0x4
>>
>>> id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_ID);
>>>
>>> The problem is when adding long frames support, it becomes:
>>> enum m_can_fifo {
>>> M_CAN_FIFO_ID = 0,
>>> M_CAN_FIFO_DLC,
>>> M_CAN_FIFO_DATA0,
>>> M_CAN_FIFO_DATA1,
>>>
>>> M_CAN_FIFO_DATA15,
>>> };
>>
>> #define M_CAN_FIFO_DATA(n)
>> (enum m_can_fifo)(M_CAN_FIFO_DATA_0 + (n) << 2)
>>
>
> This is a bit strange using and we may still have to define other
> M_CAN_FIFO_DATAx
> to avoid the enum value exceeds the defined range.
> enum m_can_fifo {
> M_CAN_FIFO_ID = 0,
> M_CAN_FIFO_DLC = 0x4,
> M_CAN_FIFO_DATA0 = 0x8,
> M_CAN_FIFO_DATA1 = 0xc,
>
> M_CAN_FIFO_DATA15 = 0xc,
> };
>
> However, actually we will not use them at all after introducing
> M_CAN_FIFO_DATA(n).
> If that, why we still need define them in enum?
>
> Comparing to this way, why not simply just do as follows:
> #define M_CAN_FIFO_ID 0x0
> #define M_CAN_FIFO_DLC0x4
> #define M_CAN_FIFO_DATA(n)(0x8 + (n) << 2)
>
> What do you think?
Looks good.
>
>>> But it's useless because we may not use enum to read fifo data anymore.
>>> It's not suitable to read fifo one by one:
>>> m_can_fifo_read(priv, fgi, M_CAN_FIFO_DATA0);
>>> m_can_fifo_read(priv, fgi, M_CAN_FIFO_DATA1);
>>> ..
>>> m_can_fifo_read(priv, fgi, M_CAN_FIFO_DATA15);
>>>
>>>
>>> Instead, we may read data according to real dlc value within a for loop
>>> like:
>>> #define M_CAN_FIFO(n) (n * 0x4)
>>> id = m_can_fifo_read(priv, fgi, M_CAN_FIFO(0));
>>> dlc = m_can_fifo_read(priv, fgi, M_CAN_FIFO(1));
>>> for (i = 0; dlc > 0; dlc -= 0x4, i++) {
>>>
>>> data[i] = m_can_fifo_read(priv, fgi, M_CAN_FIFO(i + 2));
>>> }
>>
>> id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_ID);
>> dlc = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC);
>> for (i = 0; i <= dlc; i++)
>> data[i] = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DATA(i));
>
> Does it work?
> The dlc is in bytes while m_can_fifo_read is read in words.
Doh! probably not :) But should work with something like this:
int len = DIV_ROUND_UP(dlc, 4);
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v4 2/2] can: m_can: add Bosch M_CAN controller support
On 07/15/2014 10:26 AM, Dong Aisheng wrote:
>>>>> +static void m_can_read_fifo(const struct net_device *dev, struct
>>>>> can_frame *cf,
>>>>> + u32 rxfs)
>>>>> +{
>>>>> + struct m_can_priv *priv = netdev_priv(dev);
>>>>> + u32 flags, fgi;
>>>>> +
>>>>> + /* calculate the fifo get index for where to read data */
>>>>> + fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
>>>>> + flags = m_can_fifo_read(priv, fgi, 0x0);
>>>>^^^
>>>>
>>>> Can you introduce an enum for the offsets, please adjust the signature
>>>> of m_can_fifo_read() accordingly.
>>>>
>>>
>>> I wonder enum may not be suitable.
>>> The Rx Buffer and FIFO Element is as follows:
>>>31 24 23 16 15 8 7 0
>>> R0 ESI XTD RTR ID[28:0]
>>
>> M_CAN_FIFO_ID
>>
>>> R1 ANMF FIDX[6:0] res EDL BRS DLC[3:0] RXTS[15:0]
>>
>> M_CAN_FIFO_DLC
>>
>>> R2 DB3[7:0] DB2[7:0] DB1[7:0] DB0[7:0]
>>> R3 DB7[7:0] DB6[7:0] DB5[7:0] DB4[7:0]
>>
>> M_CAN_FIFO_DATA0
>> M_CAN_FIFO_DATA1
>>
>
> You mean as follows?
> enum m_can_fifo {
> M_CAN_FIFO_ID = 0,
> M_CAN_FIFO_DLC,
= 0x4,
> M_CAN_FIFO_DATA0,
= 0x8,
> M_CAN_FIFO_DATA1,
= 0xc,
> };
>
> static inline u32 m_can_fifo_read(const struct m_can_priv *priv,
> u32 fgi, enum m_can_fifo fifo)
> {
> return readl(priv->mram_base + priv->mcfg[MRAM_RXF0].off +
> fgi * RXF0_ELEMENT_SIZE + fifo * 0x4);
> }
without the * 0x4
> id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_ID);
>
> The problem is when adding long frames support, it becomes:
> enum m_can_fifo {
> M_CAN_FIFO_ID = 0,
> M_CAN_FIFO_DLC,
> M_CAN_FIFO_DATA0,
> M_CAN_FIFO_DATA1,
>
> M_CAN_FIFO_DATA15,
> };
#define M_CAN_FIFO_DATA(n)
(enum m_can_fifo)(M_CAN_FIFO_DATA_0 + (n) << 2)
> But it's useless because we may not use enum to read fifo data anymore.
> It's not suitable to read fifo one by one:
> m_can_fifo_read(priv, fgi, M_CAN_FIFO_DATA0);
> m_can_fifo_read(priv, fgi, M_CAN_FIFO_DATA1);
> ..
> m_can_fifo_read(priv, fgi, M_CAN_FIFO_DATA15);
>
>
> Instead, we may read data according to real dlc value within a for loop like:
> #define M_CAN_FIFO(n) (n * 0x4)
> id = m_can_fifo_read(priv, fgi, M_CAN_FIFO(0));
> dlc = m_can_fifo_read(priv, fgi, M_CAN_FIFO(1));
> for (i = 0; dlc > 0; dlc -= 0x4, i++) {
>
> data[i] = m_can_fifo_read(priv, fgi, M_CAN_FIFO(i + 2));
> }
id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_ID);
dlc = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC);
for (i = 0; i <= dlc; i++)
data[i] = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DATA(i));
> So i'm not sure define that enum now is really needed.
[...]
>>>>> +static int m_can_handle_lec_err(struct net_device *dev,
>>>>> + enum m_can_lec_type lec_type)
>>>>> +{
>>>>> + struct m_can_priv *priv = netdev_priv(dev);
>>>>> + struct net_device_stats *stats = &dev->stats;
>>>>> + struct can_frame *cf;
>>>>> + struct sk_buff *skb;
>>>>> +
>>>>> + /* early exit if no lec update */
>>>>> + if (lec_type == LEC_UNUSED)
>>>>> + return 0;
>>>>
>>>> I think this is not needed, as checked by the only caller.
>>>
>>> You mean move it to caller as follows?
>>> /* handle lec errors on the bus */
>>> if ((psr & LEC_UNUSED) && ((psr & LEC_UNUSED)!= LEC_UNUSED) &&
>>
>> yes - or something like this:
>>
>> static inline bool is_lec(u32 psr)
>> {
>> u32 psr &= LEC_UNUSED
>>
>> return psr && (psr != LEC_UNUSED)
>> }
>>
>> if ((priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) &&
>> is_lec(psr)) {
>> }
>>
>
>
> Looks fine.
> Maybe is_lec_err(u32 psr) better? :-)
Yes, is_lec() was just a random placeholder :) Descriptive function
names are always preferred.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v4 2/2] can: m_can: add Bosch M_CAN controller support
On 07/15/2014 05:33 AM, Dong Aisheng wrote:
> On Mon, Jul 14, 2014 at 02:13:46PM +0200, Marc Kleine-Budde wrote:
>> On 07/14/2014 01:40 PM, Dong Aisheng wrote:
>>> The patch adds the basic CAN TX/RX function support for Bosch M_CAN
>>> controller.
>>> For TX, only one dedicated tx buffer is used for sending data.
>>> For RX, RXFIFO 0 is used for receiving data to avoid overflow.
>>> Rx FIFO 1 and Rx Buffers are not used currently, as well as Tx Event FIFO.
>>>
>>> Due to the message ram can be shared by multi m_can instances
>>> and the fifo element is configurable which is SoC dependant,
>>> the design is to parse the message ram related configuration data from
>>> device
>>> tree rather than hardcode define it in driver which can make the message
>>> ram sharing fully transparent to M_CAN controller driver,
>>> then we can gain better driver maintainability and future features upgrade.
>>>
>>> M_CAN also supports CANFD protocol features like data payload up to 64 bytes
>>> and bitrate switch at runtime, however, this patch still does not add the
>>> support for these features.
>>>
>>> Cc: Wolfgang Grandegger
>>> Cc: Marc Kleine-Budde
>>> Cc: Mark Rutland
>>> Cc: Oliver Hartkopp
>>> Cc: Varka Bhadram
>>> Signed-off-by: Dong Aisheng
>>
>> [...]
>>
>>> +static inline u32 m_can_fifo_read(const struct m_can_priv *priv,
>>> + u32 fgi, unsigned int offset)
>>> +{
>>> + return readl(priv->mram_base + priv->mcfg[MRAM_RXF0].off +
>>> + fgi * RXF0_ELEMENT_SIZE + offset);
>>> +}
>>
>> Can you add a similar function for fifo_write, please?
>
> Unlike fifo_read, we only use one tx buffer for tx function,
> fpi, correspding to fgi, is always 0.
> So i planned to add it later when adding using multi tx buffers before.
> If you like it, i could add it now.
> It could be:
> +static inline void m_can_fifo_write(const struct m_can_priv *priv,
> + u32 fpi, unsigned int offset, u32 val)
> +{
> + return writel(val, priv->mram_base + priv->mcfg[MRAM_TXB].off +
> + fpi * TXB_ELEMENT_SIZE + offset);
> +}
> +
Yes, looks good.
> static inline void m_can_config_endisable(const struct m_can_priv *priv,
> bool enable)
> {
> @@ -973,12 +980,10 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
> }
>
> /* message ram configuration */
> - fifo_addr = priv->mram_base + priv->mcfg[MRAM_TXB].off;
> - writel(id | flags, fifo_addr);
> - writel(cf->can_dlc << 16, fifo_addr + 0x4);
> - writel(*(u32 *)(cf->data + 0), fifo_addr + 0x8);
> - writel(*(u32 *)(cf->data + 4), fifo_addr + 0xc);
> -
> + m_can_fifo_write(priv, 0, 0x0, id | flags);
> + m_can_fifo_write(priv, 0, 0x4, cf->can_dlc << 16);
> + m_can_fifo_write(priv, 0, 0x8, *(u32 *)(cf->data + 0));
> + m_can_fifo_write(priv, 0, 0xc, *(u32 *)(cf->data + 4));
> can_put_echo_skb(skb, dev, 0);
>
> /* enable first TX buffer to start transfer */
>
> The '0' parameter may cause a bit misleading now, do you think it's ok?
Yes.
>>> +
>>> +static inline void m_can_config_endisable(const struct m_can_priv *priv,
>>> + bool enable)
>>> +{
>>> + u32 cccr = m_can_read(priv, M_CAN_CCCR);
>>> + u32 timeout = 10;
>>> + u32 val = 0;
>>> +
>>> + if (enable) {
>>> + /* enable m_can configuration */
>>> + m_can_write(priv, M_CAN_CCCR, cccr | CCCR_INIT);
>>> + /* CCCR.CCE can only be set/reset while CCCR.INIT = '1' */
>>> + m_can_write(priv, M_CAN_CCCR, cccr | CCCR_INIT | CCCR_CCE);
>>> + } else {
>>> + m_can_write(priv, M_CAN_CCCR, cccr & ~(CCCR_INIT | CCCR_CCE));
>>> + }
>>> +
>>> + /* there's a delay for module initialization */
>>> + if (enable)
>>> + val = CCCR_INIT | CCCR_CCE;
>>> +
>>> + while ((m_can_read(priv, M_CAN_CCCR) & (CCCR_INIT | CCCR_CCE))
>>> + != val) {
>>> + if (timeout == 0) {
>>> + netdev_warn(priv->dev, "Failed to init module\n");
>>> + return;
>>> + }
>>> +
Re: [PATCH v4 2/2] can: m_can: add Bosch M_CAN controller support
On 07/15/2014 08:56 AM, Varka Bhadram wrote: > On 07/15/2014 11:57 AM, Dong Aisheng wrote: >> On Mon, Jul 14, 2014 at 06:17:17PM +0530, Varka Bhadram wrote: >>> >>> +/* Test Register (TEST) */ >>> +#define TEST_LBCKBIT(4) >>> + >>> +/* CC Control Register(CCCR) */ >>> +#define CCCR_TESTBIT(7) >>> +#define CCCR_MONBIT(5) >>> +#define CCCR_CCEBIT(1) >>> +#define CCCR_INITBIT(0) >>> + >>> +/* Bit Timing & Prescaler Register (BTP) */ >>> +#define BTR_BRP_MASK0x3ff >>> +#define BTR_BRP_SHIFT16 >>> +#define BTR_TSEG1_SHIFT8 >>> +#define BTR_TSEG1_MASK(0x3f << BTR_TSEG1_SHIFT) >>> +#define BTR_TSEG2_SHIFT4 >>> +#define BTR_TSEG2_MASK(0xf << BTR_TSEG2_SHIFT) >>> +#define BTR_SJW_SHIFT0 >>> +#define BTR_SJW_MASK0xf >>> + >>> +/* Error Counter Register(ECR) */ >>> +#define ECR_RPBIT(15) >>> +#define ECR_REC_SHIFT8 >>> +#define ECR_REC_MASK(0x7f << ECR_REC_SHIFT) >>> +#define ECR_TEC_SHIFT0 >>> +#define ECR_TEC_MASK0xff >>> + >>> +/* Protocol Status Register(PSR) */ >>> +#define PSR_BOBIT(7) >>> +#define PSR_EWBIT(6) >>> +#define PSR_EPBIT(5) >>> +#define PSR_LEC_MASK0x7 >>> + >>> +/* Interrupt Register(IR) */ >>> +#define IR_ALL_INT0x >>> +#define IR_STEBIT(31) >>> +#define IR_FOEBIT(30) >>> +#define IR_ACKEBIT(29) >>> +#define IR_BEBIT(28) >>> +#define IR_CRCEBIT(27) >>> +#define IR_WDIBIT(26) >>> +#define IR_BOBIT(25) >>> +#define IR_EWBIT(24) >>> +#define IR_EPBIT(23) >>> +#define IR_ELOBIT(22) >>> +#define IR_BEUBIT(21) >>> +#define IR_BECBIT(20) >>> +#define IR_DRXBIT(19) >>> +#define IR_TOOBIT(18) >>> +#define IR_MRAFBIT(17) >>> +#define IR_TSWBIT(16) >>> +#define IR_TEFLBIT(15) >>> +#define IR_TEFFBIT(14) >>> +#define IR_TEFWBIT(13) >>> +#define IR_TEFNBIT(12) >>> +#define IR_TFEBIT(11) >>> +#define IR_TCFBIT(10) >>> +#define IR_TCBIT(9) >>> +#define IR_HPMBIT(8) >>> +#define IR_RF1LBIT(7) >>> +#define IR_RF1FBIT(6) >>> +#define IR_RF1WBIT(5) >>> +#define IR_RF1NBIT(4) >>> +#define IR_RF0LBIT(3) >>> +#define IR_RF0FBIT(2) >>> +#define IR_RF0WBIT(1) >>> +#define IR_RF0NBIT(0) >>> +#define IR_ERR_STATE(IR_BO | IR_EW | IR_EP) >>> +#define IR_ERR_LEC(IR_STE| IR_FOE | IR_ACKE | IR_BE | IR_CRCE) >>> +#define IR_ERR_BUS(IR_ERR_LEC | IR_WDI | IR_ELO | IR_BEU | IR_BEC \ >>> +| IR_TOO | IR_MRAF | IR_TSW | IR_TEFL | IR_RF1L \ >>> +| IR_RF0L) >>> +#define IR_ERR_ALL(IR_ERR_STATE | IR_ERR_BUS) >>> + >>> +/* Interrupt Line Select (ILS) */ >>> +#define ILS_ALL_INT00x0 >>> +#define ILS_ALL_INT10x >>> + >>> +/* Interrupt Line Enable (ILE) */ >>> +#define ILE_EINT0BIT(0) >>> +#define ILE_EINT1BIT(1) >>> + >>> +/* Rx FIFO 0/1 Configuration (RXF0C/RXF1C) */ >>> +#define RXFC_FWM_OFF24 >>> +#define RXFC_FWM_MASK0x7f >>> +#define RXFC_FWM_1(1 << RXFC_FWM_OFF) >>> +#define RXFC_FS_OFF16 >>> +#define RXFC_FS_MASK0x7f >>> + >>> +/* Rx FIFO 0/1 Status (RXF0S/RXF1S) */ >>> +#define RXFS_RFLBIT(25) >>> +#define RXFS_FFBIT(24) >>> +#define RXFS_FPI_OFF16 >>> +#define RXFS_FPI_MASK0x3f >>> +#define RXFS_FGI_OFF8 >>> +#define RXFS_FGI_MASK0x3f00 >>> +#define RXFS_FFL_MASK0x7f >>> + >>> +/* Tx Buffer Configuration(TXBC) */ >>> +#define TXBC_NDTB_OFF16 >>> +#define TXBC_NDTB_MASK0x3f >>> + >>> +/* Tx Buffer Element Size Configuration(TXESC) */ >>> +#define TXESC_TBDS_8BYTES0x0 >>> +/* Tx Buffer Element */ >>> +#define TX_BUF_XTDBIT(30) >>> +#define TX_BUF_RTR BIT(29) >>> + >>> +/* Rx Buffer Element Size Configuration(TXESC) */ >>> +#define M_CAN_RXESC_8BYTES0x0 >>&
Re: [PATCH v4 2/2] can: m_can: add Bosch M_CAN controller support
goto exit_disable_cclk;
>> +}
>> +
>> +/* register interrupt handler */
>> +err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
>> + dev);
>> +if (err < 0) {
>> +netdev_err(dev, "failed to request interrupt\n");
>> +goto exit_irq_fail;
>> +}
>> +
>> +/* start the m_can controller */
>> +m_can_start(dev);
>> +
>> +can_led_event(dev, CAN_LED_EVENT_OPEN);
>> +napi_enable(&priv->napi);
>> +netif_start_queue(dev);
>> +
>> +return 0;
>> +
>> +exit_irq_fail:
>> +close_candev(dev);
>> +exit_disable_cclk:
>> +clk_disable_unprepare(priv->cclk);
>> +exit_disable_hclk:
>> +clk_disable_unprepare(priv->hclk);
>> +return err;
>> +}
>> +
>> +static void m_can_stop(struct net_device *dev)
>> +{
>> +struct m_can_priv *priv = netdev_priv(dev);
>> +
>> +/* disable all interrupts */
>> +m_can_disable_all_interrupts(priv);
>> +
>> +clk_disable_unprepare(priv->hclk);
>> +clk_disable_unprepare(priv->cclk);
>> +
>> +/* set the state as STOPPED */
>> +priv->can.state = CAN_STATE_STOPPED;
>> +}
>> +
>> +static int m_can_close(struct net_device *dev)
>> +{
>> +struct m_can_priv *priv = netdev_priv(dev);
>> +
>> +netif_stop_queue(dev);
>> +napi_disable(&priv->napi);
>> +m_can_stop(dev);
>> +free_irq(dev->irq, dev);
>> +close_candev(dev);
>> +can_led_event(dev, CAN_LED_EVENT_STOP);
>> +
>> +return 0;
>> +}
>> +
>> +static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
>> +struct net_device *dev)
>> +{
>> +struct m_can_priv *priv = netdev_priv(dev);
>> +struct can_frame *cf = (struct can_frame *)skb->data;
>> +u32 flags = 0, id;
>> +void __iomem *fifo_addr;
>> +
>> +if (can_dropped_invalid_skb(dev, skb))
>> +return NETDEV_TX_OK;
>> +
>> +netif_stop_queue(dev);
>> +
>> +if (cf->can_id & CAN_RTR_FLAG)
>> +flags |= TX_BUF_RTR;
>> +
>> +if (cf->can_id & CAN_EFF_FLAG) {
>> +id = cf->can_id & CAN_EFF_MASK;
>> +flags |= TX_BUF_XTD;
>> +} else {
>> +id = ((cf->can_id & CAN_SFF_MASK) << 18);
>> +}
>> +
>> +/* message ram configuration */
>> +fifo_addr = priv->mram_base + priv->mcfg[MRAM_TXB].off;
>> +writel(id | flags, fifo_addr);
>> +writel(cf->can_dlc << 16, fifo_addr + 0x4);
>> +writel(*(u32 *)(cf->data + 0), fifo_addr + 0x8);
>> +writel(*(u32 *)(cf->data + 4), fifo_addr + 0xc);
>> +
>> +can_put_echo_skb(skb, dev, 0);
>> +
>> +/* enable first TX buffer to start transfer */
>> +m_can_write(priv, M_CAN_TXBTIE, 0x1);
>> +m_can_write(priv, M_CAN_TXBAR, 0x1);
>> +
>> +return NETDEV_TX_OK;
>> +}
>> +
>> +static const struct net_device_ops m_can_netdev_ops = {
>> +.ndo_open = m_can_open,
>> +.ndo_stop = m_can_close,
>> +.ndo_start_xmit = m_can_start_xmit,
>> +};
>> +
>> +static int register_m_can_dev(struct net_device *dev)
>> +{
>> + dev->flags |= IFF_ECHO;/* we support local echo */
>> +dev->netdev_ops = &m_can_netdev_ops;
>> +
>> +return register_candev(dev);
>> +}
>> +
>> +static const struct of_device_id m_can_of_table[] = {
>> +{ .compatible = "bosch,m_can", .data = NULL },
>> +{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, m_can_of_table);
>> +
>
> Move this device ids before the platform_driver struct ...
>
> No need to include .data=NULL
As I said before, I like the fact that it actively states "no data
needed" for now.
>
>> +static int m_can_of_parse_mram(struct platform_device *pdev,
>> + struct m_can_priv *priv)
>> +{
>> +struct device_node *np = pdev->dev.of_node;
>> +struct resource *res;
>> +void __iomem *addr;
>> +u32 out_val[MRAM_CFG_LEN];
>> +int ret;
>> +
>> +/* message ram could be shared */
>> +res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> "message_ram");
>> +if (!res)
>> +return -ENODEV;
>> +
>> +addr = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>> +if (!addr)
>> +return -ENOMEM;
>> +
>> +/* get message ram configuration */
>> +ret = of_property_read_u32_array(np, "bosch,mram-cfg",
>> + out_val, sizeof(out_val) / 4);
>> +if (ret) {
>> +dev_err(&pdev->dev, "can not get message ram configuration\n");
>> +return -ENODEV;
>> +}
>> +
>> +priv->mram_base = addr;
>> +priv->mcfg[MRAM_SIDF].off = out_val[0];
>> +priv->mcfg[MRAM_SIDF].num = out_val[1];
>> +priv->mcfg[MRAM_XIDF].off = priv->mcfg[MRAM_SIDF].off +
>> +priv->mcfg[MRAM_SIDF].num * SIDF_ELEMENT_SIZE;
>> +priv->mcfg[MRAM_XIDF].num = out_val[2];
>> +priv->mcfg[MRAM_RXF0].off = priv->mcfg[MRAM_XIDF].off +
>> +priv->mcfg[MRAM_XIDF].num * XIDF_ELEMENT_SIZE;
>> +priv->mcfg[MRAM_RXF0].num = out_val[3] & RXFC_FS_MASK;
>> +priv->mcfg[MRAM_RXF1].off = priv->mcfg[MRAM_RXF0].off +
>> +priv->mcfg[MRAM_RXF0].num * RXF0_ELEMENT_SIZE;
>> +priv->mcfg[MRAM_RXF1].num = out_val[4] & RXFC_FS_MASK;
>> +priv->mcfg[MRAM_RXB].off = priv->mcfg[MRAM_RXF1].off +
>> +priv->mcfg[MRAM_RXF1].num * RXF1_ELEMENT_SIZE;
>> +priv->mcfg[MRAM_RXB].num = out_val[5];
>> +priv->mcfg[MRAM_TXE].off = priv->mcfg[MRAM_RXB].off +
>> +priv->mcfg[MRAM_RXB].num * RXB_ELEMENT_SIZE;
>> +priv->mcfg[MRAM_TXE].num = out_val[6];
>> +priv->mcfg[MRAM_TXB].off = priv->mcfg[MRAM_TXE].off +
>> +priv->mcfg[MRAM_TXE].num * TXE_ELEMENT_SIZE;
>> +priv->mcfg[MRAM_TXB].num = out_val[7] & TXBC_NDTB_MASK;
>> +
>> +dev_dbg(&pdev->dev, "mram_base %p sidf 0x%x %d xidf 0x%x %d rxf0
>> 0x%x %d rxf1 0x%x %d rxb 0x%x %d txe 0x%x %d txb 0x%x %d\n",
>> +priv->mram_base,
>> +priv->mcfg[MRAM_SIDF].off, priv->mcfg[MRAM_SIDF].num,
>> +priv->mcfg[MRAM_XIDF].off, priv->mcfg[MRAM_XIDF].num,
>> +priv->mcfg[MRAM_RXF0].off, priv->mcfg[MRAM_RXF0].num,
>> +priv->mcfg[MRAM_RXF1].off, priv->mcfg[MRAM_RXF1].num,
>> +priv->mcfg[MRAM_RXB].off, priv->mcfg[MRAM_RXB].num,
>> +priv->mcfg[MRAM_TXE].off, priv->mcfg[MRAM_TXE].num,
>> +priv->mcfg[MRAM_TXB].off, priv->mcfg[MRAM_TXB].num);
>> +
>> +return 0;
>> +}
>> +
>> +static int m_can_plat_probe(struct platform_device *pdev)
>> +{
>> +struct net_device *dev;
>> +struct m_can_priv *priv;
>> +struct resource *res;
>> +void __iomem *addr;
>> +struct clk *hclk, *cclk;
>> +int irq, ret;
>> +
>> +hclk = devm_clk_get(&pdev->dev, "hclk");
>> +cclk = devm_clk_get(&pdev->dev, "cclk");
>> +if (IS_ERR(hclk) || IS_ERR(cclk)) {
>> +dev_err(&pdev->dev, "no clock find\n");
>> +return -ENODEV;
>> +}
>> +
>> +res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "m_can");
>> +addr = devm_ioremap_resource(&pdev->dev, res);
>> +irq = platform_get_irq_byname(pdev, "int0");
>> +if (IS_ERR(addr) || irq < 0)
>> +return -EINVAL;
>> +
>> +/* allocate the m_can device */
>> +dev = alloc_m_can_dev();
>> +if (!dev)
>> +return -ENOMEM;
>> +
>> +priv = netdev_priv(dev);
>> +dev->irq = irq;
>> +priv->base = addr;
>> +priv->device = &pdev->dev;
>> +priv->hclk = hclk;
>> +priv->cclk = cclk;
>> +priv->can.clock.freq = clk_get_rate(cclk);
>> +
>> +ret = m_can_of_parse_mram(pdev, priv);
>> +if (ret)
>> +goto failed_free_dev;
>> +
>> +platform_set_drvdata(pdev, dev);
>> +SET_NETDEV_DEV(dev, &pdev->dev);
>> +
>> +ret = register_m_can_dev(dev);
>> +if (ret) {
>> +dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
>> +KBUILD_MODNAME, ret);
>> +goto failed_free_dev;
>> +}
>> +
>> +devm_can_led_init(dev);
>> +
>> +dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
>> + KBUILD_MODNAME, priv->base, dev->irq);
>> +
>> +return 0;
>> +
>> +failed_free_dev:
>> +free_m_can_dev(dev);
>> +return ret;
>> +}
>> +
>> +static __maybe_unused int m_can_suspend(struct device *dev)
>> +{
>> +struct net_device *ndev = dev_get_drvdata(dev);
>> +struct m_can_priv *priv = netdev_priv(ndev);
>> +
>> +if (netif_running(ndev)) {
>> +netif_stop_queue(ndev);
>> +netif_device_detach(ndev);
>> +}
>> +
>> +/* TODO: enter low power */
>> +
>> +priv->can.state = CAN_STATE_SLEEPING;
>> +
>> +return 0;
>> +}
>> +
>> +static __maybe_unused int m_can_resume(struct device *dev)
>> +{
>> +struct net_device *ndev = dev_get_drvdata(dev);
>> +struct m_can_priv *priv = netdev_priv(ndev);
>> +
>> +/* TODO: exit low power */
>> +
>> +priv->can.state = CAN_STATE_ERROR_ACTIVE;
>> +
>> +if (netif_running(ndev)) {
>> +netif_device_attach(ndev);
>> +netif_start_queue(ndev);
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +static void unregister_m_can_dev(struct net_device *dev)
>> +{
>> +unregister_candev(dev);
>> +}
>> +
>> +static int m_can_plat_remove(struct platform_device *pdev)
>> +{
>> +struct net_device *dev = platform_get_drvdata(pdev);
>> +
>> +unregister_m_can_dev(dev);
>> +platform_set_drvdata(pdev, NULL);
>> +
>> +free_m_can_dev(dev);
>> +
>> +return 0;
>> +}
>> +
>> +static const struct dev_pm_ops m_can_pmops = {
>> +SET_SYSTEM_SLEEP_PM_OPS(m_can_suspend, m_can_resume)
>> +};
>> +
>
> Give the device ids here. So that we can see the of_match_table directly...
>
> Every driver follows the same concept...
Good point.
>
>> +static struct platform_driver m_can_plat_driver = {
>> +.driver = {
>> +.name = KBUILD_MODNAME,
>> +.of_match_table = of_match_ptr(m_can_of_table),
>
> This driver will always populate through the dts files. So no need to
> use 'of_match_ptr'.
>
>> +.pm = &m_can_pmops,
>> +},
>> +.probe = m_can_plat_probe,
>> +.remove = m_can_plat_remove,
>> +};
>> +
>> +module_platform_driver(m_can_plat_driver);
>> +
>> +MODULE_AUTHOR("Dong Aisheng ");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("CAN bus driver for Bosch M_CAN controller");
>
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v4 1/2] can: m_can: add device tree binding documentation
On 07/14/2014 01:40 PM, Dong Aisheng wrote:
> add M_CAN device tree binding documentation
>
> Cc: Wolfgang Grandegger
> Cc: Marc Kleine-Budde
> Cc: Mark Rutland
> Cc: Oliver Hartkopp
> Cc: Varka Bhadram
> Signed-off-by: Dong Aisheng
> ---
>
> Changes since v3:
> - change mram-cfg property name to bosch,mram-cfg
> - indent change
> ---
> .../devicetree/bindings/net/can/m_can.txt | 67
>
> 1 files changed, 67 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/net/can/m_can.txt
>
> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
> b/Documentation/devicetree/bindings/net/can/m_can.txt
> new file mode 100644
> index 000..5cff839
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
> @@ -0,0 +1,67 @@
> +Bosch MCAN controller Device Tree Bindings
> +-
> +
> +Required properties:
> +- compatible : Should be "bosch,m_can" for M_CAN controllers
> +- reg: physical base address and size of the M_CAN
> + registers map and Message RAM
> +- reg-names : Should be "m_can" and "message_ram"
> +- interrupts : Should be the interrupt number of M_CAN interrupt
> + line 0 and line 1, could be same if sharing
> + the same interrupt.
> +- interrupt-names: Should contain "int0" and "int1"
> +- clocks : Clocks used by controller, should be host clock
> + and CAN clock.
> +- clock-names: Should contain "hclk" and "cclk"
> +- pinctrl-: Pinctrl states as described in
> bindings/pinctrl/pinctrl-bindings.txt
> +- pinctrl-names : Names corresponding to the numbered pinctrl states
> +- bosch,mram-cfg : Message RAM configuration data.
> + Multiple M_CAN instances can share the same Message
> + RAM and each element(e.g Rx FIFO or Tx Buffer and etc)
> + number in Message RAM is also configurable,
> + so this property is telling driver how the shared or
> + private Message RAM are used by this M_CAN controller.
> +
> + The format should be as follows:
> ++rxb_elems txe_elems txb_elems>
> + The 'offset' is an address offset of the Message RAM
> + where the following elements start from. This is
> + usually set to 0x0 if you're using a private Message
> + RAM. The remain cells are used to specify how many
> + elements are used for each FIFO/Buffer.
> +
> + M_CAN includes the following elements according to
> user manual:
> + 11-bit Filter 0-128 elements / 0-128 words
> + 29-bit Filter 0-64 elements / 0-128 words
> + Rx FIFO 0 0-64 elements / 0-1152 words
> + Rx FIFO 1 0-64 elements / 0-1152 words
> + Rx Buffers0-64 elements / 0-1152 words
> + Tx Event FIFO 0-32 elements / 0-64 words
> + Tx Buffers0-32 elements / 0-576 words
> +
> + Please refer to 2.4.1 Message RAM Configuration in
> + Bosch M_CAN user manual for details.
> +
> +Example:
> +SoC dtsi:
> +m_can1: can@020e8000 {
> + compatible = "bosch,m_can";
> + reg = <0x020e8000 0x4000>, <0x02298000 0x4000>;
> + reg-names = "m_can", "message_ram";
> + interrupts = <0 114 0x04>,
> + <0 114 0x04>;
> + interrupt-names = "int0", "int1";
> + clocks = <&clks IMX6SX_CLK_CANFD>,
> + <&clks IMX6SX_CLK_CANFD>;
> + clock-names = "hclk", "cclk";
> + bosch,mram-cfg = <0x0 0 0 32 0 0 0 1>;
> + status = "disabled";
> +};
> +
> +Board dtsi:
The board is typically a dts, isn't it?
> +&m_can1 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_m_can1>;
> + status = "enabled";
> +};
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v4 2/2] can: m_can: add Bosch M_CAN controller support
On 07/14/2014 01:40 PM, Dong Aisheng wrote:
> The patch adds the basic CAN TX/RX function support for Bosch M_CAN
> controller.
> For TX, only one dedicated tx buffer is used for sending data.
> For RX, RXFIFO 0 is used for receiving data to avoid overflow.
> Rx FIFO 1 and Rx Buffers are not used currently, as well as Tx Event FIFO.
>
> Due to the message ram can be shared by multi m_can instances
> and the fifo element is configurable which is SoC dependant,
> the design is to parse the message ram related configuration data from device
> tree rather than hardcode define it in driver which can make the message
> ram sharing fully transparent to M_CAN controller driver,
> then we can gain better driver maintainability and future features upgrade.
>
> M_CAN also supports CANFD protocol features like data payload up to 64 bytes
> and bitrate switch at runtime, however, this patch still does not add the
> support for these features.
>
> Cc: Wolfgang Grandegger
> Cc: Marc Kleine-Budde
> Cc: Mark Rutland
> Cc: Oliver Hartkopp
> Cc: Varka Bhadram
> Signed-off-by: Dong Aisheng
[...]
> +static inline u32 m_can_fifo_read(const struct m_can_priv *priv,
> + u32 fgi, unsigned int offset)
> +{
> + return readl(priv->mram_base + priv->mcfg[MRAM_RXF0].off +
> + fgi * RXF0_ELEMENT_SIZE + offset);
> +}
Can you add a similar function for fifo_write, please?
> +
> +static inline void m_can_config_endisable(const struct m_can_priv *priv,
> + bool enable)
> +{
> + u32 cccr = m_can_read(priv, M_CAN_CCCR);
> + u32 timeout = 10;
> + u32 val = 0;
> +
> + if (enable) {
> + /* enable m_can configuration */
> + m_can_write(priv, M_CAN_CCCR, cccr | CCCR_INIT);
> + /* CCCR.CCE can only be set/reset while CCCR.INIT = '1' */
> + m_can_write(priv, M_CAN_CCCR, cccr | CCCR_INIT | CCCR_CCE);
> + } else {
> + m_can_write(priv, M_CAN_CCCR, cccr & ~(CCCR_INIT | CCCR_CCE));
> + }
> +
> + /* there's a delay for module initialization */
> + if (enable)
> + val = CCCR_INIT | CCCR_CCE;
> +
> + while ((m_can_read(priv, M_CAN_CCCR) & (CCCR_INIT | CCCR_CCE))
> + != val) {
> + if (timeout == 0) {
> + netdev_warn(priv->dev, "Failed to init module\n");
> + return;
> + }
> + timeout--;
> + udelay(1);
> + }
> +}
> +
> +static inline void m_can_enable_all_interrupts(const struct m_can_priv *priv)
> +{
> + m_can_write(priv, M_CAN_ILE, ILE_EINT0 | ILE_EINT1);
> +}
> +
> +static inline void m_can_disable_all_interrupts(const struct m_can_priv
> *priv)
> +{
> + m_can_write(priv, M_CAN_ILE, 0x0);
> +}
> +
> +static void m_can_read_fifo(const struct net_device *dev, struct can_frame
> *cf,
> + u32 rxfs)
> +{
> + struct m_can_priv *priv = netdev_priv(dev);
> + u32 flags, fgi;
> +
> + /* calculate the fifo get index for where to read data */
> + fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
> + flags = m_can_fifo_read(priv, fgi, 0x0);
^^^
Can you introduce an enum for the offsets, please adjust the signature
of m_can_fifo_read() accordingly.
> + if (flags & RX_BUF_XTD)
> + cf->can_id = (flags & CAN_EFF_MASK) | CAN_EFF_FLAG;
> + else
> + cf->can_id = (flags >> 18) & CAN_SFF_MASK;
> +
> + if (flags & RX_BUF_RTR) {
> + cf->can_id |= CAN_RTR_FLAG;
> + } else {
> + flags = m_can_fifo_read(priv, fgi, 0x4);
> + cf->can_dlc = get_can_dlc((flags >> 16) & 0x0F);
> + *(u32 *)(cf->data + 0) = m_can_fifo_read(priv, fgi, 0x8);
> + *(u32 *)(cf->data + 4) = m_can_fifo_read(priv, fgi, 0xC);
> + }
> +
> + /* acknowledge rx fifo 0 */
> + m_can_write(priv, M_CAN_RXF0A, fgi);
> +}
> +
> +static int m_can_do_rx_poll(struct net_device *dev, int quota)
> +{
> + struct m_can_priv *priv = netdev_priv(dev);
> + struct net_device_stats *stats = &dev->stats;
> + struct sk_buff *skb;
> + struct can_frame *frame;
> + u32 pkts = 0;
> + u32 rxfs;
> +
> + rxfs = m_can_read(priv, M_CAN_RXF0S);
> + if (!(rxfs & RXFS_FFL_MASK)) {
> + netdev_dbg(dev, "no messages in fifo0\n");
> + return 0;
> + }
> +
> + while ((rxfs & RXFS_FFL_MASK
Re: [PATCH v3 1/2] can: m_can: add device tree binding documentation
On 07/11/2014 12:29 PM, Dong Aisheng wrote:
> add M_CAN device tree binding documentation
>
> Cc: Wolfgang Grandegger
> Cc: Marc Kleine-Budde
> Cc: Mark Rutland
> Cc: Oliver Hartkopp
> Cc: Varka Bhadram
> Signed-off-by: Dong Aisheng
> ---
> .../devicetree/bindings/net/can/m_can.txt | 65
>
> 1 files changed, 65 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/net/can/m_can.txt
>
> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
> b/Documentation/devicetree/bindings/net/can/m_can.txt
> new file mode 100644
> index 000..c4cb263
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
> @@ -0,0 +1,65 @@
> +Bosch MCAN controller Device Tree Bindings
> +-
> +
> +Required properties:
> +- compatible : Should be "bosch,m_can" for M_CAN controllers
> +- reg: physical base address and size of the M_CAN
> + registers map and Message RAM
> +- reg-names : Should be "m_can" and "message_ram"
> +- interrupts : Should be the interrupt number of M_CAN interrupt
> + line 0 and line 1, could be same if sharing
> + the same interrupt.
> +- interrupt-names: Should contain "int0" and "int1"
> +- clocks : Clocks used by controller, should be host clock
> + and CAN clock.
> +- clock-names: Should contain "hclk" and "cclk"
> +- pinctrl-: Pinctrl states as described in
> bindings/pinctrl/pinctrl-bindings.txt
> +- pinctrl-names : Names corresponding to the numbered pinctrl
> states
> +- mram-cfg : Message RAM configuration data.
> + Multiple M_CAN instances can share the same Message RAM and each
> element(e.g
> + Rx FIFO or Tx Buffer and etc) number in Message RAM is also configurable,
> + so this property is telling driver how the shared or private Message RAM
> + are used by this M_CAN controller.
Will you have to modify the binding, espeically the mram-cfg, if you add
canfd support to the driver?
I think mram-cfg should have a vendor prefix, e.g.: bosch,mram-cfg
> +
> + The format should be as follows:
> + + txe_elems txb_elems>
> + The 'offset' is an address offset of the Message RAM where the following
> + elements start from. This is usually set to 0x0 if you're using a private
> + Message RAM. The remain cells are used to specify how many elements are
> used
> + for each FIFO/Buffer.
> +
> +M_CAN includes the following elements according to user manual:
> +11-bit Filter0-128 elements / 0-128 words
> +29-bit Filter0-64 elements / 0-128 words
> +Rx FIFO 00-64 elements / 0-1152 words
> +Rx FIFO 10-64 elements / 0-1152 words
> +Rx Buffers 0-64 elements / 0-1152 words
> +Tx Event FIFO0-32 elements / 0-64 words
> +Tx Buffers 0-32 elements / 0-576 words
> +
> +Please refer to 2.4.1 Message RAM Configuration in Bosch M_CAN user manual
> +for details.
> +
> +Example:
> +SoC dtsi:
> +m_can1: can@020e8000 {
> + compatible = "bosch,m_can";
> + reg = <0x020e8000 0x4000>, <0x02298000 0x4000>;
> + reg-names = "m_can", "message_ram";
> + interrupts = <0 114 0x04>,
> + <0 114 0x04>;
> + interrupt-names = "int0", "int1";
> + clocks = <&clks IMX6SX_CLK_CANFD>,
> + <&clks IMX6SX_CLK_CANFD>;
> + clock-names = "hclk", "cclk";
> + mram-cfg = <0x0 0 0 32 0 0 0 1>;
> + status = "disabled";
> +};
> +
> +Board dtsi:
> +&m_can1 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_m_can1>;
> + status = "enabled";
> +};
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v3 2/2] can: m_can: add Bosch M_CAN controller support
On 07/11/2014 02:13 PM, Varka Bhadram wrote:
[...]
>>>> +static const struct of_device_id m_can_of_table[] = {
>>>> +{ .compatible = "bosch,m_can", .data = NULL },
>>> we can simply give '0' . No need of .data = NULL. Things should be
>>> simple right :-)
>> .data should be a pointer, while "0" isn't. (Although 0 is valid C, we
>> don't want a integer 0 to initialize a pointer.) However, you can omit
>> .data = NULL completely. When initialzing via C99, any omited members of
>> the struct will automatically be initialized with 0x0. I like to see the
>> .data = NULL because it documents that there isn't any data (yet), once
>> another compatible is added, we need the .data anyways.
>
> static const struct of_device_id m_can_of_table[] = {
> { .compatible = "bosch,m_can", },
> };
>
> This is enough... right ?
Yes
Not having .data = NULL is correct, but I'd like to see it, just to
document: "there is no data needed, nobody forgot it"
...but...
Just must have a NULL-element terminating that list:
>>>> +{ /* sentinel */ },
^^
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, m_can_of_table);
>>>> +
>>>> +static int m_can_of_parse_mram(struct platform_device *pdev,
>>>> + struct m_can_priv *priv)
>>>> +{
>>>> +struct device_node *np = pdev->dev.of_node;
>>>> +struct resource *res;
>>>> +void __iomem *addr;
>>>> +u32 out_val[MRAM_CFG_LEN];
>>>> +int ret;
>>>> +
>>>> +/* message ram could be shared */
>>>> +res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>>>> "message_ram");
>>>> +if (!res)
>>>> +return -ENODEV;
>>>> +
>>>> +addr = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>>>> +if (!addr)
>>>> +return -ENODEV;
>>> Is this err return is appropriate ... ?
>> -ENOMEM seems to be more commonly used.
>>
>>>> +
>>>> +/* get message ram configuration */
>>>> +ret = of_property_read_u32_array(np, "mram-cfg",
>>>> + out_val, sizeof(out_val) / 4);
>>>> +if (ret) {
>>>> +dev_err(&pdev->dev, "can not get message ram
>>>> configuration\n");
>>>> +return -ENODEV;
>>>> +}
>>>> +
>>> Is this err return is appropriate ... ?
>> Whay do you suggest?
>>
>>>> +priv->mram_base = addr;
>>>> +priv->mcfg[MRAM_SIDF].off = out_val[0];
>>>> +priv->mcfg[MRAM_SIDF].num = out_val[1];
>>>> +priv->mcfg[MRAM_XIDF].off = priv->mcfg[MRAM_SIDF].off +
>>>> +priv->mcfg[MRAM_SIDF].num * SIDF_ELEMENT_SIZE;
>>>> +priv->mcfg[MRAM_XIDF].num = out_val[2];
>>>> +priv->mcfg[MRAM_RXF0].off = priv->mcfg[MRAM_XIDF].off +
>>>> +priv->mcfg[MRAM_XIDF].num * XIDF_ELEMENT_SIZE;
>>>> +priv->mcfg[MRAM_RXF0].num = out_val[3] & RXFC_FS_MASK;
>>>> +priv->mcfg[MRAM_RXF1].off = priv->mcfg[MRAM_RXF0].off +
>>>> +priv->mcfg[MRAM_RXF0].num * RXF0_ELEMENT_SIZE;
>>>> +priv->mcfg[MRAM_RXF1].num = out_val[4] & RXFC_FS_MASK;
>>>> +priv->mcfg[MRAM_RXB].off = priv->mcfg[MRAM_RXF1].off +
>>>> +priv->mcfg[MRAM_RXF1].num * RXF1_ELEMENT_SIZE;
>>>> +priv->mcfg[MRAM_RXB].num = out_val[5];
>>>> +priv->mcfg[MRAM_TXE].off = priv->mcfg[MRAM_RXB].off +
>>>> +priv->mcfg[MRAM_RXB].num * RXB_ELEMENT_SIZE;
>>>> +priv->mcfg[MRAM_TXE].num = out_val[6];
>>>> +priv->mcfg[MRAM_TXB].off = priv->mcfg[MRAM_TXE].off +
>>>> +priv->mcfg[MRAM_TXE].num * TXE_ELEMENT_SIZE;
>>>> +priv->mcfg[MRAM_TXB].num = out_val[7] & TXBC_NDTB_MASK;
>>>> +
>>>> +dev_dbg(&pdev->dev, "mram_base %p sidf 0x%x %d xidf 0x%x %d rxf0
>>>> 0x%x %d rxf1 0x%x %d rxb 0x%x %d txe 0x%x %d txb 0x%x %d\n",
>>>> +priv->mram_base,
>>>> +priv->mcfg[MRAM_SIDF].off, priv->mcfg[MRAM_SIDF].num,
>>>> +priv->mcfg[MRAM_XIDF].off, priv->mcfg[MRAM_XIDF].num,
>>>> +priv->mcfg[MRAM_RXF0].off, priv->mcf
Re: [PATCH v3 2/2] can: m_can: add Bosch M_CAN controller support
+
>> +return 0;
>> +}
>> +
>
> (...)
>
>> +
>> +static const struct of_device_id m_can_of_table[] = {
>> +{ .compatible = "bosch,m_can", .data = NULL },
>
> we can simply give '0' . No need of .data = NULL. Things should be
> simple right :-)
.data should be a pointer, while "0" isn't. (Although 0 is valid C, we
don't want a integer 0 to initialize a pointer.) However, you can omit
.data = NULL completely. When initialzing via C99, any omited members of
the struct will automatically be initialized with 0x0. I like to see the
.data = NULL because it documents that there isn't any data (yet), once
another compatible is added, we need the .data anyways.
>
>> +{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, m_can_of_table);
>> +
>> +static int m_can_of_parse_mram(struct platform_device *pdev,
>> + struct m_can_priv *priv)
>> +{
>> +struct device_node *np = pdev->dev.of_node;
>> +struct resource *res;
>> +void __iomem *addr;
>> +u32 out_val[MRAM_CFG_LEN];
>> +int ret;
>> +
>> +/* message ram could be shared */
>> +res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> "message_ram");
>> +if (!res)
>> +return -ENODEV;
>> +
>> +addr = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>> +if (!addr)
>> +return -ENODEV;
>
> Is this err return is appropriate ... ?
-ENOMEM seems to be more commonly used.
>
>> +
>> +/* get message ram configuration */
>> +ret = of_property_read_u32_array(np, "mram-cfg",
>> + out_val, sizeof(out_val) / 4);
>> +if (ret) {
>> +dev_err(&pdev->dev, "can not get message ram configuration\n");
>> +return -ENODEV;
>> +}
>> +
>
> Is this err return is appropriate ... ?
Whay do you suggest?
>
>> +priv->mram_base = addr;
>> +priv->mcfg[MRAM_SIDF].off = out_val[0];
>> +priv->mcfg[MRAM_SIDF].num = out_val[1];
>> +priv->mcfg[MRAM_XIDF].off = priv->mcfg[MRAM_SIDF].off +
>> +priv->mcfg[MRAM_SIDF].num * SIDF_ELEMENT_SIZE;
>> +priv->mcfg[MRAM_XIDF].num = out_val[2];
>> +priv->mcfg[MRAM_RXF0].off = priv->mcfg[MRAM_XIDF].off +
>> +priv->mcfg[MRAM_XIDF].num * XIDF_ELEMENT_SIZE;
>> +priv->mcfg[MRAM_RXF0].num = out_val[3] & RXFC_FS_MASK;
>> +priv->mcfg[MRAM_RXF1].off = priv->mcfg[MRAM_RXF0].off +
>> +priv->mcfg[MRAM_RXF0].num * RXF0_ELEMENT_SIZE;
>> +priv->mcfg[MRAM_RXF1].num = out_val[4] & RXFC_FS_MASK;
>> +priv->mcfg[MRAM_RXB].off = priv->mcfg[MRAM_RXF1].off +
>> +priv->mcfg[MRAM_RXF1].num * RXF1_ELEMENT_SIZE;
>> +priv->mcfg[MRAM_RXB].num = out_val[5];
>> +priv->mcfg[MRAM_TXE].off = priv->mcfg[MRAM_RXB].off +
>> +priv->mcfg[MRAM_RXB].num * RXB_ELEMENT_SIZE;
>> +priv->mcfg[MRAM_TXE].num = out_val[6];
>> +priv->mcfg[MRAM_TXB].off = priv->mcfg[MRAM_TXE].off +
>> +priv->mcfg[MRAM_TXE].num * TXE_ELEMENT_SIZE;
>> +priv->mcfg[MRAM_TXB].num = out_val[7] & TXBC_NDTB_MASK;
>> +
>> +dev_dbg(&pdev->dev, "mram_base %p sidf 0x%x %d xidf 0x%x %d rxf0
>> 0x%x %d rxf1 0x%x %d rxb 0x%x %d txe 0x%x %d txb 0x%x %d\n",
>> +priv->mram_base,
>> +priv->mcfg[MRAM_SIDF].off, priv->mcfg[MRAM_SIDF].num,
>> +priv->mcfg[MRAM_XIDF].off, priv->mcfg[MRAM_XIDF].num,
>> +priv->mcfg[MRAM_RXF0].off, priv->mcfg[MRAM_RXF0].num,
>> +priv->mcfg[MRAM_RXF1].off, priv->mcfg[MRAM_RXF1].num,
>> +priv->mcfg[MRAM_RXB].off, priv->mcfg[MRAM_RXB].num,
>> +priv->mcfg[MRAM_TXE].off, priv->mcfg[MRAM_TXE].num,
>> +priv->mcfg[MRAM_TXB].off, priv->mcfg[MRAM_TXB].num);
>> +
>
> dev_dbg() will insert the new lines in b/w. It wont print the values as
> you expected.
> Check this by enabling debug ...
What do you mean by b/w?
>
>> +return 0;
>> +}
>> +
>
> (...)
>
>> +
>> +static void unregister_m_can_dev(struct net_device *dev)
>> +{
>> +unregister_candev(dev);
>> +}
>> +
>
> again a function which calls a single func.
>
>> +static int m_can_plat_remove(struct platform_device *pdev)
>> +{
>> +struct net_device *dev = platform_get_drvdata(pdev);
>> +
>> +unregister_m_can_dev(dev);
>> +platform_set_drvdata(pdev, NULL);
>> +
>> +free_m_can_dev(dev);
>> +
>> +return 0;
>> +}
>> +
>> +static const struct dev_pm_ops m_can_pmops = {
>> +SET_SYSTEM_SLEEP_PM_OPS(m_can_suspend, m_can_resume)
>> +};
>> +
>> +static struct platform_driver m_can_plat_driver = {
>> +.driver = {
>> +.name = KBUILD_MODNAME,
>> +.owner = THIS_MODULE,
>
> No need to update .owner. module_platform_driver() will do for you.
> see:http://lxr.free-electrons.com/source/include/linux/platform_device.h#L190
Oh, right.
>> +.of_match_table = of_match_ptr(m_can_of_table),
>> +.pm = &m_can_pmops,
>> +},
>> +.probe = m_can_plat_probe,
>> +.remove = m_can_plat_remove,
>> +};
>> +
>> +module_platform_driver(m_can_plat_driver);
>> +
>> +MODULE_AUTHOR("Dong Aisheng ");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("CAN bus driver for Bosch M_CAN controller");
>
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2 1/1] can: m_can: add Bosch M_CAN controller support
On 07/08/2014 01:08 PM, Dong Aisheng wrote:
> On Tue, Jul 08, 2014 at 12:41:41PM +0200, Marc Kleine-Budde wrote:
>> On 07/08/2014 12:30 PM, Dong Aisheng wrote:
>>>> Regarding the mram and the offsets:
>>>>
>>>>> fifo_addr = priv->mram_base + priv->rxf0_off + fgi * RXF0_ELEMENT_SIZE;
>>>>> fifo_addr = priv->mram_base + priv->mram_off + priv->txb_off;
>>>>
>>>> Why is rxf0_off used without the mram_off and txb_off with the mram_off?
>>>> Can you please test your driver with a mram offset != in your DT.
>>>>
>>>> If I understand the code in m_can_of_parse_mram() correctly the
>>>> individual *_off are already offsets to the *mram_base, so mram_off
>>>> should not be used within the driver.
>>>
>>> Good catch!
>>> You're right! I aslo found this recently!
>>> txb_off already includes the mram_off so should not plus mram_off again.
>>> The former test did not find it because it's still not exceed the 16K ram
>>> size for m_can0. But m_can1 has such issue.
>>>
>>>> I even think mram_off should be removed from the priv.
>>>
>>> Right, i also think so.
>>>
>>> It is used for debug information formerly that we need mram_off
>>> to calculate each element address in the fifo.
>>>
>>> By removing mram_off, i'm going to change the debug information to:
>>> dev_dbg(&pdev->dev, "mram_base %p sidf 0x%x %d xidf 0x%x %d rxf0 %x %d rxf1
>>> %x %d rxb %x %d txe %x %d txb %x %d\n",
>>> priv->mram_base, priv->sidf_off, priv->sidf_elems,
>>> priv->xidf_off, priv->xidf_elems, priv->rxf0_off,
>>> priv->rxf0_elems, priv->rxf1_off, priv->rxf1_elems,
>>> priv->rxb_off, priv->rxb_elems, priv->txe_off,
>>> priv->txe_elems, priv->txb_off, priv->txb_elems);
>>>
>>> The annoying thing is the line has to be a much bigger one to avoid
>>> checkpatch warning of "WARNING: quoted string split across lines".
>>>
>>> What's your suggestion for such issue?
>>> Keeping the big line or split into two lines and leave checkpatch warning
>>> there?
>>
>> The idea behind the warning is, that you can grep for error messages
>> better, as normal grep wouldn't find an error string which spans two
>> lines. So make it a long line.
>>
>>>> Do the *_off and *_elems fit into a u8 or u16? If
>>>> so it makes sense to convert the priv accordingly.
>>>>
>>>
>>> Yes, *_off fit into u16 since MRAM has a maximum of 4352 words(17K).
>>> And *_elems fit into u8 since the max number is 128.
>>> I will change them accordingly.
>>>
>>>> What about putting the offset and the number of elements into a struct
>>>> and make use an array for rxf{0,1}?
>>>>
>>>
>>> You mean something like below?
>>> struct mram_cfg {
>>> u16 off;
>>> u8 elements;
>>> };
>>>
>>> struct m_can_priv {
>>>
>>>
>>> struct mram_cfg sidf;
>>> struct mram_cfg xidf;
>>> struct mram_cfg rxf0;
>>> struct mram_cfg rxf1;
>>
>> struct mram_cfg rxf[2];
>>
>
> It does not help too much and a bit strange for only make
> rxf0/rxf1 into array,
>
> How about making them all:
> enum m_can_mram_cfg {
> SIDF = 0,
> XIDF,
> RXF0,
> RXF1,
> RXB,
> TXE,
> TXB,
> CFG_NUM,
> };
>
> struct m_can_priv {
>
> struct mram_cfg mcfg[CFG_NUM];
> };
>
> Then in code:
>
> priv->cfg[SIDF].off =
> priv->cfg[SIDF].elements =
>
> But it could make code become much longer...
I like the idea, but can you add a common prefix to the enums. Though
makes the code even longer :)
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2 1/1] can: m_can: add Bosch M_CAN controller support
On 07/08/2014 12:30 PM, Dong Aisheng wrote:
>> Regarding the mram and the offsets:
>>
>>> fifo_addr = priv->mram_base + priv->rxf0_off + fgi * RXF0_ELEMENT_SIZE;
>>> fifo_addr = priv->mram_base + priv->mram_off + priv->txb_off;
>>
>> Why is rxf0_off used without the mram_off and txb_off with the mram_off?
>> Can you please test your driver with a mram offset != in your DT.
>>
>> If I understand the code in m_can_of_parse_mram() correctly the
>> individual *_off are already offsets to the *mram_base, so mram_off
>> should not be used within the driver.
>
> Good catch!
> You're right! I aslo found this recently!
> txb_off already includes the mram_off so should not plus mram_off again.
> The former test did not find it because it's still not exceed the 16K ram
> size for m_can0. But m_can1 has such issue.
>
>> I even think mram_off should be removed from the priv.
>
> Right, i also think so.
>
> It is used for debug information formerly that we need mram_off
> to calculate each element address in the fifo.
>
> By removing mram_off, i'm going to change the debug information to:
> dev_dbg(&pdev->dev, "mram_base %p sidf 0x%x %d xidf 0x%x %d rxf0 %x %d rxf1
> %x %d rxb %x %d txe %x %d txb %x %d\n",
> priv->mram_base, priv->sidf_off, priv->sidf_elems,
> priv->xidf_off, priv->xidf_elems, priv->rxf0_off,
> priv->rxf0_elems, priv->rxf1_off, priv->rxf1_elems,
> priv->rxb_off, priv->rxb_elems, priv->txe_off,
> priv->txe_elems, priv->txb_off, priv->txb_elems);
>
> The annoying thing is the line has to be a much bigger one to avoid
> checkpatch warning of "WARNING: quoted string split across lines".
>
> What's your suggestion for such issue?
> Keeping the big line or split into two lines and leave checkpatch warning
> there?
The idea behind the warning is, that you can grep for error messages
better, as normal grep wouldn't find an error string which spans two
lines. So make it a long line.
>> Do the *_off and *_elems fit into a u8 or u16? If
>> so it makes sense to convert the priv accordingly.
>>
>
> Yes, *_off fit into u16 since MRAM has a maximum of 4352 words(17K).
> And *_elems fit into u8 since the max number is 128.
> I will change them accordingly.
>
>> What about putting the offset and the number of elements into a struct
>> and make use an array for rxf{0,1}?
>>
>
> You mean something like below?
> struct mram_cfg {
> u16 off;
> u8 elements;
> };
>
> struct m_can_priv {
>
>
> struct mram_cfg sidf;
> struct mram_cfg xidf;
> struct mram_cfg rxf0;
> struct mram_cfg rxf1;
struct mram_cfg rxf[2];
> ..
> struct mram_cfg txb;
> };
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2 1/1] can: m_can: add Bosch M_CAN controller support
On 07/07/2014 09:10 AM, Dong Aisheng wrote: > On Fri, Jul 04, 2014 at 02:21:41PM +0200, Marc Kleine-Budde wrote: >> On 07/04/2014 01:53 PM, Dong Aisheng wrote: >>> The patch adds the basic CAN TX/RX function support for Bosch M_CAN >>> controller. >>> For TX, only one dedicated tx buffer is used for sending data. >>> For RX, RXFIFO 0 is used for receiving data to avoid overflow. >>> Rx FIFO 1 and Rx Buffers are not used currently, as well as Tx Event FIFO. >>> >>> Due to the message ram can be shared by multi m_can instances >>> and the fifo element is configurable which is SoC dependant, >>> the design is to parse the message ram related configuration data from >>> device >>> tree rather than hardcode define it in driver which can make the message >>> ram sharing fully transparent to M_CAN controller driver, >>> then we can gain better driver maintainability and future features upgrade. >>> >>> M_CAN also supports CANFD protocol features like data payload up to 64 bytes >>> and bitrate switch at runtime, however, this patch still does not add the >>> support for these features. >>> >>> Signed-off-by: Dong Aisheng >> >> Looks quite god, comments inline. >> Marc >>> --- >>> Changes since v1: >>> Addressed all comments from Mark Rutland, Hartkopp and Marc Kleine-Budde >>> - merge three patches into one >>> - create directory drivers/net/can/m_can >>> - improve binding doc >>> - make sure using valid pointer before netif_receive_skb(skb) >>> - remove debug info a bit >>> - let the stats are updated even if alloc_can_err_skb() fails >>> - other small fixes >>> >>> Test result: >>> Passed over night can-utils/canfdtest stress test on iMX6SX SDB board. >>> >>> --- >>> .../devicetree/bindings/net/can/m_can.txt | 65 ++ >> >> Please put the DT binding doc into a separate patch. >> > > Okay > >>> drivers/net/can/Kconfig|2 + >>> drivers/net/can/Makefile |1 + >>> drivers/net/can/m_can/Kconfig |4 + >>> drivers/net/can/m_can/Makefile |7 + >>> drivers/net/can/m_can/m_can.c | 1136 >>> >>> 6 files changed, 1215 insertions(+), 0 deletions(-) >>> create mode 100644 Documentation/devicetree/bindings/net/can/m_can.txt >>> create mode 100644 drivers/net/can/m_can/Kconfig >>> create mode 100644 drivers/net/can/m_can/Makefile >>> create mode 100644 drivers/net/can/m_can/m_can.c >>> >>> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt >>> b/Documentation/devicetree/bindings/net/can/m_can.txt >>> new file mode 100644 >>> index 000..3422790 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt >>> @@ -0,0 +1,65 @@ >>> +Bosch MCAN controller Device Tree Bindings >>> +- >>> + >>> +Required properties: >>> +- compatible : Should be "bosch,m_can" for M_CAN controllers >>> +- reg : physical base address and size of the M_CAN >>> + registers map and Message RAM >>> +- reg-names: Should be "m_can" and "message_ram" >>> +- interrupts : Should be the interrupt number of M_CAN >>> interrupt >>> + line 0 and line 1, could be same if sharing >>> + the same interrupt. >>> +- interrupt-names : Should contain "int0" and "int1" >> >> You make only use of one interupt in the driver. >> > > Yes, that's the purpose. > In driver, we will route all interrupts to INT0. > So not need parse INT1 currently. > However, we still define two interrupts in device tree binding > according to hw capability. > It could be helpful if anyone want to implement features like > separate different type of interrupts to different interrupt line > in the future. Okay, do I understand you correctly, it is possible to configure each interrupt source which interrupt shall be triggered? >>> +- clocks : Clocks used by controller, should be host clock >>> + and CAN clock. >>> +- clock-names : Should contain "hclk" and "cclk" >>> +- pinctr
Re: [PATCH v2 1/1] can: m_can: add Bosch M_CAN controller support
On 07/04/2014 01:53 PM, Dong Aisheng wrote: > The patch adds the basic CAN TX/RX function support for Bosch M_CAN > controller. > For TX, only one dedicated tx buffer is used for sending data. > For RX, RXFIFO 0 is used for receiving data to avoid overflow. > Rx FIFO 1 and Rx Buffers are not used currently, as well as Tx Event FIFO. > > Due to the message ram can be shared by multi m_can instances > and the fifo element is configurable which is SoC dependant, > the design is to parse the message ram related configuration data from device > tree rather than hardcode define it in driver which can make the message > ram sharing fully transparent to M_CAN controller driver, > then we can gain better driver maintainability and future features upgrade. > > M_CAN also supports CANFD protocol features like data payload up to 64 bytes > and bitrate switch at runtime, however, this patch still does not add the > support for these features. > > Signed-off-by: Dong Aisheng Looks quite god, comments inline. Marc > --- > Changes since v1: > Addressed all comments from Mark Rutland, Hartkopp and Marc Kleine-Budde > - merge three patches into one > - create directory drivers/net/can/m_can > - improve binding doc > - make sure using valid pointer before netif_receive_skb(skb) > - remove debug info a bit > - let the stats are updated even if alloc_can_err_skb() fails > - other small fixes > > Test result: > Passed over night can-utils/canfdtest stress test on iMX6SX SDB board. > > --- > .../devicetree/bindings/net/can/m_can.txt | 65 ++ Please put the DT binding doc into a separate patch. > drivers/net/can/Kconfig|2 + > drivers/net/can/Makefile |1 + > drivers/net/can/m_can/Kconfig |4 + > drivers/net/can/m_can/Makefile |7 + > drivers/net/can/m_can/m_can.c | 1136 > > 6 files changed, 1215 insertions(+), 0 deletions(-) > create mode 100644 Documentation/devicetree/bindings/net/can/m_can.txt > create mode 100644 drivers/net/can/m_can/Kconfig > create mode 100644 drivers/net/can/m_can/Makefile > create mode 100644 drivers/net/can/m_can/m_can.c > > diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt > b/Documentation/devicetree/bindings/net/can/m_can.txt > new file mode 100644 > index 000..3422790 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/can/m_can.txt > @@ -0,0 +1,65 @@ > +Bosch MCAN controller Device Tree Bindings > +- > + > +Required properties: > +- compatible : Should be "bosch,m_can" for M_CAN controllers > +- reg: physical base address and size of the M_CAN > + registers map and Message RAM > +- reg-names : Should be "m_can" and "message_ram" > +- interrupts : Should be the interrupt number of M_CAN interrupt > + line 0 and line 1, could be same if sharing > + the same interrupt. > +- interrupt-names: Should contain "int0" and "int1" You make only use of one interupt in the driver. > +- clocks : Clocks used by controller, should be host clock > + and CAN clock. > +- clock-names: Should contain "hclk" and "cclk" > +- pinctrl-: Pinctrl states as described in > bindings/pinctrl/pinctrl-bindings.txt > +- pinctrl-names : Names corresponding to the numbered pinctrl > states is pinctrl really required? > +- mram-cfg : Message RAM configuration data. > + Multiple M_CAN instances can share the same Message RAM and each > element(e.g > + Rx FIFO or Tx Buffer and etc) number in Message RAM is also configurable, > + so this property is telling driver how the shared or private Message RAM > + are used by this M_CAN controller. > + > + The format should be as follows: > + + txe_elems txb_elems> > + The 'offset' is an address offset of the Message RAM where the following > + elements start from. This is usually set to 0x0 if you're using a private > + Message RAM. The remain cells are used to specify how many elements are > used > + for each FIFO/Buffer. > + > +M_CAN includes the following elements according to user manual: > +11-bit Filter0-128 elements / 0-128 words > +29-bit Filter0-64 elements / 0-128 words > +Rx FIFO 00-64 elements / 0-1152 words > +Rx FIFO 10-64 elements / 0-1152 words > +Rx Buffers 0-64 elements / 0-1152 words > +Tx Event
Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
On 07/03/2014 11:09 AM, Dong Aisheng wrote:
> On Thu, Jul 03, 2014 at 11:04:36AM +0200, Marc Kleine-Budde wrote:
>> On 07/03/2014 10:48 AM, Dong Aisheng wrote:
>>> On Thu, Jul 03, 2014 at 09:12:49AM +0200, Marc Kleine-Budde wrote:
>>>> On 07/03/2014 05:48 AM, Dong Aisheng wrote:
>>>>> On Wed, Jul 02, 2014 at 09:13:07PM +0200, Marc Kleine-Budde wrote:
>>>>>> On 07/02/2014 07:54 PM, Oliver Hartkopp wrote:
>>>>>>> I'm not really familiar with the naming concept in device trees.
>>>>>>>
>>>>>>> What is your opinion about the remarks below?
>>>>>>
>>>>>> The entries in the DT, at least on freescale baords, follow the naming
>>>>>> scheme of the reference manual. E.g. on the mx25 it's can1 and can2:
>>>>>>
>>>>>> can1: can@43f88000 { ... }
>>>>>> can2: can@43f8c000 { ... }
>>>>>>
>>>>>> And on the mx28, its:
>>>>>>
>>>>>> can0: can@80032000 { ... }
>>>>>> can1: can@80034000 { ... }
>>>>>>
>>>>>> Because the imx25 datasheet uses a "1" based counting scheme, while the
>>>>>> imx28 uses a "0" based one.
>>>>>>
>>>>>> So it's best practise to follow the naming and numbering scheme of the
>>>>>> hardware reference manual.and if you have access to the
>>>>>> documentation of the m_can core, use clock names of the m_can core for
>>>>>> the clock-names property.
>>>>>>
>>>>>
>>>>> Based on my knowledge, device tree allows define phandle name according to
>>>>> the real device name of HW according spec while the device node name
>>>>> should
>>>>> be general(e.g can@80032000 rather than flexcan@80032000).
>>>>> For imx6sx, there are already following entries in
>>>>> arch/arm/boot/dts/imx6sx.dtsi
>>>>> flexcan1: can@0209 {...}
>>>>> flexcan2: can@02094000 {...}
>>>>> So i'd prefer to define as:
>>>>> m_can1: canfd@020e8000 {...}
>>>>> m_can2: canfd@020f {...}
>>>>>
>>>>>
>>>>> One problem is there're can alias already.
>>>>> aliases {
>>>>> can0 = &flexcan1;
>>>>> can1 = &flexcan2;
>>>>> ...
>>>>> }
>>>>> I'm not sure adding can2&can3 for mcan is properly:
>>>>> aliases {
>>>>> can0 = &flexcan1;
>>>>> can1 = &flexcan2;
>>>>> can2 = &m_can1;
>>>>> can3 = &m_can2;
>>>>> ...
>>>>> }
>>>>> Since the m_can driver does not need to use aliases,
>>>>> so i will not add them.
>>>>
>>>> IMHO It's fine too add the can{2,3} aliases to m_can, too.
>>>>
>>>
>>> I think the main problem for doing this way is that the meaning of id
>>> return by of_alias_get_id may be not persistent.
>>> e.g
>>> For MX6SX
>>> aliases {
>>> can0 = &flexcan1;
>>> can1 = &flexcan2;
>>> can2 = &m_can1;
>>> can3 = &m_can2;
>>> ...
>>> }
>>>
>>> For other platform, it could be:
>>> aliases {
>>> can0 = &m_can1;
>>> can1 = &m_can2;
>>> ...
>>> }
>>> It's hard for driver to use.
>>
>> The driver doesn't make use of it, does it?
>>
>>> And actually the M_CAN driver does not need to use the alias.
>>> So i wonder if it makes sense to add the alias for m_can devices
>>> like that.
>>
>> For example the imx53 has two different SPI units, in the alias section
>> we see:
>>
>> spi0 = &ecspi1;
>> spi1 = &ecspi2;
>> spi2 = &cspi;
>
> Thanks for the info.
> I'm not clear what's our purpose adding alias like this?
> Can you help explain it a bit?
You can use the alias in your dts to refer to the node instead of the
more complicate name. Further you bring a order to all devices of the
same type and you can make use of the number in the alias for other
purposes, which is however not used for CAN afaik.
> Do we need adding alias for all exist devices?
Have a look at the existing imx*.dtsi files. If there are already
aliases for a given device type, then it makes sense to add new devices
of that type to the alias aswell.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
On 07/03/2014 10:48 AM, Dong Aisheng wrote:
> On Thu, Jul 03, 2014 at 09:12:49AM +0200, Marc Kleine-Budde wrote:
>> On 07/03/2014 05:48 AM, Dong Aisheng wrote:
>>> On Wed, Jul 02, 2014 at 09:13:07PM +0200, Marc Kleine-Budde wrote:
>>>> On 07/02/2014 07:54 PM, Oliver Hartkopp wrote:
>>>>> I'm not really familiar with the naming concept in device trees.
>>>>>
>>>>> What is your opinion about the remarks below?
>>>>
>>>> The entries in the DT, at least on freescale baords, follow the naming
>>>> scheme of the reference manual. E.g. on the mx25 it's can1 and can2:
>>>>
>>>> can1: can@43f88000 { ... }
>>>> can2: can@43f8c000 { ... }
>>>>
>>>> And on the mx28, its:
>>>>
>>>> can0: can@80032000 { ... }
>>>> can1: can@80034000 { ... }
>>>>
>>>> Because the imx25 datasheet uses a "1" based counting scheme, while the
>>>> imx28 uses a "0" based one.
>>>>
>>>> So it's best practise to follow the naming and numbering scheme of the
>>>> hardware reference manual.and if you have access to the
>>>> documentation of the m_can core, use clock names of the m_can core for
>>>> the clock-names property.
>>>>
>>>
>>> Based on my knowledge, device tree allows define phandle name according to
>>> the real device name of HW according spec while the device node name should
>>> be general(e.g can@80032000 rather than flexcan@80032000).
>>> For imx6sx, there are already following entries in
>>> arch/arm/boot/dts/imx6sx.dtsi
>>> flexcan1: can@0209 {...}
>>> flexcan2: can@02094000 {...}
>>> So i'd prefer to define as:
>>> m_can1: canfd@020e8000 {...}
>>> m_can2: canfd@020f {...}
>>>
>>>
>>> One problem is there're can alias already.
>>> aliases {
>>> can0 = &flexcan1;
>>> can1 = &flexcan2;
>>> ...
>>> }
>>> I'm not sure adding can2&can3 for mcan is properly:
>>> aliases {
>>> can0 = &flexcan1;
>>> can1 = &flexcan2;
>>> can2 = &m_can1;
>>> can3 = &m_can2;
>>> ...
>>> }
>>> Since the m_can driver does not need to use aliases,
>>> so i will not add them.
>>
>> IMHO It's fine too add the can{2,3} aliases to m_can, too.
>>
>
> I think the main problem for doing this way is that the meaning of id
> return by of_alias_get_id may be not persistent.
> e.g
> For MX6SX
> aliases {
> can0 = &flexcan1;
> can1 = &flexcan2;
> can2 = &m_can1;
> can3 = &m_can2;
> ...
> }
>
> For other platform, it could be:
> aliases {
> can0 = &m_can1;
> can1 = &m_can2;
> ...
> }
> It's hard for driver to use.
The driver doesn't make use of it, does it?
> And actually the M_CAN driver does not need to use the alias.
> So i wonder if it makes sense to add the alias for m_can devices
> like that.
For example the imx53 has two different SPI units, in the alias section
we see:
spi0 = &ecspi1;
spi1 = &ecspi2;
spi2 = &cspi;
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
On 07/03/2014 05:48 AM, Dong Aisheng wrote:
> On Wed, Jul 02, 2014 at 09:13:07PM +0200, Marc Kleine-Budde wrote:
>> On 07/02/2014 07:54 PM, Oliver Hartkopp wrote:
>>> I'm not really familiar with the naming concept in device trees.
>>>
>>> What is your opinion about the remarks below?
>>
>> The entries in the DT, at least on freescale baords, follow the naming
>> scheme of the reference manual. E.g. on the mx25 it's can1 and can2:
>>
>> can1: can@43f88000 { ... }
>> can2: can@43f8c000 { ... }
>>
>> And on the mx28, its:
>>
>> can0: can@80032000 { ... }
>> can1: can@80034000 { ... }
>>
>> Because the imx25 datasheet uses a "1" based counting scheme, while the
>> imx28 uses a "0" based one.
>>
>> So it's best practise to follow the naming and numbering scheme of the
>> hardware reference manual.and if you have access to the
>> documentation of the m_can core, use clock names of the m_can core for
>> the clock-names property.
>>
>
> Based on my knowledge, device tree allows define phandle name according to
> the real device name of HW according spec while the device node name should
> be general(e.g can@80032000 rather than flexcan@80032000).
> For imx6sx, there are already following entries in
> arch/arm/boot/dts/imx6sx.dtsi
> flexcan1: can@0209 {...}
> flexcan2: can@02094000 {...}
> So i'd prefer to define as:
> m_can1: canfd@020e8000 {...}
> m_can2: canfd@020f {...}
>
>
> One problem is there're can alias already.
> aliases {
> can0 = &flexcan1;
> can1 = &flexcan2;
> ...
> }
> I'm not sure adding can2&can3 for mcan is properly:
> aliases {
> can0 = &flexcan1;
> can1 = &flexcan2;
> can2 = &m_can1;
> can3 = &m_can2;
> ...
> }
> Since the m_can driver does not need to use aliases,
> so i will not add them.
IMHO It's fine too add the can{2,3} aliases to m_can, too.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
On 07/02/2014 07:54 PM, Oliver Hartkopp wrote:
> I'm not really familiar with the naming concept in device trees.
>
> What is your opinion about the remarks below?
The entries in the DT, at least on freescale baords, follow the naming
scheme of the reference manual. E.g. on the mx25 it's can1 and can2:
can1: can@43f88000 { ... }
can2: can@43f8c000 { ... }
And on the mx28, its:
can0: can@80032000 { ... }
can1: can@80034000 { ... }
Because the imx25 datasheet uses a "1" based counting scheme, while the
imx28 uses a "0" based one.
So it's best practise to follow the naming and numbering scheme of the
hardware reference manual.and if you have access to the
documentation of the m_can core, use clock names of the m_can core for
the clock-names property.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
On 07/02/2014 08:20 AM, Dong Aisheng wrote:
[...]
>>> +static int m_can_do_rx_poll(struct net_device *dev, int quota)
>>> +{
>>> + struct m_can_priv *priv = netdev_priv(dev);
>>> + struct net_device_stats *stats = &dev->stats;
>>> + struct sk_buff *skb;
>>> + struct can_frame *frame;
>>> + u32 rxfs, flags, fgi;
>>> + u32 num_rx_pkts = 0;
>>> +
>>> + rxfs = m_can_read(priv, M_CAN_RXF0S);
>>> + if (!(rxfs & RXFS_FFL_MASK)) {
>>> + netdev_dbg(dev, "no messages in fifo0\n");
>>> + return 0;
>>> + }
>>> +
>>> + while ((rxfs & RXFS_FFL_MASK) && (quota > 0)) {
>>> + netdev_dbg(dev, "fifo0 status 0x%x\n", rxfs);
>>
>> Please remove the netdev_dbg(), once the driver is stable it should be
>> of no use.
>>
>
> Got it.
>
>>> + if (rxfs & RXFS_RFL)
>>> + netdev_warn(dev, "Rx FIFO 0 Message Lost\n");
>>
>> What does that mean? Can you still rx the message if it's lost?
>>
>
> It just warns that there's a message lost, but there are still other
> message in fifo to receive.
>
>>> +
>>> + skb = alloc_can_skb(dev, &frame);
>>> + if (!skb) {
>>> + stats->rx_dropped++;
>>> + return -ENOMEM;
>>
>> Have a look at the user of m_can_do_rx_poll() and how it makes use of
>> the return value.
>>
>
> Right, thanks for spotting it.
>
>>> + }
>>> +
>>> + fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
>>
>> BTW: Is this a _real_ fifo? Or evolution of the c_can/d_can interface
>> where it's not a fifo at all.
>>
>
> Yes, it is real fifo in the message ram.
>
>>> + flags = readl(priv->mram_base + priv->rxf0_off + fgi * 16);
>>> + if (flags & RX_BUF_XTD)
>>> + frame->can_id = (flags & CAN_EFF_MASK) | CAN_EFF_FLAG;
>>> + else
>>> + frame->can_id = (flags >> 18) & CAN_SFF_MASK;
>>> + netdev_dbg(dev, "R0 0x%x\n", flags);
>>
>> please remove dbg
>>> +
>>> + if (flags & RX_BUF_RTR) {
>>> + frame->can_id |= CAN_RTR_FLAG;
>>> + } else {
>>> + flags = readl(priv->mram_base +
>>> + priv->rxf0_off + fgi * 16 + 0x4);
>>> + frame->can_dlc = get_can_dlc((flags >> 16) & 0x0F);
>>> + netdev_dbg(dev, "R1 0x%x\n", flags);
>>
>> please remove
>>
>>> +
>>> + *(u32 *)(frame->data + 0) = readl(priv->mram_base +
>>> + priv->rxf0_off + fgi * 16 + 0x8);
>>> + *(u32 *)(frame->data + 4) = readl(priv->mram_base +
>>> + priv->rxf0_off + fgi * 16 + 0xC);
>>
>>
>> can you create a wrapper function to hide the pointer arithmetics here?
>> Somethig like m_can_read_fifo()
>>
>
> Yes, i could make a wrapper function for it.
>
>>> + netdev_dbg(dev, "R2 0x%x\n", *(u32 *)(frame->data + 0));
>>> + netdev_dbg(dev, "R3 0x%x\n", *(u32 *)(frame->data + 4));
>>> + }
>>> +
>>> + /* acknowledge rx fifo 0 */
>>> + m_can_write(priv, M_CAN_RXF0A, fgi);
>>> +
>>> + netif_receive_skb(skb);
>>> + netdev_dbg(dev, "new packet received\n");
>>> +
>>> + stats->rx_packets++;
>>> + stats->rx_bytes += frame->can_dlc;
>>
>> Please move the stats handling in front of netif_receive_skb() as the
>> skb and thus frame is not a valid pointer anymore.
>>
>
> Good catch!
> Will change it.
>
>>> +
>>> + can_led_event(dev, CAN_LED_EVENT_RX);
>>
>> Please move out of the loop so that it is just called once (if a CAN
>> frame is rx'ed) per m_can_do_rx_poll().
> Why that?
> The purpose is calling it for each new packet received.
It will only trigger LED blinking, and tglx pointed out, that we don't
need the overhead of calling it for every CAN frame.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH] rcar_can: add device tree support
SIMPLE_DEV_PM_OPS(rcar_can_pm_ops, rcar_can_suspend, rcar_can_resume);
>
> +static const struct of_device_id rcar_can_of_table[] __maybe_unused = {
> + { .compatible = "renesas,can-r8a7778" },
> + { .compatible = "renesas,can-r8a7779" },
> + { .compatible = "renesas,can-r8a7790" },
> + { .compatible = "renesas,can-r8a7791" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, rcar_can_of_table);
> +
> static struct platform_driver rcar_can_driver = {
> .driver = {
> .name = RCAR_CAN_DRV_NAME,
> .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(rcar_can_of_table),
> .pm = &rcar_can_pm_ops,
> },
> .probe = rcar_can_probe,
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 3/3] can: m_can: add loopback and monitor mode support
On 06/27/2014 12:00 PM, Dong Aisheng wrote: > add loopback and monitor mode support. > > Signed-off-by: Dong Aisheng Looks good. When finished with the review, I thinks it better to squash all patches into a single patch. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH 2/3] can: m_can: add bus error handling
netdev_dbg(dev, "entered error warning state\n");
> + work_done += m_can_handle_state_change(dev,
> + CAN_STATE_ERROR_WARNING);
> + }
> +
> + if ((psr & PSR_EP) &&
> + (priv->can.state != CAN_STATE_ERROR_PASSIVE)) {
> + netdev_dbg(dev, "entered error warning state\n");
> + work_done += m_can_handle_state_change(dev,
> + CAN_STATE_ERROR_PASSIVE);
> + }
> +
> + if ((psr & PSR_BO) &&
> + (priv->can.state != CAN_STATE_BUS_OFF)) {
> + netdev_dbg(dev, "entered error warning state\n");
> + work_done += m_can_handle_state_change(dev,
> + CAN_STATE_BUS_OFF);
> + }
You might want to push this into a seperate function.
> + }
> +
> + if (irqstatus & IR_ERR_BUS) {
> + if (irqstatus & IR_RF0L)
> + work_done += m_can_handle_lost_msg(dev);
> +
> + /* handle lec errors on the bus */
> + if (psr & LEC_UNUSED)
> + work_done += m_can_handle_bus_err(dev,
> + psr & LEC_UNUSED);
> +
> + /* other unproccessed error interrupts */
> + if (irqstatus & IR_WDI)
> + netdev_err(dev, "Message RAM Watchdog event due to
> missing READY\n");
> + if (irqstatus & IR_TOO)
> + netdev_err(dev, "Timeout reached\n");
> + if (irqstatus & IR_MRAF)
> + netdev_err(dev, "Message RAM access failure
> occurred\n");
same here
> + }
> +
> if (irqstatus & IR_RF0N)
> /* handle events corresponding to receive message objects */
> work_done += m_can_do_rx_poll(dev, (quota - work_done));
> @@ -369,31 +600,18 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
> if (ir & IR_ALL_INT)
> m_can_write(priv, M_CAN_IR, ir);
>
> - if (ir & IR_ERR_ALL) {
> - netdev_dbg(dev, "bus error\n");
> - /* TODO: handle bus error */
> - }
> -
> - /* save irqstatus for later using */
> - priv->irqstatus = ir;
> -
> /*
>* schedule NAPI in case of
>* - rx IRQ
> - * - state change IRQ(TODO)
> - * - bus error IRQ and bus error reporting (TODO)
> + * - state change IRQ
> + * - bus error IRQ and bus error reporting
>*/
> - if (ir & IR_RF0N) {
> + if ((ir & IR_RF0N) || (ir & IR_ERR_ALL)) {
> + priv->irqstatus = ir;
> m_can_enable_all_interrupts(priv, false);
> napi_schedule(&priv->napi);
> }
>
> - /* FIFO overflow */
> - if (ir & IR_RF0L) {
> - dev->stats.rx_over_errors++;
> - dev->stats.rx_errors++;
> - }
> -
> /* transmission complete interrupt */
> if (ir & IR_TC) {
> netdev_dbg(dev, "tx complete\n");
> @@ -446,7 +664,6 @@ static int m_can_set_bittiming(struct net_device *dev)
> * - setup bittiming
> * - TODO:
> * 1) other working modes support like monitor, loopback...
> - * 2) lec error status report enable
> */
> static void m_can_chip_config(struct net_device *dev)
> {
> @@ -515,14 +732,6 @@ static int m_can_set_mode(struct net_device *dev, enum
> can_mode mode)
> return 0;
> }
>
> -static int m_can_get_berr_counter(const struct net_device *dev,
> - struct can_berr_counter *bec)
> -{
> - /* TODO */
> -
> - return 0;
> -}
> -
> static void free_m_can_dev(struct net_device *dev)
> {
> free_candev(dev);
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
On 06/27/2014 12:00 PM, Dong Aisheng wrote:
> +static int m_can_close(struct net_device *dev)
> +{
> + struct m_can_priv *priv = netdev_priv(dev);
> +
> + netif_stop_queue(dev);
> + napi_disable(&priv->napi);
> + m_can_stop(dev);
> + free_irq(dev->irq, dev);
> + close_candev(dev);
> + can_led_event(dev, CAN_LED_EVENT_STOP);
You forgot to turn of the clock.
> +
> + return 0;
> +}
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/3] can: m_can: add Bosch M_CAN controller support
);
> +}
> +
> +static const struct of_device_id m_can_of_table[] = {
> + { .compatible = "bosch,m_can", .data = NULL },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, m_can_of_table);
> +
> +static int m_can_of_parse_mram(struct platform_device *pdev,
> + struct m_can_priv *priv)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct resource *res;
> + void __iomem *addr;
> + u32 out_val[8];
> + int ret;
> +
> + /* message ram could be shared */
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "message_ram");
> + if (!res)
> + return -ENODEV;
> +
> + addr = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> + if (!addr)
> + return -ENODEV;
> +
> + /* get message ram configuration */
> + ret = of_property_read_u32_array(np, "mram-cfg",
> + out_val, sizeof(out_val) / 4);
> + if (ret) {
> + dev_err(&pdev->dev, "can not get message ram configuration\n");
> + return -ENODEV;
> + }
> +
> + priv->mram_base = addr;
> + priv->mram_off = out_val[0];
> + priv->sidf_elems = out_val[1];
> + priv->sidf_off = priv->mram_off;
> + priv->xidf_elems = out_val[2];
> + priv->xidf_off = priv->sidf_off + priv->sidf_elems * SIDF_ELEMENT_SIZE;
> + priv->rxf0_elems = out_val[3] & RXFC_FS_MASK;
> + priv->rxf0_off = priv->xidf_off + priv->xidf_elems * XIDF_ELEMENT_SIZE;
> + priv->rxf1_elems = out_val[4] & RXFC_FS_MASK;
> + priv->rxf1_off = priv->rxf0_off + priv->rxf0_elems * RXF0_ELEMENT_SIZE;
> + priv->rxb_elems = out_val[5];
> + priv->rxb_off = priv->rxf1_off + priv->rxf1_elems * RXF1_ELEMENT_SIZE;
> + priv->txe_elems = out_val[6];
> + priv->txe_off = priv->rxb_off + priv->rxb_elems * RXB_ELEMENT_SIZE;
> + priv->txb_elems = out_val[7] & TXBC_NDTB_MASK;
> + priv->txb_off = priv->txe_off + priv->txe_elems * TXE_ELEMENT_SIZE;
> +
> + dev_dbg(&pdev->dev, "mram_base =%p mram_off =0x%x "
> + "sidf %d xidf %d rxf0 %d rxf1 %d rxb %d txe %d txb %d\n",
> + priv->mram_base, priv->mram_off, priv->sidf_elems,
> + priv->xidf_elems, priv->rxf0_elems, priv->rxf1_elems,
> + priv->rxb_elems, priv->txe_elems, priv->txb_elems);
> +
> + return 0;
> +}
> +
> +static int m_can_plat_probe(struct platform_device *pdev)
> +{
> + struct net_device *dev;
> + struct m_can_priv *priv;
> + struct pinctrl *pinctrl;
> + struct resource *res;
> + void __iomem *addr;
> + struct clk *clk;
> + int irq, ret;
> +
> + pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> + if (IS_ERR(pinctrl))
> + dev_warn(&pdev->dev,
> + "failed to configure pins from driver\n");
> +
> + clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(clk)) {
> + dev_err(&pdev->dev, "no clock find\n");
> + return PTR_ERR(clk);
> + }
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "canfd");
> + addr = devm_ioremap_resource(&pdev->dev, res);
> + irq = platform_get_irq(pdev, 0);
> + if (IS_ERR(addr) || irq < 0)
> + return -EINVAL;
> +
> + /* allocate the m_can device */
> + dev = alloc_m_can_dev();
> + if (!dev)
> + return -ENOMEM;
> +
> + priv = netdev_priv(dev);
> + dev->irq = irq;
> + priv->base = addr;
> + priv->device = &pdev->dev;
> + priv->clk = clk;
> + priv->can.clock.freq = clk_get_rate(clk);
> +
> + ret = m_can_of_parse_mram(pdev, priv);
> + if (ret)
> + goto failed_free_dev;
> +
> + platform_set_drvdata(pdev, dev);
> + SET_NETDEV_DEV(dev, &pdev->dev);
> +
> + ret = register_m_can_dev(dev);
> + if (ret) {
> + dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
> + KBUILD_MODNAME, ret);
> + goto failed_free_dev;
> + }
> +
> + devm_can_led_init(dev);
> +
> + dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
> + KBUILD_MODNAME, priv->base, dev->irq);
> +
> + return 0;
> +
> +failed_free_dev:
> + free_m_can_dev(dev);
> + return ret;
> +}
> +
> +#ifdef CONFIG_PM
please remove and use __maybe_unused unstead
> +static int m_can_suspend(struct device *dev)
> +{
> + struct net_device *ndev = dev_get_drvdata(dev);
> + struct m_can_priv *priv = netdev_priv(ndev);
> +
> + if (netif_running(ndev)) {
> + netif_stop_queue(ndev);
> + netif_device_detach(ndev);
> + }
> +
> + /* TODO: enter low power */
> +
> + priv->can.state = CAN_STATE_SLEEPING;
> +
> + return 0;
> +}
> +
> +static int m_can_resume(struct device *dev)
> +{
> + struct net_device *ndev = dev_get_drvdata(dev);
> + struct m_can_priv *priv = netdev_priv(ndev);
> +
> + /* TODO: exit low power */
> +
> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +
> + if (netif_running(ndev)) {
> + netif_device_attach(ndev);
> + netif_start_queue(ndev);
> + }
> +
> + return 0;
> +}
> +#endif
> +
> +static void unregister_m_can_dev(struct net_device *dev)
> +{
> + unregister_candev(dev);
> +}
> +
> +static int m_can_plat_remove(struct platform_device *pdev)
> +{
> + struct net_device *dev = platform_get_drvdata(pdev);
> +
> + unregister_m_can_dev(dev);
> + platform_set_drvdata(pdev, NULL);
> +
> + free_m_can_dev(dev);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops m_can_pmops = {
> + SET_SYSTEM_SLEEP_PM_OPS(m_can_suspend, m_can_resume)
> +};
> +
> +static struct platform_driver m_can_plat_driver = {
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(m_can_of_table),
> + .pm = &m_can_pmops,
> + },
> + .probe = m_can_plat_probe,
> + .remove = m_can_plat_remove,
> +};
> +
> +module_platform_driver(m_can_plat_driver);
> +
> +MODULE_AUTHOR("Dong Aisheng ");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("CAN bus driver for Bosch M_CAN controller");
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v8 1/2] can: Add xilinx CAN device tree bindings documentation.
On 05/21/2014 01:41 PM, Kedareswara rao Appana wrote: > Add xilinx CAN bindings documentation. > > Signed-off-by: Kedareswara rao Appana Applied both to can-next. Thanks, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH v7 1/2] can: xilinx CAN controller support
On 04/22/2014 01:08 PM, Appana Durga Kedareswara Rao wrote:
> Hi Marc,
>
>> -Original Message-----
>> From: Marc Kleine-Budde [mailto:m...@pengutronix.de]
>> Sent: Tuesday, April 22, 2014 3:57 PM
>> To: Appana Durga Kedareswara Rao; w...@grandegger.com; Michal Simek;
>> grant.lik...@linaro.org; robh...@kernel.org
>> Cc: linux-...@vger.kernel.org; net...@vger.kernel.org; linux-arm-
>> ker...@lists.infradead.org; linux-ker...@vger.kernel.org;
>> devicetree@vger.kernel.org
>> Subject: Re: [PATCH v7 1/2] can: xilinx CAN controller support
>>
>> On 04/22/2014 12:06 PM, Appana Durga Kedareswara Rao wrote:
>>
>>>> Meanwhile Thomas Gleixner put some effort into the c_can driver and
>>>> found some problems in most of the can driver. See comments inline.
>>>>
>>> Ok will look into that patches
>>
>> I've commented the relevant parts of your patch. Although more
>> background information can be found in the c_can patches.
>>
> Ok
>
>>>>> +/**
>>>>> + * xcan_tx_interrupt - Tx Done Isr
>>>>> + * @ndev: net_device pointer
>>>>> + * @isr: Interrupt status register value
>>>>> + */
>>>>> +static void xcan_tx_interrupt(struct net_device *ndev, u32 isr) {
>>>>> + struct xcan_priv *priv = netdev_priv(ndev);
>>>>> + struct net_device_stats *stats = &ndev->stats;
>>>>> +
>>>>> + while (priv->tx_head - priv->tx_tail > 0) {
>>>>> + priv->write_reg(priv, XCAN_ICR_OFFSET,
>>>> XCAN_IXR_TXOK_MASK);
>>>>> + if (!(isr & XCAN_IXR_TXOK_MASK))
>>>>> + break;
>>>>
>>>> This looks broken. I assume you have to issue the
>> XCAN_IXR_TXOK_MASK-
>>>> write once per tx-completed CAN frame. If you enter this loop you
>>>> write once, then isr is read, then you write again and may exit this
>>>> loop if XCAN_IXR_TXOK_MASK is not set anymore.
>>
>> Let's assume you have put 3 CAN frames into the TX-queue and we're into
>> tx-complete interrupt for the first frame and the other 2 are still not
>> completed.
>>
>> This means the while() loop is not terminated by (priv->tx_head -
>> priv->tx_tail > 0), as it can loop 3 times.
>>
>> What happens is:
>>
>> - priv->write_reg(priv, XCAN_ICR_OFFSET, XCAN_IXR_TXOK_MASK);
>> - if (!(isr & XCAN_IXR_TXOK_MASK)) -> no break
>> - can_get_echo_skb()
>> - ...
>> - isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
>> - loop ->
>> - priv->write_reg(priv, XCAN_ICR_OFFSET, XCAN_IXR_TXOK_MASK);
>> - if (!(isr & XCAN_IXR_TXOK_MASK)) -> break
>>
>> So you have 2x write_reg(priv, XCAN_ICR_OFFSET, XCAN_IXR_TXOK_MASK),
>> but only a single TX completed CAN frame.
>>
>
> Ok now got it :). Thanks for the explanation.
> Will modify the loop logic like below.
>
> while (priv->tx_head - priv->tx_tail > 0) {
> if ((isr & XCAN_IXR_TXOK_MASK)) {
> priv->write_reg(priv, XCAN_ICR_OFFSET, XCAN_IXR_TXOK_MASK);
> can_get_echo_skb(ndev, priv->tx_tail %
> priv->tx_max);
> priv->tx_tail++;
> stats->tx_packets++;
> } else {
> break;
> }
>
> }
> Are you ok with this?
what about this:
while ((priv->tx_head - priv->tx_tail > 0) &&
(isr & XCAN_IXR_TXOK_MASK)) {
[...]
isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
}
or even
while ((priv->tx_head - priv->tx_tail > 0) &&
(priv->read_reg(priv, XCAN_ISR_OFFSET) & XCAN_IXR_TXOK_MASK)) {
[...]
}
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v7 1/2] can: xilinx CAN controller support
>netdev_ops = &xcan_netdev_ops;
>>> +
>>> + /* Getting the CAN can_clk info */
>>> + priv->can_clk = devm_clk_get(&pdev->dev, "can_clk");
>>> + if (IS_ERR(priv->can_clk)) {
>>> + dev_err(&pdev->dev, "Device clock not found.\n");
>>> + ret = PTR_ERR(priv->can_clk);
>>> + goto err_free;
>>> + }
>>> + /* Check for type of CAN device */
>>> + if (of_device_is_compatible(pdev->dev.of_node,
>>> + "xlnx,zynq-can-1.0")) {
>>> + priv->bus_clk = devm_clk_get(&pdev->dev, "pclk");
>>
>> I think it makes sense to have a single name for the second clock, so that
>> this if...else... is't needed at all.
>>
> One of the comments I got from the Soren is the clocks name should match the
> name's in the device datasheet.
>
> For axi_can case the clock names is can_clk and s_axi_aclk
> For canps case the clock names is can_clk and pclk.
>
> If you want me to put a unique for the both I will do.
In an ideal world the data sheet of the IP will have a unique name for
its clock inputs, regardless the version of the IP core and the design
(processor, softcore, etc...) it is integrated into.
The first clock is named "can_clk", so we only need a single name for
the second clock. Any opinions from the devicetree people?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v7 1/2] can: xilinx CAN controller support
On 04/22/2014 09:56 AM, Appana Durga Kedareswara Rao wrote: > Hi Marc, > >> -Original Message- >> From: Michal Simek [mailto:mon...@monstr.eu] >> Sent: Monday, April 07, 2014 12:27 PM >> To: Appana Durga Kedareswara Rao >> Cc: w...@grandegger.com; m...@pengutronix.de; Michal Simek; >> grant.lik...@linaro.org; robh...@kernel.org; linux-...@vger.kernel.org; >> net...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linux- >> ker...@vger.kernel.org; devicetree@vger.kernel.org; Appana Durga >> Kedareswara Rao >> Subject: Re: [PATCH v7 1/2] can: xilinx CAN controller support >> >> On 04/02/2014 03:13 PM, Kedareswara rao Appana wrote: >>> This patch adds xilinx CAN controller support. >>> This driver supports both ZYNQ CANPS and Soft IP AXI CAN controller. >>> >>> Signed-off-by: Kedareswara rao Appana >>> --- >>> Changes for v7: >>> - Updated the driver with review comments. >>> - Moved the driver bindings doc as a separte patch. >>> Changes for v6: >>> - Updated the driver with review comments. >>> - Used the clock names specified in the data sheet. >>> - Updated the devicetree bindings doc as per Rob suggestion. >>> Changes for v5: >>> - Updated the driver with the review comments. >>> - Remove the check for the tx fifo full interrupt condition >>> form Tx interrupt routine as we are checking it in the _xmit >>> routine. >>> - Clearing the txok interrupt in the tx interrupt routine for >>> every Tx can frame. >>> Changes for v4: >>> - Added check for the tx fifo full interrupt condition in Tx interrupt >>> routine. >>> - Added be iohelper functions. >>> - Moved the clock enable/disable to probe/remove because of >>> Added big endian support for AXI CAN controller case(reading >>> a register during probe for that we need to enable clock). >>> Changes for v3: >>> - Updated the driver with the review comments. >>> - Modified the tranmit logic as per Marc suggestion. >>> - Enabling the clock when the interface is up to reduce the >>> Power consumption. >>> Changes for v2: >>> - Updated with the review comments. >>> - Removed the unnecessary debug prints. >>> - include tx,rx fifo depths in ZYNQ CANPS case also >>> --- >>> drivers/net/can/Kconfig |7 + >>> drivers/net/can/Makefile |1 + >>> drivers/net/can/xilinx_can.c | 1176 >>> ++++++++++ >>> 3 files changed, 1184 insertions(+), 0 deletions(-) create mode >>> 100644 drivers/net/can/xilinx_can.c >> >> Mark: Any update on this one? >> BTW: When you apply these patches you should apply 2/2 before 1/2. There >> is new checking for binding. >> > > Ping? Done, when you repost, can you change the order of the patches, so that the binding comes first. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH v7 1/2] can: xilinx CAN controller support
us_clk);
> + return ret;
> + }
> +
> + priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> + priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +
> + if (netif_running(ndev)) {
> + netif_device_attach(ndev);
> + netif_start_queue(ndev);
> + }
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend, xcan_resume);
> +
> +/**
> + * xcan_probe - Platform registration call
> + * @pdev:Handle to the platform device structure
> + *
> + * This function does all the memory allocation and registration for the CAN
> + * device.
> + *
> + * Return: 0 on success and failure value on error
> + */
> +static int xcan_probe(struct platform_device *pdev)
> +{
> + struct resource *res; /* IO mem resources */
> + struct net_device *ndev;
> + struct xcan_priv *priv;
> + void __iomem *addr;
> + int ret, rx_max, tx_max;
> +
> + /* Get the virtual base address for the device */
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + addr = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(addr)) {
> + ret = PTR_ERR(addr);
> + goto err;
> + }
> +
> + ret = of_property_read_u32(pdev->dev.of_node, "tx-fifo-depth", &tx_max);
> + if (ret < 0)
> + goto err;
> +
> + ret = of_property_read_u32(pdev->dev.of_node, "rx-fifo-depth", &rx_max);
> + if (ret < 0)
> + goto err;
> +
> + /* Create a CAN device instance */
> + ndev = alloc_candev(sizeof(struct xcan_priv), tx_max);
> + if (!ndev)
> + return -ENOMEM;
> +
> + priv = netdev_priv(ndev);
> + priv->dev = ndev;
> + priv->can.bittiming_const = &xcan_bittiming_const;
> + priv->can.do_set_mode = xcan_do_set_mode;
> + priv->can.do_get_berr_counter = xcan_get_berr_counter;
> + priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> + CAN_CTRLMODE_BERR_REPORTING;
> + priv->reg_base = addr;
> + priv->tx_max = tx_max;
> +
> + /* Get IRQ for the device */
> + ndev->irq = platform_get_irq(pdev, 0);
> + ndev->flags |= IFF_ECHO;/* We support local echo */
> +
> +
Re: [PATCH v5] can: xilinx CAN controller support.
* Put device into loopback mode */
> priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_LBACK_MASK);
> else
> /* The device is in normal mode */
> priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
What about using two temp variables? Like this (untested, though):
u32 reg_msg;
u32 reg_sr_mask;
if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
reg_msr = XCAN_MSR_LBACK_MASK;
reg_sr_mask = XCAN_SR_LBACK_MASK;
} else {
reg_msr = 0x0;
reg_sr_mask = XCAN_SR_NORMAL_MASK;
}
priv->write_reg(priv, XCAN_MSR_OFFSET, reg_msr);
priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
timeout = jiffies + XCAN_TIMEOUT;
while (!(priv->read_reg(priv, XCAN_SR_OFFSET) & reg_sr_mask)) {
if (time_after(jiffies, timeout)) {
netdev_warn(ndev, "time out waiting for correct mode\n")
return -ETIMEDOUT;
}
usleep_range(500, 1);
}
>
> priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> timeout = jiffies + XCAN_TIMEOUT;
> if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
> while ((priv->read_reg(priv, XCAN_SR_OFFSET)
> & XCAN_SR_LBACK_MASK) == 0) {
> if (time_after(jiffies, timeout)) {
> netdev_warn(ndev,
> "timedout for loopback mode\n");
> return -ETIMEDOUT;
> }
> usleep_range(500, 1);
> }
> } else {
> while ((priv->read_reg(priv, XCAN_SR_OFFSET)
> & XCAN_SR_NORMAL_MASK) == 0) {
> if (time_after(jiffies, timeout)) {
> netdev_warn(ndev,
> "timedout for normal mode\n");
> return -ETIMEDOUT;
> }
> usleep_range(500, 1);
> }
> }
> netdev_dbg(ndev, "status:#x%08x\n",
> priv->read_reg(priv, XCAN_SR_OFFSET));
>
> priv->can.state = CAN_STATE_ERROR_ACTIVE;
> return 0;
> }
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v5] can: xilinx CAN controller support.
ed this condition here because in
> set_reset_mode we are putting the
> Device state in Stopped state.
So the device is in CAN_STATE_STOPPED, as this code just went through
set_reset_mode() some lines ago. so it makes no sense to check if it
(still) is (the code runs serialized here).
>>
>>> + /* Enable Xilinx CAN */
>>> + priv->write_reg(priv, XCAN_SRR_OFFSET,
>> XCAN_SRR_CEN_MASK);
>>> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>> + timeout = jiffies + XCAN_TIMEOUT;
>>> + if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
>>> + while ((priv->read_reg(priv, XCAN_SR_OFFSET)
>>> + & XCAN_SR_LBACK_MASK) == 0) {
>>> + if (time_after(jiffies, timeout)) {
>>> + netdev_warn(ndev,
>>> + "timedout for loopback
>> mode\n");
>>> + return -ETIMEDOUT;
>>> + }
>>> + usleep_range(500, 1);
>>> + }
>>> + } else {
>>> + while ((priv->read_reg(priv, XCAN_SR_OFFSET)
>>> + & XCAN_SR_NORMAL_MASK) == 0) {
>>> + if (time_after(jiffies, timeout)) {
>>> + netdev_warn(ndev,
>>> + "timedout for normal
>> mode\n");
>>> + return -ETIMEDOUT;
>>> + }
>>> + usleep_range(500, 1);
>>> + }
>>> + }
>>> + netdev_dbg(ndev, "status:#x%08x\n",
>>> + priv->read_reg(priv, XCAN_SR_OFFSET));
>>> + }
>>> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>> + return 0;
>>> +}
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v5] can: xilinx CAN controller support.
On 03/10/2014 04:04 PM, Michal Simek wrote:
> Hi Marc,
>
> On 03/10/2014 03:57 PM, Marc Kleine-Budde wrote:
>> On 03/04/2014 02:20 PM, Kedareswara rao Appana wrote:
>>> This patch adds xilinx CAN controller support.
>>> This driver supports both ZYNQ CANPS and Soft IP
>>> AXI CAN controller.
>>>
>>> Signed-off-by: Kedareswara rao Appana
>>> ---
>>> This patch is rebased on the 3.14 rc5 kernel.
>>> Changes for v5:
>>> - Updated the driver with the review comments.
>>> - Remove the check for the tx fifo full interrupt condition
>>> form Tx interrupt routine as we are checking it in the _xmit
>>> routine.
>>> - Clearing the txok interrupt in the tx interrupt routine for
>>> every Tx can frame.
>>> Changes for v4:
>>> - Added check for the tx fifo full interrupt condition in
>>> Tx interrupt routine.
>>> - Added be iohelper functions.
>>> - Moved the clock enable/disable to probe/remove because of
>>> Added big endian support for AXI CAN controller case(reading
>>> a register during probe for that we need to enable clock).
>>> Changes for v3:
>>> - Updated the driver with the review comments.
>>> - Modified the tranmit logic as per Marc suggestion.
>>> - Enabling the clock when the interface is up to reduce the
>>> Power consumption.
>>> Changes for v2:
>>> - Updated with the review comments.
>>> - Removed the unnecessary debug prints.
>>> - include tx,rx fifo depths in ZYNQ CANPS case also.
>>> ---
>>> .../devicetree/bindings/net/can/xilinx_can.txt | 45 +
>>> drivers/net/can/Kconfig|7 +
>>> drivers/net/can/Makefile |1 +
>>> drivers/net/can/xilinx_can.c | 1195
>>>
>>> 4 files changed, 1248 insertions(+), 0 deletions(-)
>>> create mode 100644 Documentation/devicetree/bindings/net/can/xilinx_can.txt
>>> create mode 100644 drivers/net/can/xilinx_can.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/can/xilinx_can.txt
>>> b/Documentation/devicetree/bindings/net/can/xilinx_can.txt
>>> new file mode 100644
>>> index 000..0e57103
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/can/xilinx_can.txt
>>> @@ -0,0 +1,45 @@
>>> +Xilinx Axi CAN/Zynq CANPS controller Device Tree Bindings
>>> +-
>>> +
>>> +Required properties:
>>> +- compatible : Should be "xlnx,zynq-can-1.00.a" for Zynq CAN
>>> + controllers and "xlnx,axi-can-1.00.a" for Axi CAN
>>> + controllers.
>>> +- reg : Physical base address and size of the Axi
>>> CAN/Zynq
>>> + CANPS registers map.
>>> +- interrupts : Property with a value describing the interrupt
>>> + number.
>>> +- interrupt-parent : Must be core interrupt controller
>>> +- clock-names : List of input clock names - "ref_clk",
>>> "aper_clk"
>>> + (See clock bindings for details. Two clocks are
>>> + required for Zynq CAN. For Axi CAN
>>> + case it is one(ref_clk)).
>>> +- clocks : Clock phandles (see clock bindings for details).
>>> +- tx-fifo-depth: Can Tx fifo depth.
>>> +- rx-fifo-depth: Can Rx fifo depth.
>>> +
>>> +
>>> +Example:
>>> +
>>> +For Zynq CANPS Dts file:
>>> + zynq_can_0: zynq-can@e0008000 {
>>> + compatible = "xlnx,zynq-can-1.00.a";
>>> + clocks = <&clkc 19>, <&clkc 36>;
>>> + clock-names = "ref_clk", "aper_clk";
>>> + reg = <0xe0008000 0x1000>;
>>> + interrupts = <0 28 4>;
>>> + interrupt-parent = <&intc>;
>>> + tx-fifo-depth = <0x40>;
>>> + rx-fifo-depth = <0x40>;
>>> + };
>>> +For Axi CAN Dts file:
>>> + axi_can_0: axi-can@4000 {
>>> + compatible = "xlnx,axi-can-1.00.a";
>>> + clocks = <&clkc 0>;
>&g
Re: [PATCH v5] can: xilinx CAN controller support.
On 03/04/2014 02:20 PM, Kedareswara rao Appana wrote:
> This patch adds xilinx CAN controller support.
> This driver supports both ZYNQ CANPS and Soft IP
> AXI CAN controller.
>
> Signed-off-by: Kedareswara rao Appana
> ---
> This patch is rebased on the 3.14 rc5 kernel.
> Changes for v5:
> - Updated the driver with the review comments.
> - Remove the check for the tx fifo full interrupt condition
> form Tx interrupt routine as we are checking it in the _xmit
> routine.
> - Clearing the txok interrupt in the tx interrupt routine for
> every Tx can frame.
> Changes for v4:
> - Added check for the tx fifo full interrupt condition in
> Tx interrupt routine.
> - Added be iohelper functions.
> - Moved the clock enable/disable to probe/remove because of
> Added big endian support for AXI CAN controller case(reading
> a register during probe for that we need to enable clock).
> Changes for v3:
> - Updated the driver with the review comments.
> - Modified the tranmit logic as per Marc suggestion.
> - Enabling the clock when the interface is up to reduce the
> Power consumption.
> Changes for v2:
> - Updated with the review comments.
> - Removed the unnecessary debug prints.
> - include tx,rx fifo depths in ZYNQ CANPS case also.
> ---
> .../devicetree/bindings/net/can/xilinx_can.txt | 45 +
> drivers/net/can/Kconfig|7 +
> drivers/net/can/Makefile |1 +
> drivers/net/can/xilinx_can.c | 1195
>
> 4 files changed, 1248 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/net/can/xilinx_can.txt
> create mode 100644 drivers/net/can/xilinx_can.c
>
> diff --git a/Documentation/devicetree/bindings/net/can/xilinx_can.txt
> b/Documentation/devicetree/bindings/net/can/xilinx_can.txt
> new file mode 100644
> index 000..0e57103
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/xilinx_can.txt
> @@ -0,0 +1,45 @@
> +Xilinx Axi CAN/Zynq CANPS controller Device Tree Bindings
> +-
> +
> +Required properties:
> +- compatible : Should be "xlnx,zynq-can-1.00.a" for Zynq CAN
> + controllers and "xlnx,axi-can-1.00.a" for Axi CAN
> + controllers.
> +- reg: Physical base address and size of the Axi
> CAN/Zynq
> + CANPS registers map.
> +- interrupts : Property with a value describing the interrupt
> + number.
> +- interrupt-parent : Must be core interrupt controller
> +- clock-names: List of input clock names - "ref_clk",
> "aper_clk"
> + (See clock bindings for details. Two clocks are
> +required for Zynq CAN. For Axi CAN
> +case it is one(ref_clk)).
> +- clocks : Clock phandles (see clock bindings for details).
> +- tx-fifo-depth : Can Tx fifo depth.
> +- rx-fifo-depth : Can Rx fifo depth.
> +
> +
> +Example:
> +
> +For Zynq CANPS Dts file:
> + zynq_can_0: zynq-can@e0008000 {
> + compatible = "xlnx,zynq-can-1.00.a";
> + clocks = <&clkc 19>, <&clkc 36>;
> + clock-names = "ref_clk", "aper_clk";
> + reg = <0xe0008000 0x1000>;
> + interrupts = <0 28 4>;
> + interrupt-parent = <&intc>;
> + tx-fifo-depth = <0x40>;
> + rx-fifo-depth = <0x40>;
> + };
> +For Axi CAN Dts file:
> + axi_can_0: axi-can@4000 {
> + compatible = "xlnx,axi-can-1.00.a";
> + clocks = <&clkc 0>;
> + clock-names = "ref_clk" ;
> + reg = <0x4000 0x1>;
> + interrupt-parent = <&intc>;
> + interrupts = <0 59 1>;
> + tx-fifo-depth = <0x40>;
> + rx-fifo-depth = <0x40>;
> + };
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 9e7d95d..b180239 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -125,6 +125,13 @@ config CAN_GRCAN
> endian syntheses of the cores would need some modifications on
> the hardware level to work.
>
> +config CAN_XILINXCAN
> + tristate "Xilinx CAN"
> + depends on ARCH_ZYNQ || MICROBLAZE
Is Zynq multiarch already?
> + ---help---
> + Xilinx CAN driver. This driver supports both soft AXI CAN IP and
> + Zynq CANPS IP.
> +
> source "drivers/net/can/mscan/Kconfig"
>
> source "drivers/net/can/sja1000/Kconfig"
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index c744039..0b8e11e 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -25,5 +25,6 @@ obj-$(CONFIG_CAN_JANZ_ICAN3)+=
Re: [PATCH v4] can: xilinx CAN controller support.
On 02/28/2014 02:27 PM, Appana Durga Kedareswara Rao wrote:
>> -Original Message-
>> From: Marc Kleine-Budde [mailto:m...@pengutronix.de]
>> Sent: Friday, February 28, 2014 6:45 PM
>> To: Appana Durga Kedareswara Rao; w...@grandegger.com; Michal Simek;
>> grant.lik...@linaro.org; robh...@kernel.org; linux-...@vger.kernel.org
>> Cc: net...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linux-
>> ker...@vger.kernel.org; devicetree@vger.kernel.org
>> Subject: Re: [PATCH v4] can: xilinx CAN controller support.
>>
>> On 02/28/2014 02:07 PM, Appana Durga Kedareswara Rao wrote:
>>>>>> What happens if the interrupt handler is delayed? For example in a
>>>>>> RT enabled system the interrupt handler runs as a thread. There
>>>>>> might be other threads with higher priority. The hardware will
>>>>>> probably send all CAN frames in the FIFO, so you want to reduce the
>>>>>> overhead and loop in the tx complete handler.
>>>>>>
>>>>> Yes I agree with your comment.
>>>>> It will be good to have a loop in the Tx interrupt handler I am
>>>>> modifying the Tx interrupt handler like below.
>>>>>
>>>>> static void xcan_tx_interrupt(struct net_device *ndev, u32 isr) {
>>>>> struct xcan_priv *priv = netdev_priv(ndev);
>>>>> struct net_device_stats *stats = &ndev->stats;
>>>>>
>>>>> while (priv->tx_head - priv->tx_tail > 0) {
>>>>> if (!(isr & XCAN_IXR_TXOK_MASK)) {
>>>>> break;
>>>>> }
>>>>> can_get_echo_skb(ndev, priv->tx_tail %
>>>>> priv->xcan_echo_skb_max_tx);
>>>>> priv->tx_tail++;
>>>>> stats->tx_packets++;
>>>>> can_led_event(ndev, CAN_LED_EVENT_TX);
>>
>> Probably first clear the interrupt, because you've just handled it, then
>> check
>> if it's still present. The question is, do you have to clear the IRQ for each
>> transmitted frame, or does one clean of the interrupt clear the bit for more
>> then one transmitted frame?
>>
>
> We already were clearing it in the interrupt routine
> (xcan_interrupt). That's why clearing it at the end of the loop. We
> have to clear the IRQ (TXOK) for each transmitted frame.
Please move the clear into the loop. Then it's all in one place. Good
that you have to clear the bit for each transmitted frame. I think,
we're almost done :) When I'm okay with the driver, let Wolfgang have a
look at the error handling.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v4] can: xilinx CAN controller support.
On 02/28/2014 02:07 PM, Appana Durga Kedareswara Rao wrote:
>>>> What happens if the interrupt handler is delayed? For example in a RT
>>>> enabled system the interrupt handler runs as a thread. There might be
>>>> other threads with higher priority. The hardware will probably send
>>>> all CAN frames in the FIFO, so you want to reduce the overhead and
>>>> loop in the tx complete handler.
>>>>
>>> Yes I agree with your comment.
>>> It will be good to have a loop in the Tx interrupt handler I am
>>> modifying the Tx interrupt handler like below.
>>>
>>> static void xcan_tx_interrupt(struct net_device *ndev, u32 isr) {
>>> struct xcan_priv *priv = netdev_priv(ndev);
>>> struct net_device_stats *stats = &ndev->stats;
>>>
>>> while (priv->tx_head - priv->tx_tail > 0) {
>>> if (!(isr & XCAN_IXR_TXOK_MASK)) {
>>> break;
>>> }
>>> can_get_echo_skb(ndev, priv->tx_tail %
>>> priv->xcan_echo_skb_max_tx);
>>> priv->tx_tail++;
>>> stats->tx_packets++;
>>> can_led_event(ndev, CAN_LED_EVENT_TX);
Probably first clear the interrupt, because you've just handled it, then
check if it's still present. The question is, do you have to clear the
IRQ for each transmitted frame, or does one clean of the interrupt clear
the bit for more then one transmitted frame?
>>> isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
> --> clear Tx OK interrupt.
>
>>> }
>>> netif_wake_queue(ndev);
>>> }
>>>
>>> Are you Ok with this?
Were getting there :)
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v4] can: xilinx CAN controller support.
On 02/28/2014 01:44 PM, Appana Durga Kedareswara Rao wrote:
> Hi Marc,
>
>
>> -Original Message-
>> From: Marc Kleine-Budde [mailto:m...@pengutronix.de]
>> Sent: Friday, February 28, 2014 2:02 PM
>> To: Appana Durga Kedareswara Rao; w...@grandegger.com; Michal Simek;
>> grant.lik...@linaro.org; robh...@kernel.org; linux-...@vger.kernel.org
>> Cc: net...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linux-
>> ker...@vger.kernel.org; devicetree@vger.kernel.org
>> Subject: Re: [PATCH v4] can: xilinx CAN controller support.
>>
>> On 02/28/2014 06:50 AM, Appana Durga Kedareswara Rao wrote:
>>> Hi Marc,
>>>
>>>
>>>> -Original Message-
>>>> From: Marc Kleine-Budde [mailto:m...@pengutronix.de]
>>>> Sent: Wednesday, February 26, 2014 9:13 PM
>>>> To: Appana Durga Kedareswara Rao; w...@grandegger.com; Michal Simek;
>>>> grant.lik...@linaro.org; robh...@kernel.org;
>>>> linux-...@vger.kernel.org
>>>> Cc: net...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
>>>> linux- ker...@vger.kernel.org; devicetree@vger.kernel.org
>>>> Subject: Re: [PATCH v4] can: xilinx CAN controller support.
>>>>
>>>> On 02/26/2014 03:46 PM, Appana Durga Kedareswara Rao wrote:
>>>>> Hi Marc,
>>>>>
>>>>>
>>>>>> -Original Message-
>>>>>> From: Marc Kleine-Budde [mailto:m...@pengutronix.de]
>>>>>> Sent: Wednesday, February 26, 2014 6:52 PM
>>>>>> To: Appana Durga Kedareswara Rao; w...@grandegger.com; Michal
>> Simek;
>>>>>> grant.lik...@linaro.org; robh...@kernel.org;
>>>>>> linux-...@vger.kernel.org
>>>>>> Cc: net...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
>>>>>> linux- ker...@vger.kernel.org; devicetree@vger.kernel.org
>>>>>> Subject: Re: [PATCH v4] can: xilinx CAN controller support.
>>>>>>
>>>>>> On 02/26/2014 02:07 PM, Appana Durga Kedareswara Rao wrote:
>>>>>>>> This loop looks broken. Can you explain how it works.
>>>>>>>>
>>>>>>>> What it shoud do is:
>>>>>>>> We have put (priv->tx_head - priv->tx_tail) CAN frames into the
>> FIFO.
>>>>>>>> This means at maximum there could be this amount of CAN frames
>>>>>>>> which have been successfully transmitted. For every cycle in this
>>>>>>>> while loop you
>>>>>>>> should:
>>>>>>>> a) check if a CAN frame has successfully been transmitted
>>>>>>>>(as this CAN core uses a FIFO it should be "oldest")
>>>>>>>>A read_reg() of some kind is missing in your loop.
>>>>>>>> b) if needed, remove this event from the FIFO or
>>>>>>>>mark the interrupt as done. Whatever you hardware needs.
>>>>>>>> c) update your statistics
>>>>>>>> d) Use can_get_echo_skb to push this frame into the networking
>>>>>>>> stack
>>>>>>>> e) As a CAN frame has been transmitted successfully, wake the
>>>> tx_queue.
>>>>>>>>
>>>>>>>>> + while (priv->tx_head - priv->tx_tail > 0) {
>>>>>>>>> + if (isr & XCAN_IXR_TXFLL_MASK) {
>>>>>>>>> + priv->write_reg(priv, XCAN_ICR_OFFSET,
>>>>>>>>> + XCAN_IXR_TXFLL_MASK);
>>>>>>>>> + netif_stop_queue(ndev);
>>>>>>>>
>>>>>>>> Why do you stop the queue here? A CAN frame has successfully
>> been
>>>>>>>> transmitted, there should be room in the FIFO.
>>>>>>>>
>>>>>>>>> + break;
>>>>>>>>> + }
>>>>>>>>> + can_get_echo_skb(ndev, priv->tx_tail %
>>>>>>>>> + priv->xcan_echo_skb_max_tx);
>>>>>>>>> + priv->tx_tail++;
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>
>>>>>>> The below are the bit fields available for the Transmit FIFO.
>>>>>>> 1) In the ISR(in
Re: [PATCH v4] can: xilinx CAN controller support.
On 02/28/2014 06:50 AM, Appana Durga Kedareswara Rao wrote:
> Hi Marc,
>
>
>> -Original Message-
>> From: Marc Kleine-Budde [mailto:m...@pengutronix.de]
>> Sent: Wednesday, February 26, 2014 9:13 PM
>> To: Appana Durga Kedareswara Rao; w...@grandegger.com; Michal Simek;
>> grant.lik...@linaro.org; robh...@kernel.org; linux-...@vger.kernel.org
>> Cc: net...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linux-
>> ker...@vger.kernel.org; devicetree@vger.kernel.org
>> Subject: Re: [PATCH v4] can: xilinx CAN controller support.
>>
>> On 02/26/2014 03:46 PM, Appana Durga Kedareswara Rao wrote:
>>> Hi Marc,
>>>
>>>
>>>> -Original Message-
>>>> From: Marc Kleine-Budde [mailto:m...@pengutronix.de]
>>>> Sent: Wednesday, February 26, 2014 6:52 PM
>>>> To: Appana Durga Kedareswara Rao; w...@grandegger.com; Michal Simek;
>>>> grant.lik...@linaro.org; robh...@kernel.org;
>>>> linux-...@vger.kernel.org
>>>> Cc: net...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
>>>> linux- ker...@vger.kernel.org; devicetree@vger.kernel.org
>>>> Subject: Re: [PATCH v4] can: xilinx CAN controller support.
>>>>
>>>> On 02/26/2014 02:07 PM, Appana Durga Kedareswara Rao wrote:
>>>>>> This loop looks broken. Can you explain how it works.
>>>>>>
>>>>>> What it shoud do is:
>>>>>> We have put (priv->tx_head - priv->tx_tail) CAN frames into the FIFO.
>>>>>> This means at maximum there could be this amount of CAN frames
>>>>>> which have been successfully transmitted. For every cycle in this
>>>>>> while loop you
>>>>>> should:
>>>>>> a) check if a CAN frame has successfully been transmitted
>>>>>>(as this CAN core uses a FIFO it should be "oldest")
>>>>>>A read_reg() of some kind is missing in your loop.
>>>>>> b) if needed, remove this event from the FIFO or
>>>>>>mark the interrupt as done. Whatever you hardware needs.
>>>>>> c) update your statistics
>>>>>> d) Use can_get_echo_skb to push this frame into the networking
>>>>>> stack
>>>>>> e) As a CAN frame has been transmitted successfully, wake the
>> tx_queue.
>>>>>>
>>>>>>> + while (priv->tx_head - priv->tx_tail > 0) {
>>>>>>> + if (isr & XCAN_IXR_TXFLL_MASK) {
>>>>>>> + priv->write_reg(priv, XCAN_ICR_OFFSET,
>>>>>>> + XCAN_IXR_TXFLL_MASK);
>>>>>>> + netif_stop_queue(ndev);
>>>>>>
>>>>>> Why do you stop the queue here? A CAN frame has successfully been
>>>>>> transmitted, there should be room in the FIFO.
>>>>>>
>>>>>>> + break;
>>>>>>> + }
>>>>>>> + can_get_echo_skb(ndev, priv->tx_tail %
>>>>>>> + priv->xcan_echo_skb_max_tx);
>>>>>>> + priv->tx_tail++;
>>>>>>> + }
>>>>>>> +
>>>>>
>>>>> The below are the bit fields available for the Transmit FIFO.
>>>>> 1) In the ISR(interrupt status register) Tx Ok interrupt and Tx
>>>>> fifo full
>>>> interrupt.
>>>>> 2) in the SR(Status Register) Tx fifo full condition.
>>>>>
>>>>>
>>>>> I am modifying the entire tx interrupt logic to like below.
>>>>>
>>>>> static void xcan_tx_interrupt(struct net_device *ndev, u32 isr) {
>>>>> struct xcan_priv *priv = netdev_priv(ndev);
>>>>> struct net_device_stats *stats = &ndev->stats;
>>>>>
>>>>> while (priv->tx_head - priv->tx_tail > 0) {
>>>>> if (isr & XCAN_IXR_TXFLL_MASK) {
>>>>> priv->write_reg(priv, XCAN_ICR_OFFSET,
>>>>> XCAN_IXR_TXFLL_MASK);
>>>>> break;
>>>>> }
>>>>> can_get_echo_skb(ndev, priv->tx_tail %
>>>>> priv->xcan_echo_skb_max_t
Re: [PATCH v4] can: xilinx CAN controller support.
On 02/26/2014 03:46 PM, Appana Durga Kedareswara Rao wrote:
> Hi Marc,
>
>
>> -Original Message-
>> From: Marc Kleine-Budde [mailto:m...@pengutronix.de]
>> Sent: Wednesday, February 26, 2014 6:52 PM
>> To: Appana Durga Kedareswara Rao; w...@grandegger.com; Michal Simek;
>> grant.lik...@linaro.org; robh...@kernel.org; linux-...@vger.kernel.org
>> Cc: net...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linux-
>> ker...@vger.kernel.org; devicetree@vger.kernel.org
>> Subject: Re: [PATCH v4] can: xilinx CAN controller support.
>>
>> On 02/26/2014 02:07 PM, Appana Durga Kedareswara Rao wrote:
>>>> This loop looks broken. Can you explain how it works.
>>>>
>>>> What it shoud do is:
>>>> We have put (priv->tx_head - priv->tx_tail) CAN frames into the FIFO.
>>>> This means at maximum there could be this amount of CAN frames which
>>>> have been successfully transmitted. For every cycle in this while
>>>> loop you
>>>> should:
>>>> a) check if a CAN frame has successfully been transmitted
>>>>(as this CAN core uses a FIFO it should be "oldest")
>>>>A read_reg() of some kind is missing in your loop.
>>>> b) if needed, remove this event from the FIFO or
>>>>mark the interrupt as done. Whatever you hardware needs.
>>>> c) update your statistics
>>>> d) Use can_get_echo_skb to push this frame into the networking stack
>>>> e) As a CAN frame has been transmitted successfully, wake the tx_queue.
>>>>
>>>>> + while (priv->tx_head - priv->tx_tail > 0) {
>>>>> + if (isr & XCAN_IXR_TXFLL_MASK) {
>>>>> + priv->write_reg(priv, XCAN_ICR_OFFSET,
>>>>> + XCAN_IXR_TXFLL_MASK);
>>>>> + netif_stop_queue(ndev);
>>>>
>>>> Why do you stop the queue here? A CAN frame has successfully been
>>>> transmitted, there should be room in the FIFO.
>>>>
>>>>> + break;
>>>>> + }
>>>>> + can_get_echo_skb(ndev, priv->tx_tail %
>>>>> + priv->xcan_echo_skb_max_tx);
>>>>> + priv->tx_tail++;
>>>>> + }
>>>>> +
>>>
>>> The below are the bit fields available for the Transmit FIFO.
>>> 1) In the ISR(interrupt status register) Tx Ok interrupt and Tx fifo full
>> interrupt.
>>> 2) in the SR(Status Register) Tx fifo full condition.
>>>
>>>
>>> I am modifying the entire tx interrupt logic to like below.
>>>
>>> static void xcan_tx_interrupt(struct net_device *ndev, u32 isr) {
>>> struct xcan_priv *priv = netdev_priv(ndev);
>>> struct net_device_stats *stats = &ndev->stats;
>>>
>>> while (priv->tx_head - priv->tx_tail > 0) {
>>> if (isr & XCAN_IXR_TXFLL_MASK) {
>>> priv->write_reg(priv, XCAN_ICR_OFFSET,
>>> XCAN_IXR_TXFLL_MASK);
>>> break;
>>> }
>>> can_get_echo_skb(ndev, priv->tx_tail %
>>> priv->xcan_echo_skb_max_tx);
>>> priv->tx_tail++;
>>> stats->tx_packets++;
>>> netif_wake_queue(ndev);
>>> can_led_event(ndev, CAN_LED_EVENT_TX);
>>>
>>> }
>>
>> You just need to wake the queue once.
>
> Ok
>>
>>> }
>>>
>>>
>>> Are you Ok with the above logic?
>>
>> No, how can you tell how many frames have been transmitted?
>
> There is no register to read how many can frames are transmitted.
> The only way to know Is by reading this parameter (stats->tx_packets++;)
> through ip command
stats->tx_packets is calculated in the above loop and the loop is
broken. Let me illustrate the problem:
- xmit is called 10 times in a row
- this means you have 10 CAN frames in the TX FIFO
- a single CAN frame gets transmitted
- you get an interrupt
- you enter the above routine and loop 10 times and echo the CAN frame
back into the stack
Now every application sees 10 transmitted packages, but there is only
one transmitted. Every time you loop you have to check if the CAN frame
has already been transmitted or not. Is that possible with the hardware?
> ip -d -s link show can
> Or using ifconfig command.
>
> At the h/w level it can transmit Max upto 64 packets (Max fifo depth)
> We need to monitor the Tx fifo full bit in the ISR(Interrupt Status Register)
> or Tx fifo full bit in the SR(Status Register) and if it is full we need to
> stop the queue that is
> I am doing in the _xmit by reading the status register before proceeding the
> packet transmission.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v4] can: xilinx CAN controller support.
On 02/26/2014 02:07 PM, Appana Durga Kedareswara Rao wrote:
>> This loop looks broken. Can you explain how it works.
>>
>> What it shoud do is:
>> We have put (priv->tx_head - priv->tx_tail) CAN frames into the FIFO.
>> This means at maximum there could be this amount of CAN frames which
>> have been successfully transmitted. For every cycle in this while loop you
>> should:
>> a) check if a CAN frame has successfully been transmitted
>>(as this CAN core uses a FIFO it should be "oldest")
>>A read_reg() of some kind is missing in your loop.
>> b) if needed, remove this event from the FIFO or
>>mark the interrupt as done. Whatever you hardware needs.
>> c) update your statistics
>> d) Use can_get_echo_skb to push this frame into the networking stack
>> e) As a CAN frame has been transmitted successfully, wake the tx_queue.
>>
>>> + while (priv->tx_head - priv->tx_tail > 0) {
>>> + if (isr & XCAN_IXR_TXFLL_MASK) {
>>> + priv->write_reg(priv, XCAN_ICR_OFFSET,
>>> + XCAN_IXR_TXFLL_MASK);
>>> + netif_stop_queue(ndev);
>>
>> Why do you stop the queue here? A CAN frame has successfully been
>> transmitted, there should be room in the FIFO.
>>
>>> + break;
>>> + }
>>> + can_get_echo_skb(ndev, priv->tx_tail %
>>> + priv->xcan_echo_skb_max_tx);
>>> + priv->tx_tail++;
>>> + }
>>> +
>
> The below are the bit fields available for the Transmit FIFO.
> 1) In the ISR(interrupt status register) Tx Ok interrupt and Tx fifo full
> interrupt.
> 2) in the SR(Status Register) Tx fifo full condition.
>
>
> I am modifying the entire tx interrupt logic to like below.
>
> static void xcan_tx_interrupt(struct net_device *ndev, u32 isr)
> {
> struct xcan_priv *priv = netdev_priv(ndev);
> struct net_device_stats *stats = &ndev->stats;
>
> while (priv->tx_head - priv->tx_tail > 0) {
> if (isr & XCAN_IXR_TXFLL_MASK) {
> priv->write_reg(priv, XCAN_ICR_OFFSET,
> XCAN_IXR_TXFLL_MASK);
> break;
> }
> can_get_echo_skb(ndev, priv->tx_tail %
> priv->xcan_echo_skb_max_tx);
> priv->tx_tail++;
> stats->tx_packets++;
> netif_wake_queue(ndev);
> can_led_event(ndev, CAN_LED_EVENT_TX);
>
> }
You just need to wake the queue once.
> }
>
>
> Are you Ok with the above logic?
No, how can you tell how many frames have been transmitted?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v4] can: xilinx CAN controller support.
> + priv = netdev_priv(ndev);
> + priv->dev = ndev;
> + priv->can.bittiming_const = &xcan_bittiming_const;
> + priv->can.do_set_bittiming = xcan_set_bittiming;
> + priv->can.do_set_mode = xcan_do_set_mode;
> + priv->can.do_get_berr_counter = xcan_get_berr_counter;
> + priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> + CAN_CTRLMODE_BERR_REPORTING;
> +
> + /* Get IRQ for the device */
> + ndev->irq = platform_get_irq(pdev, 0);
> + ndev->flags |= IFF_ECHO;/* We support local echo */
> +
> + platform_set_drvdata(pdev, ndev);
> + SET_NETDEV_DEV(ndev, &pdev->dev);
> + ndev->netdev_ops = &xcan_netdev_ops;
> +
> + /* Get the virtual base address for the device */
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + priv->reg_base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(priv->reg_base)) {
> + ret = PTR_ERR(priv->reg_base);
> + goto err_free;
&g
Re: [PATCH v10 01/15] usb: doc: phy-mxs: Add more compatible strings
On 02/21/2014 10:40 AM, Peter Chen wrote: > >>> >>> Required properties: >>> -- compatible: Should be "fsl,imx23-usbphy" >>> +- compatible: "fsl,imx23-usbphy" for imx23 and imx28, "fsl,imx6q- >> usbphy" >>> + for imx6dq and imx6dl, "fsl,imx6sl-usbphy" for imx6sl >> >> Minor nit, but could we restructure this as something like the following, >> with each string on a new line: >> >> - compatible: should contain: >> * "fsl,imx23-usbphy" for imx23 and imx28 >> * "fsl,imx6q-usbphy" for imx6dq and imx6dl >> * "fsl,imx6sl-usbphy" for imx6sl >> >> It makes it a bit easier to read. > > Thanks, will change like above. > >> >> I see the existing "fsl,imx23-usbphy" is used as a fallback for >> "fsl,imx28-usbphy", "fsl,imx6q-usbphy", and "fsl,imx6sl-usbphy" in >> existing DTs. >> >> Is this expected going forward? It might be worth mentioning. >> > > These SoCs used the same FSL imx PHY, but different versions. > imx23/imx28 are the first version, more improvements are at > later SoCs (like imx6x) version. Keep "fsl,imx23-usbphy" at > imx6 dts will be user know it is from imx23's. If you think > it does not need, I can delete "fsl,imx23-usbphy" from imx6 dts. You should go after compatibility here. List (all) phys that are comaptible, start with most specific end with most generic. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH] can: xilinx CAN controller support.
On 02/17/2014 10:23 AM, Kedareswara rao Appana wrote:
> +/**
> + * xcan_get_berr_counter - error counter routine
> + * @ndev:Pointer to net_device structure
> + * @bec: Pointer to can_berr_counter structure
> + *
> + * This is the driver error counter routine.
> + * Return: 0 always
> + */
> +static int xcan_get_berr_counter(const struct net_device *ndev,
> + struct can_berr_counter *bec)
> +{
> + struct xcan_priv *priv = netdev_priv(ndev);
> + int ret;
> +
> + ret = clk_prepare_enable(priv->devclk);
> + if (ret)
> + goto err;
> +
> + ret = clk_prepare_enable(priv->aperclk);
> + if (ret)
> + goto err_clk;
> +
> + bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK;
> + bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) &
> + XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT);
You have to disable the clock when leaving this function. Otherwise the
clocks will be unbalanced.
> + return 0;
> +
> +err_clk:
> + clk_disable_unprepare(priv->devclk);
> +err:
> + return ret;
> +}
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH] can: xilinx CAN controller support.
On 02/17/2014 10:23 AM, Kedareswara rao Appana wrote:
> This patch adds xilinx CAN controller support.
> This driver supports both ZYNQ CANPS and Soft IP
> AXI CAN controller.
I just had a quick look at the driver:
[...]
> +/**
> + * xcan_tx_interrupt - Tx Done Isr
> + * @ndev:net_device pointer
> + */
> +static void xcan_tx_interrupt(struct net_device *ndev)
> +{
> + struct xcan_priv *priv = netdev_priv(ndev);
> + struct net_device_stats *stats = &ndev->stats;
> +
> + stats->tx_packets++;
> + while (priv->tx_head - priv->tx_tail > 0) {
Note, there might be still CAN frames in the TX FIFO that have not been
transmitted yet. You have to check your hardware! What to do depends on
how you FIFO is organized.
> + can_get_echo_skb(ndev, priv->tx_tail %
> + priv->xcan_echo_skb_max_tx);
> + priv->tx_tail++;
> + }
> +
> + netif_wake_queue(ndev);
> + can_led_event(ndev, CAN_LED_EVENT_TX);
> +}
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2] can: xilinx CAN controller support.
rupt Status Register (ISR) contains bits that are set when a
> particular interrupt condition occurs. If
> the corresponding mask bit in the Interrupt Enable Register is set, an
> interrupt is generated.
> Interrupt bits in the ISR can be cleared by writing to the Interrupt Clear
> Register
Thanks.
[...]
>>> +/**
>>> + * xcan_open - Driver open routine
>>> + * @ndev: Pointer to net_device structure
>>> + *
>>> + * This is the driver open routine.
>>> + * Return: 0 on success and failure value on error */ static int
>>> +xcan_open(struct net_device *ndev) {
>>> + struct xcan_priv *priv = netdev_priv(ndev);
>>> + int ret;
>>> +
>>> + ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
>>> + ndev->name, (void *)ndev);
>>> + if (ret < 0) {
>>> + netdev_err(ndev, "Irq allocation for CAN failed\n");
>>> + return ret;
>>> + }
>>> +
>>> + /* Set chip into reset mode */
>>> + ret = set_reset_mode(ndev);
>>> + if (ret < 0)
>>> + netdev_err(ndev, "mode resetting failed failed!\n");
>>
>> Is this critical?
>
> This condition usually won't fail.
> But if the controller has a problems at the h/w level in that case putted
> this err print.
> If you want me to change it as a warning will do
If there is a hardware level problem, is it better to return here with
an error (and free the IRQ)?
[...]
>>> +/**
>>> + * xcan_probe - Platform registration call
>>> + * @pdev: Handle to the platform device structure
>>> + *
>>> + * This function does all the memory allocation and registration for
>>> +the CAN
>>> + * device.
>>> + *
>>> + * Return: 0 on success and failure value on error */ static int
>>> +xcan_probe(struct platform_device *pdev) {
>>> + struct resource *res; /* IO mem resources */
>>> + struct net_device *ndev;
>>> + struct xcan_priv *priv;
>>> + int ret, fifodep;
>>> +
>>> + /* Create a CAN device instance */
>>> + ndev = alloc_candev(sizeof(struct xcan_priv),
>> XCAN_ECHO_SKB_MAX);
>>> + if (!ndev)
>>> + return -ENOMEM;
>>> +
>>> + priv = netdev_priv(ndev);
>>> + priv->dev = ndev;
>>> + priv->can.bittiming_const = &xcan_bittiming_const;
>>> + priv->can.do_set_bittiming = xcan_set_bittiming;
>>> + priv->can.do_set_mode = xcan_do_set_mode;
>>> + priv->can.do_get_berr_counter = xcan_get_berr_counter;
>>> + priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
>>> + CAN_CTRLMODE_BERR_REPORTING;
>>> +
>>> + /* Get IRQ for the device */
>>> + ndev->irq = platform_get_irq(pdev, 0);
>>> +
>>> + spin_lock_init(&priv->ech_skb_lock);
>>> + ndev->flags |= IFF_ECHO;/* We support local echo */
>>> +
>>> + platform_set_drvdata(pdev, ndev);
>>> + SET_NETDEV_DEV(ndev, &pdev->dev);
>>> + ndev->netdev_ops = &xcan_netdev_ops;
>>> +
>>> + /* Get the virtual base address for the device */
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + priv->reg_base = devm_ioremap_resource(&pdev->dev, res);
>>> + if (IS_ERR(priv->reg_base)) {
>>> + ret = PTR_ERR(priv->reg_base);
>>> + goto err_free;
>>> + }
>>> + ndev->mem_start = res->start;
>>> + ndev->mem_end = res->end;
>>> +
>>> + priv->write_reg = xcan_write_reg;
>>> + priv->read_reg = xcan_read_reg;
>>> +
>>> + ret = of_property_read_u32(pdev->dev.of_node, "tx-fifo-depth",
>>> + &fifodep);
>>> + if (ret < 0)
>>> + goto err_free;
>>> + priv->xcan_echo_skb_max_tx = fifodep;
>>> +
>>> + ret = of_property_read_u32(pdev->dev.of_node, "rx-fifo-depth",
>>> + &fifodep);
>>> + if (ret < 0)
>>> + goto err_free;
>>> + priv->xcan_echo_skb_max_rx = fifodep;
>>> +
>>> + /* Getting the CAN devclk info */
>>> + priv->devclk = devm_clk_get(&pdev->dev, "ref_clk");
>>> + if (IS_ERR(priv->devclk)) {
>>> + dev_err(&pdev->dev, "Device clock not found.\n");
>>> + ret = PTR_ERR(priv->devclk);
>>> + goto err_free;
>>> + }
>>> +
>>> + /* Check for type of CAN device */
>>> + if (of_device_is_compatible(pdev->dev.of_node,
>>> + "xlnx,zynq-can-1.00.a")) {
>>> + priv->aperclk = devm_clk_get(&pdev->dev, "aper_clk");
>>> + if (IS_ERR(priv->aperclk)) {
>>> + dev_err(&pdev->dev, "aper clock not found\n");
>>> + ret = PTR_ERR(priv->aperclk);
>>> + goto err_free;
>>> + }
>>> + } else {
>>> + priv->aperclk = priv->devclk;
>>> + }
>>> +
>>> + ret = clk_prepare_enable(priv->devclk);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "unable to enable device clock\n");
>>> + goto err_free;
>>> + }
>>> +
>>> + ret = clk_prepare_enable(priv->aperclk);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "unable to enable aper clock\n");
>>> + goto err_unprepar_disabledev;
>>> + }
>>
>> Can you keep your clocks disaled if the interface is not up?
>
> I didn't get it will you please explain?
This feature s optional, but a a good practice.
This function gets called when the driver is loaded, i.e. during boot.
So the complete CAN core will be enabled and powered, even if the
interface is down. To reduce power consumption it's better to enable the
clocks in the open() function and disable in close(). If you access some
CAN registers during probe you have to enable the clock and you probably
have to enable it in the do_get_berr_counter callback, as it may be
called if the interface is still down.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2] can: xilinx CAN controller support.
On 02/14/2014 10:13 AM, Michal Simek wrote:
>>> That's not entirely truth. If you look at Microblaze then you will see
>>> that Microblaze can be BE and LE.
>>> There is just missing endian detection which we will add to the next
>>> version.
>>
>> As far as I know the endianess of the kernel is fixed and known during
>> compile time. Correct me if I'm wrong. So there is no need for a runtime
>> detection of the endianess and so no need for {read,write}_reg function
>> pointers.
>
> Endianess of the kernel is fixed and know during compile time
> but what it is not fixed is endianess of that IP at compile time.
>
> On fpga you can use bridges, partial reconfiguration, etc where
> the only solution which is run-time endian detection via registers.
>
> For example: drivers/block/xsysace.c, drivers/spi/spi-xilinx.c, etc
Okay, now I get it. You can make it more complex then it used to be :D
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2] can: xilinx CAN controller support.
On 02/14/2014 09:55 AM, Michal Simek wrote:
> Hi Marc,
>
>>> + int waiting_ech_skb_num;
>>> + int xcan_echo_skb_max_tx;
>>> + int xcan_echo_skb_max_rx;
>>> + struct napi_struct napi;
>>> + spinlock_t ech_skb_lock;
>>> + u32 (*read_reg)(const struct xcan_priv *priv, int reg);
>>> + void (*write_reg)(const struct xcan_priv *priv, int reg, u32 val);
>>
>> Please remove read_reg, write_reg, as long as there isn't any BE support
>> in the driver, call them directly.
>
> That's not entirely truth. If you look at Microblaze then you will see
> that Microblaze can be BE and LE.
> There is just missing endian detection which we will add to the next version.
As far as I know the endianess of the kernel is fixed and known during
compile time. Correct me if I'm wrong. So there is no need for a runtime
detection of the endianess and so no need for {read,write}_reg function
pointers.
> But because MB io helper functions are broken for a while you should be
> able to use this driver on both endianess.
>
> btw: I would prefer to use ioread32 and ioread32be instead of readl.
> Is it OK for you?
Make it so. :)
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2] can: xilinx CAN controller support.
> + * device.
> + *
> + * Return: 0 on success and failure value on error
> + */
> +static int xcan_probe(struct platform_device *pdev)
> +{
> + struct resource *res; /* IO mem resources */
> + struct net_device *ndev;
> + struct xcan_priv *priv;
> + int ret, fifodep;
> +
> + /* Create a CAN device instance */
> + ndev = alloc_candev(sizeof(struct xcan_priv), XCAN_ECHO_SKB_MAX);
> + if (!ndev)
> + return -ENOMEM;
> +
> + priv = netdev_priv(ndev);
> + priv->dev = ndev;
> + priv->can.bittiming_const = &xcan_bittiming_const;
> + priv->can.do_set_bittiming = xcan_set_bittiming;
> + priv->can.do_set_mode = xcan_do_set_mode;
> + priv->can.do_get_berr_counter = xcan_get_berr_counter;
> + priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> + CAN_CTRLMODE_BERR_REPORTING;
> +
> + /* Get IRQ for the device */
> + ndev->irq = platform_get_irq(pdev, 0);
> +
> + spin_lock_init(&priv->ech_skb_lock);
> + ndev->flags |= IFF_ECHO;/* We support local
Re: [PATCH] can: xilinx CAN controller support.
On 02/11/2014 12:45 PM, Michal Simek wrote:
> Hi Marc,
>
> On 02/07/2014 10:37 AM, Marc Kleine-Budde wrote:
>> On 02/07/2014 09:42 AM, Appana Durga Kedareswara Rao wrote:
>>>>> ---
>>>>> This patch is rebased on the 3.14 rc1 kernel.
>>>>> ---
>>>>> .../devicetree/bindings/net/can/xilinx_can.txt | 43 +
>>>>> drivers/net/can/Kconfig|8 +
>>>>> drivers/net/can/Makefile |1 +
>>>>> drivers/net/can/xilinx_can.c | 1150
>>>>>
>>>>> 4 files changed, 1202 insertions(+), 0 deletions(-) create mode
>>>>> 100644 Documentation/devicetree/bindings/net/can/xilinx_can.txt
>>>>> create mode 100644 drivers/net/can/xilinx_can.c
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/net/can/xilinx_can.txt
>>>>> b/Documentation/devicetree/bindings/net/can/xilinx_can.txt
>>>>> new file mode 100644
>>>>> index 000..34f9643
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/net/can/xilinx_can.txt
>>>>> @@ -0,0 +1,43 @@
>>>>> +Xilinx Axi CAN/Zynq CANPS controller Device Tree Bindings
>>>>> +-
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible : Should be "xlnx,zynq-can-1.00.a" for Zynq
>>>> CAN
>>>>> + controllers and "xlnx,axi-can-1.00.a" for Axi CAN
>>>>> + controllers.
>>>>> +- reg : Physical base address and size of the Axi
>>>>> CAN/Zynq
>>>>> + CANPS registers map.
>>>>> +- interrupts : Property with a value describing the
>>>>> interrupt
>>>>> + number.
>>>>> +- interrupt-parent : Must be core interrupt controller
>>>>> +- clock-names : List of input clock names - "ref_clk",
>>>> "aper_clk"
>>>>> + (See clock bindings for details. Two clocks are
>>>>> + required for Zynq CAN. For Axi CAN
>>>>> + case it is one(ref_clk)).
>>>>> +- clocks : Clock phandles (see clock bindings for details).
>>>>> +- xlnx,can-tx-dpth : Can Tx fifo depth (Required for Axi CAN).
>>>>> +- xlnx,can-rx-dpth : Can Rx fifo depth (Required for Axi CAN).
>>>>> +
>>>>> +
>>>>> +Example:
>>>>> +
>>>>> +For Zynq CANPS Dts file:
>>>>> + zynq_can_0: zynq-can@e0008000 {
>>>>> + compatible = "xlnx,zynq-can-1.00.a";
>>>>> + clocks = <&clkc 19>, <&clkc 36>;
>>>>> + clock-names = "ref_clk", "aper_clk";
>>>>> + reg = <0xe0008000 0x1000>;
>>>>> + interrupts = <0 28 4>;
>>>>> + interrupt-parent = <&intc>;
>>>>
>>>> Above xlnx,can-{rx,tx}-dpth is mentioned as required, but it's not in the
>>>> Zynq example.
>>>
>>> One of the Difference b/w the AXI CAN and zynq CAN is in AXI CAN the fifo
>>> depth(tx,rx)
>>> Is user configurable. But in case of ZYNQ CAN the fifo depth is fixed for
>>> tx and rx fifo's(64)
>>> Xlnx,can-{rx,tx}-dpth is required only for AXI CAN case it is not required
>>> for zynq CAN.
>>> That's why didn't putted that property in device tree.
>>
>> The device tree should be a hardware only description and should not
>> hold any user configurable data. Please split your patch into two
>> patches. The first one should add the driver with a fixed fifo size
>> (e.g. 0x40) for the AXI, too. The second patch should make the fifo
>> configurable via device tree.
>
> can-rx/tx-dpth is not user configurable data as you think.
> This is FPGA where you can configure this parameter in design tools.
> It means these 2 values just describe real hardware and user can't just
> change it
> for different software behaviour.
I see, thanks for the clarification. I had a short grep over the
arm/boot/dts folder and it seems that fifo-depth is a
Re: [PATCH] can: xilinx CAN controller support.
;rx_bytes += cf->can_dlc;
>>> +
>>> + netdev_dbg(ndev, "%s: error status register:0x%x\n",
>>> + __func__, priv->read_reg(priv, XCAN_ESR_OFFSET));
>> }
>>> +
>>> +/**
>>> + * xcan_state_interrupt - It will check the state of the CAN device
>>> + * @ndev: net_device pointer
>>> + * @isr: interrupt status register value
>>> + *
>>> + * This will checks the state of the CAN device
>>> + * and puts the device into appropriate state.
>>> + */
>>> +static void xcan_state_interrupt(struct net_device *ndev, u32 isr) {
>>> + struct xcan_priv *priv = netdev_priv(ndev);
>>> +
>>> + /* Check for Sleep interrupt if set put CAN device in sleep state */
>>> + if (isr & XCAN_IXR_SLP_MASK)
>>> + priv->can.state = CAN_STATE_SLEEPING;
>>> +
>>> + /* Check for Wake up interrupt if set put CAN device in Active state
>> */
>>> + if (isr & XCAN_IXR_WKUP_MASK)
>>> + priv->can.state = CAN_STATE_ERROR_ACTIVE; }
>>> +
>>> +/**
>>> + * xcan_rx_poll - Poll routine for rx packets (NAPI)
>>> + * @napi: napi structure pointer
>>> + * @quota: Max number of rx packets to be processed.
>>> + *
>>> + * This is the poll routine for rx part.
>>> + * It will process the packets maximux quota value.
>>> + *
>>> + * Return: number of packets received */ static int
>>> +xcan_rx_poll(struct napi_struct *napi, int quota) {
>>> + struct net_device *ndev = napi->dev;
>>> + struct xcan_priv *priv = netdev_priv(ndev);
>>> + u32 isr, ier;
>>> + int work_done = 0;
>>> +
>>> + isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
>>> + while ((isr & XCAN_IXR_RXNEMP_MASK) && (work_done < quota)) {
>>> + if (isr & XCAN_IXR_RXOK_MASK) {
>>> + priv->write_reg(priv, XCAN_ICR_OFFSET,
>>> + XCAN_IXR_RXOK_MASK);
>>> + if (xcan_rx(ndev) < 0)
>>> + return work_done;
>>> + work_done++;
>>> + } else {
>>> + priv->write_reg(priv, XCAN_ICR_OFFSET,
>>> + XCAN_IXR_RXNEMP_MASK);
>>> + break;
>>> + }
>>> + priv->write_reg(priv, XCAN_ICR_OFFSET,
>> XCAN_IXR_RXNEMP_MASK);
>>> + isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
>>> + }
>>> +
>>> + if (work_done < quota) {
>>> + napi_complete(napi);
>>> + ier = priv->read_reg(priv, XCAN_IER_OFFSET);
>>> + ier |= (XCAN_IXR_RXOK_MASK |
>> XCAN_IXR_RXNEMP_MASK);
>>> + priv->write_reg(priv, XCAN_IER_OFFSET, ier);
>>> + }
>>> + return work_done;
>>> +}
>>> +
>>> +/**
>>> + * xcan_tx_interrupt - Tx Done Isr
>>> + * @ndev: net_device pointer
>>> + */
>>> +static void xcan_tx_interrupt(struct net_device *ndev) {
>>> + unsigned long flags;
>>> + struct xcan_priv *priv = netdev_priv(ndev);
>>> + struct net_device_stats *stats = &ndev->stats;
>>> + u32 processed = 0, txpackets;
>>> +
>>> + stats->tx_packets++;
>>> + netdev_dbg(ndev, "%s: waiting total:%d,current:%d\n", __func__,
>>> + priv->waiting_ech_skb_num, priv-
>>> waiting_ech_skb_index);
>>> +
>>> + txpackets = priv->waiting_ech_skb_num;
>>> +
>>> + if (txpackets) {
>>> + can_get_echo_skb(ndev, priv->waiting_ech_skb_index);
>>> + priv->waiting_ech_skb_index =
>>> + (priv->waiting_ech_skb_index + 1) %
>>> + priv->xcan_echo_skb_max_tx;
>>> + processed++;
>>> + txpackets--;
>>> + }
>>> +
>>> + spin_lock_irqsave(&priv->ech_skb_lock, flags);
>>> + priv->waiting_ech_skb_num -= processed;
&g
Re: [PATCH] can: xilinx CAN controller support.
gt; + *
> + * This is the driver error counter routine.
> + * Return: 0 always
> + */
> +static int xcan_get_berr_counter(const struct net_device *ndev,
> + struct can_berr_counter *bec)
> +{
> + struct xcan_priv *priv = netdev_priv(ndev);
> +
> + bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK;
> + bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) &
> + XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT);
> + return 0;
> +}
> +
> +static const struct net_device_ops xcan_netdev_ops = {
> + .ndo_open = xcan_open,
> + .ndo_stop = xcan_close,
> + .ndo_start_xmit = xcan_start_xmit,
> +};
> +
> +#ifdef CONFIG_PM_SLEEP
> +/**
> + * xcan_suspend - Suspend method for the driver
> + * @_dev:Address of the platform_device structure
> + *
> + * Put the driver into low power mode.
> + * Return: 0 always
> + */
> +static int xcan_suspend(struct device *_dev)
> +{
> + struct platform_device *pdev = container_of(_dev,
> + struct platform_device, dev);
> + struct net_device *ndev = platform_get_drvdata(pdev);
> + struct xcan_priv *priv = netdev_priv(ndev);
> +
> + if (netif_running(ndev)) {
> + netif_stop_queue(ndev);
> + netif_device_detach(ndev);
> + }
> +
> + priv->write_reg(priv, XCAN_MSR_OFFSET, XCAN_MSR_SLEEP_MASK);
> + priv->can.state = CAN_STATE_SLEEPING;
> +
> + clk_disable(priv->aperclk);
> + clk_disable(priv->devclk);
> +
> + return 0;
> +}
> +
> +/**
> + * xcan_resume - Resume from suspend
> + * @dev: Address of the platformdevice structure
> + *
> + * Resume operation after suspend.
> + * Return: 0 on success and failure value on error
> + */
> +static int xcan_resume(struct device *dev)
> +{
> + struct platform_device *pdev = container_of(dev,
> + struct platform_device, dev);
> + struct net_device *ndev = platform_get_drvdata(pdev);
> + struct xcan_priv *priv = netdev_priv(ndev);
> + int ret;
> +
> + ret = clk_enable(priv->aperclk);
> + if (ret) {
> + dev_err(dev, "Cannot enable clock.\n");
> + return ret;
> + }
> + ret = clk_enable(priv->devclk);
> + if (ret) {
> + dev_err(dev, "Cannot enable clock.\n");
> + return ret;
> + }
> +
> + priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> + priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +
> + if (netif_running(ndev)) {
> + netif_device_attach(ndev);
> + netif_start_queue(ndev);
> + }
> +
> + return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend, xcan_resume);
> +
> +/**
> + * xcan_probe - Platform registration call
> + * @pdev:Handle to the platform device structure
> + *
> + * This function does all the memory allocation and registration for the CAN
> + * device.
> + *
> + * Return: 0 on success and failure value on error
> + */
> +static int xcan_probe(struct platform_device *pdev)
> +{
> + struct resource *res; /* IO mem resources */
> + struct net_device *ndev;
> + struct xcan_priv *priv;
> + int ret, fifodep;
> +
> + /* Create a CAN device instance */
> + ndev = alloc_candev(sizeof(struct xcan_priv), XCAN_ECHO_SKB_MAX);
> + if (!ndev)
> + return -ENOMEM;
> +
> + priv = netdev_priv(ndev);
> + priv->dev = ndev;
> + priv->can.bittiming_const = &xcan_bittiming_const;
> + priv->can.do_set_bittiming = xcan_set_bittiming;
> + priv->can.do_set_mode = xcan_do_set_mode;
> + priv->can.do_get_berr_counter = xcan_get_berr_counter;
> + priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> + CAN_CTRLMODE_BERR_REPORTING;
> + priv->xcan_echo_skb_max_tx = XCAN_ECHO_SKB_MAX;
> + priv->xcan_echo_skb_max_rx = XCAN_NAPI_WEIGHT;
> +
> + /* Get IRQ for the device */
> + ndev->irq = platform_get_irq(pdev, 0);
> + ret = devm_request_irq(&pdev->dev, ndev->irq, &xcan_interrupt,
> + priv->irq_flags, dev_name(&pdev->dev),
> + (void *)ndev);
We usually request the interrupt on in the open() function
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Irq allocation for CAN failed\n");
> + goto err_free;
> + }
>
[PATCH v4 4/5] Documentation: devicetree: sja1000: add reg-io-width binding
From: Florian Vaussard Add the reg-io-width property to describe the width of the memory accesses. Cc: Grant Likely Cc: Rob Herring Cc: Pawel Moll Cc: Mark Rutland Cc: Ian Campbell Cc: Kumar Gala Cc: devicetree@vger.kernel.org Acked-by: Rob Herring Signed-off-by: Florian Vaussard Tested-by: Andreas Larsson Signed-off-by: Marc Kleine-Budde --- Documentation/devicetree/bindings/net/can/sja1000.txt | 4 1 file changed, 4 insertions(+) diff --git a/Documentation/devicetree/bindings/net/can/sja1000.txt b/Documentation/devicetree/bindings/net/can/sja1000.txt index f2105a4..b4a6d53 100644 --- a/Documentation/devicetree/bindings/net/can/sja1000.txt +++ b/Documentation/devicetree/bindings/net/can/sja1000.txt @@ -12,6 +12,10 @@ Required properties: Optional properties: +- reg-io-width : Specify the size (in bytes) of the IO accesses that + should be performed on the device. Valid value is 1, 2 or 4. + Default to 1 (8 bits). + - nxp,external-clock-frequency : Frequency of the external oscillator clock in Hz. Note that the internal clock frequency used by the SJA1000 is half of that value. If not specified, a default value -- 1.8.5.3 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/6] Documentation: devicetree: sja1000: add reg-io-width binding
On 01/31/2014 11:35 AM, Florian Vaussard wrote: > Add the reg-io-width property to describe the width of the memory > accesses. > > Cc: Grant Likely > Cc: Rob Herring > Cc: Pawel Moll > Cc: Mark Rutland > Cc: Ian Campbell > Cc: Kumar Gala > Cc: devicetree@vger.kernel.org > Acked-by: Rob Herring > Signed-off-by: Florian Vaussard I think it makes sense to squash into patch 6. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 1/2] can: mcp251x: Convert to devm-* API
On 12/16/2013 11:17 AM, Marc Kleine-Budde wrote: > On 12/15/2013 03:16 PM, Alexander Shiyan wrote: >> Replace existing resource handling in the driver with managed >> device resource, this ensures more consistent error values and >> simplifies error paths. >> >> Signed-off-by: Alexander Shiyan > > Lookgs good, applied to can-next. I have included this patch in the current pull request to David Miller's net-next. Patch 2/2 is postponed, as discussed yesterday. If you need a tree to base your patch on, feel free to use: git://gitorious.org/linux-can/linux-can-next.git for-davem Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 1/2] can: mcp251x: Convert to devm-* API
On 12/15/2013 03:16 PM, Alexander Shiyan wrote: > Replace existing resource handling in the driver with managed > device resource, this ensures more consistent error values and > simplifies error paths. > > Signed-off-by: Alexander Shiyan Lookgs good, applied to can-next. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 2/2] can: mcp251x: Add device tree support
cOn 12/15/2013 03:16 PM, Alexander Shiyan wrote: > This patch adds Device Tree support to the Microchip MCP251X driver. What about completing the clock support for the driver, too? You get the clock, but it's never enabled. It probably works on your board because it's a fixed always on clock. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH v5 15/15] usb: phy-mxs: Add sync time after controller clear phcd
On 12/09/2013 10:19 AM, Peter Chen wrote: > On Mon, Dec 09, 2013 at 10:05:17AM +0100, Marc Kleine-Budde wrote: >> On 12/09/2013 07:31 AM, Peter Chen wrote: >>> After clear portsc.phcd, PHY needs 200us stable time for switch >>> 32K clock to AHB clock. >> >> If this is a general bugbix, please move it to the start of this series >> and add stable on Cc. I think I hit the bug on the second USB port of a >> custom MX28 board, the stmp_reset_block() in the USB phy will fail. > > Not a bug-fix, just the requirement for the PHY leaves low power mode. > Does the bug exists with my this patchset, or just current mainline code? I see, I found this strange thing on v3.8, but can try on v3.13-rc3. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH v5 09/15] usb: phy-mxs: Enable IC fixes for related SoCs
On 12/09/2013 10:07 AM, Peter Chen wrote: > On Mon, Dec 09, 2013 at 09:38:17AM +0100, Marc Kleine-Budde wrote: >> On 12/09/2013 07:30 AM, Peter Chen wrote: >>> Some PHY bugs are fixed by IC logic, but these bits are not >>> enabled by default, so we enable them at driver. >> >> Which bugs are fixed by enabling this bit? Is it only suspend related? >> Can you document them or better add a pointer to the documentation. > > I will add more, in fact, it fixes the bug which flag BIT(1) and BIT(2) > stands for. > >> >> Further I don't like the idea of adding code, or enabling a feature on >> certain hardware, that is broken in the first place and fixing it in a >> later patch. Think about squashing it into the correct patch. > > No fixes are related with this patch, you can see there is no "-" > at this patch. Yes, there isn't any broken code (thus no "-"), but you first enable a feature in the hardware and in a later patch (this one) make it work properly. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH v5 15/15] usb: phy-mxs: Add sync time after controller clear phcd
On 12/09/2013 07:31 AM, Peter Chen wrote:
> After clear portsc.phcd, PHY needs 200us stable time for switch
> 32K clock to AHB clock.
If this is a general bugbix, please move it to the start of this series
and add stable on Cc. I think I hit the bug on the second USB port of a
custom MX28 board, the stmp_reset_block() in the USB phy will fail.
Is the problem documented somewhere?
Is it possible to add the delay in the clock framework? I think only the
regulator framework has a configurable delay for the regulator to stabilize.
> Signed-off-by: Peter Chen
> ---
> drivers/usb/phy/phy-mxs-usb.c | 11 +++
> 1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
> index e18fdf3..7ae5225 100644
> --- a/drivers/usb/phy/phy-mxs-usb.c
> +++ b/drivers/usb/phy/phy-mxs-usb.c
> @@ -151,6 +151,15 @@ static inline bool is_imx6sl_phy(struct mxs_phy *mxs_phy)
> return mxs_phy->data == &imx6sl_phy_data;
> }
>
> +/*
> + * PHY needs some 32K cycles to switch from 32K clock to
> + * bus (such as AHB/AXI, etc) clock.
> + */
> +static void mxs_phy_clock_switch(void)
> +{
> + usleep_range(300, 400);
> +}
> +
> static int mxs_phy_hw_init(struct mxs_phy *mxs_phy)
> {
> int ret;
> @@ -276,6 +285,7 @@ static int mxs_phy_init(struct usb_phy *phy)
> {
> struct mxs_phy *mxs_phy = to_mxs_phy(phy);
>
> + mxs_phy_clock_switch();
> clk_prepare_enable(mxs_phy->clk);
> return mxs_phy_hw_init(mxs_phy);
> }
> @@ -300,6 +310,7 @@ static int mxs_phy_suspend(struct usb_phy *x, int suspend)
> x->io_priv + HW_USBPHY_CTRL_SET);
> clk_disable_unprepare(mxs_phy->clk);
> } else {
> + mxs_phy_clock_switch();
> clk_prepare_enable(mxs_phy->clk);
> writel(BM_USBPHY_CTRL_CLKGATE,
> x->io_priv + HW_USBPHY_CTRL_CLR);
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v5 12/15] usb: phy: Add set_wakeup API
On 12/09/2013 07:31 AM, Peter Chen wrote:
> This API is used to set wakeup enable at PHY registers, in that
> case, the PHY can be waken up from suspend due to external events,
I' no native speaker, but I think it's "to be woken up".
> like vbus change, dp/dm change and id change.
>
> Signed-off-by: Peter Chen
> ---
> include/linux/usb/phy.h | 16
> 1 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
> index a747960..c6ebe1d 100644
> --- a/include/linux/usb/phy.h
> +++ b/include/linux/usb/phy.h
> @@ -111,6 +111,13 @@ struct usb_phy {
> int (*set_suspend)(struct usb_phy *x,
> int suspend);
>
> + /*
> + * Set wakeup enable for PHY, in that case, the PHY can be
> + * waken up from suspend status due to external events,
> + * like vbus change, dp/dm change and id.
> + */
> + int (*set_wakeup)(struct usb_phy *x, bool enabled);
> +
> /* notify phy connect status change */
> int (*notify_connect)(struct usb_phy *x,
> enum usb_device_speed speed);
> @@ -270,6 +277,15 @@ usb_phy_set_suspend(struct usb_phy *x, int suspend)
> }
>
> static inline int
> +usb_phy_set_wakeup(struct usb_phy *x, bool enabled)
> +{
> + if (x && x->set_wakeup)
can x be NULL?
> + return x->set_wakeup(x, enabled);
> + else
> + return 0;
> +}
> +
> +static inline int
> usb_phy_notify_connect(struct usb_phy *x, enum usb_device_speed speed)
> {
> if (x && x->notify_connect)
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v5 09/15] usb: phy-mxs: Enable IC fixes for related SoCs
On 12/09/2013 07:30 AM, Peter Chen wrote:
> Some PHY bugs are fixed by IC logic, but these bits are not
> enabled by default, so we enable them at driver.
Which bugs are fixed by enabling this bit? Is it only suspend related?
Can you document them or better add a pointer to the documentation.
Further I don't like the idea of adding code, or enabling a feature on
certain hardware, that is broken in the first place and fixing it in a
later patch. Think about squashing it into the correct patch.
> Signed-off-by: Peter Chen
> ---
> drivers/usb/phy/phy-mxs-usb.c | 28 ++--
> 1 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
> index e3df53f..d1c319b 100644
> --- a/drivers/usb/phy/phy-mxs-usb.c
> +++ b/drivers/usb/phy/phy-mxs-usb.c
> @@ -31,6 +31,10 @@
> #define HW_USBPHY_CTRL_SET 0x34
> #define HW_USBPHY_CTRL_CLR 0x38
>
> +#define HW_USBPHY_IP 0x90
> +#define HW_USBPHY_IP_SET 0x94
> +#define HW_USBPHY_IP_CLR 0x98
> +
> #define BM_USBPHY_CTRL_SFTRSTBIT(31)
> #define BM_USBPHY_CTRL_CLKGATE BIT(30)
> #define BM_USBPHY_CTRL_ENAUTOSET_USBCLKS BIT(26)
> @@ -42,6 +46,8 @@
> #define BM_USBPHY_CTRL_ENUTMILEVEL2 BIT(14)
> #define BM_USBPHY_CTRL_ENHOSTDISCONDETECTBIT(1)
>
> +#define BM_USBPHY_IP_FIX (BIT(17) | BIT(18))
> +
> #define to_mxs_phy(p) container_of((p), struct mxs_phy, phy)
>
> /* Do disconnection between PHY and controller without vbus */
> @@ -63,6 +69,9 @@
> /* The SoCs who have anatop module */
> #define MXS_PHY_HAS_ANATOP BIT(3)
>
> +/* IC has bug fixes logic */
> +#define MXS_PHY_NEED_IP_FIX BIT(4)
> +
> struct mxs_phy_data {
> unsigned int flags;
> };
> @@ -74,12 +83,14 @@ static const struct mxs_phy_data imx23_phy_data = {
> static const struct mxs_phy_data imx6q_phy_data = {
> .flags = MXS_PHY_SENDING_SOF_TOO_FAST |
> MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS |
> - MXS_PHY_HAS_ANATOP,
> + MXS_PHY_HAS_ANATOP |
> + MXS_PHY_NEED_IP_FIX,
> };
>
> static const struct mxs_phy_data imx6sl_phy_data = {
> .flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS |
> - MXS_PHY_HAS_ANATOP,
> + MXS_PHY_HAS_ANATOP |
> + MXS_PHY_NEED_IP_FIX,
> };
>
> static const struct of_device_id mxs_phy_dt_ids[] = {
> @@ -97,6 +108,16 @@ struct mxs_phy {
> struct regmap *regmap_anatop;
> };
>
> +static inline bool is_imx6q_phy(struct mxs_phy *mxs_phy)
> +{
> + return mxs_phy->data == &imx6q_phy_data;
> +}
> +
> +static inline bool is_imx6sl_phy(struct mxs_phy *mxs_phy)
> +{
> + return mxs_phy->data == &imx6sl_phy_data;
> +}
Are the two is_imx6* functions still needed?
> +
> static int mxs_phy_hw_init(struct mxs_phy *mxs_phy)
> {
> int ret;
> @@ -123,6 +144,9 @@ static int mxs_phy_hw_init(struct mxs_phy *mxs_phy)
> BM_USBPHY_CTRL_ENUTMILEVEL3,
> base + HW_USBPHY_CTRL_SET);
>
> + if (mxs_phy->data->flags & MXS_PHY_NEED_IP_FIX)
> + writel(BM_USBPHY_IP_FIX, base + HW_USBPHY_IP_SET);
> +
> return 0;
> }
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v5 08/15] usb: phy-mxs: Add implementation of nofity_suspend and notify_resume
On 12/09/2013 07:30 AM, Peter Chen wrote:
> Implementation of notify_suspend and notify_resume will be different
> according to mxs_phy_data->flags.
>
> Signed-off-by: Peter Chen
> ---
> drivers/usb/phy/phy-mxs-usb.c | 55
> ++---
> 1 files changed, 51 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
> index 0ef930a..e3df53f 100644
> --- a/drivers/usb/phy/phy-mxs-usb.c
> +++ b/drivers/usb/phy/phy-mxs-usb.c
> @@ -166,8 +166,8 @@ static int mxs_phy_suspend(struct usb_phy *x, int suspend)
> static int mxs_phy_on_connect(struct usb_phy *phy,
> enum usb_device_speed speed)
> {
> - dev_dbg(phy->dev, "%s speed device has connected\n",
> - (speed == USB_SPEED_HIGH) ? "high" : "non-high");
> + dev_dbg(phy->dev, "%s device has connected\n",
> + (speed == USB_SPEED_HIGH) ? "HS" : "FS/LS");
What about creating a mxs_phy_dbg() function that adds the "HS" "FS/LS"
prefix when printing?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v4 17/17] usb: phy-mxs: do not set PWD.RXPWD1PT1 for low speed connection
On 12/03/2013 10:19 AM, Peter Chen wrote: >> On 12/03/2013 08:37 AM, Peter Chen wrote: >>> At very rare cases, the SoF will not send out after resume with >>> low speed connection. The workaround is do not power down >>> PWD.RXPWD1PT1 bit during the suspend. >> >> Is this also a fix for newly added code? If so please also squash. > No, it is a workaround for new problem. If this would be a fix, it's better to put this as first patch into a series, so that it can be applied to the stable trees easier. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
