* [PATCHv2] clk: xgene: Don't call __pa on ioremaped address
@ 2016-10-28 16:59 Laura Abbott
2016-10-28 17:07 ` Loc Ho
2016-10-28 23:30 ` Stephen Boyd
0 siblings, 2 replies; 5+ messages in thread
From: Laura Abbott @ 2016-10-28 16:59 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd, Loc Ho
Cc: Laura Abbott, linux-clk, linux-kernel
ioremaped addresses are not linearly mapped so the physical
address can not be figured out via __pa. More generally, there
is no guarantee that backing value of an ioremapped address
is a physical address at all. The value here is only used
for debugging so just drop the call to __pa on the ioremapped
address.
Fixes: 6ae5fd381251 ("clk: xgene: Silence sparse warnings")
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v2: Fix up one more format string
I put the fixes tag to match with the cleanup that was done.
If there is interest in this fix for pre-4.2 kernels for
stable, I can submit a patch for that as well.
---
drivers/clk/clk-xgene.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/clk/clk-xgene.c b/drivers/clk/clk-xgene.c
index 3433132..1e67655 100644
--- a/drivers/clk/clk-xgene.c
+++ b/drivers/clk/clk-xgene.c
@@ -243,22 +243,20 @@ static int xgene_clk_enable(struct clk_hw *hw)
struct xgene_clk *pclk = to_xgene_clk(hw);
unsigned long flags = 0;
u32 data;
- phys_addr_t reg;
if (pclk->lock)
spin_lock_irqsave(pclk->lock, flags);
if (pclk->param.csr_reg != NULL) {
pr_debug("%s clock enabled\n", clk_hw_get_name(hw));
- reg = __pa(pclk->param.csr_reg);
/* First enable the clock */
data = xgene_clk_read(pclk->param.csr_reg +
pclk->param.reg_clk_offset);
data |= pclk->param.reg_clk_mask;
xgene_clk_write(data, pclk->param.csr_reg +
pclk->param.reg_clk_offset);
- pr_debug("%s clock PADDR base %pa clk offset 0x%08X mask 0x%08X value 0x%08X\n",
- clk_hw_get_name(hw), ®,
+ pr_debug("%s clk offset 0x%08X mask 0x%08X value 0x%08X\n",
+ clk_hw_get_name(hw),
pclk->param.reg_clk_offset, pclk->param.reg_clk_mask,
data);
@@ -268,8 +266,8 @@ static int xgene_clk_enable(struct clk_hw *hw)
data &= ~pclk->param.reg_csr_mask;
xgene_clk_write(data, pclk->param.csr_reg +
pclk->param.reg_csr_offset);
- pr_debug("%s CSR RESET PADDR base %pa csr offset 0x%08X mask 0x%08X value 0x%08X\n",
- clk_hw_get_name(hw), ®,
+ pr_debug("%s csr offset 0x%08X mask 0x%08X value 0x%08X\n",
+ clk_hw_get_name(hw),
pclk->param.reg_csr_offset, pclk->param.reg_csr_mask,
data);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCHv2] clk: xgene: Don't call __pa on ioremaped address
2016-10-28 16:59 [PATCHv2] clk: xgene: Don't call __pa on ioremaped address Laura Abbott
@ 2016-10-28 17:07 ` Loc Ho
2016-10-28 17:09 ` Laura Abbott
2016-10-28 23:30 ` Stephen Boyd
1 sibling, 1 reply; 5+ messages in thread
From: Loc Ho @ 2016-10-28 17:07 UTC (permalink / raw)
To: Laura Abbott
Cc: Michael Turquette, Stephen Boyd, linux-clk,
Linux Kernel Mailing List
Hi Laura,
> ioremaped addresses are not linearly mapped so the physical
> address can not be figured out via __pa. More generally, there
> is no guarantee that backing value of an ioremapped address
> is a physical address at all. The value here is only used
> for debugging so just drop the call to __pa on the ioremapped
> address.
>
> Fixes: 6ae5fd381251 ("clk: xgene: Silence sparse warnings")
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> v2: Fix up one more format string
>
> I put the fixes tag to match with the cleanup that was done.
> If there is interest in this fix for pre-4.2 kernels for
> stable, I can submit a patch for that as well.
> ---
> drivers/clk/clk-xgene.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/clk/clk-xgene.c b/drivers/clk/clk-xgene.c
> index 3433132..1e67655 100644
> --- a/drivers/clk/clk-xgene.c
> +++ b/drivers/clk/clk-xgene.c
> @@ -243,22 +243,20 @@ static int xgene_clk_enable(struct clk_hw *hw)
> struct xgene_clk *pclk = to_xgene_clk(hw);
> unsigned long flags = 0;
> u32 data;
> - phys_addr_t reg;
>
> if (pclk->lock)
> spin_lock_irqsave(pclk->lock, flags);
>
> if (pclk->param.csr_reg != NULL) {
> pr_debug("%s clock enabled\n", clk_hw_get_name(hw));
> - reg = __pa(pclk->param.csr_reg);
> /* First enable the clock */
> data = xgene_clk_read(pclk->param.csr_reg +
> pclk->param.reg_clk_offset);
> data |= pclk->param.reg_clk_mask;
> xgene_clk_write(data, pclk->param.csr_reg +
> pclk->param.reg_clk_offset);
> - pr_debug("%s clock PADDR base %pa clk offset 0x%08X mask 0x%08X value 0x%08X\n",
> - clk_hw_get_name(hw), ®,
> + pr_debug("%s clk offset 0x%08X mask 0x%08X value 0x%08X\n",
> + clk_hw_get_name(hw),
> pclk->param.reg_clk_offset, pclk->param.reg_clk_mask,
> data);
>
> @@ -268,8 +266,8 @@ static int xgene_clk_enable(struct clk_hw *hw)
> data &= ~pclk->param.reg_csr_mask;
> xgene_clk_write(data, pclk->param.csr_reg +
> pclk->param.reg_csr_offset);
> - pr_debug("%s CSR RESET PADDR base %pa csr offset 0x%08X mask 0x%08X value 0x%08X\n",
> - clk_hw_get_name(hw), ®,
> + pr_debug("%s csr offset 0x%08X mask 0x%08X value 0x%08X\n",
> + clk_hw_get_name(hw),
> pclk->param.reg_csr_offset, pclk->param.reg_csr_mask,
> data);
> }
The code looks fine to me. May be overly cautious here. Do you have a
board to test this out?
-Loc
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCHv2] clk: xgene: Don't call __pa on ioremaped address
2016-10-28 17:07 ` Loc Ho
@ 2016-10-28 17:09 ` Laura Abbott
2016-10-28 17:11 ` Loc Ho
0 siblings, 1 reply; 5+ messages in thread
From: Laura Abbott @ 2016-10-28 17:09 UTC (permalink / raw)
To: Loc Ho
Cc: Michael Turquette, Stephen Boyd, linux-clk,
Linux Kernel Mailing List
On 10/28/2016 10:07 AM, Loc Ho wrote:
> Hi Laura,
>
>> ioremaped addresses are not linearly mapped so the physical
>> address can not be figured out via __pa. More generally, there
>> is no guarantee that backing value of an ioremapped address
>> is a physical address at all. The value here is only used
>> for debugging so just drop the call to __pa on the ioremapped
>> address.
>>
>> Fixes: 6ae5fd381251 ("clk: xgene: Silence sparse warnings")
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>> v2: Fix up one more format string
>>
>> I put the fixes tag to match with the cleanup that was done.
>> If there is interest in this fix for pre-4.2 kernels for
>> stable, I can submit a patch for that as well.
>> ---
>> drivers/clk/clk-xgene.c | 10 ++++------
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/clk/clk-xgene.c b/drivers/clk/clk-xgene.c
>> index 3433132..1e67655 100644
>> --- a/drivers/clk/clk-xgene.c
>> +++ b/drivers/clk/clk-xgene.c
>> @@ -243,22 +243,20 @@ static int xgene_clk_enable(struct clk_hw *hw)
>> struct xgene_clk *pclk = to_xgene_clk(hw);
>> unsigned long flags = 0;
>> u32 data;
>> - phys_addr_t reg;
>>
>> if (pclk->lock)
>> spin_lock_irqsave(pclk->lock, flags);
>>
>> if (pclk->param.csr_reg != NULL) {
>> pr_debug("%s clock enabled\n", clk_hw_get_name(hw));
>> - reg = __pa(pclk->param.csr_reg);
>> /* First enable the clock */
>> data = xgene_clk_read(pclk->param.csr_reg +
>> pclk->param.reg_clk_offset);
>> data |= pclk->param.reg_clk_mask;
>> xgene_clk_write(data, pclk->param.csr_reg +
>> pclk->param.reg_clk_offset);
>> - pr_debug("%s clock PADDR base %pa clk offset 0x%08X mask 0x%08X value 0x%08X\n",
>> - clk_hw_get_name(hw), ®,
>> + pr_debug("%s clk offset 0x%08X mask 0x%08X value 0x%08X\n",
>> + clk_hw_get_name(hw),
>> pclk->param.reg_clk_offset, pclk->param.reg_clk_mask,
>> data);
>>
>> @@ -268,8 +266,8 @@ static int xgene_clk_enable(struct clk_hw *hw)
>> data &= ~pclk->param.reg_csr_mask;
>> xgene_clk_write(data, pclk->param.csr_reg +
>> pclk->param.reg_csr_offset);
>> - pr_debug("%s CSR RESET PADDR base %pa csr offset 0x%08X mask 0x%08X value 0x%08X\n",
>> - clk_hw_get_name(hw), ®,
>> + pr_debug("%s csr offset 0x%08X mask 0x%08X value 0x%08X\n",
>> + clk_hw_get_name(hw),
>> pclk->param.reg_csr_offset, pclk->param.reg_csr_mask,
>> data);
>> }
>
>
> The code looks fine to me. May be overly cautious here. Do you have a
> board to test this out?
>
> -Loc
>
Yes, that was how I caught the problem. There are no errors that I
can see with this patch applied.
Thanks,
Laura
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCHv2] clk: xgene: Don't call __pa on ioremaped address
2016-10-28 17:09 ` Laura Abbott
@ 2016-10-28 17:11 ` Loc Ho
0 siblings, 0 replies; 5+ messages in thread
From: Loc Ho @ 2016-10-28 17:11 UTC (permalink / raw)
To: Laura Abbott
Cc: Michael Turquette, Stephen Boyd, linux-clk,
Linux Kernel Mailing List
Hi,
>>
>>> ioremaped addresses are not linearly mapped so the physical
>>> address can not be figured out via __pa. More generally, there
>>> is no guarantee that backing value of an ioremapped address
>>> is a physical address at all. The value here is only used
>>> for debugging so just drop the call to __pa on the ioremapped
>>> address.
>>>
>>> Fixes: 6ae5fd381251 ("clk: xgene: Silence sparse warnings")
>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>>> ---
>>> v2: Fix up one more format string
>>>
>>> I put the fixes tag to match with the cleanup that was done.
>>> If there is interest in this fix for pre-4.2 kernels for
>>> stable, I can submit a patch for that as well.
>>> ---
>>> drivers/clk/clk-xgene.c | 10 ++++------
>>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/clk/clk-xgene.c b/drivers/clk/clk-xgene.c
>>> index 3433132..1e67655 100644
>>> --- a/drivers/clk/clk-xgene.c
>>> +++ b/drivers/clk/clk-xgene.c
>>> @@ -243,22 +243,20 @@ static int xgene_clk_enable(struct clk_hw *hw)
>>> struct xgene_clk *pclk = to_xgene_clk(hw);
>>> unsigned long flags = 0;
>>> u32 data;
>>> - phys_addr_t reg;
>>>
>>> if (pclk->lock)
>>> spin_lock_irqsave(pclk->lock, flags);
>>>
>>> if (pclk->param.csr_reg != NULL) {
>>> pr_debug("%s clock enabled\n", clk_hw_get_name(hw));
>>> - reg = __pa(pclk->param.csr_reg);
>>> /* First enable the clock */
>>> data = xgene_clk_read(pclk->param.csr_reg +
>>> pclk->param.reg_clk_offset);
>>> data |= pclk->param.reg_clk_mask;
>>> xgene_clk_write(data, pclk->param.csr_reg +
>>> pclk->param.reg_clk_offset);
>>> - pr_debug("%s clock PADDR base %pa clk offset 0x%08X mask
>>> 0x%08X value 0x%08X\n",
>>> - clk_hw_get_name(hw), ®,
>>> + pr_debug("%s clk offset 0x%08X mask 0x%08X value
>>> 0x%08X\n",
>>> + clk_hw_get_name(hw),
>>> pclk->param.reg_clk_offset,
>>> pclk->param.reg_clk_mask,
>>> data);
>>>
>>> @@ -268,8 +266,8 @@ static int xgene_clk_enable(struct clk_hw *hw)
>>> data &= ~pclk->param.reg_csr_mask;
>>> xgene_clk_write(data, pclk->param.csr_reg +
>>> pclk->param.reg_csr_offset);
>>> - pr_debug("%s CSR RESET PADDR base %pa csr offset 0x%08X
>>> mask 0x%08X value 0x%08X\n",
>>> - clk_hw_get_name(hw), ®,
>>> + pr_debug("%s csr offset 0x%08X mask 0x%08X value
>>> 0x%08X\n",
>>> + clk_hw_get_name(hw),
>>> pclk->param.reg_csr_offset,
>>> pclk->param.reg_csr_mask,
>>> data);
>>> }
>>
>>
>>
>> The code looks fine to me. May be overly cautious here. Do you have a
>> board to test this out?
>>
>> -Loc
>>
>
> Yes, that was how I caught the problem. There are no errors that I
> can see with this patch applied.
Acked-by: Loc Ho <lho@apm.com>
-Loc
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCHv2] clk: xgene: Don't call __pa on ioremaped address
2016-10-28 16:59 [PATCHv2] clk: xgene: Don't call __pa on ioremaped address Laura Abbott
2016-10-28 17:07 ` Loc Ho
@ 2016-10-28 23:30 ` Stephen Boyd
1 sibling, 0 replies; 5+ messages in thread
From: Stephen Boyd @ 2016-10-28 23:30 UTC (permalink / raw)
To: Laura Abbott; +Cc: Michael Turquette, Loc Ho, linux-clk, linux-kernel
On 10/28, Laura Abbott wrote:
> ioremaped addresses are not linearly mapped so the physical
> address can not be figured out via __pa. More generally, there
> is no guarantee that backing value of an ioremapped address
> is a physical address at all. The value here is only used
> for debugging so just drop the call to __pa on the ioremapped
> address.
>
> Fixes: 6ae5fd381251 ("clk: xgene: Silence sparse warnings")
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
Applied to clk-fixes
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-10-28 23:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-28 16:59 [PATCHv2] clk: xgene: Don't call __pa on ioremaped address Laura Abbott
2016-10-28 17:07 ` Loc Ho
2016-10-28 17:09 ` Laura Abbott
2016-10-28 17:11 ` Loc Ho
2016-10-28 23:30 ` Stephen Boyd
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).