| Message ID | 1536931375-48769-2-git-send-email-phil@raspberrypi.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Improve VCHIQ cache line size handling | expand |
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 <phil@raspberrypi.org> > --- > .../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
Hi Phil, > Phil Elwell <phil@raspberrypi.org> 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 <phil@raspberrypi.org> > > --- > > .../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. no need to hurry, my pull requests for 4.20 are already out. Please take the time to test this properly. Patch 2-4 are: Acked-by: Stefan Wahren <stefan.wahren@i2se.com> > > Phil > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 <phil@raspberrypi.org> > --- [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.
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 <phil@raspberrypi.org> >> --- > > [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
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 <phil@raspberrypi.org> >>> --- >> >> [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.
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;
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 <phil@raspberrypi.org> --- .../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(-)