| Message ID | 1536931375-48769-1-git-send-email-phil@raspberrypi.org |
|---|---|
| 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: > The size field in a Device Tree "reg" property is encoded in bytes, not > words. > > Signed-off-by: Phil Elwell <phil@raspberrypi.org> This should probably have a: Fixes: 614fa22119d6 ("ARM: dts: bcm2835: Add VCHIQ node to the Raspberry Pi boards. (v3)") as well? > --- > arch/arm/boot/dts/bcm2835-rpi.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi > index 215d8cc..29f970f 100644 > --- a/arch/arm/boot/dts/bcm2835-rpi.dtsi > +++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi > @@ -32,7 +32,7 @@ > > vchiq: mailbox@7e00b840 { > compatible = "brcm,bcm2835-vchiq"; > - reg = <0x7e00b840 0xf>; > + reg = <0x7e00b840 0x3c>; > interrupts = <0 2>; > }; > }; >
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.
Both sides of the VCHIQ communications mechanism need to agree on the cache line size. Using an incorrect value can lead to data corruption, but having the two sides using different values is usually worse. In the absence of an obvious convenient run-time method to determine the correct value in the ARCH=arm world, the downstream Raspberry Pi trees used a Device Tree property, written by the firmware, to configure the kernel driver. This method was vetoed during the upstreaming process, so a fixed value of 32 was used instead, and some corruptions ensued. This is take 2 at arriving at the correct value. Add a new compatible string - "brcm,bcm2836-vchiq" - to indicate an SoC with a 64-byte cache line. Document the new string in the binding, and use it on the appropriate platforms. The final patch is a (seemingly cosmetic) correction of the Device Tree "reg" declaration for the device node, but it doubles as an indication to the Raspberry Pi firmware that the kernel driver is running a recent kernel driver that chooses the correct value. As such it would help if the DT patches are not merged before the driver patch. v2: Replaced ARM-specific logic used to determine cache line size with a new compatible string for BCM2836 and BCM2837. Phil Elwell (4): staging/vc04_services: Use correct cache line size dt-bindings: soc: Document "brcm,bcm2836-vchiq" ARM: dts: bcm283x: Correct vchiq compatible string ARM: dts: bcm283x: Correct mailbox register sizes .../bindings/soc/bcm/brcm,bcm2835-vchiq.txt | 3 +- arch/arm/boot/dts/bcm2835-rpi.dtsi | 4 +-- arch/arm/boot/dts/bcm2836-rpi-2-b.dts | 2 +- arch/arm/boot/dts/bcm2836-rpi.dtsi | 6 ++++ arch/arm/boot/dts/bcm2837-rpi-3-b-plus.dts | 2 +- arch/arm/boot/dts/bcm2837-rpi-3-b.dts | 2 +- arch/arm/boot/dts/bcm2837-rpi-cm3.dtsi | 2 +- .../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 +++ 10 files changed, 48 insertions(+), 18 deletions(-) create mode 100644 arch/arm/boot/dts/bcm2836-rpi.dtsi