* Re: [PATCH v12 1/3] dt-bindings: i2c: npcm7xx: add NPCM I2C controller
From: robh @ 2020-05-21 19:58 UTC (permalink / raw)
To: Tali Perry
Cc: avifishman70, linux-i2c, linux-arm-kernel, yuenn, wsa,
andriy.shevchenko, devicetree, kfting, venture, brendanhiggins,
ofery, benjaminfair, openbmc, tmaimon77, robh+dt, linux-kernel
In-Reply-To: <20200521110910.45518-2-tali.perry1@gmail.com>
On Thu, 21 May 2020 14:09:08 +0300, Tali Perry wrote:
> Added device tree binding documentation for Nuvoton BMC
> NPCM I2C controller.
>
> Signed-off-by: Tali Perry <tali.perry1@gmail.com>
> ---
> .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml | 62 +++++++++++++++++++
> 1 file changed, 62 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
>
My bot found errors running 'make dt_binding_check' on your patch:
Error: Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.example.dts:22.28-29 syntax error
FATAL ERROR: Unable to parse input tree
scripts/Makefile.lib:312: recipe for target 'Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.example.dt.yaml' failed
make[1]: *** [Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.example.dt.yaml] Error 1
make[1]: *** Waiting for unfinished jobs....
Makefile:1300: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2
See https://patchwork.ozlabs.org/patch/1295115
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:
pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade
Please check and re-submit.
^ permalink raw reply
* Re: [PATCH v2 2/9] dt-bindings: gpio: Convert mrvl-gpio to json-schema
From: robh @ 2020-05-21 19:57 UTC (permalink / raw)
To: Lubomir Rintel
Cc: Alessandro Zummo, Daniel Lezcano, Marc Zyngier, Thomas Gleixner,
Jason Cooper, linux-kernel, Bartosz Golaszewski, devicetree,
Ulf Hansson, Linus Walleij, Alexandre Belloni, Rob Herring
In-Reply-To: <20200521091356.2211020-3-lkundrak@v3.sk>
On Thu, 21 May 2020 11:13:49 +0200, Lubomir Rintel wrote:
> This converts the mrvl-gpio binding to DT schema format using json-schema.
>
> Various fixes were done during the conversion, such as adding more
> properties that are in fact mandatory or extending the examples to
> include child nodes with extra GPIO blocks.
>
> The compatible strings are a mess. It is not clear why so many of them
> are needed; the driver doesn't really seem to differentiate between the
> models. Some of them, like marvell,pxa93x-gpio and marvell,pxa1928-gpio
> are not used at all, so it's not known how many interrupts they utilize.
> On the other hand, mrvl,pxa-gpio has been seen in the tree, but it
> doesn't end up in any actual DTB file.
>
> In any case -- the schema merely copies whatever was in the original
> binding document, so it's hopefully no more wrong that the original.
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>
> ---
> Changes since v1:
> - Drop marvell,pxa1928-gpio
> - Drop ranges from example with no gcb child nodes
> - Add default GPL-2.0-only license tag
> - Fill in maintainers from MAINTAINERS file
>
> .../devicetree/bindings/gpio/mrvl-gpio.txt | 48 -----
> .../devicetree/bindings/gpio/mrvl-gpio.yaml | 174 ++++++++++++++++++
> 2 files changed, 174 insertions(+), 48 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/gpio/mrvl-gpio.txt
> create mode 100644 Documentation/devicetree/bindings/gpio/mrvl-gpio.yaml
>
My bot found errors running 'make dt_binding_check' on your patch:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/gpio/mrvl-gpio.example.dt.yaml: gpio@40e00000: 'ranges' is a required property
See https://patchwork.ozlabs.org/patch/1295044
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:
pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade
Please check and re-submit.
^ permalink raw reply
* Re: [PATCH 08/12] of/irq: make of_msi_map_get_device_domain() bus agnostic
From: Bjorn Helgaas @ 2020-05-21 19:57 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: linux-arm-kernel, Diana Craciun, Bjorn Helgaas, Rob Herring,
Marc Zyngier, iommu, linux-acpi, devicetree, linux-pci,
Rafael J. Wysocki, Joerg Roedel, Hanjun Guo, Sudeep Holla,
Robin Murphy, Catalin Marinas, Will Deacon, Makarand Pawagi,
Laurentiu Tudor
In-Reply-To: <20200521130008.8266-9-lorenzo.pieralisi@arm.com>
On Thu, May 21, 2020 at 02:00:04PM +0100, Lorenzo Pieralisi wrote:
> From: Diana Craciun <diana.craciun@oss.nxp.com>
>
> of_msi_map_get_device_domain() is PCI specific but it need not be and
> can be easily changed to be bus agnostic in order to be used by other
> busses by adding an IRQ domain bus token as an input parameter.
>
> Signed-off-by: Diana Craciun <diana.craciun@oss.nxp.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Marc Zyngier <maz@kernel.org>
Acked-by: Bjorn Helgaas <bhelgaas@google.com> # pci/msi.c
> ---
> drivers/of/irq.c | 8 +++++---
> drivers/pci/msi.c | 2 +-
> include/linux/of_irq.h | 5 +++--
> 3 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index a296eaf52a5b..48a40326984f 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -613,18 +613,20 @@ u32 of_msi_map_rid(struct device *dev, struct device_node *msi_np, u32 rid_in)
> * of_msi_map_get_device_domain - Use msi-map to find the relevant MSI domain
> * @dev: device for which the mapping is to be done.
> * @rid: Requester ID for the device.
> + * @bus_token: Bus token
> *
> * Walk up the device hierarchy looking for devices with a "msi-map"
> * property.
> *
> * Returns: the MSI domain for this device (or NULL on failure)
> */
> -struct irq_domain *of_msi_map_get_device_domain(struct device *dev, u32 rid)
> +struct irq_domain *of_msi_map_get_device_domain(struct device *dev, u32 id,
> + u32 bus_token)
> {
> struct device_node *np = NULL;
>
> - __of_msi_map_rid(dev, &np, rid);
> - return irq_find_matching_host(np, DOMAIN_BUS_PCI_MSI);
> + __of_msi_map_rid(dev, &np, id);
> + return irq_find_matching_host(np, bus_token);
> }
>
> /**
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 74a91f52ecc0..9532e1d12d3f 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1556,7 +1556,7 @@ struct irq_domain *pci_msi_get_device_domain(struct pci_dev *pdev)
> u32 rid = pci_dev_id(pdev);
>
> pci_for_each_dma_alias(pdev, get_msi_id_cb, &rid);
> - dom = of_msi_map_get_device_domain(&pdev->dev, rid);
> + dom = of_msi_map_get_device_domain(&pdev->dev, rid, DOMAIN_BUS_PCI_MSI);
> if (!dom)
> dom = iort_get_device_domain(&pdev->dev, rid,
> DOMAIN_BUS_PCI_MSI);
> diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
> index 1214cabb2247..7142a3722758 100644
> --- a/include/linux/of_irq.h
> +++ b/include/linux/of_irq.h
> @@ -52,7 +52,8 @@ extern struct irq_domain *of_msi_get_domain(struct device *dev,
> struct device_node *np,
> enum irq_domain_bus_token token);
> extern struct irq_domain *of_msi_map_get_device_domain(struct device *dev,
> - u32 rid);
> + u32 id,
> + u32 bus_token);
> extern void of_msi_configure(struct device *dev, struct device_node *np);
> u32 of_msi_map_rid(struct device *dev, struct device_node *msi_np, u32 rid_in);
> #else
> @@ -85,7 +86,7 @@ static inline struct irq_domain *of_msi_get_domain(struct device *dev,
> return NULL;
> }
> static inline struct irq_domain *of_msi_map_get_device_domain(struct device *dev,
> - u32 rid)
> + u32 id, u32 bus_token)
> {
> return NULL;
> }
> --
> 2.26.1
>
^ permalink raw reply
* Re: [PATCH v9 1/2] PCI: Microchip: Add host driver for Microchip PCIe controller
From: robh @ 2020-05-21 19:56 UTC (permalink / raw)
To: Daire.McNamara
Cc: amurray, devicetree, linux-pci, robh-dt, lorenzo.pieralisi,
bhelgaas
In-Reply-To: <5dc3002680da40400b083748329d8b736219952e.camel@microchip.com>
On Wed, 20 May 2020 11:44:36 +0000, Daire.McNamara@microchip.com wrote:
>
> This patch adds device tree bindings for the Microchip
> PCIe PolarFire PCIe controller when configured in
> host (Root Complex) mode.
>
> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> ---
> .../bindings/pci/microchip,pcie-host.yaml | 94 +++++++++++++++++++
> 1 file changed, 94 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>
My bot found errors running 'make dt_binding_check' on your patch:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml: properties:interrupts: {'minItems': 1, 'maxItems': 1, 'items': [{'description': 'PCIe host controller and builtin MSI controller'}]} is not valid under any of the given schemas (Possible causes of the failure):
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml: properties:interrupts: 'minItems' is not one of ['description', 'deprecated', 'const', 'enum', 'minimum', 'maximum', 'default', '$ref']
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml: properties:interrupts:maxItems: 1 is less than the minimum of 2
Documentation/devicetree/bindings/Makefile:12: recipe for target 'Documentation/devicetree/bindings/pci/microchip,pcie-host.example.dts' failed
make[1]: *** [Documentation/devicetree/bindings/pci/microchip,pcie-host.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml: ignoring, error in schema: properties: interrupts
warning: no schema found in file: ./Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml: ignoring, error in schema: properties: interrupts
warning: no schema found in file: ./Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
Makefile:1300: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2
See https://patchwork.ozlabs.org/patch/1294277
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:
pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade
Please check and re-submit.
^ permalink raw reply
* Re: [PATCH 02/12] ACPI/IORT: Make iort_get_device_domain IRQ domain agnostic
From: Bjorn Helgaas @ 2020-05-21 19:56 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: linux-arm-kernel, Will Deacon, Hanjun Guo, Bjorn Helgaas,
Sudeep Holla, Catalin Marinas, Robin Murphy, Rafael J. Wysocki,
Marc Zyngier, iommu, linux-acpi, devicetree, linux-pci,
Rob Herring, Joerg Roedel, Makarand Pawagi, Diana Craciun,
Laurentiu Tudor
In-Reply-To: <20200521130008.8266-3-lorenzo.pieralisi@arm.com>
On Thu, May 21, 2020 at 01:59:58PM +0100, Lorenzo Pieralisi wrote:
> iort_get_device_domain() is PCI specific but it need not be,
> since it can be used to retrieve IRQ domain nexus of any kind
> by adding an irq_domain_bus_token input to it.
>
> Make it PCI agnostic by also renaming the requestor ID input
> to a more generic ID name.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Hanjun Guo <guohanjun@huawei.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Marc Zyngier <maz@kernel.org>
Acked-by: Bjorn Helgaas <bhelgaas@google.com> # pci/msi.c
> ---
> drivers/acpi/arm64/iort.c | 14 +++++++-------
> drivers/pci/msi.c | 3 ++-
> include/linux/acpi_iort.h | 7 ++++---
> 3 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 7cfd77b5e6e8..8f2a961c1364 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -567,7 +567,6 @@ static struct acpi_iort_node *iort_find_dev_node(struct device *dev)
> node = iort_get_iort_node(dev->fwnode);
> if (node)
> return node;
> -
> /*
> * if not, then it should be a platform device defined in
> * DSDT/SSDT (with Named Component node in IORT)
> @@ -658,13 +657,13 @@ static int __maybe_unused iort_find_its_base(u32 its_id, phys_addr_t *base)
> /**
> * iort_dev_find_its_id() - Find the ITS identifier for a device
> * @dev: The device.
> - * @req_id: Device's requester ID
> + * @id: Device's ID
> * @idx: Index of the ITS identifier list.
> * @its_id: ITS identifier.
> *
> * Returns: 0 on success, appropriate error value otherwise
> */
> -static int iort_dev_find_its_id(struct device *dev, u32 req_id,
> +static int iort_dev_find_its_id(struct device *dev, u32 id,
> unsigned int idx, int *its_id)
> {
> struct acpi_iort_its_group *its;
> @@ -674,7 +673,7 @@ static int iort_dev_find_its_id(struct device *dev, u32 req_id,
> if (!node)
> return -ENXIO;
>
> - node = iort_node_map_id(node, req_id, NULL, IORT_MSI_TYPE);
> + node = iort_node_map_id(node, id, NULL, IORT_MSI_TYPE);
> if (!node)
> return -ENXIO;
>
> @@ -697,19 +696,20 @@ static int iort_dev_find_its_id(struct device *dev, u32 req_id,
> *
> * Returns: the MSI domain for this device, NULL otherwise
> */
> -struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id)
> +struct irq_domain *iort_get_device_domain(struct device *dev, u32 id,
> + enum irq_domain_bus_token bus_token)
> {
> struct fwnode_handle *handle;
> int its_id;
>
> - if (iort_dev_find_its_id(dev, req_id, 0, &its_id))
> + if (iort_dev_find_its_id(dev, id, 0, &its_id))
> return NULL;
>
> handle = iort_find_domain_token(its_id);
> if (!handle)
> return NULL;
>
> - return irq_find_matching_fwnode(handle, DOMAIN_BUS_PCI_MSI);
> + return irq_find_matching_fwnode(handle, bus_token);
> }
>
> static void iort_set_device_domain(struct device *dev,
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 6b43a5455c7a..74a91f52ecc0 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1558,7 +1558,8 @@ struct irq_domain *pci_msi_get_device_domain(struct pci_dev *pdev)
> pci_for_each_dma_alias(pdev, get_msi_id_cb, &rid);
> dom = of_msi_map_get_device_domain(&pdev->dev, rid);
> if (!dom)
> - dom = iort_get_device_domain(&pdev->dev, rid);
> + dom = iort_get_device_domain(&pdev->dev, rid,
> + DOMAIN_BUS_PCI_MSI);
> return dom;
> }
> #endif /* CONFIG_PCI_MSI_IRQ_DOMAIN */
> diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> index 8e7e2ec37f1b..08ec6bd2297f 100644
> --- a/include/linux/acpi_iort.h
> +++ b/include/linux/acpi_iort.h
> @@ -29,7 +29,8 @@ struct fwnode_handle *iort_find_domain_token(int trans_id);
> #ifdef CONFIG_ACPI_IORT
> void acpi_iort_init(void);
> u32 iort_msi_map_rid(struct device *dev, u32 req_id);
> -struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id);
> +struct irq_domain *iort_get_device_domain(struct device *dev, u32 id,
> + enum irq_domain_bus_token bus_token);
> void acpi_configure_pmsi_domain(struct device *dev);
> int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id);
> /* IOMMU interface */
> @@ -40,8 +41,8 @@ int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head);
> static inline void acpi_iort_init(void) { }
> static inline u32 iort_msi_map_rid(struct device *dev, u32 req_id)
> { return req_id; }
> -static inline struct irq_domain *iort_get_device_domain(struct device *dev,
> - u32 req_id)
> +static inline struct irq_domain *iort_get_device_domain(
> + struct device *dev, u32 id, enum irq_domain_bus_token bus_token)
> { return NULL; }
> static inline void acpi_configure_pmsi_domain(struct device *dev) { }
> /* IOMMU interface */
> --
> 2.26.1
>
^ permalink raw reply
* Re: [PATCH 2/4] remoteproc: introduce version element into resource type field
From: Suman Anna @ 2020-05-21 19:52 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Rob Herring, Mathieu Poirier, Clement Leger, Loic Pallardy,
Arnaud Pouliquen, Lokesh Vutla, linux-remoteproc, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <20200521194116.GP408178@builder.lan>
On 5/21/20 2:41 PM, Bjorn Andersson wrote:
> On Thu 21 May 12:29 PDT 2020, Suman Anna wrote:
>
>> On 5/21/20 2:21 PM, Bjorn Andersson wrote:
>>> On Thu 21 May 12:06 PDT 2020, Suman Anna wrote:
>>>
>>>> Hi Bjorn,
>>>>
>>>> On 5/21/20 12:54 PM, Bjorn Andersson wrote:
>>>>> On Wed 25 Mar 13:46 PDT 2020, Suman Anna wrote:
>>>>>
>>>>>> The current remoteproc core has supported only 32-bit remote
>>>>>> processors and as such some of the current resource structures
>>>>>> may not scale well for 64-bit remote processors, and would
>>>>>> require new versions of resource types. Each resource is currently
>>>>>> identified by a 32-bit type field. Introduce the concept of version
>>>>>> for these resource types by overloading this 32-bit type field
>>>>>> into two 16-bit version and type fields with the existing resources
>>>>>> behaving as version 0 thereby providing backward compatibility.
>>>>>>
>>>>>> The version field is passed as an additional argument to each of
>>>>>> the handler functions, and all the existing handlers are updated
>>>>>> accordingly. Each specific handler will be updated on a need basis
>>>>>> when a new version of the resource type is added.
>>>>>>
>>>>>
>>>>> I really would prefer that we add additional types for the new
>>>>> structures, neither side will be compatible with new versions without
>>>>> enhancements to their respective implementations anyways.
>>>>
>>>> OK.
>>>>
>>>>>
>>>>>> An alternate way would be to introduce the new types as completely
>>>>>> new resource types which would require additional customization of
>>>>>> the resource handlers based on the 32-bit or 64-bit mode of a remote
>>>>>> processor, and introduction of an additional mode flag to the rproc
>>>>>> structure.
>>>>>>
>>>>>
>>>>> What would this "mode" indicate? If it's version 0 or 1?
>>>>
>>>> No, for indicating if the remoteproc is 32-bit or 64-bit and adjust the
>>>> loading handlers if the resource types need to be segregated accordingly.
>>>>
>>>
>>> Sorry, I think I'm misunderstanding something. Wouldn't your 64-bit
>>> remote processor need different firmware from your 32-bit processor
>>> anyways, if you want to support the wider resource? And you would pack
>>> your firmware with the appropriate resource types?
>>
>> Yes, that's correct.
>>
>>>
>>> Afaict the bit width of your remote processor, busses or memory is
>>> unrelated to the choice of number of bits used to express things in the
>>> resource table.
>>
>> I would have to add the new resource type to the loading_handlers right, so
>> it is a question of whether we want to impose any restrictions in remoteproc
>> core or not from supporting a certain resource type (eg: I don't expect
>> RSC_TRACE entries on 64-bit processors).
>>
>
> Right, but either you add support for new resource types to the
> loading_handlers, or you add the version checks within each handler,
> either way you will have to do some work to be compatible with new
> versions.
>
> Regarding what resources would be fit for a 64-bit processor probably
> relates to many things, in particular the question of what we actually
> mean when we say that a coprocessor is 64-bit. So I don't really see a
> need for the remoteproc core to prevent someone to design their
> system/firmware to have a 64-bit CPU being passed 32-bit addresses.
OK. In general, I have seen firmware developers get confused w.r.t the
resource types, that's why I was inclined to go with the restrictive
checking. Anyway, will rework the support as per the comments.
regards
Suman
^ permalink raw reply
* Re: [V6, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
From: Tomasz Figa @ 2020-05-21 19:51 UTC (permalink / raw)
To: Dongchun Zhu
Cc: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt,
mark.rutland, sakari.ailus, drinkcat, matthias.bgg, bingbu.cao,
srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang,
linux-media, devicetree, louis.kuo, shengnan.wang
In-Reply-To: <20200518132731.20855-3-dongchun.zhu@mediatek.com>
Hi Dongchun, Sakari,
On Mon, May 18, 2020 at 09:27:31PM +0800, Dongchun Zhu wrote:
> Add a V4L2 sub-device driver for DW9768 voice coil motor, providing
> control to set the desired focus via IIC serial interface.
>
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> ---
> MAINTAINERS | 1 +
> drivers/media/i2c/Kconfig | 13 ++
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/dw9768.c | 515 +++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 530 insertions(+)
> create mode 100644 drivers/media/i2c/dw9768.c
[snip]
> +/*
> + * DW9768_AAC_PRESC_REG & DW9768_AAC_TIME_REG determine VCM operation time.
> + * If DW9768_AAC_PRESC_REG is 0x41, and DW9768_AAC_TIME_REG is 0x39, VCM mode
> + * would be AAC3, Operation Time would be 0.70xTvib, that is 8.40ms.
> + */
> +#define DW9768_MOVE_DELAY_US 8400
> +#define DW9768_STABLE_TIME_US 20000
These times are only valid with the specific settings mentioned in the
comment. If one sets different settings in DT, the driver would apply
incorrect delays. Rather than hardcoded, they should be computed based
on the configured values.
That said, I wonder if we're not digging too deep now. Sakari, do you
think we could take a step back, remove the optional DT properties and
just support the fixed values for now, so that we can get a basic driver
upstream first without doubling the effort?
> +
> +static const char * const dw9768_supply_names[] = {
> + "vin", /* I2C I/O interface power */
> + "vdd", /* VCM power */
> +};
> +
> +/* dw9768 device structure */
> +struct dw9768 {
> + struct regulator_bulk_data supplies[ARRAY_SIZE(dw9768_supply_names)];
> + struct v4l2_ctrl_handler ctrls;
> + struct v4l2_ctrl *focus;
> + struct v4l2_subdev sd;
> +
> + u32 aac_mode;
> + u32 aac_timing;
> + u32 clock_dividing_rate;
> + bool aac_mode_control_enable;
> + bool aact_cnt_select_enable;
> + bool clock_dividing_rate_select_enable;
nit: Separate types from names with just 1 space.
> +};
> +
> +static inline struct dw9768 *sd_to_dw9768(struct v4l2_subdev *subdev)
> +{
> + return container_of(subdev, struct dw9768, sd);
> +}
> +
> +struct regval_list {
> + u8 reg_num;
> + u8 value;
> +};
> +
> +static int dw9768_read_smbus(struct dw9768 *dw9768, unsigned char reg,
> + unsigned char *val)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> + int ret;
> +
> + ret = i2c_smbus_read_byte_data(client, reg);
> +
> + if (ret < 0)
> + return ret;
> +
> + *val = (unsigned char)ret;
> +
> + return 0;
> +}
Why do we need this function? Couldn't we just call
i2c_smbus_read_byte_data() directly?
[snip]
> +static int dw9768_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct dw9768 *dw9768;
> + unsigned int aac_mode_select;
> + unsigned int aac_timing_select;
> + unsigned int clock_dividing_rate_select;
> + unsigned int i;
> + int ret;
> +
> + dw9768 = devm_kzalloc(dev, sizeof(*dw9768), GFP_KERNEL);
> + if (!dw9768)
> + return -ENOMEM;
> +
> + v4l2_i2c_subdev_init(&dw9768->sd, client, &dw9768_ops);
> + dw9768->aac_mode_control_enable = false;
> + dw9768->aact_cnt_select_enable = false;
> + dw9768->clock_dividing_rate_select_enable = false;
devm_kzalloc() initializes the memory to zero, so no need to set anything
to false explicitly.
> +
> + /* Optional indication of AAC mode select */
> + ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-mode",
> + &aac_mode_select);
> +
> + if (!ret) {
> + dw9768->aac_mode_control_enable = true;
> + dw9768->aac_mode = aac_mode_select;
How about making aac_mode a signed int and assigning -1 by
default? Then we don't need two separate fields in the struct.
> + }
> +
> + /* Optional indication of VCM internal clock dividing rate select */
> + ret = fwnode_property_read_u32(dev_fwnode(dev),
> + "dongwoon,clock-dividing-rate",
> + &clock_dividing_rate_select);
> +
> + if (!ret) {
> + dw9768->clock_dividing_rate_select_enable = true;
> + dw9768->clock_dividing_rate = clock_dividing_rate_select;
Ditto.
> + }
> +
> + /* Optional indication of AAC Timing */
> + ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-timing",
> + &aac_timing_select);
> +
> + if (!ret) {
> + dw9768->aact_cnt_select_enable = true;
> + dw9768->aac_timing = aac_timing_select;
Ditto.
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(dw9768_supply_names); i++)
> + dw9768->supplies[i].supply = dw9768_supply_names[i];
> +
> + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(dw9768_supply_names),
> + dw9768->supplies);
> + if (ret) {
> + dev_err(dev, "failed to get regulators\n");
> + return ret;
> + }
> +
> + ret = dw9768_init_controls(dw9768);
> + if (ret)
> + goto entity_cleanup;
> +
> + dw9768->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + dw9768->sd.internal_ops = &dw9768_int_ops;
> +
> + ret = media_entity_pads_init(&dw9768->sd.entity, 0, NULL);
> + if (ret < 0)
> + goto entity_cleanup;
> +
> + dw9768->sd.entity.function = MEDIA_ENT_F_LENS;
> +
> + pm_runtime_enable(dev);
> + if (!pm_runtime_enabled(dev)) {
> + ret = dw9768_runtime_resume(dev);
> + if (ret < 0) {
> + dev_err(dev, "failed to power on: %d\n", ret);
> + goto entity_cleanup;
> + }
> + }
> +
> + ret = v4l2_async_register_subdev(&dw9768->sd);
> + if (ret < 0)
> + goto entity_cleanup;
> +
> + return 0;
> +
> +entity_cleanup:
Need to power off if the code above powered on.
> + v4l2_ctrl_handler_free(&dw9768->ctrls);
> + media_entity_cleanup(&dw9768->sd.entity);
> + return ret;
> +}
> +
> +static int dw9768_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct dw9768 *dw9768 = sd_to_dw9768(sd);
> +
> + pm_runtime_disable(&client->dev);
First the device must be unregistered from the userspace. Otherwise there
is a race condition that risks the userspace accessing the device while the
deinitialization is happening.
Best regards,
Tomasz
^ permalink raw reply
* [PATCH] arm: dts: am335x-boneblack: add gpio-line-names
From: Drew Fustini @ 2020-05-21 19:50 UTC (permalink / raw)
To: Linus Walleij, Grygorii Strashko, Benoît Cousson,
Tony Lindgren, Rob Herring, Linux-OMAP, devicetree, linux-kernel,
Jason Kridner, Robert Nelson
Add gpio-line-names properties to the GPIO controller nodes.
The BeagleBone Black has P8 and P9 headers [0] which expose many of the
AM3358 ZCZ SoC balls to stacking expansion boards called "capes", or to
other external connections like jumper wires connected to a breadboard.
BeagleBone users will often refer to the "Cape Exanpsion Headers" pin
diagram [1] as it is in the "Bone101" getting started tutorial. [2]
Most of the P8 and P9 header pins can muxed to a GPIO line. The
gpio-line-names describe which P8 or P9 pin that line goes to and the
default mux for that P8 or P9 pin if it is not GPIO.
For example, gpiochip 1 line 0 is connected to P8 header pin 25 (P8_25)
however the default device tree has the corresponding BGA ball (ZCZ U7)
muxed to mmc1_dat0 as it is used for the on-board eMMC chip. For that
GPIO line to be used, one would need to modify the device tree to
disable the eMMC and change the pin mux for that ball to GPIO mode.
Some of the AM3358 ZCZ balls corresponding to GPIO lines are not routed
to a P8 or P9 header, but are instead wired to some peripheral device
like on-board eMMC, HDMI framer IC, or status LEDs. Those names are in
brackets to denote those GPIO lines can not be used.
Some GPIO lines are named "[NC]" as the corresponding balls are not
routed to anything on the PCB.
The goal for these names is to make it easier for a user viewing the
output of gpioinfo to determine which P8 or P9 pin is connected to a
GPIO line. The output of gpioinfo on a BeagleBone Black would be:
[0] https://git.io/JfgOd
[1] https://beagleboard.org/capes
[1] https://beagleboard.org/Support/bone101
[2] https://beagleboard.org/static/images/cape-headers.png
Reviewed-by: Jason Kridner <jason@beagleboard.org>
Reviewed-by: Robert Nelson <robertcnelson@gmail.com>
Signed-off-by: Drew Fustini <drew@beagleboard.org>
---
V1 note:
I had posted a patch with these line names for am335x-bone-common.dtsi
but Grygorii Strashko pointed out that the names are not applicable to
all BeagleBone models. Thus is have created this patch to add these
names just for the BeagleBone Black.
arch/arm/boot/dts/am335x-boneblack.dts | 144 +++++++++++++++++++++++++
1 file changed, 144 insertions(+)
diff --git a/arch/arm/boot/dts/am335x-boneblack.dts b/arch/arm/boot/dts/am335x-boneblack.dts
index d3928662aed4..5f31e832eb82 100644
--- a/arch/arm/boot/dts/am335x-boneblack.dts
+++ b/arch/arm/boot/dts/am335x-boneblack.dts
@@ -23,3 +23,147 @@ oppnitro-1000000000 {
opp-supported-hw = <0x06 0x0100>;
};
};
+
+&gpio0 {
+ gpio-line-names =
+ "[ethernet]",
+ "[ethernet]",
+ "P9_22 [spi0_sclk]",
+ "P9_21 [spi0_d0]",
+ "P9_18 [spi0_d1]",
+ "P9_17 [spi0_cs0]",
+ "[sd card]",
+ "P9_42A [ecappwm0]",
+ "P8_35 [hdmi]",
+ "P8_33 [hdmi]",
+ "P8_31 [hdmi]",
+ "P8_32 [hdmi]",
+ "P9_20 [i2c2_sda]",
+ "P9_19 [i2c2_scl]",
+ "P9_26 [uart1_rxd]",
+ "P9_24 [uart1_txd]",
+ "[ethernet]",
+ "[ethernet]",
+ "[usb]",
+ "[hdmi]",
+ "P9_41B",
+ "[ethernet]",
+ "P8_19 [ehrpwm2a]",
+ "P8_13 [ehrpwm2b]",
+ "[NC]",
+ "[NC]",
+ "P8_14",
+ "P8_17",
+ "[ethernet]",
+ "[ethernet]",
+ "P9_11 [uart4_rxd]",
+ "P9_13 [uart4_txd]";
+};
+
+&gpio1 {
+ gpio-line-names =
+ "P8_25 [emmc]",
+ "[emmc]",
+ "P8_5 [emmc]",
+ "P8_6 [emmc]",
+ "P8_23 [emmc]",
+ "P8_22 [emmc]",
+ "P8_3 [emmc]",
+ "P8_4 [emmc]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "P8_12",
+ "P8_11",
+ "P8_16",
+ "P8_15",
+ "P9_15A",
+ "P9_23",
+ "P9_14 [ehrpwm1a]",
+ "P9_16 [ehrpwm1b]",
+ "[emmc]",
+ "[usr0 led]",
+ "[usr1 led]",
+ "[usr2 led]",
+ "[usr3 led]",
+ "[hdmi]",
+ "[usb]",
+ "[hdmi audio]",
+ "P9_12",
+ "P8_26",
+ "P8_21 [emmc]",
+ "P8_20 [emmc]";
+};
+
+&gpio2 {
+ gpio-line-names =
+ "P9_15B",
+ "P8_18",
+ "P8_7",
+ "P8_8",
+ "P8_10",
+ "P8_9",
+ "P8_45 [hdmi]",
+ "P8_46 [hdmi]",
+ "P8_43 [hdmi]",
+ "P8_44 [hdmi]",
+ "P8_41 [hdmi]",
+ "P8_42 [hdmi]",
+ "P8_39 [hdmi]",
+ "P8_40 [hdmi]",
+ "P8_37 [hdmi]",
+ "P8_38 [hdmi]",
+ "P8_36 [hdmi]",
+ "P8_34 [hdmi]",
+ "[ethernet]",
+ "[ethernet]",
+ "[ethernet]",
+ "[ethernet]",
+ "P8_27 [hdmi]",
+ "P8_29 [hdmi]",
+ "P8_28 [hdmi]",
+ "P8_30 [hdmi]",
+ "[emmc]",
+ "[emmc]",
+ "[emmc]",
+ "[emmc]",
+ "[emmc]",
+ "[emmc]";
+};
+
+&gpio3 {
+ gpio-line-names =
+ "[ethernet]",
+ "[ethernet]",
+ "[ethernet]",
+ "[ethernet]",
+ "[ethernet]",
+ "[i2c0]",
+ "[i2c0]",
+ "[emu]",
+ "[emu]",
+ "[ethernet]",
+ "[ethernet]",
+ "[NC]",
+ "[NC]",
+ "[usb]",
+ "P9_31 [spi1_sclk]",
+ "P9_29 [spi1_d0]",
+ "P9_30 [spi1_d1]",
+ "P9_28 [spi1_cs0]",
+ "P9_42B [ecappwm0]",
+ "P9_27",
+ "P9_41A",
+ "P9_25",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]",
+ "[NC]";
+};
--
2.25.1
^ permalink raw reply related
* Re: [PATCH 2/4] remoteproc: introduce version element into resource type field
From: Bjorn Andersson @ 2020-05-21 19:41 UTC (permalink / raw)
To: Suman Anna
Cc: Rob Herring, Mathieu Poirier, Clement Leger, Loic Pallardy,
Arnaud Pouliquen, Lokesh Vutla, linux-remoteproc, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <57ae5678-fd0a-07a8-6165-a2cf7ccdef88@ti.com>
On Thu 21 May 12:29 PDT 2020, Suman Anna wrote:
> On 5/21/20 2:21 PM, Bjorn Andersson wrote:
> > On Thu 21 May 12:06 PDT 2020, Suman Anna wrote:
> >
> > > Hi Bjorn,
> > >
> > > On 5/21/20 12:54 PM, Bjorn Andersson wrote:
> > > > On Wed 25 Mar 13:46 PDT 2020, Suman Anna wrote:
> > > >
> > > > > The current remoteproc core has supported only 32-bit remote
> > > > > processors and as such some of the current resource structures
> > > > > may not scale well for 64-bit remote processors, and would
> > > > > require new versions of resource types. Each resource is currently
> > > > > identified by a 32-bit type field. Introduce the concept of version
> > > > > for these resource types by overloading this 32-bit type field
> > > > > into two 16-bit version and type fields with the existing resources
> > > > > behaving as version 0 thereby providing backward compatibility.
> > > > >
> > > > > The version field is passed as an additional argument to each of
> > > > > the handler functions, and all the existing handlers are updated
> > > > > accordingly. Each specific handler will be updated on a need basis
> > > > > when a new version of the resource type is added.
> > > > >
> > > >
> > > > I really would prefer that we add additional types for the new
> > > > structures, neither side will be compatible with new versions without
> > > > enhancements to their respective implementations anyways.
> > >
> > > OK.
> > >
> > > >
> > > > > An alternate way would be to introduce the new types as completely
> > > > > new resource types which would require additional customization of
> > > > > the resource handlers based on the 32-bit or 64-bit mode of a remote
> > > > > processor, and introduction of an additional mode flag to the rproc
> > > > > structure.
> > > > >
> > > >
> > > > What would this "mode" indicate? If it's version 0 or 1?
> > >
> > > No, for indicating if the remoteproc is 32-bit or 64-bit and adjust the
> > > loading handlers if the resource types need to be segregated accordingly.
> > >
> >
> > Sorry, I think I'm misunderstanding something. Wouldn't your 64-bit
> > remote processor need different firmware from your 32-bit processor
> > anyways, if you want to support the wider resource? And you would pack
> > your firmware with the appropriate resource types?
>
> Yes, that's correct.
>
> >
> > Afaict the bit width of your remote processor, busses or memory is
> > unrelated to the choice of number of bits used to express things in the
> > resource table.
>
> I would have to add the new resource type to the loading_handlers right, so
> it is a question of whether we want to impose any restrictions in remoteproc
> core or not from supporting a certain resource type (eg: I don't expect
> RSC_TRACE entries on 64-bit processors).
>
Right, but either you add support for new resource types to the
loading_handlers, or you add the version checks within each handler,
either way you will have to do some work to be compatible with new
versions.
Regarding what resources would be fit for a 64-bit processor probably
relates to many things, in particular the question of what we actually
mean when we say that a coprocessor is 64-bit. So I don't really see a
need for the remoteproc core to prevent someone to design their
system/firmware to have a 64-bit CPU being passed 32-bit addresses.
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCH 3/4] remoteproc: add support for a new 64-bit trace version
From: Suman Anna @ 2020-05-21 19:42 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Rob Herring, Mathieu Poirier, Clement Leger, Loic Pallardy,
Arnaud Pouliquen, Lokesh Vutla, linux-remoteproc, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <20200521180417.GJ408178@builder.lan>
Hi Bjorn,
On 5/21/20 1:04 PM, Bjorn Andersson wrote:
> On Wed 25 Mar 13:47 PDT 2020, Suman Anna wrote:
>
>> Introduce a new trace entry resource structure that accommodates
>> a 64-bit device address to support 64-bit processors. This is to
>> be used using an overloaded version value of 1 in the upper 32-bits
>> of the previous resource type field. The new resource still uses
>> 32-bits for the length field (followed by a 32-bit reserved field,
>> so can be updated in the future), which is a sufficiently large
>> trace buffer size. A 32-bit padding field also had to be added
>> to align the device address on a 64-bit boundary, and match the
>> usage on the firmware side.
>>
>> The remoteproc debugfs logic also has been adjusted accordingly.
>>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 40 ++++++++++++++++++++-----
>> drivers/remoteproc/remoteproc_debugfs.c | 37 ++++++++++++++++++-----
>> include/linux/remoteproc.h | 26 ++++++++++++++++
>> 3 files changed, 87 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 53bc37c508c6..b9a097990862 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -609,21 +609,45 @@ void rproc_vdev_release(struct kref *ref)
>> *
>> * Returns 0 on success, or an appropriate error code otherwise
>> */
>> -static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
>> +static int rproc_handle_trace(struct rproc *rproc, void *rsc,
>> int offset, int avail, u16 ver)
>> {
>> struct rproc_debug_trace *trace;
>> struct device *dev = &rproc->dev;
>> + struct fw_rsc_trace *rsc1;
>> + struct fw_rsc_trace2 *rsc2;
>> char name[15];
>> + size_t rsc_size;
>> + u32 reserved;
>> + u64 da;
>> + u32 len;
>> +
>> + if (!ver) {
>
> This looks like a switch to me, but I also do think this looks rather
> crude, if you spin off the tail of this function and call it from a
> rproc_handle_trace() and rproc_handle_trace64() I believe this would be
> cleaner.
Yeah, ok. Will refactor for this in v2.
>
>> + rsc1 = (struct fw_rsc_trace *)rsc;
>> + rsc_size = sizeof(*rsc1);
>> + reserved = rsc1->reserved;
>> + da = rsc1->da;
>> + len = rsc1->len;
>> + } else if (ver == 1) {
>> + rsc2 = (struct fw_rsc_trace2 *)rsc;
>> + rsc_size = sizeof(*rsc2);
>> + reserved = rsc2->reserved;
>> + da = rsc2->da;
>> + len = rsc2->len;
>> + } else {
>> + dev_err(dev, "unsupported trace rsc version %d\n", ver);
>
> If we use "type" to describe your 64-bit-da-trace then this sanity check
> would have been taken care of by the core.
>
>> + return -EINVAL;
>> + }
>>
>> - if (sizeof(*rsc) > avail) {
>> + if (rsc_size > avail) {
>> dev_err(dev, "trace rsc is truncated\n");
>> return -EINVAL;
>> }
>>
>> /* make sure reserved bytes are zeroes */
>> - if (rsc->reserved) {
>> - dev_err(dev, "trace rsc has non zero reserved bytes\n");
>> + if (reserved) {
>> + dev_err(dev, "trace rsc has non zero reserved bytes, value = 0x%x\n",
>> + reserved);
>> return -EINVAL;
>> }
>>
>> @@ -632,8 +656,8 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
>> return -ENOMEM;
>>
>> /* set the trace buffer dma properties */
>> - trace->trace_mem.len = rsc->len;
>> - trace->trace_mem.da = rsc->da;
>> + trace->trace_mem.len = len;
>> + trace->trace_mem.da = da;
>>
>> /* set pointer on rproc device */
>> trace->rproc = rproc;
>> @@ -652,8 +676,8 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
>>
>> rproc->num_traces++;
>>
>> - dev_dbg(dev, "%s added: da 0x%x, len 0x%x\n",
>> - name, rsc->da, rsc->len);
>> + dev_dbg(dev, "%s added: da 0x%llx, len 0x%x\n",
>> + name, da, len);
>>
>> return 0;
>> }
>> diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
>> index 3560eed7a360..ff43736db45a 100644
>> --- a/drivers/remoteproc/remoteproc_debugfs.c
>> +++ b/drivers/remoteproc/remoteproc_debugfs.c
>> @@ -192,7 +192,8 @@ static int rproc_rsc_table_show(struct seq_file *seq, void *p)
>> struct resource_table *table = rproc->table_ptr;
>> struct fw_rsc_carveout *c;
>> struct fw_rsc_devmem *d;
>> - struct fw_rsc_trace *t;
>> + struct fw_rsc_trace *t1;
>> + struct fw_rsc_trace2 *t2;
>> struct fw_rsc_vdev *v;
>> int i, j;
>>
>> @@ -205,6 +206,7 @@ static int rproc_rsc_table_show(struct seq_file *seq, void *p)
>> int offset = table->offset[i];
>> struct fw_rsc_hdr *hdr = (void *)table + offset;
>> void *rsc = (void *)hdr + sizeof(*hdr);
>> + u16 ver = hdr->st.v;
>>
>> switch (hdr->st.t) {
>> case RSC_CARVEOUT:
>> @@ -230,13 +232,32 @@ static int rproc_rsc_table_show(struct seq_file *seq, void *p)
>> seq_printf(seq, " Name %s\n\n", d->name);
>> break;
>> case RSC_TRACE:
>> - t = rsc;
>> - seq_printf(seq, "Entry %d is of type %s\n",
>> - i, types[hdr->st.t]);
>> - seq_printf(seq, " Device Address 0x%x\n", t->da);
>> - seq_printf(seq, " Length 0x%x Bytes\n", t->len);
>> - seq_printf(seq, " Reserved (should be zero) [%d]\n", t->reserved);
>> - seq_printf(seq, " Name %s\n\n", t->name);
>> + if (ver == 0) {
>
> Again, this is a switch, here in a switch. Just defining a new
> RSC_TRACE64 type would reduce the amount of code here...
OK.
>
>> + t1 = rsc;
>> + seq_printf(seq, "Entry %d is version %d of type %s\n",
>> + i, ver, types[hdr->st.t]);
>> + seq_printf(seq, " Device Address 0x%x\n",
>> + t1->da);
>> + seq_printf(seq, " Length 0x%x Bytes\n",
>> + t1->len);
>> + seq_printf(seq, " Reserved (should be zero) [%d]\n",
>> + t1->reserved);
>> + seq_printf(seq, " Name %s\n\n", t1->name);
>> + } else if (ver == 1) {
>> + t2 = rsc;
>> + seq_printf(seq, "Entry %d is version %d of type %s\n",
>> + i, ver, types[hdr->st.t]);
>> + seq_printf(seq, " Device Address 0x%llx\n",
>> + t2->da);
>> + seq_printf(seq, " Length 0x%x Bytes\n",
>> + t2->len);
>> + seq_printf(seq, " Reserved (should be zero) [%d]\n",
>> + t2->reserved);
>> + seq_printf(seq, " Name %s\n\n", t2->name);
>> + } else {
>> + seq_printf(seq, "Entry %d is an unsupported version %d of type %s\n",
>> + i, ver, types[hdr->st.t]);
>> + }
>> break;
>> case RSC_VDEV:
>> v = rsc;
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 526d3cb45e37..3b3bea42f8b1 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -243,6 +243,32 @@ struct fw_rsc_trace {
>> u8 name[32];
>> } __packed;
>>
>> +/**
>> + * struct fw_rsc_trace2 - trace buffer declaration supporting 64-bits
>> + * @padding: initial padding after type field for aligned 64-bit access
>> + * @da: device address (64-bit)
>> + * @len: length (in bytes)
>> + * @reserved: reserved (must be zero)
>> + * @name: human-readable name of the trace buffer
>> + *
>> + * This resource entry is an enhanced version of the fw_rsc_trace resourec entry
>> + * and the provides equivalent functionality but designed for 64-bit remote
>> + * processors.
>> + *
>> + * @da specifies the device address of the buffer, @len specifies
>> + * its size, and @name may contain a human readable name of the trace buffer.
>> + *
>> + * After booting the remote processor, the trace buffers are exposed to the
>> + * user via debugfs entries (called trace0, trace1, etc..).
>> + */
>> +struct fw_rsc_trace2 {
>
> Sounds more like fw_rsc_trace64 to me - in particular since the version
> of trace2 is 1...
Yeah, will rename this.
>
>> + u32 padding;
>> + u64 da;
>> + u32 len;
>> + u32 reserved;
>
> What's the purpose of this reserved field?
Partly to make sure the entire resource is aligned on an 8-byte, and
partly copied over from fw_rsc_trace entry. I guess 32-bits is already
large enough of a size for trace entries irrespective of 32-bit or
64-bit traces, so I doubt if we want to make the len field also a u64.
regards
Suman
>
> Regards,
> Bjorn
>
>> + u8 name[32];
>> +} __packed;
>> +
>> /**
>> * struct fw_rsc_vdev_vring - vring descriptor entry
>> * @da: device address
>> --
>> 2.23.0
>>
^ permalink raw reply
* Re: [V8, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings
From: Tomasz Figa @ 2020-05-21 19:35 UTC (permalink / raw)
To: Rob Herring
Cc: Dongchun Zhu, linus.walleij, bgolaszewski, mchehab,
andriy.shevchenko, mark.rutland, sakari.ailus, drinkcat,
matthias.bgg, bingbu.cao, srv_heupstream, linux-mediatek,
linux-arm-kernel, sj.huang, linux-media, devicetree, louis.kuo,
shengnan.wang
In-Reply-To: <20200511160207.GA32476@bogus>
Hi Rob,
On Mon, May 11, 2020 at 11:02:07AM -0500, Rob Herring wrote:
> On Sat, May 09, 2020 at 04:06:26PM +0800, Dongchun Zhu wrote:
> > Add DT bindings documentation for Omnivision OV02A10 image sensor.
> >
> > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > ---
> > .../bindings/media/i2c/ovti,ov02a10.yaml | 184 +++++++++++++++++++++
> > MAINTAINERS | 7 +
> > 2 files changed, 191 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > new file mode 100644
> > index 0000000..5468d1b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > @@ -0,0 +1,184 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright (c) 2020 MediaTek Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov02a10.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Omnivision OV02A10 CMOS Sensor Device Tree Bindings
> > +
> > +maintainers:
> > + - Dongchun Zhu <dongchun.zhu@mediatek.com>
> > +
> > +description: |-
> > + The Omnivision OV02A10 is a low-cost, high performance, 1/5-inch, 2 megapixel
> > + image sensor, which is the latest production derived from Omnivision's CMOS
> > + image sensor technology. Ihis chip supports high frame rate speeds up to 30fps
> > + @ 1600x1200 (UXGA) resolution transferred over a 1-lane MIPI interface. The
> > + sensor output is available via CSI-2 serial data output.
> > +
> > +properties:
> > + compatible:
> > + const: ovti,ov02a10
> > +
> > + reg:
> > + description: I2C device address
>
> Drop this. Nothing specific to this device.
>
> > + maxItems: 1
> > +
> > + clocks:
> > + items:
> > + - description: top mux camtg clock
> > + - description: devider clock
>
> typo
>
> > +
> > + clock-names:
> > + items:
> > + - const: eclk
> > + - const: freq_mux
> > +
> > + clock-frequency:
> > + description:
> > + Frequency of the eclk clock in Hertz.
> > +
> > + dovdd-supply:
> > + description:
> > + Definition of the regulator used as interface power supply.
> > +
> > + avdd-supply:
> > + description:
> > + Definition of the regulator used as analog power supply.
> > +
> > + dvdd-supply:
> > + description:
> > + Definition of the regulator used as digital power supply.
> > +
> > + powerdown-gpios:
> > + maxItems: 1
> > +
> > + reset-gpios:
> > + maxItems: 1
I asked a question about defining GPIO polarities some time ago, but I
guess it slipped through.
The chip documentation calls the reset pin as "RST_PAD (low level
reset)". Where should the inversion be handled, in the driver or here,
by having the DT include a necessary flag in the specifier?
Best regards,
Tomasz
^ permalink raw reply
* Re: [V8, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver
From: Tomasz Figa @ 2020-05-21 19:32 UTC (permalink / raw)
To: Dongchun Zhu
Cc: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt,
mark.rutland, sakari.ailus, drinkcat, matthias.bgg, bingbu.cao,
srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang,
linux-media, devicetree, louis.kuo, shengnan.wang
In-Reply-To: <20200509080627.23222-3-dongchun.zhu@mediatek.com>
Hi Dongchun,
On Sat, May 09, 2020 at 04:06:27PM +0800, Dongchun Zhu wrote:
> Add a V4L2 sub-device driver for OV02A10 image sensor.
>
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> ---
> MAINTAINERS | 1 +
> drivers/media/i2c/Kconfig | 13 +
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/ov02a10.c | 1094 +++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 1109 insertions(+)
> create mode 100644 drivers/media/i2c/ov02a10.c
>
Thank you for the patch. Please see my comments inline.
[snip]
> +struct ov02a10 {
> + u32 eclk_freq;
> + u32 mipi_clock_tx_speed;
> +
> + struct clk *eclk;
> + struct gpio_desc *pd_gpio;
> + struct gpio_desc *n_rst_gpio;
> + struct regulator_bulk_data supplies[OV02A10_NUM_SUPPLIES];
> +
> + bool streaming;
> + bool upside_down;
> + bool mipi_clock_tx_speed_select_enable;
> + bool mipi_clock_hs_mode_enable;
> +
> + /*
> + * Serialize control access, get/set format, get selection
> + * and start streaming.
> + */
> + struct mutex mutex;
> + struct v4l2_subdev subdev;
> + struct media_pad pad;
> + struct v4l2_ctrl *anal_gain;
> + struct v4l2_ctrl *exposure;
> + struct v4l2_ctrl *hblank;
> + struct v4l2_ctrl *vblank;
> + struct v4l2_ctrl *test_pattern;
> + struct v4l2_mbus_framefmt fmt;
nit: Remove the tabs between types and names and use single spaces. As you
can see above, even tabs don't guarantee equal alignment. And they actually
make adding fields more difficult, because if a longer field is added, the
alignment breaks.
[snip]
> +static int ov02a10_read_smbus(struct ov02a10 *ov02a10, unsigned char reg,
> + unsigned char *val)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + int ret;
> +
> + ret = i2c_smbus_read_byte_data(client, reg);
> +
nit: Unnecessary blank line.
> + if (ret < 0)
> + return ret;
> +
> + *val = (unsigned char)ret;
> +
> + return 0;
> +}
[snip]
> +static int __maybe_unused ov02a10_power_on(struct device *dev)
How is it possible that this function may be unused? If this driver allows
disabling runtime PM, then there is still a need to configure the clock,
regulator and GPIO. If not, there shouldn't be a case where this function
is unused.
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> + int ret;
> +
> + gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 0);
> + gpiod_set_value_cansleep(ov02a10->pd_gpio, 0);
> +
> + ret = clk_prepare_enable(ov02a10->eclk);
> + if (ret < 0) {
> + dev_err(dev, "failed to enable eclk\n");
> + return ret;
> + }
> +
> + ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
> + if (ret < 0) {
> + dev_err(dev, "failed to enable regulators\n");
> + goto disable_clk;
> + }
> + usleep_range(5000, 6000);
> +
> + gpiod_set_value_cansleep(ov02a10->pd_gpio, 1);
This is a "powerdown" GPIO. It must be set to 0 if the sensor is to be
powered on.
> + usleep_range(5000, 6000);
> +
> + gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 1);
> + usleep_range(5000, 6000);
> +
> + ret = ov02a10_check_sensor_id(ov02a10);
> + if (ret)
> + goto disable_regulator;
> +
> + return 0;
> +
> +disable_regulator:
> + regulator_bulk_disable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
> +disable_clk:
> + clk_disable_unprepare(ov02a10->eclk);
> +
> + return ret;
> +}
> +
> +static int __maybe_unused ov02a10_power_off(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> +
> + gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 0);
> + clk_disable_unprepare(ov02a10->eclk);
> + gpiod_set_value_cansleep(ov02a10->pd_gpio, 0);
Similar comment as above. To power off the sensor, the "powerdown" GPIO
needs to be active, i.e. 1.
[snip]
> +/*
> + * ov02a10_set_exposure - Function called when setting exposure time
> + * @priv: Pointer to device structure
> + * @val: Variable for exposure time, in the unit of micro-second
> + *
> + * Set exposure time based on input value.
> + *
> + * Return: 0 on success
> + */
> +static int ov02a10_set_exposure(struct ov02a10 *ov02a10, int val)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> + if (ret < 0)
> + return ret;
How does this page switch work? According to the documentation I have, the
register allows selecting between a few different pages. However, there
should be two page pointers - one for the AP and the other for the sensor,
so that when the AP is programming page X, the sensor can have consistent
settings from page Y. But here we only set one register and always with
page 1.
> +
> + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_EXPOSURE_H,
> + val >> OV02A10_EXP_SHIFT);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_EXPOSURE_L, val);
> + if (ret < 0)
> + return ret;
> +
> + return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> + REG_ENABLE);
This patch defines REG_GLOBAL_EFFECTIVE to 0x01. I don't see such register
mentioned in the documentation.
> +}
> +
> +static int ov02a10_set_gain(struct ov02a10 *ov02a10, int val)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_GAIN, val);
> + if (ret < 0)
> + return ret;
> +
> + return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> + REG_ENABLE);
> +}
> +
> +static int ov02a10_set_vblank(struct ov02a10 *ov02a10, int val)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + u32 vts = val + ov02a10->cur_mode->height - OV02A10_BASIC_LINE;
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
> + vts >> OV02A10_VTS_SHIFT);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_L, vts);
> + if (ret < 0)
> + return ret;
> +
> + return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> + REG_ENABLE);
> +}
> +
> +static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, int pattern)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + int ret;
> +
> + if (pattern)
> + pattern = OV02A10_TEST_PATTERN_ENABLE;
Is this necessary? Our control can be 0 for disabled and 1 for color bars.
The latter is the same as the above macro.
[snip]
> +static int ov02a10_initialize_controls(struct ov02a10 *ov02a10)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + const struct ov02a10_mode *mode;
> + struct v4l2_ctrl_handler *handler;
> + struct v4l2_ctrl *ctrl;
> + u64 exposure_max;
> + u32 pixel_rate, h_blank;
> + int ret;
> +
> + handler = &ov02a10->ctrl_handler;
> + mode = ov02a10->cur_mode;
> + ret = v4l2_ctrl_handler_init(handler, 7);
> + if (ret)
> + return ret;
> +
> + handler->lock = &ov02a10->mutex;
> +
> + ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ, 0, 0,
> + link_freq_menu_items);
> + if (ctrl)
> + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + pixel_rate = to_pixel_rate(0);
> + v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE, 0, pixel_rate, 1,
> + pixel_rate);
> +
> + h_blank = mode->hts_def - mode->width;
> + ov02a10->hblank = v4l2_ctrl_new_std(handler, NULL, V4L2_CID_HBLANK,
> + h_blank, h_blank, 1, h_blank);
> + if (ov02a10->hblank)
> + ov02a10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
Do we need to hold a pointer to this control? We don't seem to ever access
it anywhere else in the driver.
> + ov02a10->vblank = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> + V4L2_CID_VBLANK, mode->vts_def -
> + mode->height,
> + OV02A10_VTS_MAX - mode->height, 1,
> + mode->vts_def - mode->height);
> +
Ditto.
> + exposure_max = mode->vts_def - 4;
> + ov02a10->exposure = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> + V4L2_CID_EXPOSURE,
> + OV02A10_EXPOSURE_MIN,
> + exposure_max,
> + OV02A10_EXPOSURE_STEP,
> + mode->exp_def);
> +
> + ov02a10->anal_gain = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> + V4L2_CID_ANALOGUE_GAIN,
> + OV02A10_GAIN_MIN,
> + OV02A10_GAIN_MAX,
> + OV02A10_GAIN_STEP,
> + OV02A10_GAIN_DEFAULT);
Ditto.
> +
> + ov02a10->test_pattern =
> + v4l2_ctrl_new_std_menu_items(handler, &ov02a10_ctrl_ops,
> + V4L2_CID_TEST_PATTERN,
> + ARRAY_SIZE(ov02a10_test_pattern_menu) -
> + 1, 0, 0,
> + ov02a10_test_pattern_menu);
Ditto.
[snip]
> + ov02a10->pd_gpio = devm_gpiod_get(dev, "powerdown", GPIOD_OUT_HIGH);
> + if (IS_ERR(ov02a10->pd_gpio)) {
> + ret = PTR_ERR(ov02a10->pd_gpio);
> + dev_err(dev, "failed to get powerdown-gpios %d\n", ret);
> + return ret;
> + }
> +
> + ov02a10->n_rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
I've asked a question about the right convention to define active low pins,
but I don't remember seeing an answer. We need to clarify this to avoid
confusion. Especially since the current binding documentation doesn't
mention anything about the sensor being active low. Let me also post a
comment to the binding patch.
> + if (IS_ERR(ov02a10->n_rst_gpio)) {
> + ret = PTR_ERR(ov02a10->n_rst_gpio);
> + dev_err(dev, "failed to get reset-gpios %d\n", ret);
> + return ret;
> + }
> +
> + for (i = 0; i < OV02A10_NUM_SUPPLIES; i++)
> + ov02a10->supplies[i].supply = ov02a10_supply_names[i];
> +
> + ret = devm_regulator_bulk_get(dev, OV02A10_NUM_SUPPLIES,
> + ov02a10->supplies);
> + if (ret) {
> + dev_err(dev, "failed to get regulators\n");
> + return ret;
> + }
> +
> + mutex_init(&ov02a10->mutex);
> + ov02a10->cur_mode = &supported_modes[0];
> + ret = ov02a10_initialize_controls(ov02a10);
> + if (ret) {
> + dev_err(dev, "failed to initialize controls\n");
> + goto err_destroy_mutex;
> + }
> +
> + ov02a10->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + ov02a10->subdev.entity.ops = &ov02a10_subdev_entity_ops;
> + ov02a10->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> + ov02a10->pad.flags = MEDIA_PAD_FL_SOURCE;
> + ret = media_entity_pads_init(&ov02a10->subdev.entity, 1, &ov02a10->pad);
> + if (ret < 0) {
> + dev_err(dev, "failed to init entity pads: %d", ret);
> + goto err_free_handler;
> + }
> +
> + pm_runtime_enable(dev);
> + if (!pm_runtime_enabled(dev)) {
> + ret = ov02a10_power_on(dev);
> + if (ret < 0) {
> + dev_err(dev, "failed to power on: %d\n", ret);
> + goto err_clean_entity;
> + }
> + }
> +
> + ret = v4l2_async_register_subdev(&ov02a10->subdev);
> + if (ret) {
> + dev_err(dev, "failed to register V4L2 subdev: %d", ret);
> + goto err_clean_entity;
> + }
> +
> + return 0;
> +
> +err_clean_entity:
Need to power off if !pm_runtime_enabled().
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH 2/4] remoteproc: introduce version element into resource type field
From: Suman Anna @ 2020-05-21 19:29 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Rob Herring, Mathieu Poirier, Clement Leger, Loic Pallardy,
Arnaud Pouliquen, Lokesh Vutla, linux-remoteproc, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <20200521192146.GO408178@builder.lan>
On 5/21/20 2:21 PM, Bjorn Andersson wrote:
> On Thu 21 May 12:06 PDT 2020, Suman Anna wrote:
>
>> Hi Bjorn,
>>
>> On 5/21/20 12:54 PM, Bjorn Andersson wrote:
>>> On Wed 25 Mar 13:46 PDT 2020, Suman Anna wrote:
>>>
>>>> The current remoteproc core has supported only 32-bit remote
>>>> processors and as such some of the current resource structures
>>>> may not scale well for 64-bit remote processors, and would
>>>> require new versions of resource types. Each resource is currently
>>>> identified by a 32-bit type field. Introduce the concept of version
>>>> for these resource types by overloading this 32-bit type field
>>>> into two 16-bit version and type fields with the existing resources
>>>> behaving as version 0 thereby providing backward compatibility.
>>>>
>>>> The version field is passed as an additional argument to each of
>>>> the handler functions, and all the existing handlers are updated
>>>> accordingly. Each specific handler will be updated on a need basis
>>>> when a new version of the resource type is added.
>>>>
>>>
>>> I really would prefer that we add additional types for the new
>>> structures, neither side will be compatible with new versions without
>>> enhancements to their respective implementations anyways.
>>
>> OK.
>>
>>>
>>>> An alternate way would be to introduce the new types as completely
>>>> new resource types which would require additional customization of
>>>> the resource handlers based on the 32-bit or 64-bit mode of a remote
>>>> processor, and introduction of an additional mode flag to the rproc
>>>> structure.
>>>>
>>>
>>> What would this "mode" indicate? If it's version 0 or 1?
>>
>> No, for indicating if the remoteproc is 32-bit or 64-bit and adjust the
>> loading handlers if the resource types need to be segregated accordingly.
>>
>
> Sorry, I think I'm misunderstanding something. Wouldn't your 64-bit
> remote processor need different firmware from your 32-bit processor
> anyways, if you want to support the wider resource? And you would pack
> your firmware with the appropriate resource types?
Yes, that's correct.
>
> Afaict the bit width of your remote processor, busses or memory is
> unrelated to the choice of number of bits used to express things in the
> resource table.
I would have to add the new resource type to the loading_handlers right,
so it is a question of whether we want to impose any restrictions in
remoteproc core or not from supporting a certain resource type (eg: I
don't expect RSC_TRACE entries on 64-bit processors).
regards
Suman
^ permalink raw reply
* Re: [PATCH v2] arm: dts: am33xx-bone-common: add gpio-line-names
From: Drew Fustini @ 2020-05-21 19:23 UTC (permalink / raw)
To: Grygorii Strashko
Cc: Robert Nelson, Linus Walleij, Benoît Cousson, Tony Lindgren,
Rob Herring, Linux-OMAP, devicetree, linux kernel, Jason Kridner
In-Reply-To: <cdf82f9c-48fb-49a0-99e4-926dc292c109@ti.com>
On Thu, May 21, 2020 at 08:26:42PM +0300, Grygorii Strashko wrote:
>
>
> On 21/05/2020 17:41, Robert Nelson wrote:
> > > Not sure if it should be in am335x-bone-common.dtsi.
> > >
> > > For example:
> > > am335x-boneblack.dts
> > > #include "am335x-bone-common.dtsi"
> > > #include "am335x-boneblack-common.dtsi" <-- hdmi defined only here
> >
> > Ah crap, yeah that's a good point.. So if we stick it in...
> > am335x-boneblack-common.dtsi
> >
> > Then the Black-Wireless now has Ethernet...
> >
> > am335x-boneblack-wireless.dts
> > #include "am335x-bone-common.dtsi"
> > #include "am335x-boneblack-common.dtsi"
> >
> > It's going to be ugly, copy and paste mess, but i guess we might as
> > well stick it in the device " am335x-boneblack.dts"?
>
> Seems like.
>
> Unfortunately, there is no way to partially overwrite "gpio-line-names"
> property.
>
> --
> Best regards,
> grygorii
Thank you for pointing out that those gpio-line-names are really
specific to the BeagleBone Black and not applicable for other boards
like the BeagleBone Green which lack eMMC.
I will post a new patch for am335x-boneblack.dts.
After that is reviewed and applied, I will then create patches for the
the other BeagleBone models.
thanks,
drew
^ permalink raw reply
* Re: [PATCH 2/4] remoteproc: introduce version element into resource type field
From: Bjorn Andersson @ 2020-05-21 19:21 UTC (permalink / raw)
To: Suman Anna
Cc: Rob Herring, Mathieu Poirier, Clement Leger, Loic Pallardy,
Arnaud Pouliquen, Lokesh Vutla, linux-remoteproc, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <b338480e-c586-f988-f5b6-784551b7beb6@ti.com>
On Thu 21 May 12:06 PDT 2020, Suman Anna wrote:
> Hi Bjorn,
>
> On 5/21/20 12:54 PM, Bjorn Andersson wrote:
> > On Wed 25 Mar 13:46 PDT 2020, Suman Anna wrote:
> >
> > > The current remoteproc core has supported only 32-bit remote
> > > processors and as such some of the current resource structures
> > > may not scale well for 64-bit remote processors, and would
> > > require new versions of resource types. Each resource is currently
> > > identified by a 32-bit type field. Introduce the concept of version
> > > for these resource types by overloading this 32-bit type field
> > > into two 16-bit version and type fields with the existing resources
> > > behaving as version 0 thereby providing backward compatibility.
> > >
> > > The version field is passed as an additional argument to each of
> > > the handler functions, and all the existing handlers are updated
> > > accordingly. Each specific handler will be updated on a need basis
> > > when a new version of the resource type is added.
> > >
> >
> > I really would prefer that we add additional types for the new
> > structures, neither side will be compatible with new versions without
> > enhancements to their respective implementations anyways.
>
> OK.
>
> >
> > > An alternate way would be to introduce the new types as completely
> > > new resource types which would require additional customization of
> > > the resource handlers based on the 32-bit or 64-bit mode of a remote
> > > processor, and introduction of an additional mode flag to the rproc
> > > structure.
> > >
> >
> > What would this "mode" indicate? If it's version 0 or 1?
>
> No, for indicating if the remoteproc is 32-bit or 64-bit and adjust the
> loading handlers if the resource types need to be segregated accordingly.
>
Sorry, I think I'm misunderstanding something. Wouldn't your 64-bit
remote processor need different firmware from your 32-bit processor
anyways, if you want to support the wider resource? And you would pack
your firmware with the appropriate resource types?
Afaict the bit width of your remote processor, busses or memory is
unrelated to the choice of number of bits used to express things in the
resource table.
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCH V2 2/3] mmc: sdhci-msm: Use internal voltage control
From: Bjorn Andersson @ 2020-05-21 19:07 UTC (permalink / raw)
To: Veerabhadrarao Badiganti
Cc: adrian.hunter, ulf.hansson, robh+dt, linux-mmc, linux-kernel,
linux-arm-msm, devicetree, Asutosh Das, Vijay Viswanath,
Andy Gross
In-Reply-To: <1590074615-10787-3-git-send-email-vbadigan@codeaurora.org>
On Thu 21 May 08:23 PDT 2020, Veerabhadrarao Badiganti wrote:
> On qcom SD host controllers voltage switching be done after the HW
> is ready for it. The HW informs its readiness through power irq.
> The voltage switching should happen only then.
>
> Use the internal voltage switching and then control the voltage
> switching using power irq.
>
> Set the regulator load as well so that regulator can be configured
> in LPM mode when in is not being used.
>
> Co-developed-by: Asutosh Das <asutoshd@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Co-developed-by: Vijay Viswanath <vviswana@codeaurora.org>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> Co-developed-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
Looks better, thanks.
> ---
> drivers/mmc/host/sdhci-msm.c | 207 +++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 198 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
[..]
> static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
> @@ -1298,6 +1302,71 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
> sdhci_msm_hs400(host, &mmc->ios);
> }
>
> +static int sdhci_msm_set_vmmc(struct mmc_host *mmc)
> +{
> + int ret;
> +
> + if (IS_ERR(mmc->supply.vmmc))
> + return 0;
> +
> + ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);
> + if (ret)
> + dev_err(mmc_dev(mmc), "%s: vmmc set ocr with vdd=%d failed: %d\n",
> + mmc_hostname(mmc), mmc->ios.vdd, ret);
Missed this one on v1, in the event that mmc_regulator_set_ocr() return
a non-zero value it has already printed an error message. So please
replace the tail with just:
return mmc_regulator_set_ocr(...);
> +
> + return ret;
> +}
> +
> +static int sdhci_msm_set_vqmmc(struct sdhci_msm_host *msm_host,
> + struct mmc_host *mmc, bool level)
> +{
> + int load, ret;
> + struct mmc_ios ios;
> +
> + if (IS_ERR(mmc->supply.vqmmc) ||
> + (mmc->ios.power_mode == MMC_POWER_UNDEFINED) ||
> + (msm_host->vqmmc_enabled == level))
> + return 0;
> +
> + if (msm_host->vqmmc_load) {
> + load = level ? msm_host->vqmmc_load : 0;
> + ret = regulator_set_load(mmc->supply.vqmmc, load);
Sorry for the late reply on v1, but please see my explanation regarding
load and always-on regulators there.
> + if (ret) {
> + dev_err(mmc_dev(mmc), "%s: vqmmc set load failed: %d\n",
> + mmc_hostname(mmc), ret);
> + goto out;
> + }
> + }
> +
> + if (level) {
> + /* Set the IO voltage regulator to default voltage level */
> + if (msm_host->caps_0 & CORE_3_0V_SUPPORT)
> + ios.signal_voltage = MMC_SIGNAL_VOLTAGE_330;
> + else if (msm_host->caps_0 & CORE_1_8V_SUPPORT)
> + ios.signal_voltage = MMC_SIGNAL_VOLTAGE_180;
> +
> + if (msm_host->caps_0 & CORE_VOLT_SUPPORT) {
> + ret = mmc_regulator_set_vqmmc(mmc, &ios);
> + if (ret < 0) {
> + dev_err(mmc_dev(mmc), "%s: vqmmc set volgate failed: %d\n",
> + mmc_hostname(mmc), ret);
> + goto out;
> + }
> + }
> + ret = regulator_enable(mmc->supply.vqmmc);
> + } else {
> + ret = regulator_disable(mmc->supply.vqmmc);
> + }
> +
> + if (ret)
> + dev_err(mmc_dev(mmc), "%s: vqmm %sable failed: %d\n",
> + mmc_hostname(mmc), level ? "en":"dis", ret);
> + else
> + msm_host->vqmmc_enabled = level;
> +out:
> + return ret;
> +}
[..]
> +static int sdhci_msm_start_signal_voltage_switch(struct mmc_host *mmc,
> + struct mmc_ios *ios)
> +{
> + struct sdhci_host *host = mmc_priv(mmc);
> + u16 ctrl, status;
> +
> + /*
> + * Signal Voltage Switching is only applicable for Host Controllers
> + * v3.00 and above.
> + */
> + if (host->version < SDHCI_SPEC_300)
> + return 0;
> +
> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> +
> + switch (ios->signal_voltage) {
> + case MMC_SIGNAL_VOLTAGE_330:
> + if (!(host->flags & SDHCI_SIGNALING_330))
> + return -EINVAL;
> +
> + /* Set 1.8V Signal Enable in the Host Control2 register to 0 */
> + ctrl &= ~SDHCI_CTRL_VDD_180;
> + break;
> + case MMC_SIGNAL_VOLTAGE_180:
> + if (!(host->flags & SDHCI_SIGNALING_180))
> + return -EINVAL;
> +
> + /*
> + * Enable 1.8V Signal Enable in the Host Control2
> + * register
> + */
> + ctrl |= SDHCI_CTRL_VDD_180;
> + break;
> + case MMC_SIGNAL_VOLTAGE_120:
> + if (!(host->flags & SDHCI_SIGNALING_120))
> + return -EINVAL;
> + return 0;
> + default:
> + /* No signal voltage switch required */
> + return 0;
> + }
> +
> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
> +
> + /* Wait for 5ms */
> + usleep_range(5000, 5500);
> +
> + /* regulator output should be stable within 5 ms */
> + status = !!(ctrl & SDHCI_CTRL_VDD_180);
> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> + if (!!(ctrl & SDHCI_CTRL_VDD_180) == status)
You should be able to drop the !! both here and when assigning status.
Overall this looks neater, thanks for reworking it.
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCH 2/4] remoteproc: introduce version element into resource type field
From: Suman Anna @ 2020-05-21 19:06 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Rob Herring, Mathieu Poirier, Clement Leger, Loic Pallardy,
Arnaud Pouliquen, Lokesh Vutla, linux-remoteproc, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <20200521175421.GI408178@builder.lan>
Hi Bjorn,
On 5/21/20 12:54 PM, Bjorn Andersson wrote:
> On Wed 25 Mar 13:46 PDT 2020, Suman Anna wrote:
>
>> The current remoteproc core has supported only 32-bit remote
>> processors and as such some of the current resource structures
>> may not scale well for 64-bit remote processors, and would
>> require new versions of resource types. Each resource is currently
>> identified by a 32-bit type field. Introduce the concept of version
>> for these resource types by overloading this 32-bit type field
>> into two 16-bit version and type fields with the existing resources
>> behaving as version 0 thereby providing backward compatibility.
>>
>> The version field is passed as an additional argument to each of
>> the handler functions, and all the existing handlers are updated
>> accordingly. Each specific handler will be updated on a need basis
>> when a new version of the resource type is added.
>>
>
> I really would prefer that we add additional types for the new
> structures, neither side will be compatible with new versions without
> enhancements to their respective implementations anyways.
OK.
>
>> An alternate way would be to introduce the new types as completely
>> new resource types which would require additional customization of
>> the resource handlers based on the 32-bit or 64-bit mode of a remote
>> processor, and introduction of an additional mode flag to the rproc
>> structure.
>>
>
> What would this "mode" indicate? If it's version 0 or 1?
No, for indicating if the remoteproc is 32-bit or 64-bit and adjust the
loading handlers if the resource types need to be segregated accordingly.
>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 25 +++++++++++++++----------
>> drivers/remoteproc/remoteproc_debugfs.c | 17 ++++++++++-------
>> include/linux/remoteproc.h | 8 +++++++-
>> 3 files changed, 32 insertions(+), 18 deletions(-)
>>
> [..]
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 77788a4bb94e..526d3cb45e37 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -86,7 +86,13 @@ struct resource_table {
>> * this header, and it should be parsed according to the resource type.
>> */
>> struct fw_rsc_hdr {
>> - u32 type;
>> + union {
>> + u32 type;
>> + struct {
>> + u16 t;
>> + u16 v;
>> + } st;
>
> I see your "type" is little endian...
Yeah, definitely a draw-back if we want to support big-endian rprocs. Do
you have any remoteprocs following big-endian? All TI remoteprocs are
little-endian except for really old ones.
regards
Suman
^ permalink raw reply
* Re: [PATCH v2 0/4] TI K3 DSP remoteproc driver for C66x DSPs
From: Bjorn Andersson @ 2020-05-21 19:01 UTC (permalink / raw)
To: Suman Anna
Cc: Rob Herring, Mathieu Poirier, Lokesh Vutla, linux-remoteproc,
devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <5f84465e-8f63-51b4-4758-59c85d3ad597@ti.com>
On Thu 21 May 11:59 PDT 2020, Suman Anna wrote:
> Hi Bjorn,
>
> On 5/20/20 7:10 PM, Suman Anna wrote:
> > Hi All,
> >
> > The following is v2 of the K3 DSP remoteproc driver supporting the C66x DSPs
> > on the TI K3 J721E SoCs. The patches are based on the latest commit on the
> > rproc-next branch, 7dcef3988eed ("remoteproc: Fix an error code in
> > devm_rproc_alloc()").
>
> I realized I also had the R5F patches on my branch, so the third patch won't
> apply cleanly (conflict on Makefile). Let me know if you want a new revision
> posted for you to pick up the series.
>
That should be fine, thanks for the heads up!
Will give Mathieu a day or two to take a look as well.
Regards,
Bjorn
> regards
> Suman
>
> >
> > v2 includes a new remoteproc core patch (patch 1) that adds an OF helper
> > for parsing the firmware-name property. This is refactored out to avoid
> > replicating the code in various remoteproc drivers. Please see the
> > individual patches for detailed changes.
> >
> > The main dependent patches from the previous series are now staged in
> > rproc-next branch. The only dependency for this series is the common
> > ti-sci-proc helper between R5 and DSP drivers [1]. Please see the initial
> > cover-letter [2] for v1 details.
> >
> > regards
> > Suman
> >
> > [1] https://patchwork.kernel.org/patch/11456379/
> > [2] https://patchwork.kernel.org/cover/11458573/
> >
> > Suman Anna (4):
> > remoteproc: Introduce rproc_of_parse_firmware() helper
> > dt-bindings: remoteproc: Add bindings for C66x DSPs on TI K3 SoCs
> > remoteproc/k3-dsp: Add a remoteproc driver of K3 C66x DSPs
> > remoteproc/k3-dsp: Add support for L2RAM loading on C66x DSPs
> >
> > .../bindings/remoteproc/ti,k3-dsp-rproc.yaml | 190 +++++
> > drivers/remoteproc/Kconfig | 13 +
> > drivers/remoteproc/Makefile | 1 +
> > drivers/remoteproc/remoteproc_core.c | 23 +
> > drivers/remoteproc/remoteproc_internal.h | 2 +
> > drivers/remoteproc/ti_k3_dsp_remoteproc.c | 773 ++++++++++++++++++
> > 6 files changed, 1002 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/remoteproc/ti,k3-dsp-rproc.yaml
> > create mode 100644 drivers/remoteproc/ti_k3_dsp_remoteproc.c
> >
>
^ permalink raw reply
* Re: [V8, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings
From: Tomasz Figa @ 2020-05-21 18:59 UTC (permalink / raw)
To: Dongchun Zhu, Rob Herring
Cc: Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab,
Andy Shevchenko, Mark Rutland, Sakari Ailus, Nicolas Boichat,
Matthias Brugger, Cao Bing Bu, srv_heupstream,
moderated list:ARM/Mediatek SoC support,
list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>,,
Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo,
Shengnan Wang (王圣男)
In-Reply-To: <1589251221.8804.289.camel@mhfsdcap03>
Hi Dongchun, Rob,
On Tue, May 12, 2020 at 4:41 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
>
> Hi Rob,
>
> Thanks for the review.
>
> On Mon, 2020-05-11 at 11:02 -0500, Rob Herring wrote:
> > On Sat, May 09, 2020 at 04:06:26PM +0800, Dongchun Zhu wrote:
[snip]
> > > + port {
> > > + wcam_out: endpoint {
> > > + remote-endpoint = <&mipi_in_wcam>;
> > > + data-lanes = <1>;
> >
> > This doesn't match the schema which says this should be 4 entries.
> >
>
> Property "data-lanes" shows the number of lanes that sensor supports.
> If this property is omitted, four-lane operation is assumed.
> For OV02A10, it is one-lane operation, so the property is supposed to be
> set to <1>.
>
To clarify on this, the ov02a10 sensor supports only 1 lane. It's not
a driver limitation.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH v2 0/4] TI K3 DSP remoteproc driver for C66x DSPs
From: Suman Anna @ 2020-05-21 18:59 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring, Mathieu Poirier
Cc: Lokesh Vutla, linux-remoteproc, devicetree, linux-arm-kernel,
linux-kernel
In-Reply-To: <20200521001006.2725-1-s-anna@ti.com>
Hi Bjorn,
On 5/20/20 7:10 PM, Suman Anna wrote:
> Hi All,
>
> The following is v2 of the K3 DSP remoteproc driver supporting the C66x DSPs
> on the TI K3 J721E SoCs. The patches are based on the latest commit on the
> rproc-next branch, 7dcef3988eed ("remoteproc: Fix an error code in
> devm_rproc_alloc()").
I realized I also had the R5F patches on my branch, so the third patch
won't apply cleanly (conflict on Makefile). Let me know if you want a
new revision posted for you to pick up the series.
regards
Suman
>
> v2 includes a new remoteproc core patch (patch 1) that adds an OF helper
> for parsing the firmware-name property. This is refactored out to avoid
> replicating the code in various remoteproc drivers. Please see the
> individual patches for detailed changes.
>
> The main dependent patches from the previous series are now staged in
> rproc-next branch. The only dependency for this series is the common
> ti-sci-proc helper between R5 and DSP drivers [1]. Please see the initial
> cover-letter [2] for v1 details.
>
> regards
> Suman
>
> [1] https://patchwork.kernel.org/patch/11456379/
> [2] https://patchwork.kernel.org/cover/11458573/
>
> Suman Anna (4):
> remoteproc: Introduce rproc_of_parse_firmware() helper
> dt-bindings: remoteproc: Add bindings for C66x DSPs on TI K3 SoCs
> remoteproc/k3-dsp: Add a remoteproc driver of K3 C66x DSPs
> remoteproc/k3-dsp: Add support for L2RAM loading on C66x DSPs
>
> .../bindings/remoteproc/ti,k3-dsp-rproc.yaml | 190 +++++
> drivers/remoteproc/Kconfig | 13 +
> drivers/remoteproc/Makefile | 1 +
> drivers/remoteproc/remoteproc_core.c | 23 +
> drivers/remoteproc/remoteproc_internal.h | 2 +
> drivers/remoteproc/ti_k3_dsp_remoteproc.c | 773 ++++++++++++++++++
> 6 files changed, 1002 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/remoteproc/ti,k3-dsp-rproc.yaml
> create mode 100644 drivers/remoteproc/ti_k3_dsp_remoteproc.c
>
^ permalink raw reply
* Re: [PATCH v2 4/4] remoteproc/k3-dsp: Add support for L2RAM loading on C66x DSPs
From: Bjorn Andersson @ 2020-05-21 18:48 UTC (permalink / raw)
To: Suman Anna
Cc: Rob Herring, Mathieu Poirier, Lokesh Vutla, linux-remoteproc,
devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20200521001006.2725-5-s-anna@ti.com>
On Wed 20 May 17:10 PDT 2020, Suman Anna wrote:
> The resets for the DSP processors on K3 SoCs are managed through the
> Power and Sleep Controller (PSC) module. Each DSP typically has two
> resets - a global module reset for powering on the device, and a local
> reset that affects only the CPU while allowing access to the other
> sub-modules within the DSP processor sub-systems.
>
> The C66x DSPs have two levels of internal RAMs that can be used to
> boot from, and the firmware loading into these RAMs require the
> local reset to be asserted with the device powered on/enabled using
> the module reset. Enhance the K3 DSP remoteproc driver to add support
> for loading into the internal RAMs. The local reset is deasserted on
> SoC power-on-reset, so logic has to be added in probe in remoteproc
> mode to balance the remoteproc state-machine.
>
> Note that the local resets are a no-op on C71x cores, and the hardware
> does not supporting loading into its internal RAMs.
>
> Signed-off-by: Suman Anna <s-anna@ti.com>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
> ---
> v2:
> - Dropped the local-reset no-op checks from k3_dsp_rproc_prepare/unprepare()
> callbacks. The logic will be adjusted back in the C71 patch series.
> - The C71 local reset references are also removed from the comments for the
> k3_dsp_rproc_prepare() function.
> v1: https://patchwork.kernel.org/patch/11458579/
>
> drivers/remoteproc/ti_k3_dsp_remoteproc.c | 72 +++++++++++++++++++++++
> 1 file changed, 72 insertions(+)
>
> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> index e4036f5992fe..610fbbf85ee6 100644
> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> @@ -174,6 +174,9 @@ static int k3_dsp_rproc_reset(struct k3_dsp_rproc *kproc)
> return ret;
> }
>
> + if (kproc->data->uses_lreset)
> + return ret;
> +
> ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
> kproc->ti_sci_id);
> if (ret) {
> @@ -191,6 +194,9 @@ static int k3_dsp_rproc_release(struct k3_dsp_rproc *kproc)
> struct device *dev = kproc->dev;
> int ret;
>
> + if (kproc->data->uses_lreset)
> + goto lreset;
> +
> ret = kproc->ti_sci->ops.dev_ops.get_device(kproc->ti_sci,
> kproc->ti_sci_id);
> if (ret) {
> @@ -198,6 +204,7 @@ static int k3_dsp_rproc_release(struct k3_dsp_rproc *kproc)
> return ret;
> }
>
> +lreset:
> ret = reset_control_deassert(kproc->reset);
> if (ret) {
> dev_err(dev, "local-reset deassert failed, ret = %d\n", ret);
> @@ -209,6 +216,53 @@ static int k3_dsp_rproc_release(struct k3_dsp_rproc *kproc)
> return ret;
> }
>
> +/*
> + * The C66x DSP cores have a local reset that affects only the CPU, and a
> + * generic module reset that powers on the device and allows the DSP internal
> + * memories to be accessed while the local reset is asserted. This function is
> + * used to release the global reset on C66x DSPs to allow loading into the DSP
> + * internal RAMs. The .prepare() ops is invoked by remoteproc core before any
> + * firmware loading, and is followed by the .start() ops after loading to
> + * actually let the C66x DSP cores run.
> + */
> +static int k3_dsp_rproc_prepare(struct rproc *rproc)
> +{
> + struct k3_dsp_rproc *kproc = rproc->priv;
> + struct device *dev = kproc->dev;
> + int ret;
> +
> + ret = kproc->ti_sci->ops.dev_ops.get_device(kproc->ti_sci,
> + kproc->ti_sci_id);
> + if (ret)
> + dev_err(dev, "module-reset deassert failed, cannot enable internal RAM loading, ret = %d\n",
> + ret);
> +
> + return ret;
> +}
> +
> +/*
> + * This function implements the .unprepare() ops and performs the complimentary
> + * operations to that of the .prepare() ops. The function is used to assert the
> + * global reset on applicable C66x cores. This completes the second portion of
> + * powering down the C66x DSP cores. The cores themselves are only halted in the
> + * .stop() callback through the local reset, and the .unprepare() ops is invoked
> + * by the remoteproc core after the remoteproc is stopped to balance the global
> + * reset.
> + */
> +static int k3_dsp_rproc_unprepare(struct rproc *rproc)
> +{
> + struct k3_dsp_rproc *kproc = rproc->priv;
> + struct device *dev = kproc->dev;
> + int ret;
> +
> + ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
> + kproc->ti_sci_id);
> + if (ret)
> + dev_err(dev, "module-reset assert failed, ret = %d\n", ret);
> +
> + return ret;
> +}
> +
> /*
> * Power up the DSP remote processor.
> *
> @@ -352,6 +406,8 @@ static void *k3_dsp_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
> }
>
> static const struct rproc_ops k3_dsp_rproc_ops = {
> + .prepare = k3_dsp_rproc_prepare,
> + .unprepare = k3_dsp_rproc_unprepare,
> .start = k3_dsp_rproc_start,
> .stop = k3_dsp_rproc_stop,
> .kick = k3_dsp_rproc_kick,
> @@ -614,6 +670,22 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev)
> goto release_tsp;
> }
>
> + /*
> + * ensure the DSP local reset is asserted to ensure the DSP doesn't
> + * execute bogus code in .prepare() when the module reset is released.
> + */
> + if (data->uses_lreset) {
> + ret = reset_control_status(kproc->reset);
> + if (ret < 0) {
> + dev_err(dev, "failed to get reset status, status = %d\n",
> + ret);
> + goto release_mem;
> + } else if (ret == 0) {
> + dev_warn(dev, "local reset is deasserted for device\n");
> + k3_dsp_rproc_reset(kproc);
> + }
> + }
> +
> ret = rproc_add(rproc);
> if (ret) {
> dev_err(dev, "failed to add register device with remoteproc core, status = %d\n",
> --
> 2.26.0
>
^ permalink raw reply
* Re: [PATCH v2 1/4] remoteproc: Introduce rproc_of_parse_firmware() helper
From: Bjorn Andersson @ 2020-05-21 18:44 UTC (permalink / raw)
To: Suman Anna
Cc: Rob Herring, Mathieu Poirier, Lokesh Vutla, linux-remoteproc,
devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20200521001006.2725-2-s-anna@ti.com>
On Wed 20 May 17:10 PDT 2020, Suman Anna wrote:
> Add a new helper function rproc_of_parse_firmware() to the remoteproc
> core that can be used by various remoteproc drivers to look up the
> the "firmware-name" property from a rproc device node. This property
> is already being used by multiple drivers, so this helper can avoid
> repeating equivalent code in remoteproc drivers.
>
> Signed-off-by: Suman Anna <s-anna@ti.com>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
> ---
> v2: New patch
>
> drivers/remoteproc/remoteproc_core.c | 23 +++++++++++++++++++++++
> drivers/remoteproc/remoteproc_internal.h | 2 ++
> 2 files changed, 25 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 9f04c30c4aaf..c458b218d524 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1034,6 +1034,29 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, size_t len,
> }
> EXPORT_SYMBOL(rproc_of_resm_mem_entry_init);
>
> +/**
> + * rproc_of_parse_firmware() - parse and return the firmware-name
> + * @dev: pointer on device struct representing a rproc
> + * @index: index to use for the firmware-name retrieval
> + * @fw_name: pointer to a character string, in which the firmware
> + * name is returned on success and unmodified otherwise.
> + *
> + * This is an OF helper function that parses a device's DT node for
> + * the "firmware-name" property and returns the firmware name pointer
> + * in @fw_name on success.
> + *
> + * Return: 0 on success, or an appropriate failure.
> + */
> +int rproc_of_parse_firmware(struct device *dev, int index, const char **fw_name)
> +{
> + int ret;
> +
> + ret = of_property_read_string_index(dev->of_node, "firmware-name",
> + index, fw_name);
> + return ret ? ret : 0;
> +}
> +EXPORT_SYMBOL(rproc_of_parse_firmware);
> +
> /*
> * A lookup table for resource handlers. The indices are defined in
> * enum fw_resource_type.
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 4ba7cb59d3e8..e5341e91d2fc 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -28,6 +28,8 @@ struct rproc_debug_trace {
> void rproc_release(struct kref *kref);
> irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
> void rproc_vdev_release(struct kref *ref);
> +int rproc_of_parse_firmware(struct device *dev, int index,
> + const char **fw_name);
>
> /* from remoteproc_virtio.c */
> int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id);
> --
> 2.26.0
>
^ permalink raw reply
* Re: [RFC PATCH V4 0/4] media: platform: Add support for Face Detection (FD) on mt8183 SoC
From: Tomasz Figa @ 2020-05-21 18:38 UTC (permalink / raw)
To: Jerry-ch Chen
Cc: hans.verkuil@cisco.com, laurent.pinchart+renesas@ideasonboard.com,
matthias.bgg@gmail.com, mchehab@kernel.org, pihsun@chromium.org,
yuzhao@chromium.org, zwisler@chromium.org,
linux-mediatek@lists.infradead.org,
linux-arm-kernel@lists.infradead.org,
Sean Cheng (鄭昇弘),
Sj Huang (黃信璋),
Christie Yu (游雅惠),
Frederic Chen (陳俊元),
Jungo Lin (林明俊), linux-media@vger.kernel.org,
srv_heupstream, devicetree@vger.kernel.org
In-Reply-To: <CAAFQd5CP+gH3zG9fejBv_hTzeAExMoY+L38W8e4e3OSc-gVRHg@mail.gmail.com>
Hi Jerry,
On Wed, May 13, 2020 at 11:45:37PM +0200, Tomasz Figa wrote:
> Hi Jerry,
>
> On Fri, May 8, 2020 at 4:03 AM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> >
> > Hi Laurent, Tomasz, Matthias,
> >
> > gentle ping for this patch set,
> > If no new comments, I would like to send a newer version.
> >
>
> Sorry, I still haven't had a chance to look at the series, so feel
> free to send a new version and I will take a look at the new one.
>
Finally found some time to review the series. Again sorry for the delay
and thanks for your patience.
Some general comments:
1) The metadata format FourCC should be added in a separate patch,
together with documentation for it.
2) Control IDs, structs used by the userspace, etc. should be defined in
a header under include/uapi/linux.
Please also check my replies to particular patches for further comments.
Best regards,
Tomasz
^ permalink raw reply
* [PATCH 3/3] spi: rb4xx: add corresponding device tree documentation
From: Christopher Hill @ 2020-05-21 18:36 UTC (permalink / raw)
To: Mark Brown
Cc: Christopher Hill, Rob Herring, Sam Ravnborg, Maxime Ripard,
Heiko Stuebner, Linus Walleij, Stephan Gerhold, linux-spi,
devicetree, linux-kernel
In-Reply-To: <20200521183631.37806-1-ch6574@gmail.com>
This patch adds the correcsponding MikroTik vendor and device tree
documentation
Signed-off-by: Christopher Hill <ch6574@gmail.com>
---
.../bindings/spi/mikrotik,rb4xx-spi.yaml | 36 +++++++++++++++++++
.../devicetree/bindings/vendor-prefixes.yaml | 2 ++
2 files changed, 38 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/mikrotik,rb4xx-spi.yaml
diff --git a/Documentation/devicetree/bindings/spi/mikrotik,rb4xx-spi.yaml b/Documentation/devicetree/bindings/spi/mikrotik,rb4xx-spi.yaml
new file mode 100644
index 000000000000..4ddb42a4ae05
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/mikrotik,rb4xx-spi.yaml
@@ -0,0 +1,36 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/mikrotik,rb4xx-spi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MikroTik RB4xx series SPI master
+
+maintainers:
+ - Gabor Juhos <juhosg@openwrt.org>
+ - Bert Vermeulen <bert@biot.com>
+
+allOf:
+ - $ref: "spi-controller.yaml#"
+
+properties:
+ compatible:
+ const: mikrotik,rb4xx-spi
+
+ reg:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+examples:
+ - |
+ spi: spi@1f000000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "mikrotik,rb4xx-spi";
+ reg = <0x1f000000 0x10>;
+ };
+
+...
\ No newline at end of file
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index d3891386d671..d3277fe6640b 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -633,6 +633,8 @@ patternProperties:
description: Microsoft Corporation
"^mikroe,.*":
description: MikroElektronika d.o.o.
+ "^mikrotik,.*":
+ description: MikroTik
"^miniand,.*":
description: Miniand Tech
"^minix,.*":
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v2 3/4] remoteproc/k3-dsp: Add a remoteproc driver of K3 C66x DSPs
From: Bjorn Andersson @ 2020-05-21 18:34 UTC (permalink / raw)
To: Suman Anna
Cc: Rob Herring, Mathieu Poirier, Lokesh Vutla, linux-remoteproc,
devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20200521001006.2725-4-s-anna@ti.com>
On Wed 20 May 17:10 PDT 2020, Suman Anna wrote:
> The Texas Instrument's K3 J721E SoCs have two C66x DSP Subsystems in MAIN
> voltage domain that are based on the TI's standard TMS320C66x DSP CorePac
> module. Each subsystem has a Fixed/Floating-Point DSP CPU, with 32 KB each
> of L1P & L1D SRAMs that can be configured and partitioned as either RAM
> and/or Cache, and 288 KB of L2 SRAM with 256 KB of memory configurable as
> either RAM and/or Cache. The CorePac also includes an Internal DMA (IDMA),
> External Memory Controller (EMC), Extended Memory Controller (XMC) with a
> Region Address Translator (RAT) unit for 32-bit to 48-bit address
> extension/translations, an Interrupt Controller (INTC) and a Powerdown
> Controller (PDC).
>
> A new remoteproc module is added to perform the device management of
> these DSP devices. The support is limited to images using only external
> DDR memory at the moment, the loading support to internal memories and
> any on-chip RAM memories will be added in a subsequent patch. RAT support
> is also left for a future patch, and as such the reserved memory carveout
> regions are all expected to be using memory regions within the first 2 GB.
> Error Recovery and Power Management features are not currently supported.
>
> The C66x remote processors do not have an MMU, and so require fixed memory
> carveout regions matching the firmware image addresses. Support for this
> is provided by mandating multiple memory regions to be attached to the
> remoteproc device. The first memory region will be used to serve as the
> DMA pool for all dynamic allocations like the vrings and vring buffers.
> The remaining memory regions are mapped into the kernel at device probe
> time, and are used to provide address translations for firmware image
> segments without the need for any RSC_CARVEOUT entries. Any firmware
> image using memory outside of the supplied reserved memory carveout
> regions will be errored out.
>
> The driver uses various TI-SCI interfaces to talk to the System Controller
> (DMSC) for managing configuration, power and reset management of these
> cores. IPC between the A72 cores and the DSP cores is supported through
> the virtio rpmsg stack using shared memory and OMAP Mailboxes.
>
There's room for more devm_ usage, but this looks good to me.
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
> v2:
> - Dropped the pm_runtime usage
> - Replaced the private k3_dsp_rproc_get_firmware() with the newly introduced
> rproc_of_parse_firmware()
> - Addressed other minor comments from Mathieu - Revised help on Kconfig, reordered
> header files, whitespace indentation fixes, remove the stale memset comment on
> internal memories, renamed struct k3_dsp_rproc_mem to struct k3_dsp_mem.
> v1: https://patchwork.kernel.org/patch/11458577/
>
> drivers/remoteproc/Kconfig | 13 +
> drivers/remoteproc/Makefile | 1 +
> drivers/remoteproc/ti_k3_dsp_remoteproc.c | 701 ++++++++++++++++++++++
> 3 files changed, 715 insertions(+)
> create mode 100644 drivers/remoteproc/ti_k3_dsp_remoteproc.c
>
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 2c9fb870a276..ef787774b52b 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -265,6 +265,19 @@ config TI_K3_R5_REMOTEPROC
> It's safe to say N here if you're not interested in utilizing
> a slave processor
>
> +config TI_K3_DSP_REMOTEPROC
> + tristate "TI K3 DSP remoteproc support"
> + depends on ARCH_K3
> + select MAILBOX
> + select OMAP2PLUS_MBOX
> + help
> + Say m here to support TI's C66x and C71x DSP remote processor
> + subsystems on various TI K3 family of SoCs through the remote
> + processor framework.
> +
> + It's safe to say N here if you're not interested in utilizing
> + the DSP slave processors.
> +
> endif # REMOTEPROC
>
> endmenu
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index ea0c6812e4fc..9f87d222744c 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -31,3 +31,4 @@ obj-$(CONFIG_ST_REMOTEPROC) += st_remoteproc.o
> obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o
> obj-$(CONFIG_STM32_RPROC) += stm32_rproc.o
> obj-$(CONFIG_TI_K3_R5_REMOTEPROC) += ti_k3_r5_remoteproc.o
> +obj-$(CONFIG_TI_K3_DSP_REMOTEPROC) += ti_k3_dsp_remoteproc.o
> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> new file mode 100644
> index 000000000000..e4036f5992fe
> --- /dev/null
> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> @@ -0,0 +1,701 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * TI K3 DSP Remote Processor(s) driver
> + *
> + * Copyright (C) 2018-2020 Texas Instruments Incorporated - http://www.ti.com/
> + * Suman Anna <s-anna@ti.com>
> + */
> +
> +#include <linux/io.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/omap-mailbox.h>
> +#include <linux/platform_device.h>
> +#include <linux/remoteproc.h>
> +#include <linux/reset.h>
> +#include <linux/soc/ti/ti_sci_protocol.h>
> +
> +#include "omap_remoteproc.h"
> +#include "remoteproc_internal.h"
> +#include "ti_sci_proc.h"
> +
> +#define KEYSTONE_RPROC_LOCAL_ADDRESS_MASK (SZ_16M - 1)
> +
> +/**
> + * struct k3_dsp_mem - internal memory structure
> + * @cpu_addr: MPU virtual address of the memory region
> + * @bus_addr: Bus address used to access the memory region
> + * @dev_addr: Device address of the memory region from DSP view
> + * @size: Size of the memory region
> + */
> +struct k3_dsp_mem {
> + void __iomem *cpu_addr;
> + phys_addr_t bus_addr;
> + u32 dev_addr;
> + size_t size;
> +};
> +
> +/**
> + * struct k3_dsp_mem_data - memory definitions for a DSP
> + * @name: name for this memory entry
> + * @dev_addr: device address for the memory entry
> + */
> +struct k3_dsp_mem_data {
> + const char *name;
> + const u32 dev_addr;
> +};
> +
> +/**
> + * struct k3_dsp_dev_data - device data structure for a DSP
> + * @mems: pointer to memory definitions for a DSP
> + * @num_mems: number of memory regions in @mems
> + * @boot_align_addr: boot vector address alignment granularity
> + * @uses_lreset: flag to denote the need for local reset management
> + */
> +struct k3_dsp_dev_data {
> + const struct k3_dsp_mem_data *mems;
> + u32 num_mems;
> + u32 boot_align_addr;
> + bool uses_lreset;
> +};
> +
> +/**
> + * struct k3_dsp_rproc - k3 DSP remote processor driver structure
> + * @dev: cached device pointer
> + * @rproc: remoteproc device handle
> + * @mem: internal memory regions data
> + * @num_mems: number of internal memory regions
> + * @rmem: reserved memory regions data
> + * @num_rmems: number of reserved memory regions
> + * @reset: reset control handle
> + * @data: pointer to DSP-specific device data
> + * @tsp: TI-SCI processor control handle
> + * @ti_sci: TI-SCI handle
> + * @ti_sci_id: TI-SCI device identifier
> + * @mbox: mailbox channel handle
> + * @client: mailbox client to request the mailbox channel
> + */
> +struct k3_dsp_rproc {
> + struct device *dev;
> + struct rproc *rproc;
> + struct k3_dsp_mem *mem;
> + int num_mems;
> + struct k3_dsp_mem *rmem;
> + int num_rmems;
> + struct reset_control *reset;
> + const struct k3_dsp_dev_data *data;
> + struct ti_sci_proc *tsp;
> + const struct ti_sci_handle *ti_sci;
> + u32 ti_sci_id;
> + struct mbox_chan *mbox;
> + struct mbox_client client;
> +};
> +
> +/**
> + * k3_dsp_rproc_mbox_callback() - inbound mailbox message handler
> + * @client: mailbox client pointer used for requesting the mailbox channel
> + * @data: mailbox payload
> + *
> + * This handler is invoked by the OMAP mailbox driver whenever a mailbox
> + * message is received. Usually, the mailbox payload simply contains
> + * the index of the virtqueue that is kicked by the remote processor,
> + * and we let remoteproc core handle it.
> + *
> + * In addition to virtqueue indices, we also have some out-of-band values
> + * that indicate different events. Those values are deliberately very
> + * large so they don't coincide with virtqueue indices.
> + */
> +static void k3_dsp_rproc_mbox_callback(struct mbox_client *client, void *data)
> +{
> + struct k3_dsp_rproc *kproc = container_of(client, struct k3_dsp_rproc,
> + client);
> + struct device *dev = kproc->rproc->dev.parent;
> + const char *name = kproc->rproc->name;
> + u32 msg = omap_mbox_message(data);
> +
> + dev_dbg(dev, "mbox msg: 0x%x\n", msg);
> +
> + switch (msg) {
> + case RP_MBOX_CRASH:
> + /*
> + * remoteproc detected an exception, but error recovery is not
> + * supported. So, just log this for now
> + */
> + dev_err(dev, "K3 DSP rproc %s crashed\n", name);
> + break;
> + case RP_MBOX_ECHO_REPLY:
> + dev_info(dev, "received echo reply from %s\n", name);
> + break;
> + default:
> + /* silently handle all other valid messages */
> + if (msg >= RP_MBOX_READY && msg < RP_MBOX_END_MSG)
> + return;
> + if (msg > kproc->rproc->max_notifyid) {
> + dev_dbg(dev, "dropping unknown message 0x%x", msg);
> + return;
> + }
> + /* msg contains the index of the triggered vring */
> + if (rproc_vq_interrupt(kproc->rproc, msg) == IRQ_NONE)
> + dev_dbg(dev, "no message was found in vqid %d\n", msg);
> + }
> +}
> +
> +/*
> + * Kick the remote processor to notify about pending unprocessed messages.
> + * The vqid usage is not used and is inconsequential, as the kick is performed
> + * through a simulated GPIO (a bit in an IPC interrupt-triggering register),
> + * the remote processor is expected to process both its Tx and Rx virtqueues.
> + */
> +static void k3_dsp_rproc_kick(struct rproc *rproc, int vqid)
> +{
> + struct k3_dsp_rproc *kproc = rproc->priv;
> + struct device *dev = rproc->dev.parent;
> + mbox_msg_t msg = (mbox_msg_t)vqid;
> + int ret;
> +
> + /* send the index of the triggered virtqueue in the mailbox payload */
> + ret = mbox_send_message(kproc->mbox, (void *)msg);
> + if (ret < 0)
> + dev_err(dev, "failed to send mailbox message, status = %d\n",
> + ret);
> +}
> +
> +/* Put the DSP processor into reset */
> +static int k3_dsp_rproc_reset(struct k3_dsp_rproc *kproc)
> +{
> + struct device *dev = kproc->dev;
> + int ret;
> +
> + ret = reset_control_assert(kproc->reset);
> + if (ret) {
> + dev_err(dev, "local-reset assert failed, ret = %d\n", ret);
> + return ret;
> + }
> +
> + ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
> + kproc->ti_sci_id);
> + if (ret) {
> + dev_err(dev, "module-reset assert failed, ret = %d\n", ret);
> + if (reset_control_deassert(kproc->reset))
> + dev_warn(dev, "local-reset deassert back failed\n");
> + }
> +
> + return ret;
> +}
> +
> +/* Release the DSP processor from reset */
> +static int k3_dsp_rproc_release(struct k3_dsp_rproc *kproc)
> +{
> + struct device *dev = kproc->dev;
> + int ret;
> +
> + ret = kproc->ti_sci->ops.dev_ops.get_device(kproc->ti_sci,
> + kproc->ti_sci_id);
> + if (ret) {
> + dev_err(dev, "module-reset deassert failed, ret = %d\n", ret);
> + return ret;
> + }
> +
> + ret = reset_control_deassert(kproc->reset);
> + if (ret) {
> + dev_err(dev, "local-reset deassert failed, ret = %d\n", ret);
> + if (kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
> + kproc->ti_sci_id))
> + dev_warn(dev, "module-reset assert back failed\n");
> + }
> +
> + return ret;
> +}
> +
> +/*
> + * Power up the DSP remote processor.
> + *
> + * This function will be invoked only after the firmware for this rproc
> + * was loaded, parsed successfully, and all of its resource requirements
> + * were met.
> + */
> +static int k3_dsp_rproc_start(struct rproc *rproc)
> +{
> + struct k3_dsp_rproc *kproc = rproc->priv;
> + struct mbox_client *client = &kproc->client;
> + struct device *dev = kproc->dev;
> + u32 boot_addr;
> + int ret;
> +
> + client->dev = dev;
> + client->tx_done = NULL;
> + client->rx_callback = k3_dsp_rproc_mbox_callback;
> + client->tx_block = false;
> + client->knows_txdone = false;
> +
> + kproc->mbox = mbox_request_channel(client, 0);
> + if (IS_ERR(kproc->mbox)) {
> + ret = -EBUSY;
> + dev_err(dev, "mbox_request_channel failed: %ld\n",
> + PTR_ERR(kproc->mbox));
> + return ret;
> + }
> +
> + /*
> + * Ping the remote processor, this is only for sanity-sake for now;
> + * there is no functional effect whatsoever.
> + *
> + * Note that the reply will _not_ arrive immediately: this message
> + * will wait in the mailbox fifo until the remote processor is booted.
> + */
> + ret = mbox_send_message(kproc->mbox, (void *)RP_MBOX_ECHO_REQUEST);
> + if (ret < 0) {
> + dev_err(dev, "mbox_send_message failed: %d\n", ret);
> + goto put_mbox;
> + }
> +
> + boot_addr = rproc->bootaddr;
> + if (boot_addr & (kproc->data->boot_align_addr - 1)) {
> + dev_err(dev, "invalid boot address 0x%x, must be aligned on a 0x%x boundary\n",
> + boot_addr, kproc->data->boot_align_addr);
> + ret = -EINVAL;
> + goto put_mbox;
> + }
> +
> + dev_err(dev, "booting DSP core using boot addr = 0x%x\n", boot_addr);
> + ret = ti_sci_proc_set_config(kproc->tsp, boot_addr, 0, 0);
> + if (ret)
> + goto put_mbox;
> +
> + ret = k3_dsp_rproc_release(kproc);
> + if (ret)
> + goto put_mbox;
> +
> + return 0;
> +
> +put_mbox:
> + mbox_free_channel(kproc->mbox);
> + return ret;
> +}
> +
> +/*
> + * Stop the DSP remote processor.
> + *
> + * This function puts the DSP processor into reset, and finishes processing
> + * of any pending messages.
> + */
> +static int k3_dsp_rproc_stop(struct rproc *rproc)
> +{
> + struct k3_dsp_rproc *kproc = rproc->priv;
> +
> + mbox_free_channel(kproc->mbox);
> +
> + k3_dsp_rproc_reset(kproc);
> +
> + return 0;
> +}
> +
> +/*
> + * Custom function to translate a DSP device address (internal RAMs only) to a
> + * kernel virtual address. The DSPs can access their RAMs at either an internal
> + * address visible only from a DSP, or at the SoC-level bus address. Both these
> + * addresses need to be looked through for translation. The translated addresses
> + * can be used either by the remoteproc core for loading (when using kernel
> + * remoteproc loader), or by any rpmsg bus drivers.
> + */
> +static void *k3_dsp_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
> +{
> + struct k3_dsp_rproc *kproc = rproc->priv;
> + void __iomem *va = NULL;
> + phys_addr_t bus_addr;
> + u32 dev_addr, offset;
> + size_t size;
> + int i;
> +
> + if (len == 0)
> + return NULL;
> +
> + for (i = 0; i < kproc->num_mems; i++) {
> + bus_addr = kproc->mem[i].bus_addr;
> + dev_addr = kproc->mem[i].dev_addr;
> + size = kproc->mem[i].size;
> +
> + if (da < KEYSTONE_RPROC_LOCAL_ADDRESS_MASK) {
> + /* handle DSP-view addresses */
> + if (da >= dev_addr &&
> + ((da + len) <= (dev_addr + size))) {
> + offset = da - dev_addr;
> + va = kproc->mem[i].cpu_addr + offset;
> + return (__force void *)va;
> + }
> + } else {
> + /* handle SoC-view addresses */
> + if (da >= bus_addr &&
> + (da + len) <= (bus_addr + size)) {
> + offset = da - bus_addr;
> + va = kproc->mem[i].cpu_addr + offset;
> + return (__force void *)va;
> + }
> + }
> + }
> +
> + /* handle static DDR reserved memory regions */
> + for (i = 0; i < kproc->num_rmems; i++) {
> + dev_addr = kproc->rmem[i].dev_addr;
> + size = kproc->rmem[i].size;
> +
> + if (da >= dev_addr && ((da + len) <= (dev_addr + size))) {
> + offset = da - dev_addr;
> + va = kproc->rmem[i].cpu_addr + offset;
> + return (__force void *)va;
> + }
> + }
> +
> + return NULL;
> +}
> +
> +static const struct rproc_ops k3_dsp_rproc_ops = {
> + .start = k3_dsp_rproc_start,
> + .stop = k3_dsp_rproc_stop,
> + .kick = k3_dsp_rproc_kick,
> + .da_to_va = k3_dsp_rproc_da_to_va,
> +};
> +
> +static int k3_dsp_rproc_of_get_memories(struct platform_device *pdev,
> + struct k3_dsp_rproc *kproc)
> +{
> + const struct k3_dsp_dev_data *data = kproc->data;
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> + int num_mems = 0;
> + int i;
> +
> + num_mems = kproc->data->num_mems;
> + kproc->mem = devm_kcalloc(kproc->dev, num_mems,
> + sizeof(*kproc->mem), GFP_KERNEL);
> + if (!kproc->mem)
> + return -ENOMEM;
> +
> + for (i = 0; i < num_mems; i++) {
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> + data->mems[i].name);
> + if (!res) {
> + dev_err(dev, "found no memory resource for %s\n",
> + data->mems[i].name);
> + return -EINVAL;
> + }
> + if (!devm_request_mem_region(dev, res->start,
> + resource_size(res),
> + dev_name(dev))) {
> + dev_err(dev, "could not request %s region for resource\n",
> + data->mems[i].name);
> + return -EBUSY;
> + }
> +
> + kproc->mem[i].cpu_addr = devm_ioremap_wc(dev, res->start,
> + resource_size(res));
> + if (IS_ERR(kproc->mem[i].cpu_addr)) {
> + dev_err(dev, "failed to map %s memory\n",
> + data->mems[i].name);
> + return PTR_ERR(kproc->mem[i].cpu_addr);
> + }
> + kproc->mem[i].bus_addr = res->start;
> + kproc->mem[i].dev_addr = data->mems[i].dev_addr;
> + kproc->mem[i].size = resource_size(res);
> +
> + dev_dbg(dev, "memory %8s: bus addr %pa size 0x%zx va %pK da 0x%x\n",
> + data->mems[i].name, &kproc->mem[i].bus_addr,
> + kproc->mem[i].size, kproc->mem[i].cpu_addr,
> + kproc->mem[i].dev_addr);
> + }
> + kproc->num_mems = num_mems;
> +
> + return 0;
> +}
> +
> +static int k3_dsp_reserved_mem_init(struct k3_dsp_rproc *kproc)
> +{
> + struct device *dev = kproc->dev;
> + struct device_node *np = dev->of_node;
> + struct device_node *rmem_np;
> + struct reserved_mem *rmem;
> + int num_rmems;
> + int ret, i;
> +
> + num_rmems = of_property_count_elems_of_size(np, "memory-region",
> + sizeof(phandle));
> + if (num_rmems <= 0) {
> + dev_err(dev, "device does not reserved memory regions, ret = %d\n",
> + num_rmems);
> + return -EINVAL;
> + }
> + if (num_rmems < 2) {
> + dev_err(dev, "device needs atleast two memory regions to be defined, num = %d\n",
> + num_rmems);
> + return -EINVAL;
> + }
> +
> + /* use reserved memory region 0 for vring DMA allocations */
> + ret = of_reserved_mem_device_init_by_idx(dev, np, 0);
> + if (ret) {
> + dev_err(dev, "device cannot initialize DMA pool, ret = %d\n",
> + ret);
> + return ret;
> + }
> +
> + num_rmems--;
> + kproc->rmem = kcalloc(num_rmems, sizeof(*kproc->rmem), GFP_KERNEL);
> + if (!kproc->rmem) {
> + ret = -ENOMEM;
> + goto release_rmem;
> + }
> +
> + /* use remaining reserved memory regions for static carveouts */
> + for (i = 0; i < num_rmems; i++) {
> + rmem_np = of_parse_phandle(np, "memory-region", i + 1);
> + if (!rmem_np) {
> + ret = -EINVAL;
> + goto unmap_rmem;
> + }
> +
> + rmem = of_reserved_mem_lookup(rmem_np);
> + if (!rmem) {
> + of_node_put(rmem_np);
> + ret = -EINVAL;
> + goto unmap_rmem;
> + }
> + of_node_put(rmem_np);
> +
> + kproc->rmem[i].bus_addr = rmem->base;
> + /* 64-bit address regions currently not supported */
> + kproc->rmem[i].dev_addr = (u32)rmem->base;
> + kproc->rmem[i].size = rmem->size;
> + kproc->rmem[i].cpu_addr = ioremap_wc(rmem->base, rmem->size);
> + if (!kproc->rmem[i].cpu_addr) {
> + dev_err(dev, "failed to map reserved memory#%d at %pa of size %pa\n",
> + i + 1, &rmem->base, &rmem->size);
> + ret = -ENOMEM;
> + goto unmap_rmem;
> + }
> +
> + dev_dbg(dev, "reserved memory%d: bus addr %pa size 0x%zx va %pK da 0x%x\n",
> + i + 1, &kproc->rmem[i].bus_addr,
> + kproc->rmem[i].size, kproc->rmem[i].cpu_addr,
> + kproc->rmem[i].dev_addr);
> + }
> + kproc->num_rmems = num_rmems;
> +
> + return 0;
> +
> +unmap_rmem:
> + for (i--; i >= 0; i--) {
> + if (kproc->rmem[i].cpu_addr)
> + iounmap(kproc->rmem[i].cpu_addr);
> + }
> + kfree(kproc->rmem);
> +release_rmem:
> + of_reserved_mem_device_release(kproc->dev);
> + return ret;
> +}
> +
> +static void k3_dsp_reserved_mem_exit(struct k3_dsp_rproc *kproc)
> +{
> + int i;
> +
> + for (i = 0; i < kproc->num_rmems; i++)
> + iounmap(kproc->rmem[i].cpu_addr);
> + kfree(kproc->rmem);
> +
> + of_reserved_mem_device_release(kproc->dev);
> +}
> +
> +static
> +struct ti_sci_proc *k3_dsp_rproc_of_get_tsp(struct device *dev,
> + const struct ti_sci_handle *sci)
> +{
> + struct ti_sci_proc *tsp;
> + u32 temp[2];
> + int ret;
> +
> + ret = of_property_read_u32_array(dev->of_node, "ti,sci-proc-ids",
> + temp, 2);
> + if (ret < 0)
> + return ERR_PTR(ret);
> +
> + tsp = kzalloc(sizeof(*tsp), GFP_KERNEL);
> + if (!tsp)
> + return ERR_PTR(-ENOMEM);
> +
> + tsp->dev = dev;
> + tsp->sci = sci;
> + tsp->ops = &sci->ops.proc_ops;
> + tsp->proc_id = temp[0];
> + tsp->host_id = temp[1];
> +
> + return tsp;
> +}
> +
> +static int k3_dsp_rproc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + const struct k3_dsp_dev_data *data;
> + struct k3_dsp_rproc *kproc;
> + struct rproc *rproc;
> + const char *fw_name;
> + int ret = 0;
> + int ret1;
> +
> + data = of_device_get_match_data(dev);
> + if (!data)
> + return -ENODEV;
> +
> + ret = rproc_of_parse_firmware(dev, 0, &fw_name);
> + if (ret) {
> + dev_err(dev, "failed to parse firmware-name property, ret = %d\n",
> + ret);
> + return ret;
> + }
> +
> + rproc = rproc_alloc(dev, dev_name(dev), &k3_dsp_rproc_ops, fw_name,
> + sizeof(*kproc));
> + if (!rproc)
> + return -ENOMEM;
> +
> + rproc->has_iommu = false;
> + rproc->recovery_disabled = true;
> + kproc = rproc->priv;
> + kproc->rproc = rproc;
> + kproc->dev = dev;
> + kproc->data = data;
> +
> + kproc->ti_sci = ti_sci_get_by_phandle(np, "ti,sci");
> + if (IS_ERR(kproc->ti_sci)) {
> + ret = PTR_ERR(kproc->ti_sci);
> + if (ret != -EPROBE_DEFER) {
> + dev_err(dev, "failed to get ti-sci handle, ret = %d\n",
> + ret);
> + }
> + kproc->ti_sci = NULL;
> + goto free_rproc;
> + }
> +
> + ret = of_property_read_u32(np, "ti,sci-dev-id", &kproc->ti_sci_id);
> + if (ret) {
> + dev_err(dev, "missing 'ti,sci-dev-id' property\n");
> + goto put_sci;
> + }
> +
> + kproc->reset = devm_reset_control_get_exclusive(dev, NULL);
> + if (IS_ERR(kproc->reset)) {
> + ret = PTR_ERR(kproc->reset);
> + dev_err(dev, "failed to get reset, status = %d\n", ret);
> + goto put_sci;
> + }
> +
> + kproc->tsp = k3_dsp_rproc_of_get_tsp(dev, kproc->ti_sci);
> + if (IS_ERR(kproc->tsp)) {
> + dev_err(dev, "failed to construct ti-sci proc control, ret = %d\n",
> + ret);
> + ret = PTR_ERR(kproc->tsp);
> + goto put_sci;
> + }
> +
> + ret = ti_sci_proc_request(kproc->tsp);
> + if (ret < 0) {
> + dev_err(dev, "ti_sci_proc_request failed, ret = %d\n", ret);
> + goto free_tsp;
> + }
> +
> + ret = k3_dsp_rproc_of_get_memories(pdev, kproc);
> + if (ret)
> + goto release_tsp;
> +
> + ret = k3_dsp_reserved_mem_init(kproc);
> + if (ret) {
> + dev_err(dev, "reserved memory init failed, ret = %d\n", ret);
> + goto release_tsp;
> + }
> +
> + ret = rproc_add(rproc);
> + if (ret) {
> + dev_err(dev, "failed to add register device with remoteproc core, status = %d\n",
> + ret);
> + goto release_mem;
> + }
> +
> + platform_set_drvdata(pdev, kproc);
> +
> + return 0;
> +
> +release_mem:
> + k3_dsp_reserved_mem_exit(kproc);
> +release_tsp:
> + ret1 = ti_sci_proc_release(kproc->tsp);
> + if (ret1)
> + dev_err(dev, "failed to release proc, ret = %d\n", ret1);
> +free_tsp:
> + kfree(kproc->tsp);
> +put_sci:
> + ret1 = ti_sci_put_handle(kproc->ti_sci);
> + if (ret1)
> + dev_err(dev, "failed to put ti_sci handle, ret = %d\n", ret1);
> +free_rproc:
> + rproc_free(rproc);
> + return ret;
> +}
> +
> +static int k3_dsp_rproc_remove(struct platform_device *pdev)
> +{
> + struct k3_dsp_rproc *kproc = platform_get_drvdata(pdev);
> + struct device *dev = &pdev->dev;
> + int ret;
> +
> + rproc_del(kproc->rproc);
> +
> + ret = ti_sci_proc_release(kproc->tsp);
> + if (ret)
> + dev_err(dev, "failed to release proc, ret = %d\n", ret);
> +
> + kfree(kproc->tsp);
> +
> + ret = ti_sci_put_handle(kproc->ti_sci);
> + if (ret)
> + dev_err(dev, "failed to put ti_sci handle, ret = %d\n", ret);
> +
> + k3_dsp_reserved_mem_exit(kproc);
> + rproc_free(kproc->rproc);
> +
> + return 0;
> +}
> +
> +static const struct k3_dsp_mem_data c66_mems[] = {
> + { .name = "l2sram", .dev_addr = 0x800000 },
> + { .name = "l1pram", .dev_addr = 0xe00000 },
> + { .name = "l1dram", .dev_addr = 0xf00000 },
> +};
> +
> +static const struct k3_dsp_dev_data c66_data = {
> + .mems = c66_mems,
> + .num_mems = ARRAY_SIZE(c66_mems),
> + .boot_align_addr = SZ_1K,
> + .uses_lreset = true,
> +};
> +
> +static const struct of_device_id k3_dsp_of_match[] = {
> + { .compatible = "ti,j721e-c66-dsp", .data = &c66_data, },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, k3_dsp_of_match);
> +
> +static struct platform_driver k3_dsp_rproc_driver = {
> + .probe = k3_dsp_rproc_probe,
> + .remove = k3_dsp_rproc_remove,
> + .driver = {
> + .name = "k3-dsp-rproc",
> + .of_match_table = k3_dsp_of_match,
> + },
> +};
> +
> +module_platform_driver(k3_dsp_rproc_driver);
> +
> +MODULE_AUTHOR("Suman Anna <s-anna@ti.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("TI K3 DSP Remoteproc driver");
> --
> 2.26.0
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox