| Message ID | 20200513005441.1102586-4-bjorn.andersson@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwspinlock: qcom: Allow dropping the intermediate TCSR mutex syscon | expand |
On Wed, May 13, 2020 at 8:55 AM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > In all 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. So > add support for directly memory mapping this register space, to avoid > the need to represent this block using a syscon. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/hwspinlock/qcom_hwspinlock.c | 72 +++++++++++++++++++++------- > 1 file changed, 56 insertions(+), 16 deletions(-) > > diff --git a/drivers/hwspinlock/qcom_hwspinlock.c b/drivers/hwspinlock/qcom_hwspinlock.c > index f0da544b14d2..d8d4d729816c 100644 > --- a/drivers/hwspinlock/qcom_hwspinlock.c > +++ b/drivers/hwspinlock/qcom_hwspinlock.c > @@ -70,41 +70,81 @@ 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; > + struct resource *res; > + void __iomem *base; > + > + /* All modern platform has offset 0 and stride of 4k */ > + *offset = 0; > + *stride = 0x1000; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + base = devm_ioremap_resource(&pdev->dev, res); I think you can use devm_platform_ioremap_resource(pdev, 0) to simplify your code, otherwise looks good to me. Reviewed-by: Baolin Wang <baolin.wang7@gmail.com> > + 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 >
On 12-05-20, 17:54, Bjorn Andersson wrote: > In all 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. So > add support for directly memory mapping this register space, to avoid > the need to represent this block using a syscon. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/hwspinlock/qcom_hwspinlock.c | 72 +++++++++++++++++++++------- > 1 file changed, 56 insertions(+), 16 deletions(-) > > diff --git a/drivers/hwspinlock/qcom_hwspinlock.c b/drivers/hwspinlock/qcom_hwspinlock.c > index f0da544b14d2..d8d4d729816c 100644 > --- a/drivers/hwspinlock/qcom_hwspinlock.c > +++ b/drivers/hwspinlock/qcom_hwspinlock.c > @@ -70,41 +70,81 @@ 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"); any reason to drop the log? > - 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; > + struct resource *res; > + void __iomem *base; > + > + /* All modern platform has offset 0 and stride of 4k */ > + *offset = 0; > + *stride = 0x1000; Wouldn't it make sense to read this from DT rather than code in kernel?
On Thu 14 May 07:15 PDT 2020, Vinod Koul wrote: > On 12-05-20, 17:54, Bjorn Andersson wrote: > > In all 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. So > > add support for directly memory mapping this register space, to avoid > > the need to represent this block using a syscon. > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > drivers/hwspinlock/qcom_hwspinlock.c | 72 +++++++++++++++++++++------- > > 1 file changed, 56 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/hwspinlock/qcom_hwspinlock.c b/drivers/hwspinlock/qcom_hwspinlock.c > > index f0da544b14d2..d8d4d729816c 100644 > > --- a/drivers/hwspinlock/qcom_hwspinlock.c > > +++ b/drivers/hwspinlock/qcom_hwspinlock.c > > @@ -70,41 +70,81 @@ 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"); > > any reason to drop the log? > Given that we first check for the syscon and then fall back to trying to use the reg, keeping this line would cause this log line to always show up on targets putting this under /soc. So I think it's better to drop the line and then require the presence of either syscon or reg using the DT schema. > > - 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; > > + struct resource *res; > > + void __iomem *base; > > + > > + /* All modern platform has offset 0 and stride of 4k */ > > + *offset = 0; > > + *stride = 0x1000; > > Wouldn't it make sense to read this from DT rather than code in kernel? > I did consider this as well as platform specific compatibles, but realized that pretty much all 64-bit targets have these values. So given that we still can represent this using the syscon approach I don't think we need to add yet another mechanism to specify these. Thanks, Bjorn
On Tue 12 May 20:57 PDT 2020, Baolin Wang wrote: > On Wed, May 13, 2020 at 8:55 AM Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > > > In all 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. So > > add support for directly memory mapping this register space, to avoid > > the need to represent this block using a syscon. > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > drivers/hwspinlock/qcom_hwspinlock.c | 72 +++++++++++++++++++++------- > > 1 file changed, 56 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/hwspinlock/qcom_hwspinlock.c b/drivers/hwspinlock/qcom_hwspinlock.c [..] > > +static struct regmap *qcom_hwspinlock_probe_mmio(struct platform_device *pdev, > > + u32 *offset, u32 *stride) > > +{ > > + struct device *dev = &pdev->dev; > > + struct resource *res; > > + void __iomem *base; > > + > > + /* All modern platform has offset 0 and stride of 4k */ > > + *offset = 0; > > + *stride = 0x1000; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + base = devm_ioremap_resource(&pdev->dev, res); > > I think you can use devm_platform_ioremap_resource(pdev, 0) to > simplify your code, otherwise looks good to me. You're right, I better fix this before someone with Coccinelle get the chance ;) > Reviewed-by: Baolin Wang <baolin.wang7@gmail.com> > Thanks, Bjorn
On 14-05-20, 10:00, Bjorn Andersson wrote: > On Thu 14 May 07:15 PDT 2020, Vinod Koul wrote: > > > On 12-05-20, 17:54, Bjorn Andersson wrote: > > > In all 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. So > > > add support for directly memory mapping this register space, to avoid > > > the need to represent this block using a syscon. > > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > --- > > > drivers/hwspinlock/qcom_hwspinlock.c | 72 +++++++++++++++++++++------- > > > 1 file changed, 56 insertions(+), 16 deletions(-) > > > > > > diff --git a/drivers/hwspinlock/qcom_hwspinlock.c b/drivers/hwspinlock/qcom_hwspinlock.c > > > index f0da544b14d2..d8d4d729816c 100644 > > > --- a/drivers/hwspinlock/qcom_hwspinlock.c > > > +++ b/drivers/hwspinlock/qcom_hwspinlock.c > > > @@ -70,41 +70,81 @@ 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"); > > > > any reason to drop the log? > > > > Given that we first check for the syscon and then fall back to trying to > use the reg, keeping this line would cause this log line to always show > up on targets putting this under /soc. > > So I think it's better to drop the line and then require the presence of > either syscon or reg using the DT schema. ok > > > - 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; > > > + struct resource *res; > > > + void __iomem *base; > > > + > > > + /* All modern platform has offset 0 and stride of 4k */ > > > + *offset = 0; > > > + *stride = 0x1000; > > > > Wouldn't it make sense to read this from DT rather than code in kernel? > > > > I did consider this as well as platform specific compatibles, but > realized that pretty much all 64-bit targets have these values. So given > that we still can represent this using the syscon approach I don't think > we need to add yet another mechanism to specify these. Sounds good. Reviewed-by: Vinod Koul <vkoul@kernel.org>
diff --git a/drivers/hwspinlock/qcom_hwspinlock.c b/drivers/hwspinlock/qcom_hwspinlock.c index f0da544b14d2..d8d4d729816c 100644 --- a/drivers/hwspinlock/qcom_hwspinlock.c +++ b/drivers/hwspinlock/qcom_hwspinlock.c @@ -70,41 +70,81 @@ 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; + struct resource *res; + void __iomem *base; + + /* All modern platform has offset 0 and stride of 4k */ + *offset = 0; + *stride = 0x1000; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + base = devm_ioremap_resource(&pdev->dev, res); + 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)
In all 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. So add support for directly memory mapping this register space, to avoid the need to represent this block using a syscon. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- drivers/hwspinlock/qcom_hwspinlock.c | 72 +++++++++++++++++++++------- 1 file changed, 56 insertions(+), 16 deletions(-)