| Message ID | 20170328092545.4644-2-hgkr.klein@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, 2017-03-28 at 11:25 +0200, Helmut Klein wrote: > Expose the clock ids for the three none AO uarts to the dt-bindings Are they all used in the device tree ? We try to only expose what we need in the DT The recent discussion over CLKID_CPUCLK proved it was a sane thing to do. > > Signed-off-by: Helmut Klein <hgkr.klein@gmail.com> > --- > drivers/clk/meson/gxbb.h | 6 +++--- > include/dt-bindings/clock/gxbb-clkc.h | 3 +++ > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/meson/gxbb.h b/drivers/clk/meson/gxbb.h > index 8ee2022ce5d5..1edfaa5fe307 100644 > --- a/drivers/clk/meson/gxbb.h > +++ b/drivers/clk/meson/gxbb.h > @@ -194,7 +194,7 @@ > /* #define CLKID_SAR_ADC */ > #define CLKID_SMART_CARD 24 > #define CLKID_RNG0 25 > -#define CLKID_UART0 26 > +/* CLKID_UART0 */ > #define CLKID_SDHC 27 > #define CLKID_STREAM 28 > #define CLKID_ASYNC_FIFO 29 > @@ -216,7 +216,7 @@ > #define CLKID_ADC 45 > #define CLKID_BLKMV 46 > #define CLKID_AIU 47 > -#define CLKID_UART1 48 > +/* CLKID_UART1 */ > #define CLKID_G2D 49 > /* CLKID_USB0 */ > /* CLKID_USB1 */ > @@ -236,7 +236,7 @@ > /* CLKID_USB0_DDR_BRIDGE */ > #define CLKID_MMC_PCLK 66 > #define CLKID_DVIN 67 > -#define CLKID_UART2 68 > +/* CLKID_UART2 */ > /* #define CLKID_SANA */ > #define CLKID_VPU_INTR 70 > #define CLKID_SEC_AHB_AHB3_BRIDGE 71 > diff --git a/include/dt-bindings/clock/gxbb-clkc.h b/include/dt- > bindings/clock/gxbb-clkc.h > index 692846c7941b..7b329df47752 100644 > --- a/include/dt-bindings/clock/gxbb-clkc.h > +++ b/include/dt-bindings/clock/gxbb-clkc.h > @@ -15,13 +15,16 @@ > #define CLKID_SPI 34 > #define CLKID_I2C 22 > #define CLKID_SAR_ADC 23 > +#define CLKID_UART0 26 > #define CLKID_ETH 36 > +#define CLKID_UART1 48 > #define CLKID_USB0 50 > #define CLKID_USB1 51 > #define CLKID_USB 55 > #define CLKID_HDMI_PCLK 63 > #define CLKID_USB1_DDR_BRIDGE 64 > #define CLKID_USB0_DDR_BRIDGE 65 > +#define CLKID_UART2 68 > #define CLKID_SANA 69 > #define CLKID_GCLK_VENCI_INT0 77 > #define CLKID_AO_I2C 93 > -- > 2.11.0 > > > _______________________________________________ > linux-amlogic mailing list > linux-amlogic@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-amlogic
i know for sure that the bluetooth chip of my system is connected to uart_A. so this clock must be exposed. i don't know if the other 2 uarts are used on other hardware. so i will remove them from my patch. Helmut On 28.03.2017 17:51, Jerome Brunet wrote: > On Tue, 2017-03-28 at 11:25 +0200, Helmut Klein wrote: >> Expose the clock ids for the three none AO uarts to the dt-bindings > > Are they all used in the device tree ? > We try to only expose what we need in the DT > The recent discussion over CLKID_CPUCLK proved it was a sane thing to do. > >> >> Signed-off-by: Helmut Klein <hgkr.klein@gmail.com> >> --- >> drivers/clk/meson/gxbb.h | 6 +++--- >> include/dt-bindings/clock/gxbb-clkc.h | 3 +++ >> 2 files changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/clk/meson/gxbb.h b/drivers/clk/meson/gxbb.h >> index 8ee2022ce5d5..1edfaa5fe307 100644 >> --- a/drivers/clk/meson/gxbb.h >> +++ b/drivers/clk/meson/gxbb.h >> @@ -194,7 +194,7 @@ >> /* #define CLKID_SAR_ADC */ >> #define CLKID_SMART_CARD 24 >> #define CLKID_RNG0 25 >> -#define CLKID_UART0 26 >> +/* CLKID_UART0 */ >> #define CLKID_SDHC 27 >> #define CLKID_STREAM 28 >> #define CLKID_ASYNC_FIFO 29 >> @@ -216,7 +216,7 @@ >> #define CLKID_ADC 45 >> #define CLKID_BLKMV 46 >> #define CLKID_AIU 47 >> -#define CLKID_UART1 48 >> +/* CLKID_UART1 */ >> #define CLKID_G2D 49 >> /* CLKID_USB0 */ >> /* CLKID_USB1 */ >> @@ -236,7 +236,7 @@ >> /* CLKID_USB0_DDR_BRIDGE */ >> #define CLKID_MMC_PCLK 66 >> #define CLKID_DVIN 67 >> -#define CLKID_UART2 68 >> +/* CLKID_UART2 */ >> /* #define CLKID_SANA */ >> #define CLKID_VPU_INTR 70 >> #define CLKID_SEC_AHB_AHB3_BRIDGE 71 >> diff --git a/include/dt-bindings/clock/gxbb-clkc.h b/include/dt- >> bindings/clock/gxbb-clkc.h >> index 692846c7941b..7b329df47752 100644 >> --- a/include/dt-bindings/clock/gxbb-clkc.h >> +++ b/include/dt-bindings/clock/gxbb-clkc.h >> @@ -15,13 +15,16 @@ >> #define CLKID_SPI 34 >> #define CLKID_I2C 22 >> #define CLKID_SAR_ADC 23 >> +#define CLKID_UART0 26 >> #define CLKID_ETH 36 >> +#define CLKID_UART1 48 >> #define CLKID_USB0 50 >> #define CLKID_USB1 51 >> #define CLKID_USB 55 >> #define CLKID_HDMI_PCLK 63 >> #define CLKID_USB1_DDR_BRIDGE 64 >> #define CLKID_USB0_DDR_BRIDGE 65 >> +#define CLKID_UART2 68 >> #define CLKID_SANA 69 >> #define CLKID_GCLK_VENCI_INT0 77 >> #define CLKID_AO_I2C 93 >> -- >> 2.11.0 >> >> >> _______________________________________________ >> linux-amlogic mailing list >> linux-amlogic@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-amlogic >
On Tue, 2017-03-28 at 20:18 +0200, Helmut Klein wrote: > i know for sure that the bluetooth chip of my system is connected to > uart_A. so this clock must be exposed. > > i don't know if the other 2 uarts are used on other hardware. so i will > remove them from my patch. What I meant is device trees "upstream". I would expect such patch as part of a series to add support for your board in device-tree, with its bluetooth chipset. I think it would better to drop this patch from the series. You can either keep it for your personal work, or send it again when upstreaming the support for your board and/or add the support for the bluetooth chip. Cheers Jerome > > Helmut > > On 28.03.2017 17:51, Jerome Brunet wrote: > > On Tue, 2017-03-28 at 11:25 +0200, Helmut Klein wrote: > > > Expose the clock ids for the three none AO uarts to the dt-bindings > > > > Are they all used in the device tree ? > > We try to only expose what we need in the DT > > The recent discussion over CLKID_CPUCLK proved it was a sane thing to do. > > > > > > > > Signed-off-by: Helmut Klein <hgkr.klein@gmail.com> > > > --- > > > drivers/clk/meson/gxbb.h | 6 +++--- > > > include/dt-bindings/clock/gxbb-clkc.h | 3 +++ > > > 2 files changed, 6 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/clk/meson/gxbb.h b/drivers/clk/meson/gxbb.h > > > index 8ee2022ce5d5..1edfaa5fe307 100644 > > > --- a/drivers/clk/meson/gxbb.h > > > +++ b/drivers/clk/meson/gxbb.h > > > @@ -194,7 +194,7 @@ > > > /* #define CLKID_SAR_ADC */ > > > #define CLKID_SMART_CARD 24 > > > #define CLKID_RNG0 25 > > > -#define CLKID_UART0 26 > > > +/* CLKID_UART0 */ > > > #define CLKID_SDHC 27 > > > #define CLKID_STREAM 28 > > > #define CLKID_ASYNC_FIFO 29 > > > @@ -216,7 +216,7 @@ > > > #define CLKID_ADC 45 > > > #define CLKID_BLKMV 46 > > > #define CLKID_AIU 47 > > > -#define CLKID_UART1 48 > > > +/* CLKID_UART1 */ > > > #define CLKID_G2D 49 > > > /* CLKID_USB0 */ > > > /* CLKID_USB1 */ > > > @@ -236,7 +236,7 @@ > > > /* CLKID_USB0_DDR_BRIDGE */ > > > #define CLKID_MMC_PCLK 66 > > > #define CLKID_DVIN 67 > > > -#define CLKID_UART2 68 > > > +/* CLKID_UART2 */ > > > /* #define CLKID_SANA */ > > > #define CLKID_VPU_INTR 70 > > > #define CLKID_SEC_AHB_AHB3_BRIDGE 71 > > > diff --git a/include/dt-bindings/clock/gxbb-clkc.h b/include/dt- > > > bindings/clock/gxbb-clkc.h > > > index 692846c7941b..7b329df47752 100644 > > > --- a/include/dt-bindings/clock/gxbb-clkc.h > > > +++ b/include/dt-bindings/clock/gxbb-clkc.h > > > @@ -15,13 +15,16 @@ > > > #define CLKID_SPI 34 > > > #define CLKID_I2C 22 > > > #define CLKID_SAR_ADC 23 > > > +#define CLKID_UART0 26 > > > #define CLKID_ETH 36 > > > +#define CLKID_UART1 48 > > > #define CLKID_USB0 50 > > > #define CLKID_USB1 51 > > > #define CLKID_USB 55 > > > #define CLKID_HDMI_PCLK 63 > > > #define CLKID_USB1_DDR_BRIDGE 64 > > > #define CLKID_USB0_DDR_BRIDGE 65 > > > +#define CLKID_UART2 68 > > > #define CLKID_SANA 69 > > > #define CLKID_GCLK_VENCI_INT0 77 > > > #define CLKID_AO_I2C 93 > > > -- > > > 2.11.0 > > > > > > > > > _______________________________________________ > > > linux-amlogic mailing list > > > linux-amlogic@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/linux-amlogic
Hi Jerome, On Tue, Mar 28, 2017 at 9:14 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > On Tue, 2017-03-28 at 20:18 +0200, Helmut Klein wrote: >> i know for sure that the bluetooth chip of my system is connected to >> uart_A. so this clock must be exposed. >> >> i don't know if the other 2 uarts are used on other hardware. so i will >> remove them from my patch. > > What I meant is device trees "upstream". > I would expect such patch as part of a series to add support for your board in > device-tree, with its bluetooth chipset. are you sure about this? Helmut is configuring the core clock of the UART controller(s) here. we have the same thing for many other drivers as well (MMC, SAR ADC, I2C, SPI, ethernet, you name it...) - these clocks are not part of a device specific .dts but rather the SoC specific dts (for example meson-gxbb.dtsi - because these clocks are not board specific). I guess most boards are not affected because the bootloader simply enables the UART0 core/gate clock and keeps it enabled when booting the kernel. additionally our clock gates are marked with CLK_IGNORE_UNUSED so if the bootloader keeps the gates enabled then we're not disabling it either. > I think it would better to drop this patch from the series. > You can either keep it for your personal work, or send it again when upstreaming > the support for your board and/or add the support for the bluetooth chip. if we decide to pass the core/gate clock directly in SoC.dtsi then we should think about doing it for all three non-AO UARTs (uart_a, uart_b and uart_c). we can test uart_b on GPIODV_24 and GPIODV_25 on the Khadas VIM board for example (pins 22 and 23 on the header, default mode of these pins is i2c_sck_a and i2c_sda_a, but we can re-configure it). uart_c unfortunately cannot be tested on the Khadas VIM board since it's only routed to GPIOX_8 and GPIOX_9 which are hard-wired to the bluetooth module's PCM pins (BTPCM_DOUT and BTPCM_DIN). for Helmut this would mean that instead of dropping this patch (or dropping CLKID_UART1 and CLKID_UART2 from this patch) he would rather have to *add* another patch (for meson-gxbb.dtsi and meson-gxl.dtsi) which passes the core clocks to the corresponding UART controllers (similar to the CLKID_SD_EMMC_ clocks). Regards, Martin [0] http://lxr.free-electrons.com/source/drivers/clk/meson/clkc.h?v=4.10#L110
On Tue, 2017-03-28 at 23:24 +0200, Martin Blumenstingl wrote: > Hi Jerome, > > On Tue, Mar 28, 2017 at 9:14 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > > On Tue, 2017-03-28 at 20:18 +0200, Helmut Klein wrote: > > > i know for sure that the bluetooth chip of my system is connected to > > > uart_A. so this clock must be exposed. > > > > > > i don't know if the other 2 uarts are used on other hardware. so i will > > > remove them from my patch. > > > > What I meant is device trees "upstream". > > I would expect such patch as part of a series to add support for your board > > in > > device-tree, with its bluetooth chipset. > > are you sure about this? Actually no ;) I started questioning this reply as soon as I sent it. the moto so far as been "we expose what's needed" but there is many ways to interpret that. DT bindings is an ABI. Simply because we don't use part of that ABI upstream, doesn't mean we shouldn't provide it. My previous statement was clearly wrong about this, apologies. > Helmut is configuring the core clock of the > UART controller(s) here. we have the same thing for many other drivers > as well (MMC, SAR ADC, I2C, SPI, ethernet, you name it...) - these > clocks are not part of a device specific .dts but rather the SoC > specific dts (for example meson-gxbb.dtsi - because these clocks are > not board specific). Agreed Since Helmut tested it, this clock should be added to the dtsi. > I guess most boards are not affected because the bootloader simply > enables the UART0 core/gate clock and keeps it enabled when booting > the kernel. additionally our clock gates are marked with > CLK_IGNORE_UNUSED so if the bootloader keeps the gates enabled then > we're not disabling it either. > > > I think it would better to drop this patch from the series. > > You can either keep it for your personal work, or send it again when > > upstreaming > > the support for your board and/or add the support for the bluetooth chip. > > if we decide to pass the core/gate clock directly in SoC.dtsi then we > should think about doing it for all three non-AO UARTs (uart_a, uart_b > and uart_c). > we can test uart_b on GPIODV_24 and GPIODV_25 on the Khadas VIM board > for example (pins 22 and 23 on the header, default mode of these pins > is i2c_sck_a and i2c_sda_a, but we can re-configure it). > uart_c unfortunately cannot be tested on the Khadas VIM board since > it's only routed to GPIOX_8 and GPIOX_9 which are hard-wired to the > bluetooth module's PCM pins (BTPCM_DOUT and BTPCM_DIN). > > for Helmut this would mean that instead of dropping this patch (or > dropping CLKID_UART1 and CLKID_UART2 from this patch) he would rather > have to *add* another patch (for meson-gxbb.dtsi and meson-gxl.dtsi) > which passes the core clocks to the corresponding UART controllers > (similar to the CLKID_SD_EMMC_ clocks). You are right. This could be part of another series. I guess this patch fine as it is. Acked-by: Jerome Brunet <jbrunet@baylibre.com> > > > Regards, > Martin > > > [0] http://lxr.free-electrons.com/source/drivers/clk/meson/clkc.h?v=4.10#L110
Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes: [...] > for Helmut this would mean that instead of dropping this patch (or > dropping CLKID_UART1 and CLKID_UART2 from this patch) he would rather > have to *add* another patch (for meson-gxbb.dtsi and meson-gxl.dtsi) > which passes the core clocks to the corresponding UART controllers > (similar to the CLKID_SD_EMMC_ clocks). Yes, this is what I would like to see. If a new CLKID is exposed, I want to see the users of it at the same time. Kevin
On Wed, 2017-03-29 at 13:21 -0700, Kevin Hilman wrote: > Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes: > > [...] > > > for Helmut this would mean that instead of dropping this patch (or > > dropping CLKID_UART1 and CLKID_UART2 from this patch) he would rather > > have to *add* another patch (for meson-gxbb.dtsi and meson-gxl.dtsi) > > which passes the core clocks to the corresponding UART controllers > > (similar to the CLKID_SD_EMMC_ clocks). > > Yes, this is what I would like to see. > > If a new CLKID is exposed, I want to see the users of it at the same > time. Helmut, If you send another version of these patches, considering the feedback of Kevin and Martin, could you please change the subject of this patch to: dt-bindings: clock: gxbb: expose UART clocks Thx Jerome > > Kevin
On 31.03.2017 17:37, Jerome Brunet wrote: > On Wed, 2017-03-29 at 13:21 -0700, Kevin Hilman wrote: >> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes: >> >> [...] >> >>> for Helmut this would mean that instead of dropping this patch (or >>> dropping CLKID_UART1 and CLKID_UART2 from this patch) he would rather >>> have to *add* another patch (for meson-gxbb.dtsi and meson-gxl.dtsi) >>> which passes the core clocks to the corresponding UART controllers >>> (similar to the CLKID_SD_EMMC_ clocks). >> >> Yes, this is what I would like to see. >> >> If a new CLKID is exposed, I want to see the users of it at the same >> time. > > Helmut, > > If you send another version of these patches, considering the feedback of Kevin > and Martin, could you please change the subject of this patch to: > > dt-bindings: clock: gxbb: expose UART clocks > > Thx > Jerome > i've just sent v3 of my patchset. (i changed the subjects of most patches in the set) Helmut >> >> Kevin >
diff --git a/drivers/clk/meson/gxbb.h b/drivers/clk/meson/gxbb.h index 8ee2022ce5d5..1edfaa5fe307 100644 --- a/drivers/clk/meson/gxbb.h +++ b/drivers/clk/meson/gxbb.h @@ -194,7 +194,7 @@ /* #define CLKID_SAR_ADC */ #define CLKID_SMART_CARD 24 #define CLKID_RNG0 25 -#define CLKID_UART0 26 +/* CLKID_UART0 */ #define CLKID_SDHC 27 #define CLKID_STREAM 28 #define CLKID_ASYNC_FIFO 29 @@ -216,7 +216,7 @@ #define CLKID_ADC 45 #define CLKID_BLKMV 46 #define CLKID_AIU 47 -#define CLKID_UART1 48 +/* CLKID_UART1 */ #define CLKID_G2D 49 /* CLKID_USB0 */ /* CLKID_USB1 */ @@ -236,7 +236,7 @@ /* CLKID_USB0_DDR_BRIDGE */ #define CLKID_MMC_PCLK 66 #define CLKID_DVIN 67 -#define CLKID_UART2 68 +/* CLKID_UART2 */ /* #define CLKID_SANA */ #define CLKID_VPU_INTR 70 #define CLKID_SEC_AHB_AHB3_BRIDGE 71 diff --git a/include/dt-bindings/clock/gxbb-clkc.h b/include/dt-bindings/clock/gxbb-clkc.h index 692846c7941b..7b329df47752 100644 --- a/include/dt-bindings/clock/gxbb-clkc.h +++ b/include/dt-bindings/clock/gxbb-clkc.h @@ -15,13 +15,16 @@ #define CLKID_SPI 34 #define CLKID_I2C 22 #define CLKID_SAR_ADC 23 +#define CLKID_UART0 26 #define CLKID_ETH 36 +#define CLKID_UART1 48 #define CLKID_USB0 50 #define CLKID_USB1 51 #define CLKID_USB 55 #define CLKID_HDMI_PCLK 63 #define CLKID_USB1_DDR_BRIDGE 64 #define CLKID_USB0_DDR_BRIDGE 65 +#define CLKID_UART2 68 #define CLKID_SANA 69 #define CLKID_GCLK_VENCI_INT0 77 #define CLKID_AO_I2C 93
Expose the clock ids for the three none AO uarts to the dt-bindings Signed-off-by: Helmut Klein <hgkr.klein@gmail.com> --- drivers/clk/meson/gxbb.h | 6 +++--- include/dt-bindings/clock/gxbb-clkc.h | 3 +++ 2 files changed, 6 insertions(+), 3 deletions(-) -- 2.11.0