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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 |
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
>>> ---
>>> 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 |
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
On Mon, Dec 21, 2015 at 9:09 AM, Damien Riegel
<[email protected]> 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 <[email protected]>
>> > ---
>> > 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