| Message ID | 20190101233745.16433-13-lukma@denx.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | dm: Convert TPC70 to use DM and DTS in SPL and u-boot proper | expand |
On 1/2/19 12:37 AM, Lukasz Majewski wrote: > With the current code, it is not possible to assign different than default > numbers for mmc controllers. > > Several in-tree boards depend on the pre-dm setup, corresponding to > following aliases: > > mmc0 = &usdhc2; --> fsl,usdhc-index = <1> > mmc1 = &usdhc4; --> fsl,usdhc-index = <3> > > Without this patch we are either forced to use default aliasing - like: > > mmc0 = &usdhc1; > mmc1 = &usdhc2; > mmc2 = &usdhc3; > mmc3 = &usdhc4; > > to have the proper clocks setup for the controller. However, such setup > is not acceptable for some legacy scripts / code. > > With this patch - by introducing 'fsl,usdhc-index' - one can configure > (get) clock rate corresponding to used controller. > > Moreover, as this code is used in the SPL before relocation (and to save > space we strip the SPL DTS from clocks and its names) adding separate > properties seems to be the best approach here. One also avoids adding > clocks DM code to SPL. > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > --- > > drivers/mmc/fsl_esdhc.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c > index 3cdfa7f5a6..49a6834a98 100644 > --- a/drivers/mmc/fsl_esdhc.c > +++ b/drivers/mmc/fsl_esdhc.c > @@ -1401,6 +1401,7 @@ static int fsl_esdhc_probe(struct udevice *dev) > fdt_addr_t addr; > unsigned int val; > struct mmc *mmc; > + int usdhc_idx; > int ret; > > addr = dev_read_addr(dev); > @@ -1513,7 +1514,21 @@ static int fsl_esdhc_probe(struct udevice *dev) > > priv->sdhc_clk = clk_get_rate(&priv->per_clk); > } else { > - priv->sdhc_clk = mxc_get_clock(MXC_ESDHC_CLK + dev->seq); > + /* > + * Check for 'fsl,index' DTS property - as one may want to have > + * following mmc setup: NAK, DT is a hardware description. This is encoding a policy, which should not be in DT. This looks like some reimplementation of SEQ_ALIAS stuff. > + * mmc0 = &usdhc2; --> fsl,index = <1> > + * mmc1 = &usdhc4; --> fsl,index = <3> > + * > + * So we do have dev->seq = {0, 1}, which in the below code > + * doesn't correspond to correct USDHC clocks. > + * > + * For that reason a new "fsl,index" property has been > + * introduced. > + */ > + usdhc_idx = dev_read_u32_default(dev, "fsl,usdhc-index", > + dev->seq); > + priv->sdhc_clk = mxc_get_clock(MXC_ESDHC_CLK + usdhc_idx); > if (priv->sdhc_clk <= 0) { > dev_err(dev, "Unable to get clk for %s\n", dev->name); > return -EINVAL; >
On Wed, 2 Jan 2019 02:18:58 +0100 Marek Vasut <marex@denx.de> wrote: > On 1/2/19 12:37 AM, Lukasz Majewski wrote: > > With the current code, it is not possible to assign different than > > default numbers for mmc controllers. > > > > Several in-tree boards depend on the pre-dm setup, corresponding to > > following aliases: > > > > mmc0 = &usdhc2; --> fsl,usdhc-index = <1> > > mmc1 = &usdhc4; --> fsl,usdhc-index = <3> > > > > Without this patch we are either forced to use default aliasing - > > like: > > > > mmc0 = &usdhc1; > > mmc1 = &usdhc2; > > mmc2 = &usdhc3; > > mmc3 = &usdhc4; > > > > to have the proper clocks setup for the controller. However, such > > setup is not acceptable for some legacy scripts / code. > > > > With this patch - by introducing 'fsl,usdhc-index' - one can > > configure (get) clock rate corresponding to used controller. > > > > Moreover, as this code is used in the SPL before relocation (and to > > save space we strip the SPL DTS from clocks and its names) adding > > separate properties seems to be the best approach here. One also > > avoids adding clocks DM code to SPL. > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > --- > > > > drivers/mmc/fsl_esdhc.c | 17 ++++++++++++++++- > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c > > index 3cdfa7f5a6..49a6834a98 100644 > > --- a/drivers/mmc/fsl_esdhc.c > > +++ b/drivers/mmc/fsl_esdhc.c > > @@ -1401,6 +1401,7 @@ static int fsl_esdhc_probe(struct udevice > > *dev) fdt_addr_t addr; > > unsigned int val; > > struct mmc *mmc; > > + int usdhc_idx; > > int ret; > > > > addr = dev_read_addr(dev); > > @@ -1513,7 +1514,21 @@ static int fsl_esdhc_probe(struct udevice > > *dev) > > priv->sdhc_clk = clk_get_rate(&priv->per_clk); > > } else { > > - priv->sdhc_clk = mxc_get_clock(MXC_ESDHC_CLK + > > dev->seq); > > + /* > > + * Check for 'fsl,index' DTS property - as one may > > want to have > > + * following mmc setup: > > NAK, DT is a hardware description. This is encoding a policy, which > should not be in DT. Please look a few lines up in this file: mmc0 = &usdhc1; mmc1 = &usdhc2; mmc2 = &usdhc3; mmc3 = &usdhc4; The fsl_esdhc.c has hardcoded ordering for eMMC devices when setting/getting clock. If you change aliases on your dts (mmc0 -> usdhc2, etc). Then with a bit of luck your second controller will be initialized with first's one clock value :-). This of course works by chance with default ROM setup. The problem is that many boards have different mmc ordering (starting with mmc0, which in above scheme is usdhc2 controller. Also mmc1 is the usdhc4 to which eMMC is connected in many boards). Of course this could be changed, but please consider a lot of legacy code pilling up on the customer's side. The clock index @ (drivers/mmc/fsl_esdhc.c - L1517): mxc_get_clock(MXC_ESDHC_CLK + dev->seq); Here the dev->seq is 0,1 (with SEQ_ALIAS), which will provide clock values from usdhc1 and usdhc2. However, we use usdhc4 (eMMC) and usdhc2 (SD). > > This looks like some reimplementation of SEQ_ALIAS stuff. No, this is a fix for hardcoded (assumed) clock setup in this driver. The other option is to provide/port clock stuff from linux (and implement CLK_DM in u-boot at least for this part). However, this will not fix the problem described above (for other boards which use the "legacy" approach). > > > + * mmc0 = &usdhc2; --> fsl,index = <1> > > + * mmc1 = &usdhc4; --> fsl,index = <3> > > + * > > + * So we do have dev->seq = {0, 1}, which in the > > below code > > + * doesn't correspond to correct USDHC clocks. > > + * > > + * For that reason a new "fsl,index" property has > > been > > + * introduced. > > + */ > > + usdhc_idx = dev_read_u32_default(dev, > > "fsl,usdhc-index", > > + dev->seq); > > + priv->sdhc_clk = mxc_get_clock(MXC_ESDHC_CLK + > > usdhc_idx); if (priv->sdhc_clk <= 0) { > > dev_err(dev, "Unable to get clk for %s\n", > > dev->name); return -EINVAL; > > > > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On 1/2/19 11:31 AM, Lukasz Majewski wrote: > On Wed, 2 Jan 2019 02:18:58 +0100 > Marek Vasut <marex@denx.de> wrote: > >> On 1/2/19 12:37 AM, Lukasz Majewski wrote: >>> With the current code, it is not possible to assign different than >>> default numbers for mmc controllers. >>> >>> Several in-tree boards depend on the pre-dm setup, corresponding to >>> following aliases: >>> >>> mmc0 = &usdhc2; --> fsl,usdhc-index = <1> >>> mmc1 = &usdhc4; --> fsl,usdhc-index = <3> >>> >>> Without this patch we are either forced to use default aliasing - >>> like: >>> >>> mmc0 = &usdhc1; >>> mmc1 = &usdhc2; >>> mmc2 = &usdhc3; >>> mmc3 = &usdhc4; >>> >>> to have the proper clocks setup for the controller. However, such >>> setup is not acceptable for some legacy scripts / code. >>> >>> With this patch - by introducing 'fsl,usdhc-index' - one can >>> configure (get) clock rate corresponding to used controller. >>> >>> Moreover, as this code is used in the SPL before relocation (and to >>> save space we strip the SPL DTS from clocks and its names) adding >>> separate properties seems to be the best approach here. One also >>> avoids adding clocks DM code to SPL. >>> >>> Signed-off-by: Lukasz Majewski <lukma@denx.de> >>> --- >>> >>> drivers/mmc/fsl_esdhc.c | 17 ++++++++++++++++- >>> 1 file changed, 16 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c >>> index 3cdfa7f5a6..49a6834a98 100644 >>> --- a/drivers/mmc/fsl_esdhc.c >>> +++ b/drivers/mmc/fsl_esdhc.c >>> @@ -1401,6 +1401,7 @@ static int fsl_esdhc_probe(struct udevice >>> *dev) fdt_addr_t addr; >>> unsigned int val; >>> struct mmc *mmc; >>> + int usdhc_idx; >>> int ret; >>> >>> addr = dev_read_addr(dev); >>> @@ -1513,7 +1514,21 @@ static int fsl_esdhc_probe(struct udevice >>> *dev) >>> priv->sdhc_clk = clk_get_rate(&priv->per_clk); >>> } else { >>> - priv->sdhc_clk = mxc_get_clock(MXC_ESDHC_CLK + >>> dev->seq); >>> + /* >>> + * Check for 'fsl,index' DTS property - as one may >>> want to have >>> + * following mmc setup: >> >> NAK, DT is a hardware description. This is encoding a policy, which >> should not be in DT. > > Please look a few lines up in this file: > > mmc0 = &usdhc1; > mmc1 = &usdhc2; > mmc2 = &usdhc3; > mmc3 = &usdhc4; > > The fsl_esdhc.c has hardcoded ordering for eMMC devices when > setting/getting clock. > > If you change aliases on your dts (mmc0 -> usdhc2, etc). Then with a > bit of luck your second controller will be initialized with first's one > clock value :-). This of course works by chance with default ROM setup. > > The problem is that many boards have different mmc ordering (starting > with mmc0, which in above scheme is usdhc2 controller. Also mmc1 is the > usdhc4 to which eMMC is connected in many boards). Of course this could > be changed, but please consider a lot of legacy code pilling up on the > customer's side. > > The clock index @ (drivers/mmc/fsl_esdhc.c - L1517): > mxc_get_clock(MXC_ESDHC_CLK + dev->seq); > > Here the dev->seq is 0,1 (with SEQ_ALIAS), which will provide clock > values from usdhc1 and usdhc2. However, we use usdhc4 (eMMC) and usdhc2 > (SD). > >> >> This looks like some reimplementation of SEQ_ALIAS stuff. > > No, this is a fix for hardcoded (assumed) clock setup in this driver. > > The other option is to provide/port clock stuff from linux (and > implement CLK_DM in u-boot at least for this part). However, this will > not fix the problem described above (for other boards which use the > "legacy" approach). Well fix the clock code then ? All the other subsystems use the SEQ_ALIAS and DT /aliases node to define ordering, I don't see why FSL SDHC driver should get some special hacky treatment which will only make it difficult to maintain in the future .
diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c index 3cdfa7f5a6..49a6834a98 100644 --- a/drivers/mmc/fsl_esdhc.c +++ b/drivers/mmc/fsl_esdhc.c @@ -1401,6 +1401,7 @@ static int fsl_esdhc_probe(struct udevice *dev) fdt_addr_t addr; unsigned int val; struct mmc *mmc; + int usdhc_idx; int ret; addr = dev_read_addr(dev); @@ -1513,7 +1514,21 @@ static int fsl_esdhc_probe(struct udevice *dev) priv->sdhc_clk = clk_get_rate(&priv->per_clk); } else { - priv->sdhc_clk = mxc_get_clock(MXC_ESDHC_CLK + dev->seq); + /* + * Check for 'fsl,index' DTS property - as one may want to have + * following mmc setup: + * mmc0 = &usdhc2; --> fsl,index = <1> + * mmc1 = &usdhc4; --> fsl,index = <3> + * + * So we do have dev->seq = {0, 1}, which in the below code + * doesn't correspond to correct USDHC clocks. + * + * For that reason a new "fsl,index" property has been + * introduced. + */ + usdhc_idx = dev_read_u32_default(dev, "fsl,usdhc-index", + dev->seq); + priv->sdhc_clk = mxc_get_clock(MXC_ESDHC_CLK + usdhc_idx); if (priv->sdhc_clk <= 0) { dev_err(dev, "Unable to get clk for %s\n", dev->name); return -EINVAL;
With the current code, it is not possible to assign different than default numbers for mmc controllers. Several in-tree boards depend on the pre-dm setup, corresponding to following aliases: mmc0 = &usdhc2; --> fsl,usdhc-index = <1> mmc1 = &usdhc4; --> fsl,usdhc-index = <3> Without this patch we are either forced to use default aliasing - like: mmc0 = &usdhc1; mmc1 = &usdhc2; mmc2 = &usdhc3; mmc3 = &usdhc4; to have the proper clocks setup for the controller. However, such setup is not acceptable for some legacy scripts / code. With this patch - by introducing 'fsl,usdhc-index' - one can configure (get) clock rate corresponding to used controller. Moreover, as this code is used in the SPL before relocation (and to save space we strip the SPL DTS from clocks and its names) adding separate properties seems to be the best approach here. One also avoids adding clocks DM code to SPL. Signed-off-by: Lukasz Majewski <lukma@denx.de> --- drivers/mmc/fsl_esdhc.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)