| Message ID | 1391537858-28593-2-git-send-email-william.towle@codethink.co.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi William, Thank you for the patch. On Tuesday 04 February 2014 18:17:36 William Towle wrote: > The clk_div_table for cpg_sd01_div_table[] concurs with the manual > but not with values found in the device itself (which are also the > same as the ones in arch/arm/mach-shmobile/clock-r8a7790.c). > > Update the clk-rcar-gen2.c driver to have the same table as the one > used by the mach-shmobile driver which work once further issues are > fixed in the clk-rcar-gen2.c driver. > > Part of the fix for the following error where the driver reports the > output as 1MHz but is really 97.5MHz: > sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 1 MHz > > [ben.dooks@codethink.co.uk: updated patch description] > Signed-off-by: William Towle <william.towle@codethink.co.uk> > Reviewed-by: Ben Dooks <ben.dooks@codethink.co.uk> > --- > drivers/clk/shmobile/clk-rcar-gen2.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/clk/shmobile/clk-rcar-gen2.c > b/drivers/clk/shmobile/clk-rcar-gen2.c index a59ec21..df4a1e6 100644 > --- a/drivers/clk/shmobile/clk-rcar-gen2.c > +++ b/drivers/clk/shmobile/clk-rcar-gen2.c > @@ -170,6 +170,8 @@ static const struct clk_div_table cpg_sdh_div_table[] = > { }; > > static const struct clk_div_table cpg_sd01_div_table[] = { > + { 0, 2 }, { 1, 3 }, { 2, 4 }, { 3, 6 }, > + { 4, 8 }, > { 5, 12 }, { 6, 16 }, { 7, 18 }, { 8, 24 }, > { 10, 36 }, { 11, 48 }, { 12, 10 }, { 0, 0 }, With this applied the only difference between the sdh and sd0/1 dividers tables would be the { 12, 10 } entry, available for sd0/1 only. Given that the hardware does not match the documentation, could you check whether that entry is supported by sdh as well ? If so we could merge the two tables. Otherwise this patch looks good, could you please just reformat the table to avoid the mostly empty line in the middle ? > };
On 05/02/14 10:56, Laurent Pinchart wrote: > Hi William, > > Thank you for the patch. > > On Tuesday 04 February 2014 18:17:36 William Towle wrote: >> The clk_div_table for cpg_sd01_div_table[] concurs with the manual >> but not with values found in the device itself (which are also the >> same as the ones in arch/arm/mach-shmobile/clock-r8a7790.c). >> >> Update the clk-rcar-gen2.c driver to have the same table as the one >> used by the mach-shmobile driver which work once further issues are >> fixed in the clk-rcar-gen2.c driver. >> >> Part of the fix for the following error where the driver reports the >> output as 1MHz but is really 97.5MHz: >> sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 1 MHz >> >> [ben.dooks@codethink.co.uk: updated patch description] >> Signed-off-by: William Towle <william.towle@codethink.co.uk> >> Reviewed-by: Ben Dooks <ben.dooks@codethink.co.uk> >> --- >> drivers/clk/shmobile/clk-rcar-gen2.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/clk/shmobile/clk-rcar-gen2.c >> b/drivers/clk/shmobile/clk-rcar-gen2.c index a59ec21..df4a1e6 100644 >> --- a/drivers/clk/shmobile/clk-rcar-gen2.c >> +++ b/drivers/clk/shmobile/clk-rcar-gen2.c >> @@ -170,6 +170,8 @@ static const struct clk_div_table cpg_sdh_div_table[] = >> { }; >> >> static const struct clk_div_table cpg_sd01_div_table[] = { >> + { 0, 2 }, { 1, 3 }, { 2, 4 }, { 3, 6 }, >> + { 4, 8 }, >> { 5, 12 }, { 6, 16 }, { 7, 18 }, { 8, 24 }, >> { 10, 36 }, { 11, 48 }, { 12, 10 }, { 0, 0 }, > > With this applied the only difference between the sdh and sd0/1 dividers > tables would be the { 12, 10 } entry, available for sd0/1 only. Given that the > hardware does not match the documentation, could you check whether that entry > is supported by sdh as well ? If so we could merge the two tables. Otherwise > this patch looks good, could you please just reformat the table to avoid the > mostly empty line in the middle ? I would like feedback from Renesas on this issue if possible. I can have a quick try at setting the clock value to 10 in u-boot and scope it out and see what happens. Magnus or Morimoto-san, is there a chance this could be reviewed by someone in Renesas who has knowledge of the hardware block? [PS, added Kuninori Morimoto to this[
On 05/02/14 11:55, Ben Dooks wrote: > On 05/02/14 10:56, Laurent Pinchart wrote: >> Hi William, >> >> Thank you for the patch. >> >> On Tuesday 04 February 2014 18:17:36 William Towle wrote: >>> The clk_div_table for cpg_sd01_div_table[] concurs with the manual >>> but not with values found in the device itself (which are also the >>> same as the ones in arch/arm/mach-shmobile/clock-r8a7790.c). >>> >>> Update the clk-rcar-gen2.c driver to have the same table as the one >>> used by the mach-shmobile driver which work once further issues are >>> fixed in the clk-rcar-gen2.c driver. >>> >>> Part of the fix for the following error where the driver reports the >>> output as 1MHz but is really 97.5MHz: >>> sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 1 >>> MHz >>> >>> [ben.dooks@codethink.co.uk: updated patch description] >>> Signed-off-by: William Towle <william.towle@codethink.co.uk> >>> Reviewed-by: Ben Dooks <ben.dooks@codethink.co.uk> >>> --- >>> drivers/clk/shmobile/clk-rcar-gen2.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/clk/shmobile/clk-rcar-gen2.c >>> b/drivers/clk/shmobile/clk-rcar-gen2.c index a59ec21..df4a1e6 100644 >>> --- a/drivers/clk/shmobile/clk-rcar-gen2.c >>> +++ b/drivers/clk/shmobile/clk-rcar-gen2.c >>> @@ -170,6 +170,8 @@ static const struct clk_div_table >>> cpg_sdh_div_table[] = >>> { }; >>> >>> static const struct clk_div_table cpg_sd01_div_table[] = { >>> + { 0, 2 }, { 1, 3 }, { 2, 4 }, { 3, 6 }, >>> + { 4, 8 }, >>> { 5, 12 }, { 6, 16 }, { 7, 18 }, { 8, 24 }, >>> { 10, 36 }, { 11, 48 }, { 12, 10 }, { 0, 0 }, >> >> With this applied the only difference between the sdh and sd0/1 dividers >> tables would be the { 12, 10 } entry, available for sd0/1 only. Given >> that the >> hardware does not match the documentation, could you check whether >> that entry >> is supported by sdh as well ? If so we could merge the two tables. >> Otherwise >> this patch looks good, could you please just reformat the table to >> avoid the >> mostly empty line in the middle ? > > I would like feedback from Renesas on this issue if possible. I can > have a quick try at setting the clock value to 10 in u-boot and scope > it out and see what happens. > > Magnus or Morimoto-san, is there a chance this could be reviewed by > someone in Renesas who has knowledge of the hardware block? > > [PS, added Kuninori Morimoto to this[ I got William to do a quick test with the following u-boot command mw.l 0xE6150074 0xCCC sdhi0 showed 156MHz output, and it seemed to work. So there is a distinct possibility that the sdh clock also supports setting 12 for a /10
Hi Ben, On Wednesday 05 February 2014 12:41:10 Ben Dooks wrote: > On 05/02/14 11:55, Ben Dooks wrote: > > On 05/02/14 10:56, Laurent Pinchart wrote: > >> On Tuesday 04 February 2014 18:17:36 William Towle wrote: > >>> The clk_div_table for cpg_sd01_div_table[] concurs with the manual > >>> but not with values found in the device itself (which are also the > >>> same as the ones in arch/arm/mach-shmobile/clock-r8a7790.c). > >>> > >>> Update the clk-rcar-gen2.c driver to have the same table as the one > >>> used by the mach-shmobile driver which work once further issues are > >>> fixed in the clk-rcar-gen2.c driver. > >>> > >>> Part of the fix for the following error where the driver reports the > >>> > >>> output as 1MHz but is really 97.5MHz: > >>> sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 1 > >>> MHz > >>> > >>> [ben.dooks@codethink.co.uk: updated patch description] > >>> Signed-off-by: William Towle <william.towle@codethink.co.uk> > >>> Reviewed-by: Ben Dooks <ben.dooks@codethink.co.uk> > >>> --- > >>> > >>> drivers/clk/shmobile/clk-rcar-gen2.c | 2 ++ > >>> 1 file changed, 2 insertions(+) > >>> > >>> diff --git a/drivers/clk/shmobile/clk-rcar-gen2.c > >>> b/drivers/clk/shmobile/clk-rcar-gen2.c index a59ec21..df4a1e6 100644 > >>> --- a/drivers/clk/shmobile/clk-rcar-gen2.c > >>> +++ b/drivers/clk/shmobile/clk-rcar-gen2.c > >>> @@ -170,6 +170,8 @@ static const struct clk_div_table > >>> cpg_sdh_div_table[] = > >>> { }; > >>> > >>> static const struct clk_div_table cpg_sd01_div_table[] = { > >>> > >>> + { 0, 2 }, { 1, 3 }, { 2, 4 }, { 3, 6 }, > >>> + { 4, 8 }, > >>> > >>> { 5, 12 }, { 6, 16 }, { 7, 18 }, { 8, 24 }, > >>> { 10, 36 }, { 11, 48 }, { 12, 10 }, { 0, 0 }, > >> > >> With this applied the only difference between the sdh and sd0/1 dividers > >> tables would be the { 12, 10 } entry, available for sd0/1 only. Given > >> that the hardware does not match the documentation, could you check > >> whether that entry is supported by sdh as well ? If so we could merge the > >> two tables. Otherwise this patch looks good, could you please just > >> reformat the table to avoid the mostly empty line in the middle ? > > > > I would like feedback from Renesas on this issue if possible. I can > > have a quick try at setting the clock value to 10 in u-boot and scope > > it out and see what happens. > > > > Magnus or Morimoto-san, is there a chance this could be reviewed by > > someone in Renesas who has knowledge of the hardware block? > > > > [PS, added Kuninori Morimoto to this[ > > I got William to do a quick test with the following u-boot command > mw.l 0xE6150074 0xCCC > > sdhi0 showed 156MHz output, and it seemed to work. So there is a > distinct possibility that the sdh clock also supports setting 12 > for a /10 Thank you for trying it out. I'd like feedback from Renesas as well, assuming we don't get a "don't do that or it will cause the universe to collapse - woops, too late" nack, let's respin this patch and merge the two tables instead.
Hi all > > >>> The clk_div_table for cpg_sd01_div_table[] concurs with the manual > > >>> but not with values found in the device itself (which are also the > > >>> same as the ones in arch/arm/mach-shmobile/clock-r8a7790.c). > > >>> > > >>> Update the clk-rcar-gen2.c driver to have the same table as the one > > >>> used by the mach-shmobile driver which work once further issues are > > >>> fixed in the clk-rcar-gen2.c driver. > > >>> > > >>> Part of the fix for the following error where the driver reports the > > >>> > > >>> output as 1MHz but is really 97.5MHz: > > >>> sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 1 > > >>> MHz > > >>> > > >>> [ben.dooks@codethink.co.uk: updated patch description] > > >>> Signed-off-by: William Towle <william.towle@codethink.co.uk> > > >>> Reviewed-by: Ben Dooks <ben.dooks@codethink.co.uk> (snip) > > >>> static const struct clk_div_table cpg_sd01_div_table[] = { > > >>> + { 0, 2 }, { 1, 3 }, { 2, 4 }, { 3, 6 }, > > >>> + { 4, 8 }, > > >>> > > >>> { 5, 12 }, { 6, 16 }, { 7, 18 }, { 8, 24 }, > > >>> { 10, 36 }, { 11, 48 }, { 12, 10 }, { 0, 0 }, (snip) > > > I would like feedback from Renesas on this issue if possible. I can > > > have a quick try at setting the clock value to 10 in u-boot and scope > > > it out and see what happens. > > > > > > Magnus or Morimoto-san, is there a chance this could be reviewed by > > > someone in Renesas who has knowledge of the hardware block? > > > > > > [PS, added Kuninori Morimoto to this[ > > > > I got William to do a quick test with the following u-boot command > > mw.l 0xE6150074 0xCCC > > > > sdhi0 showed 156MHz output, and it seemed to work. So there is a > > distinct possibility that the sdh clock also supports setting 12 > > for a /10 > > Thank you for trying it out. I'd like feedback from Renesas as well, assuming > we don't get a "don't do that or it will cause the universe to collapse - > woops, too late" nack, let's respin this patch and merge the two tables > instead. I ask it to Renesas HW team. Please wait Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi William, Ben, Laurent > > > >>> static const struct clk_div_table cpg_sd01_div_table[] = { > > > >>> + { 0, 2 }, { 1, 3 }, { 2, 4 }, { 3, 6 }, > > > >>> + { 4, 8 }, > > > >>> > > > >>> { 5, 12 }, { 6, 16 }, { 7, 18 }, { 8, 24 }, > > > >>> { 10, 36 }, { 11, 48 }, { 12, 10 }, { 0, 0 }, According to HW team, datasheet is correct. Some HW has above un-documented implementation indeed, but these are not supported. But, 0x0100 (x1/8) on SD0FC/SD1FC is now supported and documented in latest datasheet. > > > sdhi0 showed 156MHz output, and it seemed to work. So there is a > > > distinct possibility that the sdh clock also supports setting 12 > > > for a /10 According to HW there, SDHFC will be stopped if you set 0xC. Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/02/14 06:43, Kuninori Morimoto wrote: > Hi William, Ben, Laurent > >>>>>>> static const struct clk_div_table cpg_sd01_div_table[] = { >>>>>>> + { 0, 2 }, { 1, 3 }, { 2, 4 }, { 3, 6 }, >>>>>>> + { 4, 8 }, >>>>>>> >>>>>>> { 5, 12 }, { 6, 16 }, { 7, 18 }, { 8, 24 }, >>>>>>> { 10, 36 }, { 11, 48 }, { 12, 10 }, { 0, 0 }, > > According to HW team, datasheet is correct. > Some HW has above un-documented implementation indeed, > but these are not supported. > But, 0x0100 (x1/8) on SD0FC/SD1FC is now supported and documented in > latest datasheet. Thanks, we do not have a copy of that so cannot comment. >>>> sdhi0 showed 156MHz output, and it seemed to work. So there is a >>>> distinct possibility that the sdh clock also supports setting 12 >>>> for a /10 > > According to HW there, > SDHFC will be stopped if you set 0xC. We'll look at re-working this patch and getting it re-sent.
diff --git a/drivers/clk/shmobile/clk-rcar-gen2.c b/drivers/clk/shmobile/clk-rcar-gen2.c index a59ec21..df4a1e6 100644 --- a/drivers/clk/shmobile/clk-rcar-gen2.c +++ b/drivers/clk/shmobile/clk-rcar-gen2.c @@ -170,6 +170,8 @@ static const struct clk_div_table cpg_sdh_div_table[] = { }; static const struct clk_div_table cpg_sd01_div_table[] = { + { 0, 2 }, { 1, 3 }, { 2, 4 }, { 3, 6 }, + { 4, 8 }, { 5, 12 }, { 6, 16 }, { 7, 18 }, { 8, 24 }, { 10, 36 }, { 11, 48 }, { 12, 10 }, { 0, 0 }, };
The clk_div_table for cpg_sd01_div_table[] concurs with the manual but not with values found in the device itself (which are also the same as the ones in arch/arm/mach-shmobile/clock-r8a7790.c). Update the clk-rcar-gen2.c driver to have the same table as the one used by the mach-shmobile driver which work once further issues are fixed in the clk-rcar-gen2.c driver. Part of the fix for the following error where the driver reports the output as 1MHz but is really 97.5MHz: sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 1 MHz [ben.dooks@codethink.co.uk: updated patch description] Signed-off-by: William Towle <william.towle@codethink.co.uk> Reviewed-by: Ben Dooks <ben.dooks@codethink.co.uk> --- drivers/clk/shmobile/clk-rcar-gen2.c | 2 ++ 1 file changed, 2 insertions(+)