Re: [PATCH v2 1/4] staging/vc04_services: Use correct cache line size
On 09/14/2018 11:12 AM, Phil Elwell wrote:
> On 14/09/2018 18:03, Florian Fainelli wrote:
>> On 09/14/2018 06:22 AM, Phil Elwell wrote:
>>> Use the compatible string in the DTB to select the correct cache line
>>> size for the SoC - 32 for BCM2835, and 64 for BCM2836 and BCM2837.
>>>
>>> Signed-off-by: Phil Elwell
>>> ---
>>
>> [snip]
>>
>>> @@ -170,6 +170,14 @@ static struct device *vchiq_dev;
>>> static DEFINE_SPINLOCK(msg_queue_spinlock);
>>> static struct platform_device *bcm2835_camera;
>>> +static struct vchiq_drvdata bcm2835_drvdata = {
>>> + .cache_line_size = 32,
>>> +};
>>> +
>>> +static struct vchiq_drvdata bcm2836_drvdata = {
>>> + .cache_line_size = 64,
>>> +};
>>
>> Those two structures could probably be marked const. Other than that,
>> the approach definitively looks good to me.
>
> The mutability is intentional - the structure pointer to the firmware is
> also
> stored there. This isn't the only piece of mutable static data in a driver
> that will only have a single instance, so allocating and initialising a
> per-instance structure seems needlessly complicated.
Fair enough, thanks for the explanation.
--
Florian
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/4] staging/vc04_services: Use correct cache line size
On 14/09/2018 18:03, Florian Fainelli wrote:
On 09/14/2018 06:22 AM, Phil Elwell wrote:
Use the compatible string in the DTB to select the correct cache line
size for the SoC - 32 for BCM2835, and 64 for BCM2836 and BCM2837.
Signed-off-by: Phil Elwell
---
[snip]
@@ -170,6 +170,14 @@ static struct device *vchiq_dev;
static DEFINE_SPINLOCK(msg_queue_spinlock);
static struct platform_device *bcm2835_camera;
+static struct vchiq_drvdata bcm2835_drvdata = {
+ .cache_line_size = 32,
+};
+
+static struct vchiq_drvdata bcm2836_drvdata = {
+ .cache_line_size = 64,
+};
Those two structures could probably be marked const. Other than that,
the approach definitively looks good to me.
The mutability is intentional - the structure pointer to the firmware is also
stored there. This isn't the only piece of mutable static data in a driver
that will only have a single instance, so allocating and initialising a
per-instance structure seems needlessly complicated.
Thanks for the feedback.
Phil
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/4] staging/vc04_services: Use correct cache line size
On 09/14/2018 06:22 AM, Phil Elwell wrote:
> Use the compatible string in the DTB to select the correct cache line
> size for the SoC - 32 for BCM2835, and 64 for BCM2836 and BCM2837.
>
> Signed-off-by: Phil Elwell
> ---
[snip]
> @@ -170,6 +170,14 @@ static struct device *vchiq_dev;
> static DEFINE_SPINLOCK(msg_queue_spinlock);
> static struct platform_device *bcm2835_camera;
>
> +static struct vchiq_drvdata bcm2835_drvdata = {
> + .cache_line_size = 32,
> +};
> +
> +static struct vchiq_drvdata bcm2836_drvdata = {
> + .cache_line_size = 64,
> +};
Those two structures could probably be marked const. Other than that,
the approach definitively looks good to me.
--
Florian
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/4] staging/vc04_services: Use correct cache line size
Hi Phil,
> Phil Elwell hat am 14. September 2018 um 17:22
> geschrieben:
>
>
> On 14/09/2018 14:22, Phil Elwell wrote:
> > Use the compatible string in the DTB to select the correct cache line
> > size for the SoC - 32 for BCM2835, and 64 for BCM2836 and BCM2837.
> >
> > Signed-off-by: Phil Elwell
> > ---
> > .../interface/vchiq_arm/vchiq_2835_arm.c | 4 ++-
> > .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 36
> > --
> > .../vc04_services/interface/vchiq_arm/vchiq_arm.h | 5 +++
> > 3 files changed, 34 insertions(+), 11 deletions(-)
> >
> > diff --git
> > a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
> > b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
> > index e767209..83d740f 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
> > @@ -109,7 +109,8 @@ free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
> > int vchiq_platform_init(struct platform_device *pdev, VCHIQ_STATE_T
> > *state)
> > {
> > struct device *dev = &pdev->dev;
> > - struct rpi_firmware *fw = platform_get_drvdata(pdev);
> > + struct vchiq_drvdata *drvdata = platform_get_drvdata(pdev);
> > + struct rpi_firmware *fw = drvdata->fw;
> > VCHIQ_SLOT_ZERO_T *vchiq_slot_zero;
> > struct resource *res;
> > void *slot_mem;
> > @@ -127,6 +128,7 @@ int vchiq_platform_init(struct platform_device *pdev,
> > VCHIQ_STATE_T *state)
> > if (err < 0)
> > return err;
> >
> > + g_cache_line_size = drvdata->cache_line_size;
> > g_fragments_size = 2 * g_cache_line_size;
> >
> > /* Allocate space for the channels in coherent memory */
> > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > index bc05c69..b2ae9259 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > @@ -170,6 +170,14 @@ static struct device *vchiq_dev;
> > static DEFINE_SPINLOCK(msg_queue_spinlock);
> > static struct platform_device *bcm2835_camera;
> >
> > +static struct vchiq_drvdata bcm2835_drvdata = {
> > + .cache_line_size = 32,
> > +};
> > +
> > +static struct vchiq_drvdata bcm2836_drvdata = {
> > + .cache_line_size = 64,
> > +};
> > +
> > static const char *const ioctl_names[] = {
> > "CONNECT",
> > "SHUTDOWN",
> > @@ -3573,12 +3581,26 @@ void
> > vchiq_platform_conn_state_changed(VCHIQ_STATE_T *state,
> > }
> > }
> >
> > +static const struct of_device_id vchiq_of_match[] = {
> > + { .compatible = "brcm,bcm2835-vchiq", .data = &bcm2835_drvdata },
> > + { .compatible = "brcm,bcm2836-vchiq", .data = &bcm2836_drvdata },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, vchiq_of_match);
> > +
> > static int vchiq_probe(struct platform_device *pdev)
> > {
> > struct device_node *fw_node;
> > - struct rpi_firmware *fw;
> > + const struct of_device_id *of_id;
> > + struct vchiq_drvdata *drvdata;
> > int err;
> >
> > + snd_rpi_simple.dev = &pdev->dev;
> > + of_id = of_match_node(vchiq_of_match, pdev->dev.of_node);
> > + drvdata = of_id->data;
> > + if (!drvdata)
> > + return -EINVAL;
> > +
> > fw_node = of_find_compatible_node(NULL, NULL,
> > "raspberrypi,bcm2835-firmware");
> > if (!fw_node) {
> > @@ -3586,12 +3608,12 @@ static int vchiq_probe(struct platform_device *pdev)
> > return -ENOENT;
> > }
> >
> > - fw = rpi_firmware_get(fw_node);
> > + drvdata->fw = rpi_firmware_get(fw_node);
> > of_node_put(fw_node);
> > - if (!fw)
> > + if (!drvdata->fw)
> > return -EPROBE_DEFER;
> >
> > - platform_set_drvdata(pdev, fw);
> > + platform_set_drvdata(pdev, drvdata);
> >
> > err = vchiq_platform_init(pdev, &g_state);
> > if (err != 0)
> > @@ -3661,12 +3683,6 @@ static int vchiq_remove(struct platform_device *pdev)
> > return 0;
> > }
> >
> > -static const struct of_device_id vchiq_of_match[] = {
> > - { .compatible = "brcm,bcm2835-vchiq", },
> > - {},
> > -};
> > -MODULE_DEVICE_TABLE(of, vchiq_of_match);
> > -
> > static struct platform_driver vchiq_driver = {
> > .driver = {
> > .name = "bcm2835_vchiq",
> > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> > b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> > index 40bb0c6..2f3ebc9 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> > @@ -123,6 +123,11 @@ typedef struct vchiq_arm_state_struct {
> >
> > } VCHIQ_ARM_STATE_T;
> >
> > +struct vchiq_drvdata {
> > + const unsigned int cache_line_size;
> > + struct rpi_firmware *fw;
> > +
Re: [PATCH v2 1/4] staging/vc04_services: Use correct cache line size
On 14/09/2018 14:22, Phil Elwell wrote:
Use the compatible string in the DTB to select the correct cache line
size for the SoC - 32 for BCM2835, and 64 for BCM2836 and BCM2837.
Signed-off-by: Phil Elwell
---
.../interface/vchiq_arm/vchiq_2835_arm.c | 4 ++-
.../vc04_services/interface/vchiq_arm/vchiq_arm.c | 36 --
.../vc04_services/interface/vchiq_arm/vchiq_arm.h | 5 +++
3 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index e767209..83d740f 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -109,7 +109,8 @@ free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
int vchiq_platform_init(struct platform_device *pdev, VCHIQ_STATE_T *state)
{
struct device *dev = &pdev->dev;
- struct rpi_firmware *fw = platform_get_drvdata(pdev);
+ struct vchiq_drvdata *drvdata = platform_get_drvdata(pdev);
+ struct rpi_firmware *fw = drvdata->fw;
VCHIQ_SLOT_ZERO_T *vchiq_slot_zero;
struct resource *res;
void *slot_mem;
@@ -127,6 +128,7 @@ int vchiq_platform_init(struct platform_device *pdev,
VCHIQ_STATE_T *state)
if (err < 0)
return err;
+ g_cache_line_size = drvdata->cache_line_size;
g_fragments_size = 2 * g_cache_line_size;
/* Allocate space for the channels in coherent memory */
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index bc05c69..b2ae9259 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -170,6 +170,14 @@ static struct device *vchiq_dev;
static DEFINE_SPINLOCK(msg_queue_spinlock);
static struct platform_device *bcm2835_camera;
+static struct vchiq_drvdata bcm2835_drvdata = {
+ .cache_line_size = 32,
+};
+
+static struct vchiq_drvdata bcm2836_drvdata = {
+ .cache_line_size = 64,
+};
+
static const char *const ioctl_names[] = {
"CONNECT",
"SHUTDOWN",
@@ -3573,12 +3581,26 @@ void vchiq_platform_conn_state_changed(VCHIQ_STATE_T
*state,
}
}
+static const struct of_device_id vchiq_of_match[] = {
+ { .compatible = "brcm,bcm2835-vchiq", .data = &bcm2835_drvdata },
+ { .compatible = "brcm,bcm2836-vchiq", .data = &bcm2836_drvdata },
+ {},
+};
+MODULE_DEVICE_TABLE(of, vchiq_of_match);
+
static int vchiq_probe(struct platform_device *pdev)
{
struct device_node *fw_node;
- struct rpi_firmware *fw;
+ const struct of_device_id *of_id;
+ struct vchiq_drvdata *drvdata;
int err;
+ snd_rpi_simple.dev = &pdev->dev;
+ of_id = of_match_node(vchiq_of_match, pdev->dev.of_node);
+ drvdata = of_id->data;
+ if (!drvdata)
+ return -EINVAL;
+
fw_node = of_find_compatible_node(NULL, NULL,
"raspberrypi,bcm2835-firmware");
if (!fw_node) {
@@ -3586,12 +3608,12 @@ static int vchiq_probe(struct platform_device *pdev)
return -ENOENT;
}
- fw = rpi_firmware_get(fw_node);
+ drvdata->fw = rpi_firmware_get(fw_node);
of_node_put(fw_node);
- if (!fw)
+ if (!drvdata->fw)
return -EPROBE_DEFER;
- platform_set_drvdata(pdev, fw);
+ platform_set_drvdata(pdev, drvdata);
err = vchiq_platform_init(pdev, &g_state);
if (err != 0)
@@ -3661,12 +3683,6 @@ static int vchiq_remove(struct platform_device *pdev)
return 0;
}
-static const struct of_device_id vchiq_of_match[] = {
- { .compatible = "brcm,bcm2835-vchiq", },
- {},
-};
-MODULE_DEVICE_TABLE(of, vchiq_of_match);
-
static struct platform_driver vchiq_driver = {
.driver = {
.name = "bcm2835_vchiq",
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
index 40bb0c6..2f3ebc9 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -123,6 +123,11 @@ typedef struct vchiq_arm_state_struct {
} VCHIQ_ARM_STATE_T;
+struct vchiq_drvdata {
+ const unsigned int cache_line_size;
+ struct rpi_firmware *fw;
+};
+
extern int vchiq_arm_log_level;
extern int vchiq_susp_log_level;
This version doesn't compile (wrong defconfig used when building), but any
criticism of the
approach before v3 is welcome.
Phil
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 1/4] staging/vc04_services: Use correct cache line size
Use the compatible string in the DTB to select the correct cache line
size for the SoC - 32 for BCM2835, and 64 for BCM2836 and BCM2837.
Signed-off-by: Phil Elwell
---
.../interface/vchiq_arm/vchiq_2835_arm.c | 4 ++-
.../vc04_services/interface/vchiq_arm/vchiq_arm.c | 36 --
.../vc04_services/interface/vchiq_arm/vchiq_arm.h | 5 +++
3 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index e767209..83d740f 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -109,7 +109,8 @@ free_pagelist(struct vchiq_pagelist_info *pagelistinfo,
int vchiq_platform_init(struct platform_device *pdev, VCHIQ_STATE_T *state)
{
struct device *dev = &pdev->dev;
- struct rpi_firmware *fw = platform_get_drvdata(pdev);
+ struct vchiq_drvdata *drvdata = platform_get_drvdata(pdev);
+ struct rpi_firmware *fw = drvdata->fw;
VCHIQ_SLOT_ZERO_T *vchiq_slot_zero;
struct resource *res;
void *slot_mem;
@@ -127,6 +128,7 @@ int vchiq_platform_init(struct platform_device *pdev,
VCHIQ_STATE_T *state)
if (err < 0)
return err;
+ g_cache_line_size = drvdata->cache_line_size;
g_fragments_size = 2 * g_cache_line_size;
/* Allocate space for the channels in coherent memory */
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index bc05c69..b2ae9259 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -170,6 +170,14 @@ static struct device *vchiq_dev;
static DEFINE_SPINLOCK(msg_queue_spinlock);
static struct platform_device *bcm2835_camera;
+static struct vchiq_drvdata bcm2835_drvdata = {
+ .cache_line_size = 32,
+};
+
+static struct vchiq_drvdata bcm2836_drvdata = {
+ .cache_line_size = 64,
+};
+
static const char *const ioctl_names[] = {
"CONNECT",
"SHUTDOWN",
@@ -3573,12 +3581,26 @@ void vchiq_platform_conn_state_changed(VCHIQ_STATE_T
*state,
}
}
+static const struct of_device_id vchiq_of_match[] = {
+ { .compatible = "brcm,bcm2835-vchiq", .data = &bcm2835_drvdata },
+ { .compatible = "brcm,bcm2836-vchiq", .data = &bcm2836_drvdata },
+ {},
+};
+MODULE_DEVICE_TABLE(of, vchiq_of_match);
+
static int vchiq_probe(struct platform_device *pdev)
{
struct device_node *fw_node;
- struct rpi_firmware *fw;
+ const struct of_device_id *of_id;
+ struct vchiq_drvdata *drvdata;
int err;
+ snd_rpi_simple.dev = &pdev->dev;
+ of_id = of_match_node(vchiq_of_match, pdev->dev.of_node);
+ drvdata = of_id->data;
+ if (!drvdata)
+ return -EINVAL;
+
fw_node = of_find_compatible_node(NULL, NULL,
"raspberrypi,bcm2835-firmware");
if (!fw_node) {
@@ -3586,12 +3608,12 @@ static int vchiq_probe(struct platform_device *pdev)
return -ENOENT;
}
- fw = rpi_firmware_get(fw_node);
+ drvdata->fw = rpi_firmware_get(fw_node);
of_node_put(fw_node);
- if (!fw)
+ if (!drvdata->fw)
return -EPROBE_DEFER;
- platform_set_drvdata(pdev, fw);
+ platform_set_drvdata(pdev, drvdata);
err = vchiq_platform_init(pdev, &g_state);
if (err != 0)
@@ -3661,12 +3683,6 @@ static int vchiq_remove(struct platform_device *pdev)
return 0;
}
-static const struct of_device_id vchiq_of_match[] = {
- { .compatible = "brcm,bcm2835-vchiq", },
- {},
-};
-MODULE_DEVICE_TABLE(of, vchiq_of_match);
-
static struct platform_driver vchiq_driver = {
.driver = {
.name = "bcm2835_vchiq",
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
index 40bb0c6..2f3ebc9 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
@@ -123,6 +123,11 @@ typedef struct vchiq_arm_state_struct {
} VCHIQ_ARM_STATE_T;
+struct vchiq_drvdata {
+ const unsigned int cache_line_size;
+ struct rpi_firmware *fw;
+};
+
extern int vchiq_arm_log_level;
extern int vchiq_susp_log_level;
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
