In modern Qualcomm platforms the mutex region of the TCSR is forked off into
its own block, all with a offset of 0 and stride of 4096, and in some of these
platforms no other registers in this region is accessed from Linux. Update the
binding and the implementation to allow the TCSR mutex to be represented
without an intermediate syscon node.
Bjorn Andersson (4):
dt-bindings: hwlock: qcom: Migrate binding to YAML
dt-bindings: hwlock: qcom: Allow device on mmio bus
hwspinlock: qcom: Allow mmio usage in addition to syscon
arm64: dts: qcom: sm8250: Drop tcsr_mutex syscon
.../bindings/hwlock/qcom-hwspinlock.txt | 39 -----------
.../bindings/hwlock/qcom-hwspinlock.yaml | 65 +++++++++++++++++
arch/arm64/boot/dts/qcom/sm8250.dtsi | 17 ++---
drivers/hwspinlock/qcom_hwspinlock.c | 70 ++++++++++++++-----
4 files changed, 125 insertions(+), 66 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
create mode 100644 Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.yaml
--
2.26.2
Now that we don't need the intermediate syscon to represent the TCSR
mutexes, update the dts to describe the TCSR mutex directly under /soc.
The change also fixes the sort order of the nodes.
Signed-off-by: Bjorn Andersson <[email protected]>
---
Changs since v1:
- Adjusted sort order of the nodes
arch/arm64/boot/dts/qcom/sm8250.dtsi | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index 7050adba7995..67a1b6f3301b 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -144,12 +144,6 @@ scm: scm {
};
};
- tcsr_mutex: hwlock {
- compatible = "qcom,tcsr-mutex";
- syscon = <&tcsr_mutex_regs 0 0x1000>;
- #hwlock-cells = <1>;
- };
-
memory@80000000 {
device_type = "memory";
/* We expect the bootloader to fill in the size */
@@ -376,6 +370,12 @@ ufs_mem_phy_lanes: lanes@1d87400 {
};
};
+ tcsr_mutex: hwlock@1f40000 {
+ compatible = "qcom,tcsr-mutex";
+ reg = <0x0 0x01f40000 0x0 0x40000>;
+ #hwlock-cells = <1>;
+ };
+
intc: interrupt-controller@17a00000 {
compatible = "arm,gic-v3";
#interrupt-cells = <3>;
@@ -486,11 +486,6 @@ rpmhpd_opp_turbo_l1: opp10 {
};
};
- tcsr_mutex_regs: syscon@1f40000 {
- compatible = "syscon";
- reg = <0x0 0x01f40000 0x0 0x40000>;
- };
-
timer@17c20000 {
#address-cells = <2>;
#size-cells = <2>;
--
2.26.2
In modern Qualcomm platforms the mutex region of the TCSR is forked off
into its own block, all with a offset of 0 and stride of 4096, and in
some of these platforms no other registers in this region is accessed
from Linux.
So add support for directly memory mapping this register space, to avoid
the need to represent this block using a syscon.
Reviewed-by: Baolin Wang <[email protected]>
Reviewed-by: Vinod Koul <[email protected]>
Signed-off-by: Bjorn Andersson <[email protected]>
---
Changes since v1:
- Use devm_platform_ioremap_resource()
drivers/hwspinlock/qcom_hwspinlock.c | 70 +++++++++++++++++++++-------
1 file changed, 54 insertions(+), 16 deletions(-)
diff --git a/drivers/hwspinlock/qcom_hwspinlock.c b/drivers/hwspinlock/qcom_hwspinlock.c
index f0da544b14d2..364710966665 100644
--- a/drivers/hwspinlock/qcom_hwspinlock.c
+++ b/drivers/hwspinlock/qcom_hwspinlock.c
@@ -70,41 +70,79 @@ static const struct of_device_id qcom_hwspinlock_of_match[] = {
};
MODULE_DEVICE_TABLE(of, qcom_hwspinlock_of_match);
-static int qcom_hwspinlock_probe(struct platform_device *pdev)
+static struct regmap *qcom_hwspinlock_probe_syscon(struct platform_device *pdev,
+ u32 *base, u32 *stride)
{
- struct hwspinlock_device *bank;
struct device_node *syscon;
- struct reg_field field;
struct regmap *regmap;
- size_t array_size;
- u32 stride;
- u32 base;
int ret;
- int i;
syscon = of_parse_phandle(pdev->dev.of_node, "syscon", 0);
- if (!syscon) {
- dev_err(&pdev->dev, "no syscon property\n");
- return -ENODEV;
- }
+ if (!syscon)
+ return ERR_PTR(-ENODEV);
regmap = syscon_node_to_regmap(syscon);
of_node_put(syscon);
if (IS_ERR(regmap))
- return PTR_ERR(regmap);
+ return regmap;
- ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 1, &base);
+ ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 1, base);
if (ret < 0) {
dev_err(&pdev->dev, "no offset in syscon\n");
- return -EINVAL;
+ return ERR_PTR(-EINVAL);
}
- ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 2, &stride);
+ ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 2, stride);
if (ret < 0) {
dev_err(&pdev->dev, "no stride syscon\n");
- return -EINVAL;
+ return ERR_PTR(-EINVAL);
}
+ return regmap;
+}
+
+static const struct regmap_config tcsr_mutex_config = {
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .val_bits = 32,
+ .max_register = 0x40000,
+ .fast_io = true,
+};
+
+static struct regmap *qcom_hwspinlock_probe_mmio(struct platform_device *pdev,
+ u32 *offset, u32 *stride)
+{
+ struct device *dev = &pdev->dev;
+ void __iomem *base;
+
+ /* All modern platform has offset 0 and stride of 4k */
+ *offset = 0;
+ *stride = 0x1000;
+
+ base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(base))
+ return ERR_CAST(base);
+
+ return devm_regmap_init_mmio(dev, base, &tcsr_mutex_config);
+}
+
+static int qcom_hwspinlock_probe(struct platform_device *pdev)
+{
+ struct hwspinlock_device *bank;
+ struct reg_field field;
+ struct regmap *regmap;
+ size_t array_size;
+ u32 stride;
+ u32 base;
+ int i;
+
+ regmap = qcom_hwspinlock_probe_syscon(pdev, &base, &stride);
+ if (IS_ERR(regmap) && PTR_ERR(regmap) == -ENODEV)
+ regmap = qcom_hwspinlock_probe_mmio(pdev, &base, &stride);
+
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
array_size = QCOM_MUTEX_NUM_LOCKS * sizeof(struct hwspinlock);
bank = devm_kzalloc(&pdev->dev, sizeof(*bank) + array_size, GFP_KERNEL);
if (!bank)
--
2.26.2
Hi Bjorn,
On Mon, Jun 22, 2020 at 12:59:55AM -0700, Bjorn Andersson wrote:
> In modern Qualcomm platforms the mutex region of the TCSR is forked off
> into its own block, all with a offset of 0 and stride of 4096, and in
> some of these platforms no other registers in this region is accessed
> from Linux.
>
> So add support for directly memory mapping this register space, to avoid
> the need to represent this block using a syscon.
>
> Reviewed-by: Baolin Wang <[email protected]>
> Reviewed-by: Vinod Koul <[email protected]>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
>
> Changes since v1:
> - Use devm_platform_ioremap_resource()
>
> drivers/hwspinlock/qcom_hwspinlock.c | 70 +++++++++++++++++++++-------
> 1 file changed, 54 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/hwspinlock/qcom_hwspinlock.c b/drivers/hwspinlock/qcom_hwspinlock.c
> index f0da544b14d2..364710966665 100644
> --- a/drivers/hwspinlock/qcom_hwspinlock.c
> +++ b/drivers/hwspinlock/qcom_hwspinlock.c
> @@ -70,41 +70,79 @@ static const struct of_device_id qcom_hwspinlock_of_match[] = {
> };
> MODULE_DEVICE_TABLE(of, qcom_hwspinlock_of_match);
>
> -static int qcom_hwspinlock_probe(struct platform_device *pdev)
> +static struct regmap *qcom_hwspinlock_probe_syscon(struct platform_device *pdev,
> + u32 *base, u32 *stride)
> {
> - struct hwspinlock_device *bank;
> struct device_node *syscon;
> - struct reg_field field;
> struct regmap *regmap;
> - size_t array_size;
> - u32 stride;
> - u32 base;
> int ret;
> - int i;
>
> syscon = of_parse_phandle(pdev->dev.of_node, "syscon", 0);
> - if (!syscon) {
> - dev_err(&pdev->dev, "no syscon property\n");
> - return -ENODEV;
> - }
> + if (!syscon)
> + return ERR_PTR(-ENODEV);
>
> regmap = syscon_node_to_regmap(syscon);
> of_node_put(syscon);
> if (IS_ERR(regmap))
> - return PTR_ERR(regmap);
> + return regmap;
>
> - ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 1, &base);
> + ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 1, base);
> if (ret < 0) {
> dev_err(&pdev->dev, "no offset in syscon\n");
> - return -EINVAL;
> + return ERR_PTR(-EINVAL);
> }
>
> - ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 2, &stride);
> + ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 2, stride);
> if (ret < 0) {
> dev_err(&pdev->dev, "no stride syscon\n");
> - return -EINVAL;
> + return ERR_PTR(-EINVAL);
> }
>
> + return regmap;
> +}
> +
> +static const struct regmap_config tcsr_mutex_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .max_register = 0x40000,
Where does the 0x40000 come from?
It seems like this driver has QCOM_MUTEX_NUM_LOCKS = 32 hardcoded.
With a stride of 4096 = 0x1000 you get 0x1000 * 32 = 0x20000.
This is also the reg size used in msm8996.dtsi and msm8916.dtsi for
example, while sdm845.dtsi and sm8250.dtsi specify 0x40000.
Are you not exposing all available locks on the newer SoCs?
I'm not sure how important max_register is... But I guess it should be
either correct for all SoCs or not specified at all (since it's
optional)?
(That is assuming the hwlock can be also used directly via MMIO on
MSM8996 and MSM8916. It looks to me like it has its own register
space there as well...)
Thanks,
Stephan
On Tue 14 Jul 09:04 PDT 2020, Stephan Gerhold wrote:
> Hi Bjorn,
>
> On Mon, Jun 22, 2020 at 12:59:55AM -0700, Bjorn Andersson wrote:
> > In modern Qualcomm platforms the mutex region of the TCSR is forked off
> > into its own block, all with a offset of 0 and stride of 4096, and in
> > some of these platforms no other registers in this region is accessed
> > from Linux.
> >
> > So add support for directly memory mapping this register space, to avoid
> > the need to represent this block using a syscon.
> >
> > Reviewed-by: Baolin Wang <[email protected]>
> > Reviewed-by: Vinod Koul <[email protected]>
> > Signed-off-by: Bjorn Andersson <[email protected]>
> > ---
> >
> > Changes since v1:
> > - Use devm_platform_ioremap_resource()
> >
> > drivers/hwspinlock/qcom_hwspinlock.c | 70 +++++++++++++++++++++-------
> > 1 file changed, 54 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/hwspinlock/qcom_hwspinlock.c b/drivers/hwspinlock/qcom_hwspinlock.c
> > index f0da544b14d2..364710966665 100644
> > --- a/drivers/hwspinlock/qcom_hwspinlock.c
> > +++ b/drivers/hwspinlock/qcom_hwspinlock.c
> > @@ -70,41 +70,79 @@ static const struct of_device_id qcom_hwspinlock_of_match[] = {
> > };
> > MODULE_DEVICE_TABLE(of, qcom_hwspinlock_of_match);
> >
> > -static int qcom_hwspinlock_probe(struct platform_device *pdev)
> > +static struct regmap *qcom_hwspinlock_probe_syscon(struct platform_device *pdev,
> > + u32 *base, u32 *stride)
> > {
> > - struct hwspinlock_device *bank;
> > struct device_node *syscon;
> > - struct reg_field field;
> > struct regmap *regmap;
> > - size_t array_size;
> > - u32 stride;
> > - u32 base;
> > int ret;
> > - int i;
> >
> > syscon = of_parse_phandle(pdev->dev.of_node, "syscon", 0);
> > - if (!syscon) {
> > - dev_err(&pdev->dev, "no syscon property\n");
> > - return -ENODEV;
> > - }
> > + if (!syscon)
> > + return ERR_PTR(-ENODEV);
> >
> > regmap = syscon_node_to_regmap(syscon);
> > of_node_put(syscon);
> > if (IS_ERR(regmap))
> > - return PTR_ERR(regmap);
> > + return regmap;
> >
> > - ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 1, &base);
> > + ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 1, base);
> > if (ret < 0) {
> > dev_err(&pdev->dev, "no offset in syscon\n");
> > - return -EINVAL;
> > + return ERR_PTR(-EINVAL);
> > }
> >
> > - ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 2, &stride);
> > + ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 2, stride);
> > if (ret < 0) {
> > dev_err(&pdev->dev, "no stride syscon\n");
> > - return -EINVAL;
> > + return ERR_PTR(-EINVAL);
> > }
> >
> > + return regmap;
> > +}
> > +
> > +static const struct regmap_config tcsr_mutex_config = {
> > + .reg_bits = 32,
> > + .reg_stride = 4,
> > + .val_bits = 32,
> > + .max_register = 0x40000,
>
> Where does the 0x40000 come from?
>
I presumably copied it off the dts I was looking (sm8250) as I wrote
this, but...
> It seems like this driver has QCOM_MUTEX_NUM_LOCKS = 32 hardcoded.
> With a stride of 4096 = 0x1000 you get 0x1000 * 32 = 0x20000.
>
> This is also the reg size used in msm8996.dtsi and msm8916.dtsi for
> example, while sdm845.dtsi and sm8250.dtsi specify 0x40000.
> Are you not exposing all available locks on the newer SoCs?
>
> I'm not sure how important max_register is... But I guess it should be
> either correct for all SoCs or not specified at all (since it's
> optional)?
>
...you're right. I think it should be omitted.
> (That is assuming the hwlock can be also used directly via MMIO on
> MSM8996 and MSM8916. It looks to me like it has its own register
> space there as well...)
>
If used on e.g. MSM8996 we still need to make sure the syscon is there,
so that the modem subsystem halt registers is available to the mpss
remoteproc. But specifying compatible as "qcom,tcsr-mutex", "syscon";
would use the new scheme and still would allow that access.
I merged patch 1-3 yesterday, so it would have to be an incremental
patch. I've put it on my todo list, but if you write up a patch I'd be
happy to merge it :)
Thanks,
Bjorn
On Mon, 22 Jun 2020 at 11:00, Bjorn Andersson
<[email protected]> wrote:
>
> Now that we don't need the intermediate syscon to represent the TCSR
> mutexes, update the dts to describe the TCSR mutex directly under /soc.
>
> The change also fixes the sort order of the nodes.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On Mon, Jun 22, 2020 at 12:59:56AM -0700, Bjorn Andersson wrote:
> Now that we don't need the intermediate syscon to represent the TCSR
> mutexes, update the dts to describe the TCSR mutex directly under /soc.
>
> The change also fixes the sort order of the nodes.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Thanks,
Mani
> ---
>
> Changs since v1:
> - Adjusted sort order of the nodes
>
> arch/arm64/boot/dts/qcom/sm8250.dtsi | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> index 7050adba7995..67a1b6f3301b 100644
> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> @@ -144,12 +144,6 @@ scm: scm {
> };
> };
>
> - tcsr_mutex: hwlock {
> - compatible = "qcom,tcsr-mutex";
> - syscon = <&tcsr_mutex_regs 0 0x1000>;
> - #hwlock-cells = <1>;
> - };
> -
> memory@80000000 {
> device_type = "memory";
> /* We expect the bootloader to fill in the size */
> @@ -376,6 +370,12 @@ ufs_mem_phy_lanes: lanes@1d87400 {
> };
> };
>
> + tcsr_mutex: hwlock@1f40000 {
> + compatible = "qcom,tcsr-mutex";
> + reg = <0x0 0x01f40000 0x0 0x40000>;
> + #hwlock-cells = <1>;
> + };
> +
> intc: interrupt-controller@17a00000 {
> compatible = "arm,gic-v3";
> #interrupt-cells = <3>;
> @@ -486,11 +486,6 @@ rpmhpd_opp_turbo_l1: opp10 {
> };
> };
>
> - tcsr_mutex_regs: syscon@1f40000 {
> - compatible = "syscon";
> - reg = <0x0 0x01f40000 0x0 0x40000>;
> - };
> -
> timer@17c20000 {
> #address-cells = <2>;
> #size-cells = <2>;
> --
> 2.26.2
>
Hello,
any particular reason, why you did applied this patch only to sm8250?
Is it safe to convert rest of tcsr-mutex nodes to new schema without
additional testing?
Thanks
David
On Sat 25 Dec 06:57 PST 2021, David Heidelberg wrote:
> Hello,
>
> any particular reason, why you did applied this patch only to sm8250?
>
I was working on a 8250 board when this annoyed me, and I wanted to keep
the "example" clean so I only did that platform and then forgot to go
back and clean up the rest. (Same thing with the smem compatible moving
to reserved-memory).
> Is it safe to convert rest of tcsr-mutex nodes to new schema without
> additional testing?
>
I can't think of any reason it wouldn't be safe, so please feel free to
prepare a patch.
Thanks,
Bjorn