Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1295121 > unrolled thread

[PATCH 1/2] can: sja1000: add documentation for Technologic Systems version

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] can: sja1000: add documentation for Technologic Systems version Damien Riegel <damien.riegel@savoirfairelinux.com> - 2015-12-18 21:20 +0100
    [PATCH 2/2] can: sja1000: of: add compatibility with Technologic Systems version Damien Riegel <damien.riegel@savoirfairelinux.com> - 2015-12-18 21:20 +0100
      Re: [PATCH 2/2] can: sja1000: of: add compatibility with Technologic  Systems version Marc Kleine-Budde <mkl@pengutronix.de> - 2015-12-18 21:50 +0100
        Re: [PATCH 2/2] can: sja1000: of: add compatibility with Technologic  Systems version Marc Kleine-Budde <mkl@pengutronix.de> - 2015-12-18 22:10 +0100
        Re: [PATCH 2/2] can: sja1000: of: add compatibility with Technologic  Systems version Damien Riegel <damien.riegel@savoirfairelinux.com> - 2015-12-18 22:10 +0100
    Re: [PATCH 1/2] can: sja1000: add documentation for Technologic  Systems version Rob Herring <robh@kernel.org> - 2015-12-20 04:40 +0100
      Re: [PATCH 1/2] can: sja1000: add documentation for Technologic  Systems version Damien Riegel <damien.riegel@savoirfairelinux.com> - 2015-12-21 16:20 +0100
        Re: [PATCH 1/2] can: sja1000: add documentation for Technologic  Systems version Rob Herring <robh@kernel.org> - 2015-12-22 19:30 +0100

#1295121 — [PATCH 1/2] can: sja1000: add documentation for Technologic Systems version

This commit adds documentation for the Technologic Systems version of
SJA1000. The difference with the NXP version is in the way the registers
are accessed.

Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
---
 Documentation/devicetree/bindings/net/can/sja1000.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/can/sja1000.txt b/Documentation/devicetree/bindings/net/can/sja1000.txt
index b4a6d53..7a158d5 100644
--- a/Documentation/devicetree/bindings/net/can/sja1000.txt
+++ b/Documentation/devicetree/bindings/net/can/sja1000.txt
@@ -2,7 +2,7 @@ Memory mapped SJA1000 CAN controller from NXP (formerly Philips)
 
 Required properties:
 
-- compatible : should be "nxp,sja1000".
+- compatible : should be one of "nxp,sja1000", "technologic,sja1000".
 
 - reg : should specify the chip select, address offset and size required
 	to map the registers of the SJA1000. The size is usually 0x80.
@@ -14,6 +14,7 @@ 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.
+	Must be set to 2 for technologic version.
 	Default to 1 (8 bits).
 
 - nxp,external-clock-frequency : Frequency of the external oscillator
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1295123 — [PATCH 2/2] can: sja1000: of: add compatibility with Technologic Systems version

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 <damien.riegel@savoirfairelinux.com>
---
 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);
+}
+
+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);
+}
+
 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);
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1295132 — Re: [PATCH 2/2] can: sja1000: of: add compatibility with Technologic Systems version

[Multipart message — attachments visible in raw view] — view raw

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 <damien.riegel@savoirfairelinux.com>
> ---
>  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-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

[toc] | [prev] | [next] | [standalone]

#1295151 — Re: [PATCH 2/2] can: sja1000: of: add compatibility with Technologic Systems version

[Multipart message — attachments visible in raw view] — view raw

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 <damien.riegel@savoirfairelinux.com>
>>> ---
>>>  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-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

[toc] | [prev] | [next] | [standalone]

#1295157 — Re: [PATCH 2/2] can: sja1000: of: add compatibility with Technologic Systems version

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 <damien.riegel@savoirfairelinux.com>
> > ---
> >  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.

Thanks,
Damien
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1295512 — Re: [PATCH 1/2] can: sja1000: add documentation for Technologic Systems version

On Fri, Dec 18, 2015 at 03:17:24PM -0500, Damien Riegel wrote:
> This commit adds documentation for the Technologic Systems version of
> SJA1000. The difference with the NXP version is in the way the registers
> are accessed.
> 
> Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> ---
>  Documentation/devicetree/bindings/net/can/sja1000.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/sja1000.txt b/Documentation/devicetree/bindings/net/can/sja1000.txt
> index b4a6d53..7a158d5 100644
> --- a/Documentation/devicetree/bindings/net/can/sja1000.txt
> +++ b/Documentation/devicetree/bindings/net/can/sja1000.txt
> @@ -2,7 +2,7 @@ Memory mapped SJA1000 CAN controller from NXP (formerly Philips)
>  
>  Required properties:
>  
> -- compatible : should be "nxp,sja1000".
> +- compatible : should be one of "nxp,sja1000", "technologic,sja1000".
>  
>  - reg : should specify the chip select, address offset and size required
>  	to map the registers of the SJA1000. The size is usually 0x80.
> @@ -14,6 +14,7 @@ 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.
> +	Must be set to 2 for technologic version.
>  	Default to 1 (8 bits).

Really, this should default to 2 for technologic version and not be 
required.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1296033 — Re: [PATCH 1/2] can: sja1000: add documentation for Technologic Systems version

On Sat, Dec 19, 2015 at 09:37:42PM -0600, Rob Herring wrote:
> On Fri, Dec 18, 2015 at 03:17:24PM -0500, Damien Riegel wrote:
> > This commit adds documentation for the Technologic Systems version of
> > SJA1000. The difference with the NXP version is in the way the registers
> > are accessed.
> > 
> > Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> > ---
> >  Documentation/devicetree/bindings/net/can/sja1000.txt | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/can/sja1000.txt b/Documentation/devicetree/bindings/net/can/sja1000.txt
> > index b4a6d53..7a158d5 100644
> > --- a/Documentation/devicetree/bindings/net/can/sja1000.txt
> > +++ b/Documentation/devicetree/bindings/net/can/sja1000.txt
> > @@ -2,7 +2,7 @@ Memory mapped SJA1000 CAN controller from NXP (formerly Philips)
> >  
> >  Required properties:
> >  
> > -- compatible : should be "nxp,sja1000".
> > +- compatible : should be one of "nxp,sja1000", "technologic,sja1000".
> >  
> >  - reg : should specify the chip select, address offset and size required
> >  	to map the registers of the SJA1000. The size is usually 0x80.
> > @@ -14,6 +14,7 @@ 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.
> > +	Must be set to 2 for technologic version.
> >  	Default to 1 (8 bits).
> 
> Really, this should default to 2 for technologic version and not be 
> required.

Would something along the line of "This property is <not used|ignored>
for technologic version." be more appropriate?

Thanks,
Damien
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1296967 — Re: [PATCH 1/2] can: sja1000: add documentation for Technologic Systems version

On Mon, Dec 21, 2015 at 9:09 AM, Damien Riegel
<damien.riegel@savoirfairelinux.com> wrote:
> On Sat, Dec 19, 2015 at 09:37:42PM -0600, Rob Herring wrote:
>> On Fri, Dec 18, 2015 at 03:17:24PM -0500, Damien Riegel wrote:
>> > This commit adds documentation for the Technologic Systems version of
>> > SJA1000. The difference with the NXP version is in the way the registers
>> > are accessed.
>> >
>> > Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
>> > ---
>> >  Documentation/devicetree/bindings/net/can/sja1000.txt | 3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/net/can/sja1000.txt b/Documentation/devicetree/bindings/net/can/sja1000.txt
>> > index b4a6d53..7a158d5 100644
>> > --- a/Documentation/devicetree/bindings/net/can/sja1000.txt
>> > +++ b/Documentation/devicetree/bindings/net/can/sja1000.txt
>> > @@ -2,7 +2,7 @@ Memory mapped SJA1000 CAN controller from NXP (formerly Philips)
>> >
>> >  Required properties:
>> >
>> > -- compatible : should be "nxp,sja1000".
>> > +- compatible : should be one of "nxp,sja1000", "technologic,sja1000".
>> >
>> >  - reg : should specify the chip select, address offset and size required
>> >     to map the registers of the SJA1000. The size is usually 0x80.
>> > @@ -14,6 +14,7 @@ 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.
>> > +   Must be set to 2 for technologic version.
>> >     Default to 1 (8 bits).
>>
>> Really, this should default to 2 for technologic version and not be
>> required.
>
> Would something along the line of "This property is <not used|ignored>
> for technologic version." be more appropriate?

Yes, exactly.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web